4
\$\begingroup\$

I have taken your inputs on my original question and formed a new implementation of the Monty Hall Simulation:

Again keep an eye on CleanCode and OOP

The Price to be won:

enum Price {
    GOAT, CAR
}

The Door keeping that price in secret

class Door {

    private final Price price;

    Door(Price price){
        this.price = price;
    }

    Price getPrice() {
        return price;
    }
}

the Player who participates in an Monty Hall Game

class Player {

    private final boolean preferresChange;
    private int winCounter;

    Player(boolean preferresChange){
        this.preferresChange = preferresChange;
    }

    boolean preferresChange(){
        return preferresChange;
    }

    void increaseWinCounter() {
        winCounter = winCounter + 1;
    }

    int getWinCounter(){
        return winCounter;
    }
}

the MontyHall in which the player takes action

class MontyHall {

    private final List<Door> doors;
    private Door selected;
    private Door openOne;
    private final Random random;
    private final int amountOfDoors;

    MontyHall(int seed, int amount) {
        random = new Random(seed);
        amountOfDoors = amount;
        doors = IntStream.range(0, amountOfDoors).
            mapToObj(e -> new Door(Price.GOAT)).
            collect(Collectors.toList());
        doors.set(random.nextInt(amountOfDoors), new Door(Price.CAR));
    }

    void chooseDoor(){
        selected = doors.get(random.nextInt(amountOfDoors));
    }

    void openDoor(){
        openOne = doors.stream().
            filter(d -> !d.equals(selected) && Price.GOAT == d.getPrice()).
            collect(Collectors.toList()).
            get(0);
    }

    void changeDoor(){
        selected = doors.stream().
            filter(d -> !d.equals(openOne) && !d.equals(selected)).
            collect(Collectors.toList()).
            get(0);
    }

    Price getPrice(){
        return selected.getPrice();
    }

}

and the simulator who hosts the games

class Simulator {

    public static void main(String[] args) {
        Simulator simulator = new Simulator();
        simulator.simulate(10000, 3);
    }

    private void simulate(int iterations, int amountDoors) {
        Player changer = new Player(true);
        Player stayer = new Player(false);

        Random random = new Random();
        for (int i = 0; i < iterations; i ++){
            int seed = random.nextInt();
            playOneRound(changer, seed, amountDoors);
            playOneRound(stayer, seed, amountDoors);
        }
        System.out.println("changer: "+changer.getWinCounter());
        System.out.println("stayer : "+stayer.getWinCounter());
    }

    private void playOneRound(Player player, int seed, int amountDoors) {
        playOneRound(player, new MontyHall(seed, amountDoors));
    }

    private void playOneRound(Player player, MontyHall montyHall) {
        montyHall.chooseDoor();
        montyHall.openDoor();
        if (player.preferresChange()){
            montyHall.changeDoor();
        }
        if (Price.CAR == montyHall.getPrice()){
            player.increaseWinCounter();
        }
    }

}
\$\endgroup\$
  • \$\begingroup\$ damn i missed that Flag-part totally :-/ \$\endgroup\$ – Martin Frank Feb 11 at 11:02
  • 1
    \$\begingroup\$ When you change the 3 into a 10, the probabilities don't change much. But they should. The changer should now have a winning probability of 90%. \$\endgroup\$ – Roland Illig Feb 11 at 11:03
  • \$\begingroup\$ @RolandIllig i didn't undertand the problem fully, monty removes not n-2 doors in my simulation but always 1 door - this leads to a different behaviour... maybe we'll skipt that part and have a closer look at OOP and CleanCode (i'll add a proper implementation soon, where that flaw would be fixed) \$\endgroup\$ – Martin Frank Feb 11 at 11:12
  • \$\begingroup\$ @RolandIllig it should be named then opendDoors() - note the plural s ^^ \$\endgroup\$ – Martin Frank Feb 11 at 11:14
  • 1
    \$\begingroup\$ @Theoriok Thank you for pointing that out - one important thing in SW-Development is to name things properly! (i'm from Germany - bit still no excuse ^^) \$\endgroup\$ – Martin Frank Feb 12 at 9:25
4
\$\begingroup\$

Positive Feedback

I liked how you follow the rules of Clean Code, that you write small methods and small classes and give all readable names!

You've introduced your own data types that make the code more readable. The MontyHall-Algorithm can now be read like a poem:

montyHall.chooseDoor();
montyHall.openDoor();
if (player.preferresChange()){
   montyHall.changeDoor();
}
if (Price.CAR == montyHall.getPrice()){
   player.increaseWinCounter();
}

And all your methods have only one intention level what makes them so easy to understand.

Some Criticism

Class Names Refer to Unexpected Behavior

I read on Wikipedia:

You [the player] pick a door, say No. 1, and the host, who knows what's behind the doors, opens another door, say No. 3

After I read your code I was a little bit confused at some places. But not because of the quality of your code, which is for me very high, but because of the quote above.

In your game the Player can't pick a Door. Instead the Player gets picked a door by MontyHall: (montyHall.chooseDoor()).

Additional to that the Player has a winCounter, but actually the Simulation should track how often a Player wins the MontyHall.

No Outcome

What would you expect your main to do without any knowledge about our code base?

public static void main(String[] args) {
    Simulator simulator = new Simulator();
    simulator.simulate(10000, 3);
}

It could run something like a game loop or start a GUI to interact with but instead it writes data to the console.

Much cleaner would be:

public static void main(String[] args) {
    Simulator simulator = new Simulator();
    simulator.printSimulations(10000, 3);
}

But now imagine you want to compare a "Changer" and a "Stayer". You can't do it because you have no outcome! You through your outcome away by printing it to the console instead of storing and returning it.

public static void main(String[] args) {
    Simulator simulator = new Simulator();
    SimulationResult result = simulator.simulate(10000, 3);
    int average = result.calculateAverageOfStayer();
    System.out.println(average);
}

Feature Envy

A method accesses the data of another object more than its own data.

The method playOneRound in playOneRound is in my eyes a feature envy. All the tasks should be done by MontyHall. So the method playOneRound should look like:

private void playOneRound(Player player, MontyHall montyHall) {
    montyHall.playGameWith(player);
}

Use Methods instead of Getters and Operations

In your code I can find code structure like x.getValue == something, but much better to read would be x.isSomething()

Some inspiration for you:

Price.GOAT == d.getPrice() means door.isHidingGoat()

Price.CAR == montyHall.getPrice() means montyHall.isWon()

!d.equals(selected) means door.notEquals(selected)


In your older post @Simon Forsberg mentioned:

I have a feeling that using a class for StraightPlayer, ChangePlayer and ShowMaster will be a bit overkill.

I know you still have the flag argument in Player. But that's fine. In general, refactoring can be an overkill at all. And you can see that for this little problem you can divide the logic into so many different classes and methods. When you write code, you have to find a balance between all OOP/clean code and your task.

Thanks for this cool problem and that you let me review it. I learned a lot while read your code :]

\$\endgroup\$
  • \$\begingroup\$ even though i thought this implementation is now much better (ok, it IS much better) you still found so many issues - and i am very glad to learn from such an advanced code reviewer! your Answer helped really a lot!! thanks for your input and your kindness! \$\endgroup\$ – Martin Frank Feb 12 at 6:40
  • \$\begingroup\$ i don't think there will be a further follow up question, if i would implement those features from your answer the code would be fine then! thanks again! \$\endgroup\$ – Martin Frank Feb 12 at 6:41
2
\$\begingroup\$
enum Price {

Is this what you meant? Why is it a price? Price for what? This would make much more sense if it were a prize.

        montyHall.chooseDoor();
        montyHall.openDoor();
        if (player.preferresChange()){
            montyHall.changeDoor();
        }

This seems incorrect. Monty doesn't choose the door; the player does. Monty opens the door. The player chooses and potentially changes that choice. And preferresChange is misspelled. It should be prefersChange.

In the simulation, Let's Make a Deal is the name of the show. Monty Hall is the host. So the class that you are calling MontyHall should be named LetsMakeADeal or simply GameShow.

You are hard coding some things. For example, your simulation is the classic Marilyn vos Savant one where Monty always opens a door and never opens the door with a car. That is a strategy. You should probably code that as a particular implementation. There are other possible strategies that will produce different results. For example, what if Monty only ever offers the choice when the player has selected the door with the car behind it? What if Monty can open the door and display the car? These strategies will change the results.

\$\endgroup\$
  • \$\begingroup\$ you are very right, i had a lot olf probolems with the translation as you can see... i thought 'MontyHallis the game! Now i learned from you the game is called Let's make a Deal` that makes the whole simulator kind of awkward. \$\endgroup\$ – Martin Frank Feb 13 at 5:21
  • \$\begingroup\$ on the issue with having no proper interception between player and door - i was aware of that but i'm still struggling with it. I can clearly see your point and Roman (see answer above) pointed that issue out as well... i still don't know how to improve the code. can you give me a direction into how one could do it? \$\endgroup\$ – Martin Frank Feb 13 at 5:24
  • \$\begingroup\$ As mentioned as comment on my question from Theoriok, it is priZe not PriCe again my english language skill played a trick on me. \$\endgroup\$ – Martin Frank Feb 13 at 5:26

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.