1
\$\begingroup\$

I started out creating a simple game in an Object Oriented way to practice.

The rules are pretty simple

  • There are two players
  • You let one player choose if they want Heads or Tails.
  • You assign the leftover option to the other player.
  • You simulate a coin flip and say who won that game.

More than the logic, naming conventions etc. I was looking at the most object-oriented way of structuring code to implement this from this exercise.

I created three classes for this, the Player, Coin and CoinGame and an enum as well.

The Player Class

enum CoinOptions
{
    Heads, Tails
}

internal class Player
{
    public string playerName { get; set; }
    public CoinOptions coinOption { get; set; }
    public Player(string name)
    {
        playerName = name;
    }
    public CoinOptions chooseCoinOption()
    {

        //Simulate the user picking one option

        //Console.WriteLine("Chose a coin option");
        //Console.ReadKey();

        //Lets Assume they pick Heads 
        coinOption = CoinOptions.Heads;

        return coinOption;
    }
    public void setCoinOption(CoinOptions opponent)
    {
        coinOption = opponent == CoinOptions.Heads ? CoinOptions.Tails : CoinOptions.Heads;
    }
    public bool didPlayerWin(CoinOptions winningFlip)
    {
        return winningFlip == coinOption;
    }
}

Coin class

class Coin
{
    private Random random;
    public Coin(Random rand)
    {
        random = rand;
    }
    public CoinOptions flipCoin()
    {

        double probability = random.NextDouble();
        return probability > 0.5 ? CoinOptions.Heads : CoinOptions.Tails;
    }
}

CoinGame Class

internal class CoinGame
{
    private List<Player> players = new List<Player>(Enumerable.Repeat(default(Player), 2));
    private Coin coin;
    private CoinOptions winningFlip;

    public CoinGame(Coin coinUsed, List<string> playerNames = null)
    {
        for (int i = 0; i < players.Count; i++)
        {
            var playerName = playerNames != null ? playerNames[i] : $"player{i}";
            players[i] = new Player(playerName);
        }
        coin = coinUsed;
    }

    public void playCoinFlip()
    {
        int chooseIndex = new Random().Next(2);
        int applyIndex = chooseIndex == 0 ? 1 : 0;
        var selectedOption = players[chooseIndex].chooseCoinOption();
        players[applyIndex].setCoinOption(selectedOption);
        winningFlip = coin.flipCoin();
    }
    public void gameResult()
    {
        foreach (var player in players)
        {
            if(player.didPlayerWin(winningFlip))
            {
                Console.WriteLine($"The player {player.playerName} won choosing {player.coinOption}");
            }
            else
            {
                Console.WriteLine($"The player {player.playerName} lost choosing {player.coinOption}");
            }
        }
    }

}

And the calling code

class Program
{
    static void Main(string[] args)
    {
        var coin = new Coin(new Random(12312));
        var coinGame = new CoinGame(coin);
        coinGame.playCoinFlip();
        coinGame.gameResult();
        Console.ReadKey();
    }
}

My main concern is if there is a better strategy to

  • Initialize the players in the game
  • Allowing a random user to choose their coin option while simultaneously setting the other user with the leftover option
\$\endgroup\$
6
\$\begingroup\$

Smurf naming convention

Link

internal class Player
{
    public string playerName { get; set; }

playerName is redundant since it's a preoperty of the Player class. Name is sufficient.


Assigning a coin option to a player

There are a few issues here, it's hard to separately address them.

Firstly, you're violating SRP by making your Player class responsible for UI interactions:

public CoinOptions chooseCoinOption()
{
    //Console.WriteLine("Chose a coin option");
    //Console.ReadKey();

    return coinOption;
}

I see where your confusion comes from. "The player chooses the side of the coin" is a correct statement. But, and this is an important distinction, this is not equivalent to "the Player class should handle the UI logic for choosing the side of the coin". The second statement is incorrect as it breaks SRP.

The Player class needs to be told what its option is (= an external caller sets the value). The decision logic for deciding which value to set is not part of the Player class' responsibility. It shouldn't care why or how a particular side was chosen, it should only care which side was chosen.

chooseCoinOption() needs to be removed.

Secondly, the setCoinOption(CoinOptions opponent) method's intention really irks me.

public void setCoinOption(CoinOptions opponent)
{
    coinOption = opponent == CoinOptions.Heads ? CoinOptions.Tails : CoinOptions.Heads;
}

As an aside, opponent is a really bad name for this variable. When reading your code I initially assumed opponent to be of the Player type. A better name for your parameter would've been opponentCoinOption.

You're adding in a lot of custom logic that does not simplify the code. Anyone who calls this method needs to know exactly what it does (= pick the other option), which means that it's a meaningless encapsulation. Calling the method does not meaningfully simplify the external caller's responsibility.

When removing the opponent parameter, you are left with a default set method. And since the property already has a public setter:

public CoinOptions coinOption { get; set; }

There is no point to also having a setter method. You need to remove one of the setters. Which one you remove is up to you. I prefer just using the default set; in this case.


Checking for victory

public bool didPlayerWin(CoinOptions winningFlip)
{
    return winningFlip == coinOption;
}

This method doesn't really provide anything. It's a wrapper around an equality check, and the value you're comparing (coinOption) is already publically accessible.

I'd remove this method altogether.

Secondly, I just want to point out that the Player class should not be determining what a Win is. A win is defined by the rules of the game, and the Player does not define the rules of the game. This is again violating SRP.

As a simple example, imagine if the rules of the game were changed so that you lose the game when your option is called. Suddenly, your method name conflicts with the rules of the game, which proves the point that your Player class somehow depends on the rules of the game you're playing, which means you're leaking your abstractions.

I know this is pedantic, but if you had named this method HasCoinOptionEqualTo(CoinOptions option), I would not have considered this an SRP vioaltion, because you are no longer checking for a win, you are simply checking an equality (which coincidentally just happens to be the win condition of the game you're playing).


Minor comments

 private List<Player> players = new List<Player>(Enumerable.Repeat(default(Player), 2));

I don't see you having defined a default for Player anywhere, which means that you're simply creating a list with two null values. What is the benefit of doing so? I suggest simply doing new List<Player>() and adding the players at a later stage.

However, I would take it further and do away with the list altogether. You will always have two players, and you're going to have to create logic to always retrieve the correct player from the list. It's much easier (believe me, I've built a few two player games before) if you just keep these as two separate properties:

private Player player1;
private Player player2;

class Coin { ... }

I see no added purpose to making an object out of Coin. This can be a simple static class:

public static class Coin
{
    private static Random random = new Random();

    public static CoinOptions Flip()
    {
        // ...
    }
}

internal class CoinGame
{
    public void playCoinFlip() { ... }

    public void gameResult() { ... }
}

Similar to mentioned before, don't mix your game logic with your UI logic. gameResult() should not be in here.

Instead, playCoinFlip() should return the outcome of the game (in whatever form you choose), and then the UI logic can choose to display the outcome however it wants.

This ties into how your main function works:

coinGame.playCoinFlip();
coinGame.gameResult();

Doing it this way requires the main function to call different methods in a particular order, which again requires the main method to know how the CoinGame works internally, thus infringing on your encapsulation.

It would be much cleaner if you could do something like:

Player winner = coinGame.playGame();

Console.WriteLine($"{winner.Name} won the coin flip!");

public void playCoinFlip()
{
    int chooseIndex = new Random().Next(2);
    int applyIndex = chooseIndex == 0 ? 1 : 0;
    var selectedOption = players[chooseIndex].chooseCoinOption();
    players[applyIndex].setCoinOption(selectedOption);
    winningFlip = coin.flipCoin();
}

What I find very weird is that you let the external caller (your main function) choose the names of the players, but you don't allow the external caller to choose the coin option that each player chooses.

I understand that since either choice is a 50/50, it doesn't really matter here. But if that is the case, then Player shouldn't even need a CoinOptions to begin with.

I would reshuffle this so your CoinGame receives two players, rather than two names. This gives your external caller to choose the coin options.

public CoinGame(Player p1, Player p2)
{
    player1 = p1;
    player2 = p2;
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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