JDoe Java Program

[eldon xu]

Recently, I did a java program test, my code is:
>>>>>>>>> JDoe.java

import com.zappos.elevator.*;
import java.util.List;

public class JDoe implements SingleElevatorController {

    public JDoe() {
    }

    public Instruction getNextOperation(SingleElevatorSystemData data) {
        int currentFloor = data.getCurrentFloor();

        if (data.isDoorOpen()) {
            return Instruction.CLOSE_DOORS;
        }
        if (data.isFloorButtonOn(currentFloor)) {
            if (currentFloor == 0) {
                return Instruction.OPEN_DOORS_UP;
            }
            if (currentFloor == data.getFloorCount() - 1) {
                return Instruction.OPEN_DOORS_DOWN;
            }
            if (data.isCallButtonOn(currentFloor, Direction.UP)) {
                return Instruction.OPEN_DOORS_UP;
            } else {
                return Instruction.OPEN_DOORS_DOWN;
            }
        }
        if (data.isCallButtonOn(currentFloor, Direction.UP)) {
            return Instruction.OPEN_DOORS_UP;
        }
        if (data.isCallButtonOn(currentFloor, Direction.DOWN)) {
            return Instruction.OPEN_DOORS_DOWN;
        }
        if (!data.getLightedFloorButtons().isEmpty()) {
            int nearestFloor = findNextFloor(data.getLightedFloorButtons(), currentFloor);
            if (nearestFloor > currentFloor) {
                return Instruction.MOVE_UP;
            }
            if (nearestFloor < currentFloor) {
                return Instruction.MOVE_DOWN;
            }
        }
        if (!data.getAllCallButtons().isEmpty()) {
            int nearestFloor = findNextFloor(data.getAllCallButtons(), currentFloor);
            if (nearestFloor > currentFloor) {
                return Instruction.MOVE_UP;
            }
            if (nearestFloor < currentFloor) {
                return Instruction.MOVE_DOWN;
            }
        }
        if (currentFloor > 0) {
            return Instruction.MOVE_DOWN;
        }

        return Instruction.NO_OP;
    }

    private int findNextFloor(List floors, int currentFloor) {
		int nextFloor = currentFloor;

        if (!floors.isEmpty()) {
            nextFloor = (Integer) floors.get(0);
            for (int tempFloor : (List<Integer>) floors) {
                if (Math.abs(currentFloor - tempFloor) < Math.abs(currentFloor - nextFloor)) {
                    nextFloor = tempFloor;
                }
            }
        }

        return nextFloor;
    }
}

<<<<<<<<<

I got a review as following:

1. It did run however it has warnings and there were many issues.
2. Some of the instructions were not followed properly
3. Not creative about his approach
4. Code is difficult to debug due to so many return statements being used

It is just hard to believe. Can anybody think about any issue based on the review ?

[Jeff Verdegan]

eldon xu wrote:
I got a review as following:

1. It did run however it has warnings and there were many issues.
2. Some of the instructions were not followed properly
3. Not creative about his approach
4. Code is difficult to debug due to so many return statements being used

It is just hard to believe. Can anybody think about any issue based on the review ?

What's hard to believe?

1. It did run however it has warnings and there were many issues.

Either it has warnings or it doesn't. There's nothing subjective there. You can see for yourself when you compile.
So I believe this one.

2. Some of the instructions were not followed properly

Nobody here can know whether you followed the instructions. Presumably the individuals grading the test know, however.
So I believe this one.

3. Not creative about his approach

I don't consider a bunch of ifs to be creative, regardless of what the assignment was.
So I believe this one.

4. Code is difficult to debug due to so many return statements being used

I'm not an adherent to the "one return" faith. I think a few is fine. But you do have kind of a lot. Still, the pattern seems pretty simple to derive, so it's not horrible.
So I believe the grader could feel this way, but I agree with him only about 50% on this one.

[eldon xu]

The assignemt is here: http://kydevelopment.com/elevator/index.html
You can compile and run the program without any problem.

[Jesper de Jong]

To give you an idea of how this could be done differently, so that you don't have to many if-statements in one big method, is by implementing this with a state machine.

The elevator can be seen as an object which can be in different states: moving, standing still with doors opened, with doors closed, etc. How the elevator behaves depends on what state it is currently in. You could model this explicitly in your program: create a State superclass or interface, and then create subclasses for each possible state. Handle events in the state classes, putting the behaviour for the event for a specific state in the class for that state.

[eldon xu]

Thank you offering different approaching. That is a very nice thought. But this programming task is to write a class that implements "SingleElevatorController". it only has 1 function, Instruction getNextOperation(SingleElevatorSystemData data). Any other approcah seems making the program lengthy and more complicated, in other words, it will runs slow.

[Jeff Verdegan]

eldon xu wrote:Any other approcah seems making the program lengthy and more complicated, in other words, it will runs slow.

No. Proper design will not make it run significantly slower, and in many cases, will make it run significantly faster.

[eldon xu]

For this case, would you mind offering some codes that will make it significantly faster, given a specific interface and only one function in it.

[fred rosenberger]

eldon xu wrote:Any other approcah (sic)...will runs slow.

Do you have any actual empirical evidence supporting such a statement? I mean, that is a pretty bold statement to make without any.

Further, even if it does slow down your code my a nanosecond, will anyone ever really notice? If writing cleaner code saves you hours of debugging but costs a few extra clock cycles, isn't it worth it?

[Randall Twede]

it wasn't my assignment, so i sure don't know all that was required, but i sure would think twice about all those if statements. some seem to repeat even. if it even just returned once for each possible instruction, it would be an improvement.

[bob mark]

I run the testing program, it is great. It perfectly followed the requirement and got excellent result. No any problem or issue.

1. It did run however it has warnings and there were many issues.
---- I did not see any problem or issues. Did they point out any real stuff?

2. Some of the instructions were not followed properly
---- I saw it completely followed the instructions and got the perfect result. Did they point out anything you missed?

3. Not creative about his approach
---- The program met the requirement, there is no requirement for "creative" here.

4. Code is difficult to debug due to so many return statements being used
---- The program code is perfect. I do not see any difficulty to debug. These return statements are good program pattern. The program is very clean and concisely.

Reviewer gave such valuation is not reasonable. You are very good, do not be discouraged.