This app is a simple taxi emulator. The only feedback I've got on it was "bad internal architecture", that's why I would be grateful to receive more adequate and humiliating review.

Simply-speaking, it should be a static lib, all the functionality must execute in the same thread (but! thread-safe). There should be a taxi station with cash desk, drivers and cabs. Each driver has its own car and own cash desk. One can re-assign the car to another driver. Car breaks after each 50 000 km. The DoMainProcessing function should wake every 2 seconds from outside, check if driver already took passenger to his place, fill in cash desk and possibly fix cars.

Full description

The weakest place in my code, I think, is DoMainProcessing. It just goes through all drivers and cars checking their condition. But I just had no better idea to do this in one thread.

Main class header:

#pragma once
#include "CashBox.h"
#include "Driver.h"
#include "Car.h"
#include <unordered_map>
#include <unordered_set>
#include <memory>

static const std::string DRIVER_CAR_LOCK = "DriverCar";

static const int NO_ERR = 0;
static const int CAR_IS_BUSY = 1;
static const int DRIVER_IS_BUSY = 2;
static const int NO_DRIVER_FOUND = 3;
static const int CAR_IS_BROKEN = 4; 

class TaxiPark
    //Basic function. Checks if which trips are finished
    // and repairs broken cars
    void DoMainProcessing();


    //Cars and drivers management
    int AddDriver(DWORD ID, std::shared_ptr<Car> car);
    int AddCar(DWORD ID);
    int ChangeCar(DWORD driverID, std::shared_ptr<Car> newCar);

    //Get driver/car info by ID
    const std::shared_ptr<Driver> GetDriverById(DWORD ID);
    const std::shared_ptr<Car> GetCarById(DWORD ID);

    //Get all ID's
    const std::vector<DWORD> GetAllCarID();
    const std::vector<DWORD> GetAllDriverID();

    //Try to set driver status as sick/on vacation if driver is not on trip
    int SetDriverStatus(DWORD driverID, bool onVacation, bool isSick);

    //Try to start trip for chosen passenger with chosen driver if it possible
    //(driver isn't already busy/on vacation/sick, car isn't broken)
    int TakeClients(Passenger client, DWORD driverID, DWORD distance, DWORD speed );    

    CashBox cashBox;

    std::unordered_map<DWORD, std::shared_ptr<Driver>> Drivers;
    std::unordered_map<DWORD, std::shared_ptr<Car>> Cars;

DoMainProcessing function:

void TaxiPark::DoMainProcessing()
    auto mut = CreateMutex(NULL, FALSE, DRIVER_CAR_LOCK.c_str());

    WaitForSingleObject(mut, INFINITE);
        std::for_each(Drivers.begin(), Drivers.end(), [this] (std::pair<DWORD, std::shared_ptr<Driver>> it)
            std::shared_ptr<Driver> driver(it.second);
            auto driverID = it.first;
            if (driver->IsTripFinished())
                Drivers.emplace(std::make_pair<DWORD,std::shared_ptr<Driver>>(driverID, driver));
        std::for_each(Cars.begin(), Cars.end(), [this] (std::pair<DWORD, std::shared_ptr<Car>> it)
            std::shared_ptr<Car> car(it.second);
            auto carID = it.first;
            if (car->IsBroken())
                if (cashBox.GetBalance() >= CAR_REPAIR_COST)
                Cars.emplace(std::make_pair<DWORD,std::shared_ptr<Car>>(carID, car));

Here's the full code listing.

P.S.: I know that unordered_map is a bad idea in this case. It gives no advantages in terms of speed, but requires exceed work in need of changing elements.

  • Use standard #include guards, not the non-standard #pragma once.
  • Use standard data types, not DWORD.
  • Change


static const int NO_ERR = 0;
static const int CAR_IS_BUSY = 1;
static const int DRIVER_IS_BUSY = 2;
static const int NO_DRIVER_FOUND = 3;
static const int CAR_IS_BROKEN = 4; 

into an enum:

  • DoMainProcessing is a horrible name and sounds like a function that does way too much. (Update: Okay, that's part of the exercise. Oh, well.)
  • Consider typedef std::shared_ptr<Car> CarPtr or something, to make the code less cluttered.
  • Avoid bool in interfaces, as it leads to poor readability and is error prone.

Instead of:

int SetDriverStatus(DWORD driverID, bool onVacation, bool isSick);

Use an enum indicator (or a string) instead:

enum DriverStatus { /* ... */ };
void SetDriverStatus(DriverID driver, DriverStatus newStatus)
    // ...

The function should be void. (If you are currently returning the old state: Stop doing that, and have a separate function for that. If you are returning a status number to indicate success or not: Use exceptions instead.)

  • You should probably use the C++ threading library instead of your CreateMutex. (Update: Again, your exercise hinders you from doing the right thing.)

  • Your use of for_each should be substituted with a range-based for loop.

  • The exercise states:

There must be some function, we call it DoMainProcessing, that periodically gets called by an external source using the emulator (for example, every second or two). It should detect when the driver made the carriage of the customer (depending distance and average speed) and is ready to perform the next one. Then, it should repair the broken car (subject of availability of sufficient number of funds).

The way I understand this, you should update the state of each of your cars and drivers in this function. You should probably record the time between each call, and add the distance traveled to a counter.

Very roughly something like this:

void TaxiPark::DoMainProcessing()
    // Use a real library for time, I'm just illustrating.
    Time now = getTime();
    TimeDiff time_difference = now - previous_processing_time;
    previous_processing_time = now;

    for (auto& driver : Drivers) {

    // Updates for cars ...

Then, in Driver::update(), you should increase distance traveled based on speed and time difference. If the distance is greater than or equal to the distance of the current ride, the ride is done. Do other state updates as necessary.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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