TicTacToe Objects calisthenics OOP

Question

I'm learning Objects calisthenics and I was practicing with the typical Tic Tac Toe game, I was wondering if you could gimme feedback about my mistakes (Algorithmic problems, logic, and especially OOP following Objects calisthenics rules). Thanks in advance!

• Only One Level Of Indentation Per Method
• Don’t Use The ELSE Keyword
• Wrap All Primitives And Strings First Class Collections
• One Dot Per Line
• Don’t Abbreviate
• Keep All Entities Small
• No Classes With More Than Two Instance Variables
• No Getters/Setters/Properties

Player.cs

namespace Tic_tac_toe
{
 public class Token
 {
 // TODO: Get rid off this auto propertie.
 public char TokenSelected { get; }
 public Token(char token) => TokenSelected = token;
 }
 public class Player
 {
 private readonly Matrix _matrix = new Matrix(3, 3);
 public Token Token { get; }
 public Player(Token token)
 {
 Token = token;
 }
 public void PutTokenAt(Coords coords)
 {
 if (_matrix.IsNotEmpty(coords))
 _matrix.SetToken(coords, Token);
 }
 public bool CheckIfPlayerWon()
 {
 return _matrix.CheckRow(Token) ||
 _matrix.CheckColumn(Token) ||
 _matrix.CheckSlashDiagonal(Token) ||
 _matrix.CheckBackSlashDiagonal(Token);
 }
 }
}

Coords.cs

using System.Collections.Generic;
using System.Linq;
namespace Tic_tac_toe
{
 public class Coords
 {
 public Coords(int x, int y)
 {
 X = x;
 Y = y;
 }
 public int X { get; }
 public int Y { get; }
 }
 public class CoordsCollection
 {
 private readonly List<Coords> _coordsList;
 public CoordsCollection()
 {
 _coordsList = new List<Coords>();
 }
 public void Add(Coords coords)
 {
 _coordsList.Add(coords);
 }
 public int Sum(int[,] magicSquare)
 {
 return _coordsList.Sum(cord => magicSquare[cord.X, cord.Y]);
 }
 }
}

Matrix.cs

namespace Tic_tac_toe
{
 public class Matrix
 {
 private readonly int _n;
 private readonly int _m;
 private readonly char[,] _value;
 private Coords _lastMove;
 public Matrix(int n, int m)
 {
 _n = n;
 _m = m;
 _value = new char[_n, _m];
 CreateMatrix();
 }
 private void CreateMatrix()
 { 
 for (int i = 0; i < _n; i++)
 for (int j = 0; j < _m; j++)
 _value[i, j] = '.';
 }
 public bool IsNotEmpty(Coords coords)
 {
 return _value[coords.X, coords.Y] == '.';
 }
 public void SetToken(Coords coords, Token token)
 {
 _value[coords.X, coords.Y] = token.TokenSelected;
 _lastMove = coords;
 }
 private bool MagicSquare(CoordsCollection listCord)
 {
 // https://en.wikipedia.org/wiki/Magic_square
 int[,] mS = new int[3, 3];
 mS[0, 0] = 2; mS[0, 1] = 7; mS[0, 2] = 6;
 mS[1, 0] = 9; mS[1, 1] = 5; mS[1, 2] = 1;
 mS[2, 0] = 4; mS[2, 1] = 3; mS[2, 2] = 8;
 return listCord.Sum(mS) == 15;
 }
 public bool CheckRow(Token token)
 {
 CoordsCollection rowCordList = new CoordsCollection();
 for (int x = 0; x < _m; x++)
 if (_value[x, _lastMove.Y] == token.TokenSelected)
 rowCordList.Add(new Coords(x, _lastMove.Y));
 return MagicSquare(rowCordList);
 }
 public bool CheckColumn(Token token)
 {
 CoordsCollection columnsCordList = new CoordsCollection();
 for (int y = 0; y < _n; y++)
 if (_value[y, _lastMove.Y] == token.TokenSelected)
 columnsCordList.Add(new Coords(_lastMove.X, y));
 return MagicSquare(columnsCordList);
 }
 public bool CheckBackSlashDiagonal(Token token)
 {
 CoordsCollection backSlashCordsList = new CoordsCollection();
 for (int i = 0; i < _n; i++)
 if (_value[i, i] == token.TokenSelected)
 backSlashCordsList.Add(new Coords(i, i));
 return MagicSquare(backSlashCordsList);
 }
 public bool CheckSlashDiagonal(Token token)
 {
 CoordsCollection slashCordsList = new CoordsCollection();
 for (int x = 0, y = _n - 1; x < _m; x++, y--)
 if (_value[x, y] == token.TokenSelected)
 slashCordsList.Add(new Coords(x, y));
 return MagicSquare(slashCordsList);
 }
 }
}

• Peter Csala: Which version of C# are you using?
• Sharki: @PeterCsala Visual studio 2019 (8.0) C# .Net Core 3.1

Answer 1

Token

This class is an overkill. I mean I do understand that you want to follow this Wrap All Primitives And Strings First Class Collections rule. But I think you misunderstand the intent.

Let's suppose you expect a valid e-mail address from your user. You create a wrapper around string to validate it and then be able to pass it along as an e-mail. So others don't have to perform any assessment, since there is a guarantee that whenever they receive an Email instance then it is valid.

Your Token class does not perform any validation so it can not give any guarantee. It is just yet another layer of abstraction.

If you really want to have it then create it as struct to express your immutable intent

public struct Token
{
 public char Selected;
 public Token(char token)
 => Selected = token;
}

Player

If you need to pass the Token for each and every matrix methods then it is a good sign for refactoring. Pass the token as a constructor parameter instead.

public class Player
{
 private readonly Matrix _matrix;
 
 public Player(Token token)
 => _matrix = new Matrix(3, 3, token.Selected);
 public void PutTokenAt(Coordinate coordinate)
 => _matrix.SetToken(coordinate);
 
 public bool CheckIfPlayerWon()
 => _matrix.CheckRow() ||
 _matrix.CheckColumn() ||
 _matrix.CheckSlashDiagonal() ||
 _matrix.CheckBackSlashDiagonal();
}

Coords

Yes, I've renamed your Coords class because that abbreviation just makes your code less understandable. Try to avoid abbreviations for your public API

Also, I would encourage you to try to use the same terminology across your whole domain. From your code it is pretty channeling to understand how the following things relate to each other: X, _n, column

public struct Coordinate
{
 public int Column;
 public int Row;
 public Coordinate(int x, int y)
 => (Column, Row) = (x, y);
}

• Yet again this can be a struct to express immutability.
  • Since C# 9 you could use record for this purpose as well
• (Column, Row) = (x, y): Here we take advantage of ValueTuple's deconstruction

CoordsCollection

I've added an AddRange method to this class to be able to use LINQ inside the Matrix class

public class CoordinateCollection
{
 private readonly List<Coordinate> collection = new List<Coordinate>();
 public void AddRange(IEnumerable<Coordinate> cords)
 => collection.AddRange(cords);
 public int Sum(int[,] magicSquare)
 => collection.Sum(cord => magicSquare[cord.Column, cord.Row]);
}

• Since C# 9 you can take advantage of target-typed new expression, which would simplify the initialization of collection:

private readonly List<Coordinate> collection = new ();

• I've renamed your _coordsList to collection, because the latter does not tell anything about the type itself which helps evolution of your code

Matrix

Let's start with initialization:

public class Matrix
{
 private const char initValue = '.';
 private readonly int columns;
 private readonly int rows;
 private readonly char[,] value;
 private readonly char token;
 private Coordinate lastMove;
 public Matrix(int n, int m, char selectedToken)
 {
 (columns, rows, token) = (n, m, selectedToken);
 value = new char[columns, rows];
 Initialize();
 }
 private void Initialize()
 {
 for (int column = 0; column < columns; column++)
 for (int row = 0; row < rows; row++)
 value[column, row] = initValue;
 }
 ...
}

• As I said earlier please try to use the same terminology across your domain
• The CreateMatrix is a really bad name for a private method
  • Since you are not creating a new matrix rather populating it with initial values
  • Also suffixing a private method with a class seems unreasonable
  • Initialize is just fine

SetToken

public void SetToken(Coordinate coords)
{
 if (value[coords.Column, coords.Row] != initValue) return;
 value[coords.Column, coords.Row] = token;
 lastMove = coords;
}

• Exposing the validation separately from the actual code is really error-prone
  • You can not enforce that IsNotEmpty is called prior SetToken
• If you reuse the same character across two different methods then put it into a constant
  • Without that if you update the Initialize to use ';' from now on and you forget to change that inside the IsNotEmpty then your code is broken

MagicSquare

• If your method returns with a bool then please prefix it with Is or Has
• I haven't dig into the details why do you need this square but it seems fragile
  • What if the matrix is 2x2 or 5x1 or 10X15? Will it work?
• You can take advantage of collection initializer for multidimensional arrays as well

int[,] magicSquare = new int[3, 3]
{
 { 2, 7, 6 },
 { 9, 5, 1 },
 { 4, 3, 8 }
};

• You should create this array only once rather than each and every time when you call this method

// https://en.wikipedia.org/wiki/Magic_square
private static int[,] magicSquare = new int[3, 3]
{
 { 2, 7, 6 },
 { 9, 5, 1 },
 { 4, 3, 8 }
};
private bool HasWinner(CoordinateCollection listCord)
 => listCord.Sum(magicSquare) == 15;

CheckRow

I've rewritten it in the way to use LINQ instead of for loop

public bool CheckRow()
{
 var rowCordList = new CoordinateCollection();
 rowCordList.AddRange(Enumerable.Range(0, columns)
 .Where(column => value[column, lastMove.Row] == token)
 .Select(column => new Coordinate(column, lastMove.Row)));
 return HasWinner(rowCordList);
}

CheckColumn

Here I think you have accidentally mixed the indexing parameters. According to my understanding it should look like this:

public bool CheckColumn()
{
 var columnsCordList = new CoordinateCollection();
 columnsCordList.AddRange(Enumerable.Range(0, rows)
 .Where(row => value[lastMove.Column, row] == token)
 .Select(row => new Coordinate(lastMove.Column, row)));
 return HasWinner(columnsCordList);
}

---

UPDATE #1: reflect to questions

If I get it correctly, every time that I want to give a home to a type that has not more logic than being a type itself I could use a struct instead of create a class, right?

It is not that simple. struct is a value type and most of the time is stored on the stack. Whenever you make a modification then in reality you create a new instance with different value. So, the original value is immutable.

In case of C# 9 records are classes under the hood. By using the with keyword you can create a new instance with a different value. Since C# 10 you have more control over the record structure, for example you can define a record struct.

About the target-typed new expression how clean is to use it instead the whole name?

It is basically the counterpart of the implicitly typed variable feature.

var collection = new List<int>():
List<int> collection = new();

In both cases you can abuse them, like var result = GetExternalCall(). But if you use them wisely then they can shorten your code without ruining legibility.

I have found this feature pretty useful whenever I want to initialize readonly fields.

I also saw you used a lot the expression bodied members (=>), a lot of people told me not to use it because it makes the code less readable. Since you're using it, I'd like to ask you what do you think about it?

Yet again if it is used wisely then it is not a problem. There are some methods which will never have longer implementation body than a single line then you can use it freely.

The debate over this feature is similar to the if (condition) return result; vs if (condition) { return result; }. Conciseness vs easier extensibility

Since MagicSquare is initialized out of methods as an attribute I was wondering if it could be marked as readonly (even as a const for this case)?

You can't mark an array as const but you can mark it as a readonly. But it does not prevent you to add or remove items to/from the collection. It only avoids unintentional overwrite of the whole collection (to collectionB or null).

If you want to prevent addition and removal then consider to use ImmutableArray

Exposing the validation separately from the actual code is really error-prone. You can not enforce that IsNotEmpty is called prior SetToken. << I did not get it and I'd like to know what you mean with that

If you mark the IsNotEmpty method as public then you are exposing it to the users of the class. Even though your intention is that the caller of SetToken should call the IsNotEmpty prior, there is no guarantee that is is called at all

// Your Intention
if (IsNotEmpty(...)) //guard expression
 SetToken(...) 
// Accidentally
SetToken(...) //without guard expression

The compiler will not notify the caller of the SetToken that you should call the IsNotEmpty if you want to call SetToken.

So, it is advisable to move your guard expression into the to be called method. If you want to expose it that's fine but make sure it is also called inside the method as well (prefer double checks over zero check).

I find LINQ an amazing feature of C# that I MUST to learn. Since explain how the AddRange works is kinda out of scope and I can just take the time enough to figure out for myself, could you maybe tell me a good site or resource to learn LINQ?

The best way to get started with LINQ is the 101 LINQ Samples site.

In short the code does the following:

• Enumerable.Range(0, columns): It creates a number sequence from 0 to columns where the step is 1. For example, if columns is 5 then 0, 1, 2, 3, 4
• .Where(column => ...): It filters out items where the specified condition is not met
• .Select(column => new Coordinate(...)): It creates a new instance for each item in the sequence which is not filtered out. Basically it maps the numbers to coordinates

Also one last thing I forgot to ask, for the No Getters/Setters/Properties rule is it fine to have a public field instead of a property with a getter (notice coordinate class as an example of this) and why?

Coordinate is not a class rather than a struct. In case of struct the members are initialized while the object is created. So it really does not matter whether they are fields or properties.

Some people never use fields for public members. It is more like a personal preference. If you use auto-generated properties with public setter and getter then there is no real difference.

Since C# 9 you can define init only setters, which emphasises immutability.

---

Final thought: even though these rules/principles/guidance are good you don't have to take them too seriously. They are created to prevent common pitfalls, but if you understand the risk and the added overhead/complexity is greater than the benefit then you can violate the rules. If you document your decision (the whys and why nots) in comments then future maintainer of your code will also understand your code better.

• Sharki: Thanks a lot for your feedback, I appreciate it so much. I have some questions; If I get it correctly, every time that I want to give a home to a type that has not more logic than being a type itself I could use a struct instead of create a class, right? I also didn't know that C# had desconstrucion. (Thanks for that tip).
• Sharki: About the target-typed new expression how clean is to use it instead the whole name? (Which I agree that the whole name feels kinda redundant). I also saw you used a lot the expression bodied members (=>), a lot of people told me not to use it because it makes the code less readable. Since you're using it, I'd like to ask you what do you think about it?
• Peter Csala: @Sharki Good questions. Let me update my post to reflect to all.
• Sharki: Thank you very much for your long answer, the feedback and the articles and resources you refer to. I really really really do appreciate a lot. Your reply help me so much and I've learned a lot of things through your whole post, it was pretty clear. Thanks so much! Have a good day :)
• Jörg W Mittag: In addition to Coordinate being a struct (in all versions of C#) or a record (starting with C# 9), starting with C# 10, it should arguably be a record struct.

Answer 2

I think there are some good pointers in calisthenics rules, but KISS principles should always trump.

Consider the following example:

class Animal
{
 public Animal(string type, string color)
 {
 Type = type;
 Color = color;
 }
 public string Type;
 public string Color;
};
class Program
{
 static void Main(string[] args)
 {
 var animal = new Animal("horse", "brown");
 Console.WriteLine(animal.Type);
 }
}

Here we are breaking the One Dot Per Line rule in Console.WriteLine(animal.Type);

How can we solve this?

Option A: Create a new function to write stuff to the console:

static void WriteLineToConsole(string text)
{
 Console.WriteLine(text);
}
static void Main(string[] args)
{
 var animal = new Animal("horse", "brown");
 WriteLineToConsole(animal.Type);
}

Option B: Create a new function to return the type of animal:

static string GetAnimalType(Animal animal)
{
 return animal.Type;
}
static void Main(string[] args)
{
 var animal = new Animal("horse", "brown");
 Console.WriteLine(GetAnimalType(animal);
}

Both options A and B add unnecessary complications and make the program harder to read.

---

I have noticed that you are breaking Only One Level Of Indentation Per Method in a couple of places like in CheckColumn but would not recommend that you change them as this would only add unnecessary complications.

---

Keeping with KISS principles. Checking Win states using magic squares is pretty cool but it does not really add much value and makes things much more complicated than they need to be.

• Sharki: Hey, thanks for your reply not only in this post but in other one I did, I remember I read your reply but didn't say anything so now it's perfect time to say thanks for it! Turning to the question, (also I'm sorry for my english) I find it really useful the KISS concept, but I'm not sure how feels the company about it. I also would like to ask, does objects calisthenics should have getters/setters? Since I find really easy to call to a get instead of creating a whole method for returning a simple attribute.
• Sharki: What do you think is the best approach for OOP? The approach my company is following is TDD + Objects calisthenics and some SOLID principles, they also don't do inheritance but works with interfaces. I don't know if you could gimme some tips or some advice to get better at this. Thanks in advance and again sorry for my English!
• jdt: @Sharki, I’m not sure. Some people have very strong feelings about these rules. I would say to mostly go with the flow unless doing so turns your program into a cluttered mess.