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

• Simon Forsberg: I have a feeling that using a class for StraightPlayer, ChangePlayer and ShowMaster will be a bit overkill but it will be interesting to see what you come up with there. You have some other very good points here though.
• VLAZ: Not sure if it's worth bringing up but the simulation has a flaw in my eyes - an entirely new set of doors is created every time. That's not a code problem but a logic problem because it doesn't accurately reflect changing the choice. Given how it works right now, it seems better if it ran over the same door combinations first by changing choices, second without changing choices. Then you would get a more precise win percentage. Although both cases should be fairly close in what they report.
• Martin Frank: @SimonForsberg that missing OO-Concept was my very reason to delete that question. After i postet my code i saw all these missing concepts, but i'm still glad you encouraged me to keep the question online.
• Simon Forsberg: Just out of curiousity, will that code still follow?