2
\$\begingroup\$

(See also the next iteration.)

I have this small simulation program simulating a toilet seat. The toilet seat may be in two positions: up or down. If a male arrives to the toilet to urinate, and notices that it is down, he must put it upwards. What we are measuring, are two strategies:

  1. Each visitor leaves the seat in the same position as they used it.
  2. Each visitor puts the seat down.

My code follows:

package com.github.coderodde.simulation.toiletseat;

import java.util.Objects;
import java.util.Random;

public final class ToiletSeatSimulator {

    private static final int MINIMUM_QUEUE_LENGTH = 1;

    private static enum Gender {
        FEMALE,
        MALE,
    }

    private static enum Operation {
        URINATE,
        POOP,
    }

    private static enum ToiletSeatPosition {
        UP,
        DOWN,
    }

    private final int queueLength;
    private final double femailProportion;
    private final double urinationProportion;
    private final boolean closeDown;
    private final Random random;
    private ToiletSeatPosition toiletSeatPosition;
    private int seatsMoved = 0;

    public ToiletSeatSimulator(int queueLength, 
                               double femaileProportion, 
                               double urinationProportion,
                               boolean closeDown,
                               Random random) {
        this.queueLength = checkQueueLength(queueLength);
        this.femailProportion = checkFemaleProportion(femaileProportion);
        this.urinationProportion = 
                checkUrinationProportion(urinationProportion);

        this.closeDown = closeDown;
        this.random = Objects.requireNonNull(random, "The Random is null.");
        this.toiletSeatPosition = ToiletSeatPosition.DOWN;
    }

    public int simulate() {
        for (int i = 0; i < queueLength; i++) {
            performOperation(getRandomGender(), 
                             getRandomOperation());
        }

        return seatsMoved;
    }

    private void performOperation(Gender gender, Operation operation) {
        switch (operation) {
            case URINATE:
                performUrination(gender);
                break;

            case POOP:
                performPoop(gender);
                break;

            default:
                throw new IllegalStateException(
                        "Unkwnown Gender enum: " + gender);
        }

        if (closeDown && toiletSeatPosition == ToiletSeatPosition.UP) {
            toiletSeatPosition = ToiletSeatPosition.DOWN;
            seatsMoved++;
        }
    }

    private void performUrination(Gender gender) {
        switch (gender) {
            case FEMALE:
                performFemaleUrination();
                break;

            case MALE:
                performMaleUrination();
                break;

            default:
                throw new IllegalStateException(
                        "Unknown Gender enum: " + gender);
        }
    }

    private void performFemaleUrination() {
        if (toiletSeatPosition == ToiletSeatPosition.UP) {
            toiletSeatPosition = ToiletSeatPosition.DOWN;
            seatsMoved++;
        }
    }

    private void performMaleUrination() {
        if (toiletSeatPosition == ToiletSeatPosition.DOWN) {
            toiletSeatPosition = ToiletSeatPosition.UP;
            seatsMoved++;
        }
    }

    private void performPoop(Gender gender) {
        switch (gender) {
            case FEMALE:
                performFemalePoop();
                break;

            case MALE:
                performMalePoop();
                break;

            default:
                throw new IllegalStateException(
                        "Unknown Gender enum: " + gender);
        }
    }

    private void performFemalePoop() {
        if (toiletSeatPosition == ToiletSeatPosition.UP) {
            toiletSeatPosition = ToiletSeatPosition.DOWN;
            seatsMoved++;
        }
    }

    private void performMalePoop() {
        if (toiletSeatPosition == ToiletSeatPosition.UP) {
            toiletSeatPosition = ToiletSeatPosition.DOWN;
            seatsMoved++;
        }
    }

    private Gender getRandomGender() {
        double coin = random.nextDouble(); // In the range [0, 1).
        return coin < femailProportion ? Gender.FEMALE : Gender.MALE;
    }

    private Operation getRandomOperation() {
        double coin = random.nextDouble();
        return coin < urinationProportion ? Operation.URINATE : Operation.POOP;
    }

    private static int checkQueueLength(int queueLength) {
        if (queueLength < MINIMUM_QUEUE_LENGTH) {
            throw new IllegalArgumentException(
                    "Queue length is too small: " 
                            + queueLength 
                            + ". Must be at most " 
                            + MINIMUM_QUEUE_LENGTH
                            + ".");
        }

        return queueLength;
    }

    private static double checkFemaleProportion(double femaleProportion) {
        if (Double.isInfinite(femaleProportion)) {
            throw new IllegalArgumentException(
                    "Female proportion is infinite in absolute value: " 
                            + femaleProportion);
        }

        if (Double.isNaN(femaleProportion)) {
            throw new IllegalArgumentException("Female proportion is NaN.");
        }

        if (femaleProportion < 0.0 || femaleProportion > 1.0) {
            throw new IllegalArgumentException(
                    "Female proportion is out of bounds: "
                            + femaleProportion
                            + ". Must be within closed range [0,1].");
        }

        return femaleProportion;
    }

    private static double checkUrinationProportion(double urinationProportion) {
        if (Double.isInfinite(urinationProportion)) {
            throw new IllegalArgumentException(
                    "Urination proportion is infinite in absolute value: " 
                            + urinationProportion);
        }

        if (Double.isNaN(urinationProportion)) {
            throw new IllegalArgumentException("Urination proportion is NaN.");
        }

        if (urinationProportion < 0.0 || urinationProportion > 1.0) {
            throw new IllegalArgumentException(
                    "Urination proportion is out of bounds: "
                            + urinationProportion
                            + ". Must be within closed range [0,1].");
        }

        return urinationProportion;
    }
}

The demo follows:

package com.github.coderodde.simulation.toiletseat;

import java.util.Random;

public final class Demo {

    private static final int QUEUE_LENGTH = 1000;
    private static final double FEMALE_PROPORTION = 0.55;
    private static final double URINATION_PROPORTION = 0.2;

    public static void main(String[] args) {
        long seed = System.currentTimeMillis();
        System.out.println("<<< Seed = " + seed + ">>>");

        Random random1 = new Random(seed);
        Random random2 = new Random(seed);

        ToiletSeatSimulator simulator1 = 
                new ToiletSeatSimulator(
                        QUEUE_LENGTH, 
                        FEMALE_PROPORTION,
                        URINATION_PROPORTION,
                        false,
                        random1);

        System.out.println(
                "Number of seat moves when changing seat position " 
                        + "on demand: " 
                        + simulator1.simulate());

        ToiletSeatSimulator simulator2 = 
                new ToiletSeatSimulator(
                        QUEUE_LENGTH, 
                        FEMALE_PROPORTION,
                        URINATION_PROPORTION,
                        true,
                        random2);

        System.out.println(
                "Number of seat moves when changing seat position back to " 
                        + "closed: " 
                        + simulator2.simulate());
    }
}

The typical output is:

<<< Seed = 1666884294029>>>
Number of seat moves when changing seat position on demand: 146
Number of seat moves when changing seat position back to closed: 162

Critique request

As always, I would like to hear any comments. Especially, I doubt the correctness of my implementation, since the above figures are rather optimistic to me.

\$\endgroup\$
3
  • \$\begingroup\$ Why ToiletSeatSimulator:: performOperation throws "Unkwnown Gender enum: " + gender? \$\endgroup\$
    – vnp
    Commented Oct 29, 2022 at 4:47
  • \$\begingroup\$ @vnp It’s supposed to report the Gender. Not quite sure but I expect it to print “null” if Gender is null, or print the newly added enum like “DIVERSE” when a new Gender enum is added. \$\endgroup\$
    – coderodde
    Commented Oct 29, 2022 at 5:05
  • \$\begingroup\$ For what I see, it shall report an illegal operation. If I use the booth to vape, why you'd deny me a gender? \$\endgroup\$
    – vnp
    Commented Oct 30, 2022 at 4:04

2 Answers 2

1
\$\begingroup\$

Normally I encourage people to write more functions rather than fewer, but in this case I think it needs to come back the other way.

I would change a few of the variable names, but then we all have our own style and preferences. For instance, the overall context of "Toilet" can be given once in the name of the class and then we know what sort of seat it is and can just use Seat rather than ToiletSeat.

If you just specify the range that is valid for the doubles, you don't need to check for infinites and NaN, as those aren't in the range!

You can leave out the default branch of the switches - that way the compiler can alert you if you update the instances of the enum and forget to specify how they get handled in the switch.

GENERALLY speaking, when I use switch, I like to just state what we're going to do in the case, and then return. I find it simpler to not have to worry about code that might run after or setting state that gets checked later in the same method (increase cyclomatic complexity). Sometimes that means duplicating some code and that's okay.

You should set the starting seat position and movement count at the start of simulate so it isn't affected by previous runs.

We only need the total number of iterations in the simulate method, so we can take it as an argument there.

I think it's fine NOT to introduce more object oriented / polymorphic stuff, as it's just not needed here and not likely (?) to be needed in the future.

import java.util.Objects;
import java.util.Random;

public final class ToiletSeatSim {
    private final double femaleRatio;
    private final double peeRatio;
    private final boolean alwaysLeaveSeatDown;
    private final Random random;

    private SeatPosition seatPosition;
    private int movements = 0;

    public ToiletSeatSim(Random random, double femaleRatio,
            double peeRatio, boolean alwaysLeaveSeatDown) {
        if (femaleRatio < 0.0 || femaleRatio > 1.0) {
            throw new IllegalArgumentException("Female ratio is out of bounds: "
                    + femaleRatio + ". Must be within closed range [0,1].");
        }

        if (peeRatio < 0.0 || peeRatio > 1.0) {
            throw new IllegalArgumentException("Pee ratio is out of bounds: " + peeRatio
                    + ". Must be within closed range [0,1].");
        }

        this.random = Objects.requireNonNull(random, "The Random is null.");
        this.femaleRatio = femaleRatio;
        this.peeRatio = peeRatio;
        this.alwaysLeaveSeatDown = alwaysLeaveSeatDown;
    }

    public int simulate(int iterations) {
        seatPosition = SeatPosition.DOWN;
        movements = 0;
        for (int i = 0; i < iterations; i++) {
            Gender gender = getRandomGender();
            Operation operation = getRandomOperation();
            performOperation(operation, gender);
        }
        return movements;
    }
    
    private static enum SeatPosition {
        UP,
        DOWN,
    }

    private void setSeatPosition(SeatPosition position) {
        if (this.seatPosition == position) {
            return;
        }
        this.seatPosition = position;
        movements++;
    }

    private static enum Gender {
        FEMALE,
        MALE,
    }
    
    private Gender getRandomGender() {
        return random.nextDouble() < femaleRatio ? Gender.FEMALE : Gender.MALE;
    }

    private static enum Operation {
        PEE,
        POOP,
    }
    
    private void performOperation(Operation operation, Gender gender) {
        switch (operation) {
        case POOP:
            setSeatPosition(SeatPosition.DOWN);
            return;
        case PEE:
            switch (gender) {
            case FEMALE:
                setSeatPosition(SeatPosition.DOWN);
                return;
            case MALE:
                setSeatPosition(SeatPosition.UP);
                if (alwaysLeaveSeatDown) {
                    setSeatPosition(SeatPosition.DOWN);
                }
                return;
            }
        }
    }
    
    private Operation getRandomOperation() {
        return random.nextDouble() < peeRatio ? Operation.PEE : Operation.POOP;
    }
}
\$\endgroup\$
1
\$\begingroup\$

The main problem here are hardcoded enums which make you write a switch statement, list all the possible variants and then write a default condition that will never occur. What if you want to add in more genders or more fun toilet activities? Each of these switches will have to be expanded with more boilerplate.

You can avoid this partially by using polymorphism.

Add an abstract method performOperation(Operation operation) to your Gender enum and override it in genders.

public enum Gender {
    FEMALE {
        @Override
        void performOperation(Operation operation) {
            // ...
        };
    },
    MALE {
        @Override
        void performOperation(Operation operation) {
            // ...
        };
    };

    abstract void performOperation(Operation operation);
}

Now you can just write gender.performOperation(...) and avoid gender switch statements. Sadly it's impossible to avoid branching entirely since the number of possible gender-operation combinations is the product of their numbers, so you can't do the same trick with operations.


Some nitpicks:

All the check... functions are redundant since your class doesn't get any data from outside so the checks can never actually fail. There is no sense in generating a number between 0 and 1 and then checking if it's under 0 since you just generated it to be at least 0.

femailProportion - typo

throw new IllegalStateException("Unkwnown Gender enum: " + gender); - this is repeated in switch (operation).

long seed = System.currentTimeMillis(); - seeds generated in such way don't change much between launches for obvious reasons. This is not a big problem in your case though.

In general the code is very well written!


As for the results, they seem to match data from my hidden cameras pretty accurately, so the implementation is probably correct.

\$\endgroup\$
1
  • \$\begingroup\$ ”more fun toilet activities...” You, Sir, are being naughty. 😁 \$\endgroup\$
    – coderodde
    Commented Oct 28, 2022 at 4:03

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.