Clean Code / OOP on Monty Hall Simulation Implementation

Question

can you review the this code for the Monty Hall simulation, mainly on the aspects of OOP and CleanCode

public static void main(String[] args) {
    final int iterations = 10000;
    System.out.println("chance with    change = " + getChanceForPlay(iterations, true));
    System.out.println("chance without change = " + getChanceForPlay(iterations, false));
}

private static double getChanceForPlay(int iterations, boolean doChange) {
    double win = 0;
    for (int i = 0; i < iterations; i++) {
        List<Price> doors = createDoors();
        int selectedDoor = random.nextInt(3); //the player selects one of three doors

        //let's remove one goat (but not the selected door)
        for (int currentDoor = 0; currentDoor < 3; currentDoor++) {
            if (currentDoor != selectedDoor && doors.get(currentDoor) == Price.GOAT) {
                doors.remove(currentDoor);

                //re-Index, if required
                selectedDoor = currentDoor < selectedDoor? selectedDoor-1:selectedDoor;
                break;
            }
        }

        //player changes door
        if (doChange) {
            selectedDoor = selectedDoor == 0 ? 1 : 0;
        }

        if (doors.get(selectedDoor) == Price.CAR) {
            win++;
        }
    }
    return win / iterations;
}

private static List<Price> createDoors() {
    List<Price> doors = new ArrayList<>();
    doors.add(Price.GOAT);
    doors.add(Price.GOAT);
    doors.add(Price.CAR);
    Collections.shuffle(doors);
    return doors;
}

private enum Price {
    GOAT, CAR
}

I know many other people made the same thing. It's more of an academical question...

some thoughs upon it done by myself:

• should i break up the code more? maybe one method for removing one door by the showmaster?
• usage of integer for doors - i think that's not so fancy...
• overusage of brackets
• too much comments (tell don't ask)
• too complicated (brain overload)

Edit:

there exists an follow up question on how to do it better

Answer 1

.. review .. mainly on the aspects of OOP and CleanCode

To Some of Your Thoughts

usage of integer for doors - i think that's not so fancy...

Additional to a Door class we could add classes for Player, ShowMaster, and MontyHall too.

should I break up the code more? maybe one method for removing one door by the showmaster?

This job is be done when we split up the code with classes into different logical units.

too much comments (tell don't ask)

Comments are add all not bad but when you group your code in a method you all ready see semi-independent logic. In Robert C. Martins words: “Don’t Use a Comment When You Can Use a Function or a Variable”

Review

Bad news first.. I see no oop at all in your code.. An indicator for non-oop code is the heavy use off static and no own data-types. With other words: You write a huge procedural algorithm.

Find Objects

My first step would be to wrap this method into a class called MontyHall and give it a public method caluclateChance. I would avoid the get-prefix because it is not a getter.

After that, we can search for cohesion and abstractions. Your comments helps us very well!

The comment //the player selects one of three doors shows us that we could create a Player. After that the ShowMaster wants //.. remove one goat (but not the selected door). Than our //player changes door if he wants to.

We should think again about a Door. Currently, the doors are represented as a list of integers. But in the domain of Monty Hill, a door is something that hides a price and has a number.

class Door {
    private int number;
    private Price price;

    // all-args constructor..

    // hashcode and equals-method..
}

Code Smell

Magic Number

The magic number 3 appears two times in your code:

• random.nextInt(3)
• if (..., currentDoor < 3; ..)

The 3 represents the number of doors and can be replaced by int NUMBER_OF_DOORS = doors.size(), or you can hide it in an object, which would be the preferred way.

Flag Argument

The method getChanceForPlay takes as second argument a Boolean value, which creates uses if (doChange) { /*...*/} to create a branch in your method. The downside of this flag is that we need to test each branch when writing a unit test (but ok.. in this we have only on possible branch).

This flag has a logical cohesion to a Player. Let's say a Player is an abstract data type and there are two possible types of players: StraightPlayer and ChangePlayer. With polymorphism we can pass both players and call player.chooseDoor. While the StraightPlayer chooses the same door as before and ChangePlayer changes his/her selection.

Code

// will follow