"Barbershop"-esque Semaphore Implementation

Recently, I submitted a project that simulates a barbershop-style problem; the task was to create a Hotel that contained guests, front-desk employees, and bellhops. Each of these were to be implemented with their respective Semaphores, with mutual exclusion being kept to a minimum if possible.

My code works as expected, but perhaps I could have used fewer semaphores.

```
package test;
import java.util.LinkedList;
import java.util.Queue;
import java.util.Random;
import java.util.concurrent.Semaphore;

public class Hotel implements Runnable{
/ Constants /
public static final int NUMGUESTS = 25;
public static final int NUMEMPLOYEES = 2;
public static final int NUMBELLHOP = 2;
/****/

/ Semaphores /
public static Semaphore custReady;
public static Semaphore custReqHelp;
public static Semaphore employee;
public static Semaphore leaveEmployee;
public static Semaphore bellHop;
public static Semaphore bagsDelivered;
public static Semaphore givenRoom;
public static Semaphore bagsDropped;
public static Semaphore mutex1;
public static Semaphore mutex2;
public static Semaphore mutex3;
public static Semaphore finished[];
public static Semaphore inRoom[];
/****/

/ Variables /
public static int empHelped[];
public static int hopHelped[];
public static int guestJoins;
public static Queue guestQueue;
public static Queue bagQueue;
public Thread hotel;
/****/

/ Instantiate all semaphores, arrays, and queues that are required /
public Hotel() {
custReady = new Semaphore(0, true);
custReqHelp = new Semaphore(0, true);
leaveEmployee = new Semaphore(0, true);
bagsDelivered = new Semaphore(0, true);
bagsDropped = new Semaphore(0, true);
mutex1 = new Semaphore(1, true);

You implemented your pieces as Runnable, which is a good first step.

public Guest(int num, Hotel hotel) {
    Random random = new Random();
    this.hotel = hotel;
    guestNum = num;
    numBags  = random.nextInt(MAXBAGS - MINBAGS + 1);

    System.out.println("Guest "  + guestNum + " created");
    guest = new Thread(this);
    guest.start();
}

First - boiler plate code should look like boiler plate code. In the constructor, when you are initializing state from constructor arguments, the name of the variable being set should match the name of the argument, unless there is some compelling reason to do otherwise. So the variable should be named num or guestNum in both places.

Second - that variable should be named guestId -- it's a unique identifier for the guest; num should represent a count of something.

Third - random numbers are hard to test; leave yourself a back door if you can. For instance:

for (int i = 0; i < NUMGUESTS; i++) {
    Random random = new Random();
    int numBags  = random.nextInt(MAXBAGS - MINBAGS + 1);
    new Guest(i, numBags, hotel);
}

Now, in your implementation, MAXBAGS and MINBAGS are scoped to Guest, so maybe those details shouldn't leak out. Consider then a static member function in Guest.

public static int getBagCount(Random random) {
    return random.nextInt(MAXBAGS-MINBAGS + 1)
}

Alternatively, you might implement a Factory to generate bag counts

class LuggageFactory {
    private final int maxBags;
    private final int minBags;

    // ...
    public int getBagCount(Random random) {
        return random.nextInt(maxBags - minBags + 1);
    }
}

Having a single class to test may make it easier to discover the bug when Guest.MINBAGS is not zero.

guest = new Thread(this);
    guest.start();

This is a bad idea at two levels. First, you shouldn't be doing this kind of work in the constructor. You'd be much better off to put it in the outer loop. Second, unless you have a really compelling reason to do otherwise, you should
be submitting the Runnable to an instance of an ExecutorService.

ExecutorService executor = // ...

for (int i = 0; i < NUMGUESTS; i++) {
    Random random = new Random();
    int numBags  = random.nextInt(MAXBAGS - MINBAGS + 1);
    executor.submit(new Guest(i, numBags, hotel));
}

Don't use static member variables where you can use instance member variables instead. You are passing a hotel to each of the actors in your system, so that already gives you a container to track everything. The hint that you should
really be doing this is the fact that you were initializing all of the static members in an instance constructor

public Hotel() {
    // These should be members of this Hotel instance, not static
    custReady     = new Semaphore(0, true);
    custReqHelp   = new Semaphore(0, true);
    // ...

Also, you should avoid having one object reach into the internals of another.

Hotel.mutex1.acquire();
        Hotel.guestQueue.add(this);
        Hotel.mutex1.release();

So this is telling us that there should be a method on hotel to note the arrival of Guests. Something like...

class Hotel  {
    public void add(Guest guest) {
        this.mutex1.acquire();
        this.guestQueue.add(guest);
        this.mutex1.release();
    }

All of the logic for coordinating the Semaphore for the hotel should be encapsulated within methods of the hotel itself.

StackExchange Code Review Q#109488, answer score: 5