Object-oriented design implementation of an Elevator

Question

I am a newbie when it comes to Object Oriented Programming and I have read a lot of online articles and lessons so I want to try it out on my own. I've written a console app in C# and tried implementing Object Oriented Design and SOLID Principles.

The console app checks if a person has an access to a certain floor. Also, The app is in its initial stage so right now the Elevator accomodates a single person.

Although the app works. any feedback,comments, and suggestions will be greatly appreciated especially in terms of SOLID principles or OOP Design.

here is the code below

class Program
{
 static void Main(string[] args)
 {
 Person maintenanceEmployee = new MaintenanceEmployee();
 Elevator elev = new Elevator(maintenanceEmployee);
 elev.GoToFloor(Floors.FourthFloor);
 Person guest = new Guest();
 Elevator elev2 = new Elevator(guest);
 elev2.GoToFloor(Floors.SecondFloor);
 Person empfromthirdfloor = new EmployeeFromThirdFloor();
 Elevator elev3 = new Elevator(empfromthirdfloor);
 elev2.GoToFloor(Floors.ThirdFloor);
 Console.ReadLine();
 }
}
public enum Floors
{
 FirstFloor = 1,
 SecondFloor = 2,
 ThirdFloor = 3,
 FourthFloor = 4,
 FifthFloor = 5
}
public class Elevator
{
 Person person;
 public Elevator(Person _person)
 {
 this.person = _person;
 }
 public void GoToFloor(Floors _floor)
 {
 if (person.HasAccess(_floor))
 {
 Console.WriteLine("Elevating to " + _floor.ToString());
 }
 else
 {
 Console.WriteLine("No Access to " + _floor.ToString());
 }
 }
}
public abstract class Person
{
 public Floors[] accessibleFloors;
 public bool HasAccess(Floors _floor)
 {
 foreach (Floors i in accessibleFloors)
 {
 if (_floor == i)
 {
 return true;
 }
 }
 return false;
 }
}
public class MaintenanceEmployee : Person
{
 public Floors[] _accessibleFloors = { Floors.FirstFloor, Floors.SecondFloor, Floors.ThirdFloor, Floors.FourthFloor, Floors.FifthFloor };
 public MaintenanceEmployee()
 {
 //Not sure if this is the best practice
 //Reason why I did it like this is because
 //I need to change the arrays of accessibleFloors
 //in the abstract class "Person" in order to run successfully
 base.accessibleFloors = _accessibleFloors;
 }
}
public class EmployeeFromThirdFloor : Person
{
 public Floors[] _accessibleFloors = { Floors.ThirdFloor };
 public EmployeeFromThirdFloor()
 {
 //Not sure if this is the best practice
 //Reason why I did it like this is because
 //I need to change the arrays of accessibleFloors
 //of the abstract class "Person" in order to run successfully
 base.accessibleFloors = _accessibleFloors;
 }
}
public class Guest : Person
{
 public Floors[] _accessibleFloors = { Floors.SecondFloor };
 public Guest()
 {
 //Not sure if this is the best practice
 //Reason why I did it like this is because
 //I need to change the arrays of accessibleFloors
 //of the abstract class "Person" in order to run successfully
 base.accessibleFloors = _accessibleFloors;
 }
}

• dfhwze: Is it safe to assume an elevator can at most contain 1 person?
• t3chb0t: Please add a description about what you application does. Elevator Sample is too general.
• joseMana: @dfwze well for now this is just its initial design, at this stage I just like to know if the coding is on track in terms of OOP or SOLID.

Answer 1

If I may add a single consideration to dfhwze's thorough answer:

Having an enum defining floors is OK for a building with 5 floors, but would be overwhelming if we are talking Burj Khalifa with its 163 floors. Besides that it is a rather rigid concept having a predefined number of floors - supposed you want to reuse your object model for other buildings.

Therefore I would create a Floor class too:

public class Floor
{
 private Floor(string name, int index)
 {
 Name = name;
 Index = index;
 }
 public string Name { get; }
 public int Index { get; }
 public static IEnumerable<Floor> CreateFloors(params string[] names)
 {
 return names.Select((n, i) => new Floor(n, i));
 }
}

Answer 2

Person + Derived Classes

• accessibleFloors is a breach of Open–closed principle. Anyone is able to modify this collection.
• accessibleFloors could be considered a breach of Single responsibility principle. Consider having a Role class and Person storing his Roles set. This way, you can also mitigate redundant code and remove the need of derived classes as EmployeeFromThirdFloor, Guest and MaintenanceEmployee (in this trivial example these classes are not required).
• accessibleFloors can be null. The code is not able to handle this situation.
• HasAccess is way to convoluted: accessibleFloors.Contains(_floor); would do it.

public abstract class Person
{
 public Floors[] accessibleFloors;
 public bool HasAccess(Floors _floor)
 {
 foreach (Floors i in accessibleFloors)
 {
 if (_floor == i)
 {
 return true;
 }
 }
 return false;
 }
}

We can create a class Role.

public class Role 
{
 public HashSet<Floors> AccessibleFloors { get; }
 public Role (HashSet<Floors> accessibleFloors) 
 {
 AccessibleFloors = accessibleFloors 
 ?? throw new ArgumentNullException(nameof(accessibleFloors));
 }
 public bool HasAccess(Floor floor)
 { 
 return AccessibleFloors.Contains(floor);
 }
 // override equals and gethashcode ..
}

We can refactor Person.

public class Person
{
 public HashSet<Role> Roles { get; }
 public Person(HashSet roles) {
 Roles = roles ?? throw new ArgumentNullException(nameof(roles));
 }
 public bool HasAccess(Floors floor)
 {
 return Roles.Any(r => r.HasAccess(floor));
 }
}

Because of Inheritance, the derived classes do not require to create their own instance variable _accessibleFloors.

base.accessibleFloors = _accessibleFloors;

---

Elevator

• There is tight-coupling between an Elevator and a Person. You force the elevator to exist only with a person. What about an empty elevator?
• GoToFloor is not usable code. Consider returning a Boolean and putting the Console.WriteLine in the calling code.
• The code cannot handle when Person is null.

public class Elevator
{
 Person person;
 public Elevator(Person _person)
 {
 this.person = _person;
 }
 public void GoToFloor(Floors _floor)
 {
 if (person.HasAccess(_floor))
 {
 Console.WriteLine("Elevating to " + _floor.ToString());
 }
 else
 {
 Console.WriteLine("No Access to " + _floor.ToString());
 }
 }
}

Elevator could be refactored as follows.

public class Elevator
{
 public Person Person { get; private set; }
 public bool IsFull => Person != null;
 public bool IsEmpty => Person == null;
 public bool Enter(Person person)
 {
 if (person == null) throw new ArgumentNullException(nameof(person));
 if (IsFull) return false;
 Person = person;
 return true;
 }
 public bool Exit(Floors floor)
 {
 if (IsEmpty) return false;
 if (!Person.HasAccess(floor)) return false;
 Person = null;
 return true;
 }
}

---

Program

class Program
{
 static void Main(string[] args)
 {
 var guestRoles = new HashSet<Role> { 
 new Role(new HashSet<Floors> { Floors.FirstFloor }) 
 };
 var guest = new Person(guestRoles);
 var elevator = new Elevator();
 
 if (elevator.Enter(guest)) 
 {
 Console.WriteLine("guest has entered elevator");
 }
 else 
 {
 Console.WriteLine("guest cannot enter elevator: elevator is full");
 }
 if (elevator.Exit(Floors.FirstFloor)) 
 {
 Console.WriteLine("guest has exited elevator at FirstFloor");
 }
 else 
 {
 Console.WriteLine("guest cannot exit elevator at FirstFloor: access is denied");
 }
 Console.ReadLine();
 }
}

• joseMana: wow, I appreciate the remodelling and refactoring of the code. Thank you.

Answer 3

To model Elevator better, you may have following two options:

1. Singleton class Elevator which has existence independent of Person.
2. Rename Elevator to ElevatorTrip. This way ElevatorTrip can exist without Person.