Elevator Interview Problem OOP

Question

Can someone please critique my Elevator problem - I wanted to use OOP principles and coding standards. Also, does logic make sense?

1. Once elevator starts going up it needs to finish all requests for above floors, same for down.

So I imagine that if the elevator is moving up and the request is floor below it will need to finish all up/above requests etc.

---

internal class Program
{
    private const string _quit = "q";

    private static void Main(string[] args)
    {
        var manager = new Manager();
        var input = string.Empty;

        while (input != _quit) {
            Console.Write("Enter floor: ");
            input = Console.ReadLine();
            int floor;
            if (int.TryParse(input, out floor))
                manager.ButtonPressed(floor);
            else if (input == _quit)
                Console.WriteLine("GoodBye!");
            else
                Console.WriteLine("You have pressed an incorrect floor, Please try again");
        }
    }
}

internal enum Status
{
    GoingUp,
    GoingDown,
    Stopped
}

internal class Elevator
{
    public int TopFloor;

    public Elevator(int topFloor)
    {
        TopFloor = topFloor;
    }

    public int CurrentFloor { get; set; } = 1;
    public Status Status { get; set; } = Status.Stopped;

    public void MoveUp(int floor)
    {
        Status = Status.GoingUp;
        Console.WriteLine("Going up to: {0}", floor);
        CurrentFloor = floor;
        OpenDoor();
        CloseDoor();
    }

    public void MoveDown(int floor)
    {
        Status = Status.GoingDown;
        Console.WriteLine("Going down to: {0}", floor);
        CurrentFloor = floor;
        OpenDoor();
        CloseDoor();
    }

    private void OpenDoor()
    {
        Console.WriteLine("Door opening");
    }

    private void CloseDoor()
    {
        Console.WriteLine("Door closing, at floor: {0}", this.CurrentFloor);
    }
}

internal class Request
{
    public Request(int floor)
    {
        Floor = floor;
    }

    public int Floor { get; set; }
}

internal class Manager
{
    private readonly Queue<Request> _downRequests = new Queue<Request>();
    private readonly Elevator _elevator = new Elevator(10);
    private readonly Queue<Request> _upRequests = new Queue<Request>();

    public void ButtonPressed(int floor)
    {
        if (floor > _elevator.TopFloor) {
            Console.WriteLine("Only have {0} floors", _elevator.TopFloor);
            return;
        }

        if (floor > _elevator.CurrentFloor) 
            _upRequests.Enqueue(new Request(floor));
        else
            _downRequests.Enqueue(new Request(floor));

        Move(floor);
    }

    private void Move(int floor)
    {
        switch (_elevator.Status) {
            case Status.GoingDown:
                while(_downRequests.Count > 0)
                    _elevator.MoveDown(_downRequests.Dequeue().Floor);

                _elevator.Status = Status.Stopped;
                break;

            case Status.GoingUp:
                while(_upRequests.Count > 0)
                    _elevator.MoveUp(_upRequests.Dequeue().Floor);

                _elevator.Status = Status.Stopped;
                break;

            case Status.Stopped:
                if (floor > _elevator.CurrentFloor)
                    _elevator.Status = Status.GoingUp;
                else if (floor <= _elevator.CurrentFloor) {
                    _elevator.Status = Status.GoingDown;
                }
                Move(floor);

                break;
            default:
                break;
        }
    }
}

Revised code based on feedback: Elevator Interview Problem OOP - Revised

Answer 1

I would say that your logic is flawed at the following points:

• Your elevator only moves when a button is pressed, so how do you handle multiple button presses? For a proper scenario you would have a loop testing if there are input, and then read input, and otherwise check to see if there are further requests in queue.

• Your test for going up or down, is done when the button is pressed. If this were asynchronous someone could first hit the button at the top floor, and then at the second highest, but your code would mark the second press as going up. But due to the queue it would go to the top floor first, and then try to go up to the floor below?!

  A better logic would be to have a data structure holding the pressed button in an ordered sequence, and whenever the elevator moves in either direction it would continue moving in that direction if there are more buttons pressed in that sequence. Then it would turn and check if any other buttons are pressed.

• In the case Status.Stopped the logic seems sketchy, at least if the move of the elevator was disconnected from the button press action, which it should be. Most likely this logic would break, if moving and pressing was disconnected.

So the main claim I would say is to decouple the pressing of buttons from the moving of the elevator. With current code this would most likely break stuff.

Besides that, the code seems decent and clean.

Alternative approach

A proper Elevator would most likely handle it's own moving. To have the move coordinated by the Manager seems kind of strange. I would rather have the button press action done by the Manager, and sending events to the Elevator which would have its own Event handler.

In the event loop within the Elevator, I would then look at which way it was moving, and see if there are other requests for floors in that direction. If not, flip direction and see if other requests exists. Stop when no more requests exists, and if a new event is fired, repeat process.

This would also allow for the Manager to send events from the buttons on each given floor, or from inside of the elevator, or possibly from a control room. In all cases a single event of wanted floor would be sent.

Such a scheme would also incorporate a proper separation of concerns, as the Elevator would only focus on handling events, and moving according to them. And the Manager would actually manage the user input triggering events, which finally would move the Elevator.

I'm not quite sure what structure I would use for the requests, but I would not use one for up and one for down, as that would change as the Elevator moves. I think I would either have an ordered list of sorts, or possibly a static list of all floors with booleans for which floor have been requested already.

Answer 2

I see a few things I would do differently. For one, I would not use a Queue. At best, it's providing you an order in which floor buttons were pressed. While the buttons may be pressed in any order, an elevator moves in sequential order be it up or down.

I would keep a simple List<bool> where true denotes a floor to stop at. Once you stop at a floor, no need to Dequeue. Instead just set the flag to false. By keeping it simple, you won't have to jump through hoops if you are going up but someone presses buttons for 10, 5, 2, 9, 7.