3
\$\begingroup\$

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

\$\endgroup\$
6
\$\begingroup\$

.. 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

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Simon Forsberg Feb 8 at 18:00
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – VLAZ Feb 9 at 10:43
  • \$\begingroup\$ @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. \$\endgroup\$ – Martin Frank Feb 11 at 5:23
  • \$\begingroup\$ Just out of curiousity, will that code still follow? \$\endgroup\$ – Simon Forsberg 2 days ago

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.