10
\$\begingroup\$

It seems that one of the more popular game formats is a grid-based one. To that end I thought I'd design a generic game grid class. The generic type is used to represent the value that is displayed in each cell of the grid. For instance, a Console based game might use a char type for the cell value. Whereas, a form based game might use a Label. I designed the class to be inherited so that the game class will have access to the data without exposing it needlessly.

GameGrid class

public class GameGrid<T>
{
    protected class Coordinates
    {
        public int Row { get; set; }
        public int Col { get; set; }
        public int RightDiagRow { get; set; }
        public int RightDiagPos { get; set; }
        public int LeftDiagRow { get; set; }
        public int LeftDiagPos { get; set; }
        public Coordinates()
        {
            Row = -1;
            Col = -1;
            RightDiagRow = -1;
            RightDiagPos = -1;
            LeftDiagRow = -1;
            LeftDiagPos = -1;
        }
        public Coordinates(int row, int col, int rightDiagRow, int rightDiagPos, int leftDiagRow, int leftDiagPos)
        {
            Row = row;
            Col = col;
            RightDiagRow = rightDiagRow;
            RightDiagPos = rightDiagPos;
            LeftDiagRow = leftDiagRow;
            LeftDiagPos = leftDiagPos;
        }
    }
    protected class Cell
    {
        public T Value { get; set; }
        public Coordinates Location { get; set; }
        public Cell()
        {
            Value = default(T);
            Location = new Coordinates();
        }
        public Cell(int row, int col,int rightDiagRow,int rightDiagPos, int leftDiagRow,int leftDiagPos)
        {
            Value = default(T);
            Location = new Coordinates(row, col, rightDiagRow, rightDiagPos, leftDiagRow, leftDiagPos);
        }
        public override string ToString()
        {
            return Value.ToString();
        }
    }
    const int DEFAULT_GRID_SIZE = 3;
    protected int GridSize { get; set; }
    protected Cell[][] Rows { get; set; }
    protected Cell[][] Cols { get; set; }
    protected Cell[][] RightDiagRows { get; set; }
    protected Cell[][] LeftDiagRows { get; set; }

    /// <summary>
    /// New GameGrid set to a default size of 3
    /// </summary>
    protected GameGrid()
    {
        GridSize = DEFAULT_GRID_SIZE;
        InitGrid();
    }
    /// <summary>
    /// New GameGrid set to gridSize
    /// </summary>
    protected GameGrid(int gridSize)
    {
        GridSize = gridSize;
        InitGrid();
    }
    private void InitGrid()
    {
        Rows = new Cell[GridSize][];
        Cols = new Cell[GridSize][];
        int diagSize = (GridSize * 2) - 1;
        RightDiagRows = new Cell[diagSize][];
        LeftDiagRows = new Cell[diagSize][];
        for(int i = 0; i < GridSize;i++)
        {
            Rows[i] = new Cell[GridSize];
            Cols[i] = new Cell[GridSize];
            RightDiagRows[i] = new Cell[GridSize];
            LeftDiagRows[i] = new Cell[GridSize];
        }
        for(int i = GridSize; i < diagSize;i++)
        {
            RightDiagRows[i] = new Cell[GridSize];
            LeftDiagRows[i] = new Cell[GridSize];
        }
        for (int row = 0; row < GridSize; row++)
        {
            for (int col = 0; col < GridSize; col++)
            {
                int rightDiagRow = ((GridSize * 2) - 2) - (row + col);
                int rightDiagPos = rightDiagRow < GridSize ? (GridSize - 1) - col : row;
                int leftDiagRow = Math.Abs((row - col) - (GridSize - 1));
                int leftDiagPos = leftDiagRow < GridSize ? col : row;
                Cell newCell = new Cell(row, col, rightDiagRow, rightDiagPos, leftDiagRow, leftDiagPos);
                Rows[row][col] = newCell;
                Cols[col][row] = newCell;
                RightDiagRows[rightDiagRow][rightDiagPos] = newCell;
                LeftDiagRows[leftDiagRow][leftDiagPos] = newCell;
            }
        }
        for(int i = 0; i < RightDiagRows.Length;i++)
        {
            RightDiagRows[i] = RightDiagRows[i].Where(x => x != null).ToArray();
            LeftDiagRows[i] = LeftDiagRows[i].Where(x => x != null).ToArray();
        }
    }
}

I've put the Coordinates in a nested class to hold the indexing information for each collection that represents the grid. I made the Cell class to hold the value as well its own Coordinate information. Since the collections representing the different columns and rows are of this Cell type the individual elements will be accessed by reference so that the data size stays quite small.

Its use is very simple. Inherit the class, passing the type needed to represent the cell and call the constructor in the derived class's constructor.

public class TicTacToe : GameGrid<char>

public TicTacToe():base()

I made the default size to be 3. I couldn't think of any need for a grid smaller than that.

I tried to thorough with my collections so that a game can access the grid by any column, row, or diagonal.

\$\endgroup\$
  • 5
    \$\begingroup\$ "I couldn't think of any need for a grid smaller than that." Well, 2 is the smallest possible edge case for a grid in general. \$\endgroup\$ – πάντα ῥεῖ Apr 23 '18 at 2:09
  • \$\begingroup\$ How are you going to use it if there's nothing public? \$\endgroup\$ – t3chb0t Apr 23 '18 at 7:36
  • \$\begingroup\$ The derived class uses it. \$\endgroup\$ – tinstaafl Apr 23 '18 at 11:07
16
\$\begingroup\$
  • For instance, a Console based game might use a char type for the cell value.
  • Whereas, a form based game might use a Label.

You seem to be mixing gameplay grids with layout grids. I strongly suggest distinguishing between the two, as these are two separate parts of the logic.

To that extent, I'd like to reframe your mindset from a grid approach to a coordinate approach. Your code actually already (partly) does so, but your explanation in the question suggests that your mental picture of this class is different.


Your code

Nested classes

While there is nothing wrong with using nested classes, I'd say that you're better off not nesting them in this case.

Nested classes are mainly used when you want your class to use internal classes that are not accessible to any external caller. However, I would expect that any external caller who uses Grid would also be interested in using Cell and Coordinates objects if they want to store references to objects that are important to them.

While your intentions can still be achieved with public nested classes (as you are doing here), the only other benefit to having nested classes (as far as I'm aware) is that nested classes can access their parent class' private properties; which you do not seem to rely on.

I suggest not nesting the classes in order to keep Grid lighter; since you're not using any of the benefits of nested classes.

Protected nested classes

You may have missed this because it's not implemented yet, but I doubt you'll want to set Coordinates and Cell as protected. This means that external callers cannot use the classes, which hinders developers for no discernible benefit.

For example, every public method in Game would be unable to use Coordinates as a parameter, and would always have to use int rowIndex, int colIndex to achieve the same result. It seems nicer if external callers could use your Coordinates class as well.

Though you may have hidden by intention, I'd argue the same for Cell. It'd be nice for an external caller to use it. Unless you want them to exclusively use coordinates and never the cell itself; which I could see an argument for as well.

Think about what public methods you want to give to your Game (or derived) class; and consider if you'll ever want to pass in/return Coordinates or Cell objects.

Generics

public class GameGrid<T>
{
    protected class Cell
    {
        public T Value { get; set; }
    }
}

I was surprised to see this actually works. I am assuming it does, otherwise this code would not be suited to CodeReview.SE.

Nonetheless, I would suggest that you make Cell generic as well. It's counterintuitive for Cell to have a generic field if it is not a generic class.
At some point, your game itself may want to store a reference to a cell value. But if it uses the Cell type, it's not obvious what type Cell.Value will be. By making Cell generic, you force your external callers to be aware of the cell value type that is being used.

Note that when you don't nest your classes, Cell will automatically have to be generic.

The grid arrays

Rows = new Cell[GridSize][];
Cols = new Cell[GridSize][];

for(int i = 0; i < GridSize;i++)
{
    Rows[i] = new Cell[GridSize];
    Cols[i] = new Cell[GridSize];
}

Since this is a grid where every row will have the same amount of columns, it would be more appropriate to use a Cell[,] (multidimensional array instead of a Cell[][] (jagged array).

One of the main differences here is that a jagged array can have rows of different lengths:

var grid = new Cell[3][];

grid[0] = new Cell[3];
grid[1] = new Cell[1];
grid[2] = new Cell[25];

This is not how you want your grid to work.

Both of these come with their own up- and downsides. More detailed explanation here.

Diagonals?

I've been looking at your code for over half an hour, and I have no idea what the Diag properties represent. The code is not clear on this at all. This is the best "explanation" I could find in the code:

int rightDiagRow = ((GridSize * 2) - 2) - (row + col);
int rightDiagPos = rightDiagRow < GridSize ? (GridSize - 1) - col : row;
int leftDiagRow = Math.Abs((row - col) - (GridSize - 1));
int leftDiagPos = leftDiagRow < GridSize ? col : row;

I still have no clue what any of these values represent. This snippet is littered with magical numbers and even more magical calculations that do not in any way make it clear what is being calculated.

Doing the math, for the center cell of a 3x3 grid, the values are:

int rightDiagRow = 2;
int rightDiagPos = 1;
int leftDiagRow = 2;
int leftDiagPos = 1;

...and I still have no idea what any of this means.

You also do this:

int diagSize = (GridSize * 2) - 1;

This still makes no sense to me. For a GridSize of 3, the diagSize is 5. I can't think of a single instance where 5 denotes anything about a 3x3 grid.

Since a grid can be defined via rows and columns alone, I'm going to assume that the diagonals are indeed intended to reference relations (that, according to you, cannot be easily expressed using rows/cols) between existing cells.
This is not a good approach. You should be defining your internal data structure as a minimum viable structure (avoid redundancy, avoid the possibility of two information sources contradicting each other).

Diagonals should be retrieved on the fly. For example:

Cell sourceCell = GetCell();
int sourceX = sourceCell.Location.Col; 
int sourceY = sourceCell.Location.Row; 

Cell topRightDiagonalCell =    Rows[sourceY + 1][sourceX - 1];
Cell bottomRightDiagonalCell = Rows[sourceY + 1][sourceX + 1];
Cell topLeftDiagonalCell =     Rows[sourceY - 1][sourceX - 1];
Cell bottomLeftDiagonalCell =  Rows[sourceY - 1][sourceX + 1];

This is a simplified example. You'll need to check for issues with the index going out of range, among other things.

Too many collections.

Rows[i] = new Cell[GridSize];
Cols[i] = new Cell[GridSize];

I don't understand why you have both a Rows and Cols array. Note that I do see that you made sure to have both arrays use the same object:

Cell newCell = new Cell(...);
Rows[row][col] = newCell;
Cols[col][row] = newCell;

However, there's still no reason to have two arrays (even if their contained objects are shared). The only difference between the two is that you swap their index values around.

I would argue that this functionality is not necessary. Coordinate pairs have a given order, there's little benefit gained from allowing someone to use a different order (since you'd still require them to knowingly use the correct Rows or Cols array).
But even if you feel it's relevant to make this possible; you should be using methods to give your external caller the same functionalities, while only using a single array under the hood:

private Cell[][] rows;

public Cell Row(int a, int b)
{
    return rows[a][b];
}

public Cell Col(int a, int b)
{
    return rows[b][a];
}

These methods work exactly like Rows and Cols do in your code, but my example only stores a single array.

Note: This is a specific counter suggestion. I would refactor this on a larger scale, which I'll get to in a moment.

Coordinates

I like that you used a separate class to contain a coordinate pair. This is a very good OOP approach. However, there is room for improvement here.

As I've mentioned before, you shouldn't be storing the diagonal values. These can be calculated on the fly, so it makes no sense to pre-calculate them and then store them. Doing so makes it possible for this stored value to be changed/overwritten at some point (e.g. an unknown bug), and then everything will go haywire.

Your overarching code often relies on two ints to denote a location. It's much more intuitive to use X and Y instead of Col and Row. To be fair, I do think that you're currently thinking in rows/columns and have therefore made your variable reflect your approach.
This paragraph may be more subjective than I currently think it is (I'm open to feedback from readers in the comments).

Consider making Coordinates a struct instead of a class. There are several benefits to doing so

Firstly, as per MSDN:

CONSIDER defining a struct instead of a class if instances of the type are small and commonly short-lived or are commonly embedded in other objects.

Secondly; structs cannot contain null. This actually makes sense for your case, as a null coordinate is not meaningful.

Thirdly, consider this code:

Coordinates c1 = new Coordinates(2,3);
Coordinates c2 = new Coordinates(2,3);

bool areEqual = c1.Equals(c2);

If Coordinates is a class, areEqual will be false. If Coordinates is a struct, areEqual will be true. Using structs will simplify checking for equal values. When comparing classes, two different objects with the same properties will not be considered equal to each other!
Note: This default behavior can be overridden. You can make it so your class compares its properties too. But this requires you to explicitly define the behavior, whereas this is the default behavior for structs.

Coordinates - redundancy

You're storing the same set of coordinates, related to the same cell, twice (the code is simplified):

Cell newCell = new Cell(row,col);

Rows[row][col] = newCell;
Cols[col][row] = newCell;

While this is not technically a problem in and of itself, it leaves the door open to data inconsistencies. You could put put a cell whose location is set to (1,2) on a different location (e.g. (3,4)) on the board. Note that this is further compounded by the fact that you could even use completely different locations in Rows (e.g. (3,4)) and Cols (e.g. (5,6))!

Cell newCell = new Cell(1,2);

Rows[3][4] = newCell;
Cols[5][6] = newCell;

Sure, you could be diligent and make sure that you never store inconsistent values. But a better approach would be to store the values only once. That way, it's impossible for the data to contradict itself, since it's only stored in one place.

There are different approaches here:

  • Keep the coordinates as a property of the cell; but store the cells in an index-less collection (e.g. a List<Cell<T>>)
  • Keep the indexed array, but do not also store the coordinates in the cell.

This is a matter of preference. I'd opt for the first option, but that's because I don't like using array indexes all throughout my code (I find it a bit messy, but I do concede it can be very efficient at times).
Either choice is fine. I mostly want to advocate against doing the same thing multiple times.

To be fair, using the coordinates in both locations (as you do now) is a third viable option. I can see an argument for wanting to access the coordinates in both ways; but this does come at the cost of opening the door to data inconsistency. You can keep doing it the way you've been doing it, just know that it'll cost you a bit more overhead to ensure that you don't make mistakes.

I do still oppose using both Rows and Cols. I see no discernible benefit to have two arrays with inverted indexes.

ToString() abuse

I noticed this:

public override string ToString()
{
    return Value.ToString();
}

This seems like a bad idea. You shouldn't use ToString() as a shortcut to displaying a particular property, unless that property fully and uniquely defines the object.

Note that there is nothing technically wrong with your implementation. But you need to account for the fact that ToString() is heavily used in the overall .NET framework, most notably during error handling. ToString() should give you information about the object in a way that makes sense.

If in your TicTacToe game, you encounter an error when performing a cell operation, would this be a meaningfull error message?

Error: Illegal operation in X.

There may be several cells with an X in them on the board. In which cell did this take place? You simply don't know.

A counterexample:

public override string ToString()
{
    return $"( Row = {this.Location.Row} ; Col = {this.Location.Col} ; Value = {this.Value} )";
}

Looking at the same error message:

Error: Illegal operation in ( Row = 1 ; Col = 2 ; Value = X ).

Note that if you want a method which returns a string version of your value, you can still create one. There's nothing wrong with creating such a method (if it has a justifiable use), but I simply don't agree with using the existing ToString() method to this end.

Parameterless constructors

You've done this in both Coordinates and Cell. Based on what these classes represent, it makes no sense to create a coordinate pair with no coordinates, or a cell with no coordinates and value.

I suggest removing the parameterless constructors. This forces developers to supply relevant information if they want to create an object, since the only constructors that exist require parameters.

A good generalized approach is to have required values as a parameter in your constructor, and have optional values as a publically settable property. This creates cleaner code for whoever uses your class in the future.

Default values

This is a minor comment, but you can simplify how you set the default grid size:

public Grid(int gridSize = DEFAULT_GRID_SIZE)
{
    this.GridSize = gridSize;
}

You don't need a parameterless constructor anymore. If the user does not supply a value, gridSize will automatically be set to the value of DEFAULT_GRID_SIZE. Note that this only works because DEFAULT_GRID_SIZE is a const.


A revised example

public class Coordinates
{
    public int X { get; set; }
    public int Y { get; set; }

    public Coordinates(int x, int y)
    {
        X = x;
        Y = y;
    }
}

public class Cell<T>
{
    public T Value { get; set; }
    public Coordinates Location { get; set; }

    public Cell(int x, int y)
    {
        Value = default(T);
        Location = new Coordinates(x, y);
    }
}

public class GameGrid<T>
{
    const int DEFAULT_GRID_SIZE = 3;
    protected int GridSize { get; set; }
    protected Cell<T>[,] Board { get; set; }

    public Grid(int gridSize = DEFAULT_GRID_SIZE)
    {
        this.GridSize = gridSize;
        InitGrid();
    }

    private void InitGrid()
    {
        Board = new Cell[GridSize,GridSize];
        for (int x = 0; x < GridSize; x++)
        {
            for (int y = 0; y < GridSize; y++)
            {
                Board[x][y] = new Cell<T>(x,y);
            }
        }
    }

    public enum TopBottom { Top, Bottom }
    public enum LeftRight { Left, Right }

    public Cell GetDiagonalCell(Coordinates source, TopBottom topbottom, LeftRight leftright)
    {
         if((source.Location.X == 0 && leftright == LeftRight.Left)
             || (source.Location.X == GridSize-1 && leftright == LeftRight.Right)
             || (source.Location.Y == 0 && leftright == TopBottom.Top)
             || (source.Location.Y == GridSize-1 && leftright == TopBottom.Bottom))
         {
              //Diagonal falls off the edge of the grid
              return null;
         }

         var destinationX = leftright == LeftRight.Left
                                  ? source.Location.X - 1
                                  : source.Location.X + 1;    
         var destinationY = topbottom == TopBottom.Top
                                  ? source.Location.Y - 1
                                  : source.Location.Y + 1;

         return Board[destinationX,destinationY];
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Perhaps it's easier to see that Cell is also a generic type if you write out its 'full' name: GameGrid<T>.Cell. \$\endgroup\$ – Pieter Witvoet Apr 23 '18 at 9:47
  • \$\begingroup\$ Looks a lot cleaner, perhaps better names for the enums - Vertical, Horizontal or maybe a single enum with Flags attribute. \$\endgroup\$ – Denis Apr 23 '18 at 10:31
  • \$\begingroup\$ @Denis: All good suggestions. I was trying to keep it a bit simpler for the sake of the answer since it was only one of many changes :) \$\endgroup\$ – Flater Apr 23 '18 at 10:44
  • \$\begingroup\$ yeah same here - no idea what this diagonal stuff is about. \$\endgroup\$ – BKSpurgeon Apr 23 '18 at 12:10
  • 1
    \$\begingroup\$ I like this in general, but I'm not so sure that I would allow Cell.Location to be settable. It could allow cells to claim a Location that is different from their position in the Board array, though there are other ways for cells to "get out of sync" even with them lacking a setter. Also, shouldn't your Cell and Coordinate classes be public or internal? protected members directly in a namespace do not compile. \$\endgroup\$ – Bradley Uffner Apr 23 '18 at 15:50
5
\$\begingroup\$

Keep the code simple

I don't really understand what your code is doing or why it is doing what it is doing. So my suggestion is to keep your code simple - simple enough for third parties to understand without much thought. Questions that came to mind:

  • why is there a class for cells? What is it actually doing? Why is there a coordinates class? What is it doing? (added based on Flater's comments: These classes don't really have any behaviour and are basically redundant data structures - it adds needless complexity IMO, but others disagree: Flater suggests that it can be conceptually easier to group together certain concepts in order to make the code less bulky, or perhaps to group together certain concepts to make it easier to understand conceptually. A benefit of that is that you can test for equality - but now we are moving away from the concept of a raw data structure and heading into the territory of behaviour - if that is the case, then there is definitely utility in doing so as per Flater's suggestions.).
  • Why are you initialising to negative values in the constructor and then overwriting it? (Flater says to initialise to a default value, but i'm unconvinced - should coordinates have negative values on a grid like this?)
  • What is the diagonal row thing? What is meant by this? int rightDiagRow = ((GridSize * 2) - 2) - (row + col);? I suggest using more meaningful names and to also extract code into separate methods with meaningful names to make your code easier to read. In other words, put ((GridSize * 2) - 2) - (row + col); into a separate method with a name: createDiagonalGrid() etc. etc.
  • You are exposing almost everything. While this is good if you are creating a data structure, it's not so good if you are trying to create a class which exposes certain behaviour. Hide the interals and give them methods to get the information that they really want without letting them access the actual information directly.

'Keep it simple' is very easy to say - but how does that help you? Having a vague idea of what you might be after, I have attempted to refashion another implementation, which perhaps, might benefit you if perused.

I first started with some tests and then, knowing that I wanted to get and set values and save them in a grid, fashioned the following. You could, of course, change the implementation to get a square grid if required - or in any other way according to your choosing, for example:

The Classes

public abstract class Grid<T>
{
    private T[,] gridRepository;

    public Grid(int rows, int columns)
    {
        this.gridRepository = new T[rows, columns];
    }

    virtual public T GetValue(int rowNumber, int columnNumber)
    {
        return gridRepository[rowNumber, columnNumber];
    }

    virtual public void SetValue(int rowNumber, int columnNumber, T inputItem)
    {
        gridRepository[rowNumber, columnNumber] = inputItem;
    }
}

public class IntGrid : Grid<int>
{
    public IntGrid(int rows, int columns)
        : base(rows, columns)
    {
    }
}

public class StringGrid : Grid<string>
{
    public StringGrid(int rows, int columns)
        : base(rows, columns)
    {
    }
}

Basic Tests

[TestFixture]
public class GridTests
{
    [TestCase]
    public void SetUpGrid_Started_void()
    {
        Grid<int> grid = new IntGrid(3, 4);
        Assert.AreEqual(0, grid.GetValue(1, 3));
    }

    [TestCase]
    public void GetValue_SetAndRetrieve_Returns3()
    {
        Grid<int> grid = new IntGrid(3, 4);
        grid.SetValue(1, 1, 3);

        Assert.AreEqual(3, grid.GetValue(1, 1));
    }

    [TestCase]
    public void GetValue_SetAndRetrieveInStringGrid_Returns_WeAreTheChampions()
    {
        Grid<string> grid = new StringGrid(3, 4);
        grid.SetValue(1, 1, "We are the champions");
        Assert.AreEqual("We are the champions", grid.GetValue(1, 1));
    }
}

Good luck and I hope this helps. You can of course consider cases where a crazy number is requested (i.e. a negative column), or other scenarios where fault initializing values are passed to the constructor, but I hope this is enough to help you get the gist.

\$\endgroup\$
  • 1
    \$\begingroup\$ Some of your bullet points are not really issues; imo. (2) Why is there a coordinates class? For the same reason that game engines often rely on a Vector class; to group coordinates into a simple object instead of handling int values. Less clutter in method signatures, less variable juggling, easier equality checks. (3) Why are you initialising to negative values in the constructor and then overwriting it? To provide a default value if no constructor arguments were supplied. He uses a negative number because 0 would be a valid coordinate (= the first row, the first column). \$\endgroup\$ – Flater Apr 23 '18 at 12:16
  • \$\begingroup\$ @Flater chrs for the feedbacks i've accordingly updated my answer. \$\endgroup\$ – BKSpurgeon Apr 23 '18 at 13:26
  • 1
    \$\begingroup\$ These classes don't really have any behaviour and are basically redundant data structures - it adds needless complexity IMO That's misrepresenting the argument. The point of using this class is to make the code less bulky; by having to pass a single Coordinates parameter instead of two ints, reducing the amount of variables you have to juggle (myLocation instead of myLocationX and myLocationY), and the added ability to perform a simple equality check (locationA == locationB, as opposed to locationAX == locationBX && locationAY == locationBY). \$\endgroup\$ – Flater Apr 23 '18 at 13:36

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.