Elevator Design

Question

I want to design an elevator system, which has functionality for pressing button from outside of the elevator (Not selecting a direction - Only calls the passenger’s existing floor). And also select floor functionality needed to select a floor for target.

Inside controls have priority over outside controls.

While traveling the elevator only stops a station (via outside control) if it is on its way. I have tried to implement state patterns but it didn't work as I expected.

Can you please put a comment that how can I improve the following solution.

Thanks.

Elevator.java

package OOPDesign.elevatorStates;

import java.util.LinkedList;
import java.util.Queue;

public class Elevator {

    public static Elevator instance;

    static Queue<Integer> requestQueue;
    static ElevatorStateContext elevatorStateContext;
    static int currentFloor;

    private Elevator(){
        requestQueue = new LinkedList<>();
        elevatorStateContext = new ElevatorStateContext();
        currentFloor = 0;
    }

    public static Elevator getInstance(){
        if(instance == null){
            instance = new Elevator();
        }
        return instance;
    }

    static void receiveRequest(int passengersFloor){
        requestQueue.add(passengersFloor);

        processQueue(requestQueue);
    }

    static void processQueue(Queue<Integer> requestQueue) {

        int passengersFloor = requestQueue.peek();
        System.out.println("Currently going "+ passengersFloor);
        if(passengersFloor>currentFloor)
            elevatorStateContext.setState(State.UP);
        else
            elevatorStateContext.setState(State.DOWN);
        System.out.println("Direction "+ elevatorStateContext.getDirection());
        requestQueue.remove();
        try {
            Thread.sleep(1000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        System.out.println("Arrived "+ passengersFloor);
        if(requestQueue.isEmpty()){
            elevatorStateContext.setState(State.IDLE);
            System.out.println("Direction "+ elevatorStateContext.getDirection());
        }
    }


}

ElevatorControlSystem.java

package OOPDesign.elevatorStates;

public class ElevatorControlSystem {

    Elevator elevator;

    public ElevatorControlSystem(){
        elevator = Elevator.getInstance();
    }

    void pressButton(int passengersFloor){
        dispatchRequest("OUTSIDE",passengersFloor);
    }


    void selectFloor(int targetFloor){
        dispatchRequest("INSIDE",targetFloor);
    }

    private void dispatchRequest(String priority, int passengersFloor) {

        int elevatorFloor = Elevator.currentFloor;
        // If elevator is idle it can serve to any request
        if(Elevator.elevatorStateContext.getDirection() == State.IDLE){
            Elevator.receiveRequest(passengersFloor);
        }else {

            if (priority == "INSIDE") {
                Elevator.receiveRequest(passengersFloor);
            } else { // Outside calls
                if (Elevator.elevatorStateContext.getDirection() == State.UP && passengersFloor > elevatorFloor) {
                    Elevator.receiveRequest(passengersFloor);
                } else if (Elevator.elevatorStateContext.getDirection() == State.DOWN && passengersFloor < elevatorFloor) {
                    Elevator.receiveRequest(passengersFloor);
                } else {
                    System.out.println("Can not give service");
                }
            }
        }

    }
}

ElevatorState.java

package OOPDesign.elevatorStates;

interface ElevatorState {

    public State status(ElevatorStateContext ctx);
}

ElevatorStateContext.java

package OOPDesign.elevatorStates;

public class ElevatorStateContext {

    private State elevatorState;

    public ElevatorStateContext(){
        elevatorState = State.IDLE;
    }

    public void setState(State state){
        elevatorState = state;
    }

    public State getDirection(){
        return elevatorState;
    }

}

Enum

package OOPDesign.elevatorStates;

public enum State {
    IDLE,
    UP,
    DOWN;
}

Test class

package OOPDesign.elevatorStates;

public class ElevatorTest {

    public static void main(String[] args) {

        ElevatorControlSystem ecs = new ElevatorControlSystem();

        ecs.pressButton(5);

        ecs.selectFloor(3);

        ecs.pressButton(6);
        ecs.selectFloor(1);
        ecs.pressButton(9);
        ecs.pressButton(10);
        ecs.pressButton(11);

    }
}

Answer 1

If this design is for a school project, and you've already submitted it, I fear you will have already failed the assignment. This design is bad.

Static

As a guiding principle, Java programs should use the identifier static exactly once, specifically in the declaration public static void main(String args[]). Any other usages of static should sound warning bells; you might be doing something wrong.

Consider a hotel. It often has multiple elevators. With static int currentFloor, every elevator must be on the same floor! With static ElevatorStateContext elevatorStateContext, every elevator is moving in the same direction! This is a poor design choice. Even if you have exactly one elevator, that elevator should own its own data, instead of having it stored as global class variables.

Often, hotels have multiple elevator systems. There might be a main elevator bank, for guests, as well as service elevators for hotel staff. There could be elevators in multiple towers of the hotel, and parking garage elevators. In short, there could be more than one ElevatorControlSystem, and each would own its own set of elevators. Each time ElevatorControlSystem is constructed, it executes elevator = Elevator.getInstance(); ... which means each control system is trying to operate the same elevator???

---

Identity -vs- Equality

The test if (priority == "INSIDE") is testing object identity. Only if the String stored in priority has the same identity as the String embedded in this compilation unit will the test pass. This is fragile, and will break as soon as you try passing in the String "INSIDE" created elsewhere. You should check for equality: if (priority.equals("INSIDE")).

Or, use an enum for INSIDE -vs- OUTSIDE, like you are already using for elevator state.

---

Useless Items

Useless Encapsulation

ElevatorStateContext contains State elevatorState, and a setter and a getter, which add no additional behaviour, checks, or functionality.

What is the purpose of this class? You could directly store and fetch the State in Elevator, and eliminate this class. Or was something else supposed to be included in the ElevatorStateContext, like whether the doors were open/opening/closed/closing or which floors have been requested?

Useless Queue

You have a queue which will only ever hold at most one item. These lines add the item, and then immediately pull the item off for processing:

    requestQueue.add(passengersFloor);
    processQueue(requestQueue);

Inside processQueue(), you peek() and remove() items from the queue separately:

    int passengersFloor = requestQueue.peek();
    ...
    requestQueue.remove();
    ...

But there is no point to peeking; you always remove the item, so the following simpler code would work:

    int passengersFloor = requestQueue.remove();
    ...

---

Unused Items

Unused Singleton

You execute elevator = Elevator.getInstance(); but nowhere are you actually using the elevator member variable.

Unused Interface

You declare interface ElevatorState, but nothing ever implements that interface.

---

Sequential Tests / Operation

Due to dispatchRequest() and processQueue() completely handling requests one at a time, the test code is completely incapable of testing an elevator in motion when another request comes in. If 5 people request the elevator on the main floor, and get in, and press the floors buttons 5, 2, 6, 3, 4 ...

    ecs.pressButton(1);
    ecs.selectFloor(5);
    ecs.selectFloor(2);
    ecs.selectFloor(6);
    ecs.selectFloor(3);
    ecs.selectFloor(4);

the elevator will move to the 5th floor, bypassing 2, 3 and 4. And then it will move to the 2nd floor, bypassing 4 and 3. Then it will move to the 6th floor, bypassing 3 and 4. And then it will move to the 3rd floor bypassing 4. And finally it will move to the 4th floor. Painfully inefficient.