Simple taxi emulator

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
#include
#include

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
{
public:
//Basic function. Checks if which trips are finished
// and repairs broken cars
void DoMainProcessing();

~TaxiPark(void){};
TaxiPark(void){};

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

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

//Get all ID's
const std::vector GetAllCarID();
const std::vector 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 pass

• Use standard #include guards, not the non-standard #pragma once.

• Use standard data types, not DWORD.

• Change

this:

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:

enum CarStatus { NO_ERROR, CAR_IS_BUSY, DRIVER_IS_BUSY, NO_DRIVER_FOUND, CAR_IS_BROKEN };

• 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 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) {
        driver.update(time_difference);
    }

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

StackExchange Code Review Q#28745, answer score: 6