Game status with different states and equatability

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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-:)

Answer 4

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.