-8
\$\begingroup\$

A school has 100 lockers and 100 students. All lockers are closed on the first day of school. As the students enter, the first student, denoted S1, opens every locker. Then the second student, S2, begins with the second locker, denoted L2, and closes every other locker. Student S3 begins with the third locker and changes every third locker (closes it if it was open, and opens it if it was closed). Student S4 begins with locker L4 and changes every fourth locker. Student S5 starts with L5 and changes every fifth locker, and so on, until student S100 changes L100.

After all the students have passed through the building and changed the lockers, which lockers are open?

Write a program to find your answer.

(Hint: Use an array of 100 Boolean elements, each of which indicates whether a locker is open (true) or closed (false). Initially, all lockers are closed.)

Here are the two methods I came up with to solve this problem.

First method :

public class Test {
  public static void main(String[] args) {
    // Declare a constant value for the number of lockers
    final int NUMBER_OF_LOCKER = 100;

    // Create an array to store the status of each array
    // The first student closed all lockers, each lockers[i] is false
    boolean[] lockers = new boolean[NUMBER_OF_LOCKER];

    // Each student changes the lockers
    for (int j = 1; j <= NUMBER_OF_LOCKER; j++) {
      // Student Sj changes every jth locker
      // starting from the lockers[j - 1].
      for (int i = j - 1; i < NUMBER_OF_LOCKER; i += j) {
        lockers[i] = !lockers[i];
      }
    }

    // Find which one is open
    for (int i = 0; i < NUMBER_OF_LOCKER; i++) {
      if (lockers[i])
        System.out.println("Locker " + (i + 1) + " is open");
    }
  }
}

Second method :

    public class Main {
        public static void main(String[] args) {
            boolean[] lockers = new boolean[101];
            //Open all multiples of 1 before moving on to 2
            for (int i = 1; i < lockers.length; i++) {
                lockers[i] = true;
            }


            //open every locker for every multiple of i
            for (int i = 2; i <= 100; i++) {
                for (int j = 1; i * j <= 100; j++) {
                    lockers[i * j] = (lockers[i * j] == true) ? false : true;
                }
            }

            //Display the indices of the open lockers
            for (int i = 0; i < lockers.length; i++) {
                if (lockers[i] == true)
                    System.out.println("locker " + i + " is open.");
            }
        }
    }

What do you think of the code? Can it be cleaner/neater?

\$\endgroup\$
  • 3
    \$\begingroup\$ Please don't re-post a question that was first closed as being off-topic. If you're having difficulties with the code, and you can narrow it down to a specific, reproducible problem or error, you can try at Stack Overflow. But do not copy your question as-is there, as you will likely get poor reception there as well. \$\endgroup\$ – Phrancis Jul 15 '15 at 15:47
  • 3
    \$\begingroup\$ What made you decide to write two different methods for the same purpose? Is the second written with a different mindset? \$\endgroup\$ – Mast Jul 15 '15 at 16:39
  • \$\begingroup\$ This question is not ready to be reopened, and if it is reopened, I will be voting to close it as too broad. This isn't a comparative review. It is two questions posted as a single question. I understand we allow comparative review questions (even that his debated here), but that's not what this is. There is no description about the differences, there's no logic given for why we should make the effort to review two separate implementations here. This needs to be two separate questions. \$\endgroup\$ – nhgrif Jul 15 '15 at 16:40
  • \$\begingroup\$ The only doors that are open are the integers that have an odd number of factors i.e. the squares (1, 4, 9, 16, 25, 36...). \$\endgroup\$ – Vedaad Shakib Jul 17 '15 at 6:30
7
\$\begingroup\$

Constants :

Please create constants as global variable, as it is a constant who should never be changed.
Now each time you call your method there is a memory allocation for that constant.

private final int NUMBER_OF_LOCKERS = 100;

Set correct comments and comment where needed :

// The first student closed all lockers, each lockers[i] is false

As I read the requirements, this is not true.
All lockers are closed and the first students opens all the lockers.

boolean[] lockers = new boolean[NUMBER_OF_LOCKER];

This creates your starting situation, no more comment needed.
The following comments are also not needed, in the next paragraph we see why.

Separation of concerns :

You separate your concerns by a blank line and a comment what you are going to do.
Why not make a new method with a well chosen name so your comment isn't needed.
Example :

private final int NUMBER_OF_LOCKERS = 100;

public static void main(String[] args) {
    boolean[] lockers = new boolean[NUMBER_OF_LOCKERS];
    openAndCloseLockers(lockers);
    printOpenLockers(lockers);
}

Like this we see we have a process, opening and closing the lockers.
After the process, we want an output.
This has nothing to do with the process itself so put it in another method.
Like this we can use this last method again, maybe to show our start situation.

naming variable:

A thing what comes almost always back, is give good names to your variable.
While you do it (almost) correct for the NUMBER_OF_LOCKER, you don't do it for your counters.
As you could see earlier, if you use Number of xxx => the X should be plurial.
Personally I would rename j to student and i to studendAtLocker.

0 based or 1 based:

Why do you want to do the 1 based for students so you have to substract 1 in you next loop.
Almost every programmer knows how to handle 0 based items and how to read it.
Just keep it 0 based and alter to 1 based when you need to output it.

Same thing for the output, you can alter with a minimum so you don't need to count constantly.

int locker = 0;
while (locker < lockers.length) {
    if (lockers[locker++])
        System.out.println("locker " + locker + " is open.");
    }
}

Some explication :
I changed the for to a while, so we have to be carefull and see that we raise locker or we have an infinitive loop.
So the if I altered also, see next paragraph for explication.
Now with the if we test if the locker is open or not.
If you see I use locker++ in the if, what means :

boolean isOpen = lockers[locker];
locker = locker +1;
if (isOpen) {
   //....
}

We take locker 0 => raise the "counter" and we output already 1 if we output it.

correct if statements:

An if statement you have to watch as a method what need's a boolean as parameter.
You don't use that method like setVisible(otherComponent.isVisible()=true); but you use it like setVisible(otherComponent.isVisible());
This counts also for the if statement.
You are doing this :

if (true = true) {

Witch is the same as

if (true) {

You do it correct in first method, and missed it in the second method.
The same thing for :

lockers[i * j] = (lockers[i * j] == true) ? false : true;

Better usage for loop:

As last I want to show you this method.

//open every locker for every multiple of i
for (int i = 2; i <= 100; i++) {
    for (int j = 1; i * j <= 100; j++) {
        lockers[i * j] = (lockers[i * j] == true) ? false : true;
    }
}

Take in mind all the previous things, but there is one thing I want to make clear.
A for loop can handle more then counting 1 up or 1 down.
Your making it more complex then it should be.
You multiply 3 times while it's not needed. If you just count j to i as for condition, you have the same effect :

for (int i = 2; i <= 100; i= i++) {
    for (int j = 1; j <= 100; j = j + i) {
        lockers[j] = !lockers[j];
    }
}

As you can see this will do the same.
I also changed you turnary operater because it's just not needed to do it.
It's just an assignment of the inversion of the boolean.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.