patterncsharpMinor
Tic Tac Toe implemented in WinForms using Model-Viewer-Presenter Pattern
Viewed 0 times
presentertoeimplementedticviewerwinformstacusingmodelpattern
Problem
I had to build a 2-player Tic Tac Toe app in C# w/ WinForms for a pre-interview screen. I got past that stage successfully and the app works flawlessly, but I would like to get some feedback.
Although I've been programming C# for a while, I don't have too much experience with certain architectural paradigms such as Model-Viewer-Presenter. In a bid to learn more, I built the Tic Tac Toe app following the MVP Pattern.
GitHub
Program.cs
GridModel.cs
Should potentially be called GameModel.cs instead?
```
namespace TicTacToe.Model
{
class GridModel : IGridModel
{
private char[,] grid;
private bool playerOneTurn;
///
/// Gets and sets the boolean indicating whether it's the first players turn.
///
public bool PlayerOneTurn
{
get { return playerOneTurn; }
set { playerOneTurn = value; }
}
public GridModel()
{
grid = new char[3, 3];
playerOneTurn = true;
}
///
/// Creates a new empty game grid and gives the turn to the first player.
///
public void NewGrid()
{
grid = new char[3, 3];
playerOneTurn = true;
}
///
/// Sets the player's marker on the specif
Although I've been programming C# for a while, I don't have too much experience with certain architectural paradigms such as Model-Viewer-Presenter. In a bid to learn more, I built the Tic Tac Toe app following the MVP Pattern.
GitHub
Program.cs
using System;
using System.Windows.Forms;
using TicTacToe.Model;
using TicTacToe.Presenter;
namespace TicTacToe
{
static class Program
{
///
/// The main entry point for the application.
/// Creates a new view and model and wires them with a presenter.
///
[STAThread]
static void Main()
{
Application.EnableVisualStyles();
Application.SetCompatibleTextRenderingDefault(false);
TicTacToeForm ticTacToeForm = new TicTacToeForm();
GamePresenter presenter = new GamePresenter(ticTacToeForm, new GridModel());
Application.Run(ticTacToeForm);
}
}
}GridModel.cs
Should potentially be called GameModel.cs instead?
```
namespace TicTacToe.Model
{
class GridModel : IGridModel
{
private char[,] grid;
private bool playerOneTurn;
///
/// Gets and sets the boolean indicating whether it's the first players turn.
///
public bool PlayerOneTurn
{
get { return playerOneTurn; }
set { playerOneTurn = value; }
}
public GridModel()
{
grid = new char[3, 3];
playerOneTurn = true;
}
///
/// Creates a new empty game grid and gives the turn to the first player.
///
public void NewGrid()
{
grid = new char[3, 3];
playerOneTurn = true;
}
///
/// Sets the player's marker on the specif
Solution
-
I think, your Presenter is doing too much. The logic about who wins, the next move, etc. should go to the model. The logic about highlighting, colors, X and O's constants, message strings etc. belong to the View.
-
You are appending an empty string to a char for the side-effect of converting to a string:
b.Text = model.GetPiece(row, col) + "";
Don't do that. Use the ToString() function on chars.
-
Avoid repeating string literals like '\0', 'X', etc. all the time. Use string constants:
const char EMPTY_SQUARE = '\0';
-
Don't duplicate your code. Try to identify repeating patterns and extract them either in small functions, or code more cleverly.
Every case in the switch does the same thing. Only the contents of the message box change.
Check the method IsWinOrTie() in the Presenter. (Btw, Good Lord! What a tsunami of numbers...)
The pattern is: A "selection" of 3 squares in the board (being either a row, a column, or a diagonal). We want to do something with that (check if the contents belong to the same player, highlight, etc.)
There could be e.g. an enumeration describing the selection and methods returning the corresponding 3 squares. Then IsWinOrTie() could simply do a loop on enumerations and check in a one liner:
-
The method HighlightWin() takes 6 parameters. This is terrible. Consider creating a struct:
struct Position {
public int row;
public int col;
}
You can pass an array of such structs instead.
I think, your Presenter is doing too much. The logic about who wins, the next move, etc. should go to the model. The logic about highlighting, colors, X and O's constants, message strings etc. belong to the View.
-
You are appending an empty string to a char for the side-effect of converting to a string:
b.Text = model.GetPiece(row, col) + "";
Don't do that. Use the ToString() function on chars.
-
Avoid repeating string literals like '\0', 'X', etc. all the time. Use string constants:
const char EMPTY_SQUARE = '\0';
-
Don't duplicate your code. Try to identify repeating patterns and extract them either in small functions, or code more cleverly.
switch (IsWinOrTie())
{
case 'X':
MessageBox.Show("Congrats to Player X! Press 'Restart' to play a new round.", "X has won");
gridPanel.Enabled = false;
break;
case 'O':
MessageBox.Show("Congrats to Player O! Press 'Restart' to play a new round.", "O has won");
gridPanel.Enabled = false;
break;
case 't':
MessageBox.Show("It's a tie! Press 'Restart' to play a new round.", "Tie");
gridPanel.Enabled = false;
break;
default:
break;
}Every case in the switch does the same thing. Only the contents of the message box change.
Check the method IsWinOrTie() in the Presenter. (Btw, Good Lord! What a tsunami of numbers...)
The pattern is: A "selection" of 3 squares in the board (being either a row, a column, or a diagonal). We want to do something with that (check if the contents belong to the same player, highlight, etc.)
There could be e.g. an enumeration describing the selection and methods returning the corresponding 3 squares. Then IsWinOrTie() could simply do a loop on enumerations and check in a one liner:
squareA != EMPTY_SQUARE && squareA == squareB && squareB == squareB && squareB == squareC.-
The method HighlightWin() takes 6 parameters. This is terrible. Consider creating a struct:
struct Position {
public int row;
public int col;
}
You can pass an array of such structs instead.
Code Snippets
switch (IsWinOrTie())
{
case 'X':
MessageBox.Show("Congrats to Player X! Press 'Restart' to play a new round.", "X has won");
gridPanel.Enabled = false;
break;
case 'O':
MessageBox.Show("Congrats to Player O! Press 'Restart' to play a new round.", "O has won");
gridPanel.Enabled = false;
break;
case 't':
MessageBox.Show("It's a tie! Press 'Restart' to play a new round.", "Tie");
gridPanel.Enabled = false;
break;
default:
break;
}squareA != EMPTY_SQUARE && squareA == squareB && squareB == squareB && squareB == squareC.Context
StackExchange Code Review Q#150036, answer score: 2
Revisions (0)
No revisions yet.