# OOP elevator design evaluation

I am not so much used to OOP designing, So I took one of the very commonly used interview question , "Designing the Elevator System". Below is the prototype.

I would like to get some feedback on the design like

• Best practices which will have a particular aspect
• Flexibility in the code
• Some important generic feature missing
• Areas to concentrate to write better code and design

I know that these kind of things have no finite answer or just one answer but I would like to know my required areas of improvement.

### Original Design:

class elevator{
private:
//The Lift box the elevator controls
liftbox box;
//The total number of levels
const int LEVEL;
//The request for various floors
set<int> req;
//Triggers the search to move to the next floor if required
void moveLiftToNext();
//Instructs the lift box to move to that floor
void moveLiftTo(int);
public:
//Gets the total levels the box can move
int getLevel();
//For Emergency
void setEmergency();
void unsetEmergency();
};

class liftbox{
private:
elevator ele;
int currLevel; //Current position
bool direction; //True - up ; False- Down
public:
//Instruction to move the box to certain level
void Move(int x){
if(x < 0 || x >= ele.getLevel())
return; //invalid number
//Move the lift
//update direction and currLevel
}
//returns the current level. Used by Elevator
int getCurrLevel();
//returns the current direction of movement.Used by Elevator
int getDirection();
void setEmergency(){
//Do the required
ele.setEmergency();
}
void unsetEmergency(){
ele.unsetEmergency();
}
//can passed to the elevator
};

private:
//Some DS to represent the dailpad
//Lift box is belongs to
liftbox box;
public:
void recieveCommand(int x){
//Depending on the value we can do the following
box.setEmergency();
//or
box.unsetEmergency();
//or
}
};


### Updated Design:

class elevator{
private:
//The Lift box the elevator controls
liftboxControlUnit & mLiftBoxCtrlUnit;

//constructor
elevator(int Level=1, int NoOfBanks =1 );

//Destructor
~elevator();

//Triggers the search to move to the next floor if required
void moveLiftToNext();

public:

//Add the request to the queue. The single button outside the elevator door

}

//For Emergency. Should be accessible to everyone !
void setEmergency();
void unsetEmergency();

};

typedef enum Direction{
UP,
DOWN
}direction;

class liftboxControlUnit{
private:

//The request for various floors
set<int> mRequestQueue;

//The various banks for the whole system
vector<Bank> mBanks;

//The total number of levels. Remains the same for one building
const int mTotalLevel;

//Instruction to move the box to certain level
void processRequest(){

//Do the logic to move the box.

}

//can passed to the elevator
mRequestQueue.insert(x);
}

//Can be set by elevator class
void setEmergency(){
//Do the required
//Set Emergency on all Banks
}

void unsetEmergency(){
//UnsetEmegency on all banks
}

void emergencyListener(){
//Listen to all the banks if emergency has been set
}

void BankFreeListener(){
//Listen to the banks if any is free

//If so then
processRequest();
}

public:
//Constructor
liftboxControlUnit(int TotalLevels, int NoOfBanks): mTotalLevel(TotalLevels){
for(int i=0 ; i lessthan NoOfBanks; ++ i)
mBanks.push_back(Bank(0,UP));
}
friend class elevator;
};

class Bank{
private:

//Current Location
int mPresentLevel;

//Current direction of movement
direction mDirection;

//Currently moving
bool mEngaged;

//Manipulate the bank
void move(int NoOfMoves){
setEngaged();

//Move the elevator

unsetEngaged();
}

//getters
int getPresentLevel() const;
int getDirection() const;

//setters
void setPresentLevel(int);
void setDirection(direction);

//Manipulate the engaged flag
bool isEngaged() const;
bool setEngaged();
bool unsetEngaged();

//For emergency
void reset();

}

public:
Bank(int StartingLevel, direction Direction): mPresentLevel(StartingLevel),
mDirection(Direction),
mEngaged(false),
{

}

//For emergency . Should be available for all.
void SetEmergency();
void UnsetEmergency();
bool isEmergency();

friend class liftboxControlUnit;
};

private:
//Some DS to represent the state . probably a 2D Array.

void renderDisplay();

public:

//Constructor

void getCommand(int x){
//Depending on the value we can do the following

//Make necessary changes to the display
renderDisplay();
}

friend class Bank;
};

• Before I comment/look at the code. I would like to point out that the elevator design interview question is more about design than actual coding. It is designed to see if you understand how different parts in a complex system can interact with each other. – Martin York Nov 1 '11 at 11:57

### First the C++ code:

class elevator{
liftbox box;

class liftbox{
elevator ele;


This is not going to work as you expect it.
Each class contains an object of the other type means that both will have completely different objects. If you want them to refer back to each other in some sort of parent child relationship then one of them needs to be a reference or pointer to the parent.

const int LEVEL;


int getCurrLevel();
int getDirection();


Methods that return information about the state of the object without actually changing the state should be marked const.

public/private


I am not sure I agree with some of the decisions about making methods public. I think very few objects should get to interact with these systems sometimes friendship can help in limitting access. (Now friendship increases coupling with the friend, but if it decreases the external public interface it will decrease coupling with objects that have no rights to modify the object). Anyway I would expect to see a justification as to why methods are public. If anybody should be able to call them fine. If nobody but another object should call them you need to make a better case.

### Design

Looking at it from a design perspective.

Not sure I see the distinction between an elevator/liftbox

In big buildings some lifts do not go to all floors.

One of the things about elevators is that they usually come in banks and do not operate independently. For really big building multiple banks will be combined but will work independently (unless there is some major emergency). How are you going to organize your code so that multiple lifts(sorry elevators) can work together. Also I want to see how you can decouple the elevator object from bank control logic. (ie I don't expect to see all the control logic in the bank, I would like to see control logic in the elevator but decision logic in bank). But if their are multiple banks to coordinate I want the logic for a higher level control.

The real meat of this problem is how to de-couple the objects from each other. What patterns do you think are being used here.

## Updated Code:

Not sure you understand a bank of elevators work (based on the code).

A bank is a group of 2 or more elevators that work together to serve a set of floors.

I still do not see the pattern you are using to decouple the elevators from the Bank.

• Thank you for the feedback. I have updated the design . Please give your comments on the modifications please. About the first comment true, the design is screwed ! Is there a prototype or a standard way to establish a parent - child relationship . Could you provide an example ? I have performed some decoupling , Let me know how better I can do it ! – bsoundra Nov 1 '11 at 19:49
• @bsoundra: Don;t edit your file like that. If you change things drastically then peoples answer do not match up to the question and it becomes hard to vote on them. If you have major modifications edit your question and ADD a new section with the updated design. – Martin York Nov 1 '11 at 19:52
• Thanks . I too was editing parallelly. I will this in mind the next time. – bsoundra Nov 1 '11 at 19:58
• I did not understand the bank concept prior. In the current Design - elevator - the whole system, liftboxControlUnit - the controller, Bank - each lift box. The bank concept can be incorporated by adding the resrictions as a part of the Bank (i.e. the lift box). Would it be a bad idea ? Considering this design idea, is there any place that needs decoupling ? – bsoundra Nov 1 '11 at 20:12
• When you say a pattern , are you expecting a design pattern ? – bsoundra Nov 1 '11 at 20:18

I agree with the points made by Loki Astari. I would like to add a few.

1. Instead of the liftbox asking the elevator for the total number of levels, the lift box could be given the total number in it's constructor (that is, if the liftbox is to be created by the elevator).
2. It's not a major point, but the use of a boolean to specify the direction isn't the easiest to read. I would be tempted to replace it with an enum that would allow the options of up or down. This would be much easier to read and would remove the need for the comment to explain it.
3. With regards to naming of variables, I would strongly advise you use meaningful names, and try to avoid abbreviations where possible. If someone was reading over this code and it was in a larger scale program, reading a reference to ele might be misinterpreted depending on the context of the rest of the code being read. ele probably isn't the best of examples, particularly for this question, but it's something that should be considered for future programs.
4. Lastly, similarly to point 3, I would recommend differentiating between your local and global variables. For example, in some coding standards, programmers prefix their global variables with an underscore (e.g. _currLevel). I write my global variables in PasCal case, prefixed with the 'm' character, to signify that they are member variables (e.g. mCurrLevel). It is good to try and adopt similar naming conventions, as it really helps in making code easier to read.
• Dial pads usually also reflect back the set of requests which are already registered (which makes those buttons light-up). This means there could be way to telling all button pads - once activity happens on outside dial pads as well.

• Number of requests are always more than one - usually so it deserves a sorted link list.

• the levels:
const int LEVEL; : this should have been a property of Box rather than elevator.

• in general, if you are referring to dial pad for inside the elevator pad. For both it is better that it is abstracted as a child of elevator rather than lift box.

• where do you put the list of destinations upward and down-word - and who manages the algorithm to follow?

Overall this is good.

• Few justifications. First comment: Yes, I gave it a thought but some reason left it. Can have a displaymethod to manipulate the display. Second, I had a set of ints, the ints went missing due to formatting. Sets will take the same space as Linked list but also faster than Linked list . Also it gives faster computation of the next level to go after , it services the current level. Third, height is constant factor for only the building but the same liftbox can be placed in any building. Fourth, Could you please explain more ? Fifth, the logic will be in elevator::moveLiftToNext() . Thankyou – bsoundra Nov 1 '11 at 4:30

## protected by Jamal♦Nov 11 at 6:59

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).