patterncppModerate

Vehicle-crossing simulation

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

vehiclecrossingsimulation

Problem

It seems like this code should be shorter, but with all the error checking it is long and hard to follow. This is for a simulation of vehicles crossing a bridge, and this part is dealing with mutexes. I tried extracting a function that is passed the mutex and locks/unlocks it and prints any errors, but when I did that it broke the code (it's been my experience on this site to not include any broken code so I won't elaborate on what I tried).

/*global variables*/
pthread_mutex_t Bridge = PTHREAD_MUTEX_INITIALIZER;
pthread_mutex_t okToOutput = PTHREAD_MUTEX_INITIALIZER;
/*...*/
/*this is the function that gets passed to pthread_create()*/
void* startSim(void *arg)
{

    Vehichle* inData = (Vehichle*)arg;
    if(usleep(inData->sleepT1))
        cerr licencePlate licencePlate direction) licencePlate direction) sleepT2))
        cerr licencePlate licencePlate direction) << endl;
    if(pthread_mutex_unlock(&okToOutput))
        cerr << "failed to unlock output" << endl;
    if(pthread_mutex_unlock(&Bridge))
        cerr << "failed to lock bridge" << endl;

    return NULL;
}

My compiler doesn't properly support C++11 so no smart pointers etc.

Solution

As mentioned in the comments, there's not much to review.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Factor out common code

In this case, the code gets a lock, does something, and releases the lock. That could easily be isolated to a common function:

class Locker
{
     pthread_mutex_t&   mutex;
     bool               ok;
   public:
     operator bool() const {return ok;}

     Locker(pthread_mutex_t& mutex)
         : mutex(mutex)
         , ok(true)
     {
         if(pthread_mutex_lock(&mutex) != 0) {
             cerr licencePlate direction) << endl;       
    }
}

To do this correctly, a failure to lock should exit the routine and not attempt to send output data.

Avoid C-style casts

Instead of (Vehicle *)arg use static_cast(arg). It makes much more clear to humans reading your code what you're doing.

Code Snippets

class Locker
{
     pthread_mutex_t&   mutex;
     bool               ok;
   public:
     operator bool() const {return ok;}

     Locker(pthread_mutex_t& mutex)
         : mutex(mutex)
         , ok(true)
     {
         if(pthread_mutex_lock(&mutex) != 0) {
             cerr << "failed to lock output" << endl;
             ok = false;
         }
     }
     ~Locker()
     {
         if (ok) {
             if(pthread_mutex_unlock(&mutex) != 0) {
                 cerr << "failed to unlock output" << endl;
             }
         }
     }
};          
void report(Vehicle *v, const char *msg)
{
    Locker    lock(okToOutput);
    if (lock) {
        cout << "Vehicle " << v->licencePlate << msg << dirToStr(v->direction) << endl;       
    }
}

Context

StackExchange Code Review Q#68040, answer score: 10

Revisions (0)

No revisions yet.