4
\$\begingroup\$

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:
    //Adds request to the queue
    void addRequest(int);
    //Gets the total levels the box can move
    int getLevel();
    //For Emergency
    void setEmergency();
    void unsetEmergency();
};

class liftbox{
private:
    dailpad pad;
    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
    void addRequest(int);
};


class dailpad{
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
        box.addRequest(x);
    }
};

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:

    //Adds request to the queue
    void addRequest(int FloorNumber){

    //Add the request to the queue. The single button outside the elevator door
    mLiftBoxCtrlUnit.addRequest(FloorNumber);

    }

    //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
    void addRequest(int x){
        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:

    //The dailpad inside the bank
    dailpad & mpad;

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

    //Dailpad Listener
    void dailpadListener(){

    }


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

    }

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

    friend class liftboxControlUnit;
};


class dailpad{

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

    void renderDisplay();

public:

    //Constructor
    dailpad();

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

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

    friend class Bank;
};
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Martin York Nov 1 '11 at 11:57
5
\$\begingroup\$

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;

Traditional all caps is reserved for macros. Break the tradition at your own cost.

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.

\$\endgroup\$
  • \$\begingroup\$ 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 ! \$\endgroup\$ – bsoundra Nov 1 '11 at 19:49
  • \$\begingroup\$ @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. \$\endgroup\$ – Martin York Nov 1 '11 at 19:52
  • \$\begingroup\$ Thanks . I too was editing parallelly. I will this in mind the next time. \$\endgroup\$ – bsoundra Nov 1 '11 at 19:58
  • \$\begingroup\$ 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 ? \$\endgroup\$ – bsoundra Nov 1 '11 at 20:12
  • \$\begingroup\$ When you say a pattern , are you expecting a design pattern ? \$\endgroup\$ – bsoundra Nov 1 '11 at 20:18
2
\$\begingroup\$

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.
\$\endgroup\$
0
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – bsoundra Nov 1 '11 at 4:30

protected by Jamal 2 days ago

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

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.