TicTacToe in MVP Winforms

I took the source code from this question, (thank you gaessaki for the motivation!) and did a lot of refactoring.

I use 4 projects:

• Common: contains mainly interfaces and enumerations referenced by other

projects.

• Model: Knows nothing of the others, just maintains the game logic and state.

• Presenter: References the Model directly and accesses the View via the IView interface. Acts as mediator between them.

• View: References the Presenter. Is pretty dumb, can only query the board situation and the game status and display them. For every action (playing a move, restarting, etc.) it simply informs the presenter.

The View is actually the executable project who references all others and in its Program.cs instantiates a Model, a View, a Presenter and connects them. But other than that, the visual components only touch the presenter. I did not want to create another project just to this instantiation.

The intention is to be able to interchange the view (e.g. use console or WPF), keeping the model and the presenter.

I appreciate your comments. I have experience with C# and programming in general, but not with MVP. I think, I tend to overcomplicate the design.

Help classes

ExceptionBuilder

using System;

namespace Mfanou.Common {
    public static class ExceptionBuilder {
        public static void CheckArgumentRangeInclusive(string varName, int value, int lowerRange, int upperRange) {
            if (value  upperRange)
                throw new ArgumentOutOfRangeException(varName);
        }
    }
}

Common

GameAction enumeration

namespace Mfanou.TicTacToe.Common {
    public enum GameAction {
        Restart,
        Exit
    }
}

Move enumeration

namespace Mfanou.TicTacToe.Common {
    public enum Move {
        ShowPreview,
        HidePreview,
        Play
    }
}

IPlayer

namespace Mfanou.TicTacToe.Common {
    public interface IPlayer {
        int Id { get; }
    }
}

ISquareContent

```
namespace Mfanou.TicTacToe

To begin with...

public static class ExceptionBuilder {
    public static void CheckArgumentRangeInclusive(string varName, int value, int lowerRange, int upperRange) {
        if (value  upperRange)
            throw new ArgumentOutOfRangeException(varName);
    }
}

This is not a builder. It's a validator so I suggest naming it like ArgumentValidator and the method ValidateArgumentRangeInclusive.

public enum Move {
    ShowPreview,
    HidePreview,
    Play
}

To me, ShowPreview and HidePreview are rather view options then something that has anything to do with Move.

public static readonly int ROWCOL_MIN = 1;
public static readonly int ROWCOL_MAX = 3;

We don't use UPPER_CASE for constants in C# and the name ROWCOL isn't clear. Is it row or column? You can put them inside a static class to give the a better meaning.

private void CheckRowColRange(string varName, int value) {
    ExceptionBuilder.CheckArgumentRangeInclusive(varName, value, ROWCOL_MIN, ROWCOL_MAX);
}

This method can be made static because it does not require any state information from the owning class.

public interface IGameStatus {
    bool IsOver { get; }

    /// Valid only when IsEmpty is true.
    bool IsTie { get; }

    /// Valid only when IsEmpty is true and IsTie is false.
    IPlayer WinningPlayer { get; }
}

There is no IsEmpty.

public Game() {
    MoveFactory = new MoveFactory(this);
    GameActionFactory = new GameActionFactory(this);

    Board = new Board();
    Turn = new Turn(Player.GetAll());

    new RestartAction(this).Execute();
}

The constructor should not be doing any thing but initializing data. Something like new RestartAction(this).Execute(); is a very bad idea and I'd be really surprised when I created an instance of a Game and it already did something even though it's not fully created yet. What's even worse, the RestartAction dependency is not passed as a such via the costructor so there is no way to override it for testing.

public IGameStatus Status => InternalStatus;
internal GameStatus InternalStatus { get; set; }

This doesn't seem right. Why would you make an internal status settable and the public one not? This looks like hacking something.

StackExchange Code Review Q#159399, answer score: 5