Ttdfjt

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;
 

Person + Derived Classes

• 
  accessibleFloors is a breach of Open–closed principle. Anyone is able to modiy 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();
 

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));