7
\$\begingroup\$

I have created an simple elevator design for 100 floors building with just one elevator.

I have dependency between classes which I want to eliminate, any thoughts/suggestion to make it more better in-terms of SOLID principles?

I have created following set of classes and detailed their purpose

class Request: This holds all the request made from the 'floor' keypad and inside the keypad. 'm_floor' represent the floor to which Elevator need to travel i.e. If floor keypad pressed from 7th floor then m_floor set as 7.

m_direction represent the DIrection in which lift need to travel after boarding. That means 0th floor can have Direction as 'UP' only and 100th floor can have direction 'DOWN' only

class RequestData: Holds all the previous request made and stored in down or up queue based on the intended direction to go.

...

#pragma once
#include<queue>
#include<memory>
#include<chrono>
#include<mutex>
#include <fstream>
#include <sstream>
#include <iostream>
#include <thread>

using namespace std::chrono;

std::mutex mu;
std::condition_variable cond;

constexpr int MAX_REQ_SIZE = 100;


enum class Direction {
    UP, DOWN
};

enum class State {
    MOVING, STOPPED
};

enum class Door {
    OPEN, CLOSED
};

class Request
{
public:
    milliseconds m_time;
    int m_floor;
    Direction m_direction;

    Request(const Request&) = default;
    Request& operator=(const Request&) = default;


    Request(milliseconds time, int floor, Direction direction)
    {
        m_time = time;
        m_floor = floor;
        m_direction = direction;
    }
};

using UNIQUE_REQ = std::shared_ptr<Request>;

struct UpComparator {
    bool operator()(UNIQUE_REQ r1, UNIQUE_REQ r2)
    {
        // return "true" if r1 floor value is less than r2
        return r1->m_floor < r2->m_floor;
    }
};

struct DownComparator {
    bool operator()(UNIQUE_REQ r1, UNIQUE_REQ r2)
    {
        // return "true" if r1 floor value greater than r2
        return r1->m_floor > r2->m_floor;
    }
};

class RequestData
{
public:

    void AddToDownQueu(UNIQUE_REQ req)
    {
        downQueue.emplace(req);
        if (down_request_time == milliseconds::zero())
        {
            down_request_time = req->m_time;
        }
    }

    void AddToUpQueu(UNIQUE_REQ req)
    {
        upQueue.emplace(req);
        if (up_request_time == milliseconds::zero())
        {
            up_request_time = req->m_time;
        }
    }

    milliseconds up_request_time{ milliseconds::zero() };
    milliseconds down_request_time{ milliseconds::zero() };
    std::priority_queue<UNIQUE_REQ, std::vector<UNIQUE_REQ>, UpComparator> upQueue;
    std::priority_queue<UNIQUE_REQ, std::vector<UNIQUE_REQ>, DownComparator> downQueue; 
};

class ElevatorStatus
{
public: 
    State m_elevator_state{ State::STOPPED };
    Door m_door_state{Door::CLOSED};
    Direction m_current_direction{ Direction::UP };
    std::queue<UNIQUE_REQ> m_current_queue;
    float m_current_location{ 0 };
};


class ElevatorProcessor
{
public:
    ElevatorProcessor() : m_request_data{ std::make_unique<RequestData>() },
        current_elevator_status{ std::make_unique<ElevatorStatus>() }
    {

    }

    void ProcessCurrentRequests()
    {
        while (true)
        {
            std::unique_lock<std::mutex> locker(mu);
            cond.wait(locker, [this]() {return (!current_elevator_status->m_current_queue.empty() ||
                !m_request_data->upQueue.empty() ||
                !m_request_data->downQueue.empty()); });

            if (!current_elevator_status->m_current_queue.empty())
            {
                auto r = current_elevator_status->m_current_queue.front();
                goToFloor(r->m_floor);

                current_elevator_status->m_current_queue.pop();
            }
            else
            {
                preProcessNextQueue();
            }

        }
    }

    std::unique_ptr<RequestData> m_request_data;
    std::unique_ptr<ElevatorStatus> current_elevator_status;

private:
    void goToFloor(int floor) {
        std::cout << "Navigating to the floor: " << floor << std::endl;
        current_elevator_status->m_elevator_state = State::MOVING;
        for (float i = current_elevator_status->m_current_location;
            i <= floor; i = (float)(i + 0.1))
        {
            std::this_thread::sleep_for(0.02s);
        }

        current_elevator_status->m_current_location = floor;
        current_elevator_status->m_door_state = Door::OPEN;
        current_elevator_status->m_elevator_state = State::STOPPED;

        std::this_thread::sleep_for(5s);

        current_elevator_status->m_door_state = Door::CLOSED;
    }

    void preProcessNextQueue()
    {
        if (m_request_data->up_request_time > m_request_data->down_request_time)
        {
            current_elevator_status->m_current_direction = Direction::UP;
            while (!m_request_data->upQueue.empty())
            {
                current_elevator_status->m_current_queue.push(m_request_data->upQueue.top());
                m_request_data->upQueue.pop();
            }
            m_request_data->up_request_time = milliseconds::zero();

        }
        else
        {
            current_elevator_status->m_current_direction = Direction::DOWN;
            while (!m_request_data->downQueue.empty())
            {
                current_elevator_status->m_current_queue.push(m_request_data->downQueue.top());
                m_request_data->downQueue.pop();
            }
            m_request_data->down_request_time = milliseconds::zero();
        }
    }
};

class ElevatorUser
{
public:

    ElevatorUser(std::shared_ptr<ElevatorProcessor> proc) :m_elevator_proc{ proc }
    {

    }

    void ReadInputData(const std::string filepath)
    {
        std::string line{};

        std::ifstream infile(filepath);
        while (std::getline(infile, line))
        {
            std::istringstream iss(line);
            int floor;
            std::string str_direction;
            if (!(iss >> floor >> str_direction)) { break; } // error
            Direction dir = (str_direction == "up") ? Direction::UP : Direction::DOWN;

            if (dir == Direction::UP)
            {
                std::cout << "Placing reuest for goin UP to floor: " << floor << std::endl;
            }
            else
            {
                std::cout << "Placing reuest for goin DOWN to floor: " << floor << std::endl;
            }
            call(floor, dir);
            std::this_thread::sleep_for(1s);
        }

    }
private: 
    void call(int to_floor, Direction direction)
    {
        std::unique_lock<std::mutex> locker(mu);
        cond.wait(locker, [this]() {return m_elevator_proc->current_elevator_status->m_current_queue.size() < MAX_REQ_SIZE; });

        milliseconds current_time = duration_cast<milliseconds>(
            system_clock::now().time_since_epoch());

        UNIQUE_REQ new_req = std::make_shared<Request>(current_time, to_floor, direction);


        if (direction == Direction::UP)
        {
            if (to_floor >= m_elevator_proc->current_elevator_status->m_current_location)
            {
                m_elevator_proc->current_elevator_status->m_current_queue.emplace(new_req);
            }
            else
            {
                m_elevator_proc->m_request_data->AddToUpQueu(new_req);
            }
        }
        else
        {
            if (to_floor <= m_elevator_proc->current_elevator_status->m_current_location)
            {
                m_elevator_proc->current_elevator_status->m_current_queue.emplace(new_req);
            }
            else
            {
                m_elevator_proc->m_request_data->AddToDownQueu(new_req);
            }
        }

        locker.unlock();
        cond.notify_one();

    }

    std::shared_ptr<ElevatorProcessor> m_elevator_proc;
};

int main(void)
{
    std::shared_ptr<ElevatorProcessor> elevator_proc = std::make_shared<ElevatorProcessor>();

    std::unique_ptr<ElevatorUser> elevator_user = std::make_unique<ElevatorUser>(elevator_proc);

    const std::string path = "D:\\hacker_input\\elevator_input.txt";
    std::thread t1(&ElevatorUser::ReadInputData, elevator_user.get(), path);

    std::thread t2(&ElevatorProcessor::ProcessCurrentRequests, elevator_proc.get());

    t1.join();
    t2.join();
    return 0;
}

...

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

I see some things that may help you improve your program.

Don't use #pragma once

The use of #pragma once is a common extension, but it's not in the standard and thus represents at least a potential portability problem. It's also completely unnecessary in a .cpp file. See SF.8

Use the required #includes

The code uses std::condition_variable which means that it should #include <condition_variable>. It was not difficult to infer, but it helps reviewers if the code is complete.

Use consistent formatting

The code as posted has inconsistent spacing for the #include files. Most people find the version with a space to be more easily readable. Pick a style and apply it consistently.

Don't hardcode file names

Generally, it's not a good idea to hardcode a file name in software, and generally especially bad if it's an absolute file name (as contrasted with one with a relative path). Instead, it would be better to allow the user of the program to specify the name, as with a command line parameter.

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Separate interface from implementation

It makes the code somewhat longer for a code review, but it's often very useful to separate the interface from the implementation. In C++, this is usually done by putting the interface into separate .h files and the corresponding implementation into .cpp files. It helps users (or reviewers) of the code see and understand the interface and hides implementation details. The other important reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file. In other words, split your existing .h file into a .h file and a .cpp file.

Fix spelling errors

The code has AddToUpQueu() instead of AddToUpQueue() and "Placing reuest for goin UP to floor: " instead of "Placing request for going UP to floor: ". These kinds of typos don't bother the compiler at all, but they will bother human readers of the code and human users of the code and make it a little more difficult to understand and maintain.

Gracefully exit the program

Right now, the program has no way to end. That's not a good design. Instead, consider some special values such as "9999 quit" that would signal to both threads that it's time to end.

Remember to lock all shared resources

There are calls to print on std::cout from both threads without any locks. This means that output could be interleaved. Instead, I'd recommend either using the C++20 std::osyncstream or create your own as in Multithreaded console-based monster battle with earliest-deadline-first scheduler

Don't abuse pointers

There is an abundance of std::shared_ptr and std::unique_ptr which is dubious. For example, there is no reason that the elevator_user in main shouldn't simply be this:

ElevatorUser elevator_user{elevator_proc};
std::thread t1(&ElevatorUser::ReadInputData, std::ref(elevator_user), path);

Similarly, within class ElevatorProcessor the member variables don't need to be std::unique_ptr. They should just be simple variables.

Rethink the class design

Having all data members of the ElevatorProcessor class as public should be a red flag that causes you to re-examine the class design. In real life an ElevatorUser has no view into the state of an elevator processor except indicators to show what floors the elevators are on. The user makes a request and it's up to the processor, not the user, to figure out how to act on that request. Also, why is there a separate ElevatorStatus class when this information could simply be put into the ElevatorProcessor class? Also, it seems to me that ElevatorProcessor could and should be named Elevator instead, unless you are planning to have multiple elevators being controlled by a single ElevatorProcessor.

Also, the way the code is currently written, the user is synchronized to the elevator and requests are made no more often than once every 5 seconds. That's an inaccurate use of threads. What should happen instead is that the ElevatorProcessor class should not sleep at all so that it can instantly respond to a button press, but keep track of where the elevator is at any instant of time.

\$\endgroup\$
1
  • \$\begingroup\$ Hi Edward, Thanks for detailed review. I will work on it and re-submit the code. \$\endgroup\$ Jun 25, 2020 at 16:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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