1
\$\begingroup\$

The status of a game can be:

  • running, when it is not over yet,
  • tie, when over but no player won,
  • win, when over and a certain player won.

Being able to check equality of statuses is desired.

With these in mind, I coded a GameStatus class which implements IEquatable:

class GameStatus : IEquatable<GameStatus> {
    public static GameStatus Running() => new GameStatus() { IsOver = false };
    public static GameStatus Tie() => new GameStatus() { IsOver = true, IsTie = true };
    public static GameStatus Win(Player winner) =>
        new GameStatus() { IsOver = true, IsTie = false, Winner = winner };

    public bool IsOver { get; private set; }
    public bool IsTie { get; private set; }
    public Player Winner { get; private set; }

    public override bool Equals(object obj) => Equals(obj as GameStatus);
    public bool Equals(GameStatus gs) {
        return gs != null ?
            (IsOver == gs.IsOver) && (IsTie == gs.IsTie) && (Winner == gs.Winner) :
            false;
    }

    public override int GetHashCode() => HashHelper.GetHashCode(IsOver, IsTie, Winner);

    public static bool operator ==(GameStatus arg1, GameStatus arg2) =>
        ReferenceEquals(arg1, null) ? ReferenceEquals(arg2, null) : arg1.Equals(arg2);
    public static bool operator !=(GameStatus arg1, GameStatus arg2) => !(arg1 == arg2);

    private GameStatus() { }
}

using the Player enumeration and the HashHelper class:

enum Player {
    Red,
    Black
};

static class HashHelper {
    public static int GetHashCode<T1, T2, T3>(T1 arg1, T2 arg2, T3 arg3) {
        unchecked {
            return 103 * arg1.GetHashCode() + 31 * arg2.GetHashCode() + arg3.GetHashCode();
        }
    }
}

A GameStatus can be compared to the Running, Tie, Win(winner) immutables, or with other statuses. Overriding the Equals(), GetHashCode() and the == != operators assures that every comparison should be correct.


However, there was something missing.

According to the business logic, there is no point of asking whether status is tie, when the game is running. Similarly there is no point asking for the winner when the game is running or a tie. So the IsTie and Winner properties had to be modified:

class GameStatusStrict : IEquatable<GameStatusStrict> {
    public static GameStatusStrict Running() => new GameStatusStrict() { _isOver = false };
    public static GameStatusStrict Tie() => new GameStatusStrict() { _isOver = true, _isTie = true };
    public static GameStatusStrict Win(Player winner) =>
        new GameStatusStrict() { _isOver = true, _isTie = false, _winner = winner };

    public bool IsOver => _isOver;
    public bool IsTie {
        get {
            Checker.Check(nameof(IsOver), IsOver, true);
            return _isTie;
        }
    }
    public Player Winner {
        get {
            Checker.Check(nameof(IsOver), IsOver, true);
            Checker.Check(nameof(IsTie), IsTie, false);
            return _winner;
        }
    }

    public override bool Equals(object obj) => Equals(obj as GameStatusStrict);
    public bool Equals(GameStatusStrict gs) {
        return gs != null ?
            (_isOver == gs._isOver) && (_isTie == gs._isTie) && (_winner == gs._winner) :
            false;
    }

    public override int GetHashCode() => HashHelper.GetHashCode(_isTie, _isTie, _winner);

    public static bool operator ==(GameStatusStrict arg1, GameStatusStrict arg2) =>
        ReferenceEquals(arg1, null) ? ReferenceEquals(arg2, null) : arg1.Equals(arg2);
    public static bool operator !=(GameStatusStrict arg1, GameStatusStrict arg2) => !(arg1 == arg2);

    private bool _isOver;
    private bool _isTie;
    private Player _winner;

    private GameStatusStrict() { }
}

The Checker class:

static class Checker {
    public static void Check(string varName, bool value, bool expectedValue) {
        if (value != expectedValue)
            throw new InvalidOperationException(string.Format("{0} should be {1}.", varName, expectedValue));
    }
}

My own remarks:

  • Is GameStatusStrict doing too much (i.e. over-engineering) by checking on properties?
  • Any suggestions for the names of Checker class and Check()?
  • The methods Equals(object), Equals(GameStatus), GetHashCode(), operator ==() and operator !=() seem to be pretty much boilerplate that will appear in many classes. The only changing code is the logic "use all three attributes IsOver, IsTie, Winner and my class type".
  • GameStatus and GameStatusStrict classes seem to be simple enough so as not to need unit testing. Do you agree?
\$\endgroup\$
  • 3
    \$\begingroup\$ Looks too complicated and overengineered at a 1st glance IMO. \$\endgroup\$ – πάντα ῥεῖ Apr 20 '17 at 18:44
  • \$\begingroup\$ (I added one bullet in the end of the question) Along with the general review, a comparative review between both classes (simplicity vs more checking) would be good. \$\endgroup\$ – Manolis Apr 20 '17 at 18:59
2
\$\begingroup\$

What I see akward from the beggining is that the class is returning instances of the self type on static methods like Running(), Tie() and Win(Player winner). This is alike method chaining but not returning the same instance, it is returning a new instance, and its being accessed statically.

I would expect a Game class oriented to be the thumbprint of many Game instances. On Game constructor I would set the default properties for a new game. (Not really needed since default value of Enum in this case would be running)

public class Game() {
  private Status status;
  public Game() {
    status = Status.Running;  
  }
}

The player is just an enum that is part of the actual result. So I would iclude that on the status of the game. Otherwise, if a class exposes an boolean isOver Property and an enum Winner Property the default case of this properties is isOver = false, winner = Player.Red and it also can lead to unwanted convinations, like when a match is over and tie cannot have a winner.

So since players are just meant to indicate the result I would make it be part of the status set.

public Enum Status {
  Running,
  Tie,
  RedWon,
  BlackWon
}

Then if you need to expose functions to end the game you can expose Tie and Win that will set the internal status of the class.

public class Game() {
  private Status status = Status.Running;
  public Tie() {
    status = Status.Tie;
  }
  public RedWins() {
    status = Status.RedWon;
  }
  public BlackWins() { 
    status = Status.BlackWon;
  }
}

You also can just pass the status in to a SetStatus function and change it.

In order to compare two games you can just compare them by using the status property.

Over time if you consider adding more players you will need to think of another solution and set the status to be composite of player variable and status.

Let me know if that helps, if the idea is to really use IEquatable and have status and players separately we can look for another way of doing it.

\$\endgroup\$
  • 1
    \$\begingroup\$ You are totally correct about disapproving the return of new instances and about focusing on the thumbprint of game status. However, I dislike including PlayerXWon in the status. This would create the need of conversion between enums Status and Player. See my own answer in the question as well. \$\endgroup\$ – Manolis Apr 21 '17 at 15:34
  • \$\begingroup\$ @Manolis I wasn't sure also about having a state PlayerWon, I also thought about it at first, but I keept myself to that this idea just because the player was used just to be part of the winning state and also if the comparison is required (by taking a look at the IEquatable), it makes it simple to compare just one enum. Also if we keep two enums there is always a default winner value on game even if the game has a status running or tie. It's an interesting case. \$\endgroup\$ – bruno.bologna Apr 21 '17 at 19:50
1
\$\begingroup\$

I'll try to asnwer the last quesiton which is:

GameStatus and GameStatusStrict classes seem to be simple enough so as not to need unit testing. Do you agree?

Nope, I don't because you can easily make mistakes in code that is too obvious to test it. Here are a few examples:

public static bool operator ==(GameStatusStrict arg1, GameStatusStrict arg2) =>
    ReferenceEquals(arg1, null) ? ReferenceEquals(arg2, null) : arg1.Equals(arg2);

This is wrong because it returns true if either object is null. It should return true only if both objects are really equal:

public static bool operator ==(GameStatusStrict arg1, GameStatusStrict arg2) =>
    !ReferenceEquals(arg1, null) &&
    !ReferenceEquals(arg2, null) &&
    arg1.Equals(arg2);

Also in this method there is enough room for making things not working correctly:

public bool Equals(GameStatus gs) {
    return gs != null ?
        (IsOver == gs.IsOver) && (IsTie == gs.IsTie) && (Winner == gs.Winner) :
        false;
}

This one isn't wrong per se but it's not any simple logic either. One ! in the wrong place and it doesn't work anymore. A unit test should verify this works as expected.

Actually you can simplify this one too. You don't need the ternary operators here because you can just chain the experssions with &&

public bool Equals(GameStatusStrict gs) =>
    gs != null &&
    _isOver == gs._isOver &&
    _isTie == gs._isTie && 
    _winner == gs._winner;

One more example why you should test it... because you already do it with

static class Checker {
    public static void Check(string varName, bool value, bool expectedValue) {
        if (value != expectedValue)
            throw new InvalidOperationException(string.Format("{0} should be {1}.", varName, expectedValue));
    }
}

It's so simple that you apparently don't trust yourself already and check the values anyway.

\$\endgroup\$
  • \$\begingroup\$ Your comment about the == being incorrect is wrong. It only says they are equal if both objects are null not if only one of them is. \$\endgroup\$ – RobH Apr 21 '17 at 14:44
  • \$\begingroup\$ The operator ==() is correct, BUT the fact that we have to discuss and argue about it being correct or not, definitely means it is not simple enough. :) \$\endgroup\$ – Manolis Apr 21 '17 at 15:50
  • \$\begingroup\$ @RobH that's not really better but probably depends on what you considere equal. Indeed, null == null is true but how usefull is this? If I compare two objects for their equality and they are both null and further processing depends on them not being null (which usually is the case) then a crash is inevitable. It's strage to say that one null game is equal to another null game. One could say it's inconclusive but there is no third state so false is IMO better then consider them equal and risk a NullReferenceException. \$\endgroup\$ – t3chb0t Apr 21 '17 at 16:42
  • \$\begingroup\$ @t3chb0t - an admirable goal but that behaviour is not consistent with the C# specification. If both are null the equals function should return true. Read section 14.9 ecma-international.org/publications/standards/Ecma-334.htm \$\endgroup\$ – RobH Apr 23 '17 at 19:49
0
\$\begingroup\$

Considering the comments of Bruno, a keep-it-simple approach is:

enum Player {
    Red,
    Black
};

enum GameStatus {
    Running,
    Tie,
    Win
};

class Game {
    ...
    public GameStatus Status { get; private set; }
    public Player Winner { get; private set; }

    public void SetRunning() => Status = GameStatus.Running;
    public void SetTie() => Status = GameStatus.Tie;
    public void SetWin(Player player) {
        Status = GameStatus.Win;
        Winner = player;
    }

    ...
}

So, the status can be running, tie, or win. In case of win, the winner can be enquired. This approach does not need a class of its own for the status, can as well simply lie inside the Game.

The approach can be further simplified by removing the Set... methods and making the properties Status and Winner publicly settable.

The disadvantage is that the logic "the winner only makes sense when there is a win" is not enforced.

Indeed, returning new instances and trying to compare them does not make sense. It overcomplicates everything. Though I didn't like the idea of including the players in the Status enumeration.

Raising exceptions in a property getter is BTW not recommended. (Now I know. q-:)

\$\endgroup\$
0
\$\begingroup\$

I would note that you can avoid having to implement Equals and GetHashCode and equality operators if you memoize everything. I'd be inclined to write your class like this:

abstract class GameStatus 
{
  private GameStatus() { } // No one else can create one.
  private class TiedGame : GameStatus { ... }
  public static readonly GameStatus Tied = new TiedGame();
  private class RunningGame : GameStatus { ... }
  public static readonly GameStatus Running = new RunningGame();
  private class WonGame : GameStatus { ... }
  private static Dictionary<Player, WonGame> d = new ...;
  public static GameStatus Won(Player p)
  {
    WonGame w;
    if (!d.TryGetValue(p, out w))
    {
        w = new WonGame(p);
        d.Add(p, w);
    }
    return w;
  }
  ... abstract members, and so on ...
}

Now you have reference equality for every different possible GameStatus, so you don't have to worry about implementing value equality.

\$\endgroup\$

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.