Problem
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<int, bool> _sorted = new SortedList<int, bool>();
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 < floor)
MoveUp();
else
MoveDown();
break;
case Status.GoingUp:
MoveUp();
break;
default:
break;
}
}
public void MoveUp()
{
var max = _sorted.OrderByDescending(x => x.Key).First();
for (var i = CurrentFloor; i <= 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 => x.Key).First();
for (var i = CurrentFloor; i >= 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);
}
}
Solution
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 => 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 < 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
does logic make sense?
Not yet. It could be better.
private readonly SortedList<int, bool> _sorted = new SortedList<int, bool>();
You use a SortedList only to change the ordering twice anyway!
_sorted.OrderByDescending(x => x.Key).First(); _sorted.OrderBy(x => x.Key).First();
In this case you might as well be using just a list but there will never be more then one item in this collection because the AddRequest method has an invalid name and should actually be called MoveToFloor. It adds the target floor to the list and at the end Stop removes it from the list so its Count is always <= 1. You are ordering a list that can only ever have at most one item.
Your elevator does not support multiple button presses because as soon as you press the first one it moves to the target floor immediately not allowing to add more requests and start moving after a few seconds. It’s not asynchronous so you could just use an int instead of a list.
Does not use TopFloor.
Does not check the floor is in a valid range.
Does not use the bool in the SortedList. Why even use SortList? You are not using access by index. You could just have an array with bool for floor request.
The queues is not allowed to build up – it just goes to the floor. You have logic about stopping on floors that really does nothing.
This is all your call actually does:
public void AddRequest(int floor)
{
CurrentFloor = floor;
Status = Status.Stopped;
}
If your queue did really work like a queue. You decide the top/bottom floor and don’t allow for a new top/bottom during the trip.
You need the elevator operating on a separate thread. Start with something like this:
System.Collections.Concurrent.ConcurrentBag<int> floors = new System.Collections.Concurrent.ConcurrentBag<int>();
Task t = Task.Factory.StartNew(() => {
// Just loop.
for(int i = 0; i < 100; i++)
{
Thread.Sleep(1000);
Console.Write(floors.Count);
}
});
for (int i = 0; i < 100; i++)
{
Thread.Sleep(1000);
floors.Add(i);
}
t.Wait();
More complete. Needs some OO work but it works like an elevator. It looks for the next floor every time. When it does not find a next then it reverses direction. The ConcurrentDictionary is thread safe. ConcurrentBag does not like to remove.
public class Elevator
{
Random rand = new Random();
System.Collections.Concurrent.ConcurrentDictionary<int, int> floors = new System.Collections.Concurrent.ConcurrentDictionary<int, int>();
public int TopFloor { get; }
public bool UpDown { get; private set; }
public int Floor { get; private set; } = 1;
public int? NextFloor { get; private set; } = null;
public void RequestFloor (int requestFloor)
{
if (requestFloor < 1 || requestFloor > TopFloor)
throw new IndexOutOfRangeException();
floors[requestFloor]++;
}
private void RunElevator()
{
Task t = Task.Factory.StartNew(() =>
{
for (int i = 0; i < 1000; i++)
{
Thread.Sleep(3000);
Console.WriteLine("");
if (floors.Count > 0)
{
Console.WriteLine("On floor {0}", Floor);
bool haveDestination = false;
if (UpDown)
{
for (int f = Floor + 1; f <= TopFloor; f++)
{
if(floors[f] > 0)
{
NextFloor = f;
haveDestination = true;
break;
}
}
}
else
{
for (int f = Floor - 1; f >= 1; f--)
{
if (floors[f] > 0)
{
NextFloor = f;
haveDestination = true;
break;
}
}
}
if(haveDestination)
{
floors[(int)NextFloor] = 0; //empty the floor
Console.WriteLine("Next floor {0}", NextFloor);
Floor = (int)NextFloor;
}
else
{
Console.WriteLine("Queue is clear");
NextFloor = null;
UpDown = !UpDown;
}
}
}
});
}
public void TestElevator()
{
for (int i = 0; i <= 1000; i++)
{
Thread.Sleep(2000);
RequestFloor(rand.Next(1, TopFloor + 1));
}
}
public Elevator(int topFloor = 20)
{
if (topFloor <= 1 || topFloor > 100)
throw new IndexOutOfRangeException();
TopFloor = topFloor;
for (int i = 1; i <= TopFloor; i++)
{
floors.TryAdd(i, 0);
}
RunElevator();
TestElevator();
}
}