Elevator Interview Problem OOP - Revised Elevator program code challenge, revised:

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

internal enum Status
{
    GoingUp,
    GoingDown,
    Stopped
}

internal class Elevator
{
    private readonly SortedList&lt;int, bool&gt; _sorted = new SortedList&lt;int, bool&gt;();
    public readonly int TopFloor;

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

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

    public void AddRequest(int floor)
    {
        if (!_sorted.ContainsKey(floor))
            _sorted.Add(floor, false);

        switch (Status) {
            case Status.GoingDown:
                MoveDown();
                break;

            case Status.Stopped:
                if (CurrentFloor &lt; floor)
                    MoveUp();
                else
                    MoveDown();
                break;

            case Status.GoingUp:
                MoveUp();
                break;

            default:
                break;
        }
    }

    public void MoveUp()
    {
        var max = _sorted.OrderByDescending(x =&gt; x.Key).First();
        for (var i = CurrentFloor; i &lt;= max.Key; i++) // Go to top most requested floor
            if (_sorted.ContainsKey(i))
                Stop(i);
            else
                continue;

        Status = Status.Stopped;
    }

    public void MoveDown()
    {
        var min = _sorted.OrderBy(x =&gt; x.Key).First();
        for (var i = CurrentFloor; i &gt;= min.Key; i--)
            if (_sorted.ContainsKey(i))
                Stop(i);
            else
                continue;

        Status = Status.Stopped;
    }

    public void Stop(int floor)
    {
        _sorted.Remove(floor);
        CurrentFloor = floor;
        Console.WriteLine("Stopped at: {0}", CurrentFloor);
    }
}

internal class Manager
{
    private readonly Elevator _elevator = new Elevator(10);

    public void ButtonPressed(int floor)
    {
         // testing...
        _elevator.AddRequest(10);
        _elevator.AddRequest(10);
        _elevator.AddRequest(5);
        _elevator.AddRequest(2);
        _elevator.AddRequest(9);
        _elevator.AddRequest(6);
    }
}
Elevator program code challenge, revised:


where is that?



_sorted is a very very bad name. I have to read all the code to figure out exactly what that is.




public void AddRequest(int floor)



Rename the parameter to something like requestedFloor
Rename the method to something like CallElevator or RequestFloor



Does not account for which floor a call is coming from. I think it would matter if the elevator passing 2nd floor for the 7th, then a request comes for the 5th - originating from the 1st floor.

Looks like the code throws away requests for floors already in the list. But if I'm on the 1st floor asking for the 5th, why ignore my request just because the elevator is carrying someone to 5 already? 



Fuzzy code

It is hard to tell exactly what the elevator logic is. And harder to tell if it meets requirements.


var min = _sorted.OrderBy(x =&gt; x.Key).First();



Clever but not clear. What is the elevator doing? What is the floor-stop choosing logic? This is "the bowels of the code" exposed at the highest logic level. I should be reading what the elevator is doing, not googling MSDN.




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



Why are these public? In principle a well made OO class hides state and exposes functionality.




case Status.Stopped:    
    if (CurrentFloor &lt; floor)
        MoveUp();
    else
        MoveDown();
    break;



Flawed logic. If the floor (requested floor?) is the same as the current floor, the elevator goes down. Even if this code works is not the point. I should not have to use a debugger to know if the code covers edge cases.

P.S. what happens when we're on the 1st floor? Does it still go down?



Two collections makes sense: a queue of floor requests and an ordered list of floors the elevator is traversing. This fundamentally decouples two things that should not be conflated, but are so in this code.

Edit

Decisions on fundamental data structures strongly influences higher level design. In this case I think floor decision/prioritzation functionality would more likely end up in it's own class(es) - decoupling the decision logic from the elevator's movement. 

This decoupling would make it easier to fix any issues due to incomplete/vague requirements. With the current code, things would break and change everywhere.

end Edit