TetrisBoard Class in C#

Question

I am writing a Tetris program in C# for programming practice, and I've got it working. This is the second post in a multi-part series. My previous post covered the TetrisPiece. You can view the TetrisPiece code and critique here. Future posts may cover the WinForm, the AI, and maybe a graphical board and sounds that I will code later.

Design

Some design decisions worth mentioning before you read the code include:

• The board is represented by a char array.
• Pieces are placed on the board by taking a 5x5 int array with the piece in it, overlapping it with the board char array, and making sure the destination squares are empty.
• The 5x5 grid's empty spaces are allowed to hang off of any part of the board. The 5x5 grid's filled spaces are allowed to hang off the top of the board (in case the player rotates a newly spawned piece that is still located on the top row), but not the sides or bottom.
• The piece is kept floating (char[,] BoardWithoutCurrentPiece and the 5x5 grid are kept separate) until a timer tick when it can't move down anymore. At that point the piece is added to BoardWithoutCurrentPiece and a new piece is created.

Feedback

These were commented on in the Code Review for the TetrisPiece class, so no need to repeat them here. I will incorporate the feedback when I revise the code for both classes:

• C# variables should be in camelCase.
• Convert constant lists to enums.
• The TetrisPiece class should be broken into classes for PieceProvider, Piece, and the 7 pieces.

Specific areas that might need improvement include:

• I ended up writing a lot of array manipulation functions, such as FillCharArrayWithChar() and Copy2DCharArray(). Perhaps there are native methods for these already in the C# libraries.
• In the past, it has been suggested that I not use if x == true and if x == false. I find it increases code readability though. Thoughts?
• To test if a move is legal, I make a deep copy of the piece, then move or rotate the TestPiece. If moving/rotating doesn't throw an error, I then set CurrentPiece = TestPiece. Is this the best technique for this?
• I read that using comments is bad, and that code should be written in a way that comments are not needed. Do I have too many comments?
• I read that having a bunch of nested loops and conditionals usually means that the method should be broken into more methods. PlacePiece() has a lot of nested loops and conditionals. Does that mean I should break that method into more methods? Any suggestions on how to do that?
• Should the exception classes be moved into their own file?
• Should the exception classes be shorter? I copied the code from somewhere else, and that code was a bit verbose.

---

using System;
using System.Diagnostics;

namespace MillenniumTetris
{
    class TetrisBoard
    {
        // TODO: Convert variables to camelCase

        #region Variables, Constants, Literals
        private char[,] BoardWithoutCurrentPiece;
        private char[,] BoardWithCurrentPiece;
        private TetrisPiece CurrentPiece;
        // TODO: private TetrisPiece NextPiece;

        private const int BOARD_ROWS = 17;
        private const int BOARD_COLUMNS = 10;
        // TODO: Test different board sizes, especially board widths. Make sure the new pieces
            // come in centered, and that BOARD_ROWS = 4 doesn't throw errors.

        private const bool ENFORCE_TOP_OF_BOARD_BOUNDARY = true;
        private const bool DO_NOT_ENFORCE_TOP_OF_BOARD_BOUNDARY = false;
        #endregion

        #region Public Methods
        public TetrisBoard()
        {
            BoardWithoutCurrentPiece = new char[BOARD_ROWS, BOARD_COLUMNS];

            BoardWithoutCurrentPiece = FillCharArrayWithChar(BoardWithoutCurrentPiece, ' ');

            BoardWithCurrentPiece = Copy2DCharArray(BoardWithoutCurrentPiece);
        }

        public void Tick()
        {
            if (CurrentPiece == null)
            {
                SpawnNewPiece();
            }
            else
            {
                MovePieceDownAndTryToSnap();
            }
        }

        public char[,] GetBoardAsCharArray()
        {
            return BoardWithCurrentPiece;
        }

        public void RotatePiece(TetrisPiece.Orientation TypeOfRotation)
        {
            TetrisPiece TestPiece = new TetrisPiece(CurrentPiece);
            TestPiece.Rotate(TypeOfRotation);

            try
            {
                PlacePiece(TestPiece, DO_NOT_ENFORCE_TOP_OF_BOARD_BOUNDARY);

                // if no errors, execute this code to move the piece
                CurrentPiece = TestPiece;
                // TODO: play sound
            }
            catch (Exception ex) when (ex is SquareOccupiedException || ex is SquareOffTheBoardException)
            {
                // If the piece can't be rotated, don't do anything. We don't want to throw a
                // program-ending exception, and we don't want to snap the piece into place.
            }
        }

        public void MovePieceOneSquare(TetrisPiece.Direction myDirection)
        {
            TetrisPiece TestPiece = new TetrisPiece(CurrentPiece);
            TestPiece.MoveOneSquare(myDirection);

            try
            {
                PlacePiece(TestPiece, DO_NOT_ENFORCE_TOP_OF_BOARD_BOUNDARY);

                // if no errors, execute this code to move the piece
                CurrentPiece = TestPiece;
            }
            catch (Exception ex) when (ex is SquareOccupiedException || ex is SquareOffTheBoardException)
            {
                // If the piece can't be moved, don't do anything. We don't want to throw a
                // program-ending exception, and we don't want to snap the piece into place.
                // To snap the piece into place, use the MovePieceDownAndTryToSnap() method.
            }
        }
        #endregion

        #region Private Methods
        private void MovePieceDownAndTryToSnap()
        {
            TetrisPiece TestPiece = new TetrisPiece(CurrentPiece);
            TestPiece.MoveOneSquare(TetrisPiece.Direction.Down);

            try
            {
                PlacePiece(TestPiece, DO_NOT_ENFORCE_TOP_OF_BOARD_BOUNDARY);

                // if no errors, execute this code to move the piece
                CurrentPiece = TestPiece;
            }
            catch (Exception ex) when (ex is SquareOccupiedException || ex is SquareOffTheBoardException)
            {
                SnapPieceIntoPlace();
            }
        }

        private char[,] Copy2DCharArray(char[,] SourceArray)
        {
            int rows = SourceArray.GetLength(0);
            int columns = SourceArray.GetLength(1);

            char[,] ArrayCopy = new char[rows, columns];

            for (int RowNum = 0; RowNum < rows; RowNum++)
            {
                for (int ColNum = 0; ColNum < columns; ColNum++)
                {
                    ArrayCopy[RowNum, ColNum] = SourceArray[RowNum, ColNum];
                }
            }

            return ArrayCopy;
        }

        private char[,] FillCharArrayWithChar(char[,] ArrayToFill, char CharToFill)
        {
            int rows = ArrayToFill.GetLength(0);
            int columns = ArrayToFill.GetLength(1);

            for (int RowNum = 0; RowNum < rows; RowNum++)
            {
                for (int ColNum = 0; ColNum < columns; ColNum++)
                {
                    ArrayToFill[RowNum, ColNum] = CharToFill;
                }
            }

            return ArrayToFill;
        }

        private void PlacePiece(TetrisPiece Piece, bool EnforceTopOfBoardBoundary)
        {
            // TODO: check for ArgumentException
            // TODO: check shape grid size, make sure it is 5, else throw error
            int[,] Shape = Piece.Shape;
            int[] Location = Piece.Location;
            char OneLetterName = Piece.OneLetterName;

            char[,] TestBoard = Copy2DCharArray(BoardWithoutCurrentPiece);

            // TODO: refactor to eliminate magic number 5
            for (int row = 0; row < 5; row++)
            {
                for (int column = 0; column < 5; column++)
                {
                    if (Shape[row, column] == 1)
                    {
                        int RowToTry = Location[0] + row;
                        int ColumnToTry = Location[1] + column;

                        if (RowToTry > (BOARD_ROWS - 1) || ColumnToTry > (BOARD_COLUMNS - 1) || ColumnToTry < 0)
                        {
                            throw new SquareOffTheBoardException("Unable to place piece because a target square is off the board.");
                        }
                        else if (RowToTry < 0 && EnforceTopOfBoardBoundary == false)
                        {
                            // Do nothing. This just means the piece is hanging off the top of
                            // the board. This is allowed, but trying to write it to the board
                            // would throw an exception.
                        }
                        else if (RowToTry < 0 && EnforceTopOfBoardBoundary == true)
                        {
                            throw new SquareOffTheBoardException("Unable to place piece because a target square is off the board.");
                        }
                        else if (TestBoard[RowToTry, ColumnToTry] != ' ')
                        {
                            throw new SquareOccupiedException("Unable to place piece because a target square is not empty.");
                        }
                        else
                        {
                            TestBoard[RowToTry, ColumnToTry] = OneLetterName;
                        }
                    }
                }
            }

            // if no errors, go ahead and finalize the piece move
            BoardWithCurrentPiece = Copy2DCharArray(TestBoard);
        }

        private void SnapPieceIntoPlace()
        {
            // TODO: Play sound.
            PlacePiece(CurrentPiece, ENFORCE_TOP_OF_BOARD_BOUNDARY);
            BoardWithoutCurrentPiece = Copy2DCharArray(BoardWithCurrentPiece);

            RemoveFullRows();

            SpawnNewPiece();
        }

        private void RemoveFullRows()
        {
            char[,] BoardChanges = new char[BOARD_ROWS, BOARD_COLUMNS];
            BoardChanges = FillCharArrayWithChar(BoardChanges, ' ');
            bool RowFull;
            bool RowsWereRemoved = false;
            int NextRowToWrite = BOARD_ROWS - 1;

            for (int RowNum = BOARD_ROWS - 1; RowNum >= 0; RowNum--)
            {
                RowFull = true;

                for (int ColNum = 0; ColNum < BOARD_COLUMNS; ColNum++)
                {
                    if (BoardWithCurrentPiece[RowNum, ColNum] == ' ')
                    {
                        RowFull = false;
                        break;
                    }
                }

                if (RowFull == false)
                {
                    BoardChanges = CopyCharArrayRow(BoardWithCurrentPiece, RowNum, BoardChanges, NextRowToWrite);
                    NextRowToWrite--;
                }
                else
                {
                    RowsWereRemoved = true;
                }
            }

            BoardWithCurrentPiece = Copy2DCharArray(BoardChanges);
            BoardWithoutCurrentPiece = Copy2DCharArray(BoardChanges);

            // TODO: Check if RowsWereRemoved = True
                // If so, increment Lines box on WinForm and play a sound
        }

        private char[,] CopyCharArrayRow(char[,] SourceArray, int SourceRow, char[,] DestinationArray, int DestinationRow)
        {
            int SourceArrayRows = SourceArray.GetLength(0);
            int SourceArrayColumns = SourceArray.GetLength(1);
            int DestinationArrayRows = DestinationArray.GetLength(0);
            int DestinationArrayColumns = DestinationArray.GetLength(1);

            if (SourceArrayRows != DestinationArrayRows || SourceArrayColumns != DestinationArrayColumns)
            {
                throw new ArgumentException("Source and destination arrays must be sized identically to perform row copy operation.");
            }

            for ( int ColNum = 0; ColNum < SourceArrayColumns; ColNum++ )
            {
                DestinationArray[DestinationRow,ColNum] = SourceArray[SourceRow,ColNum];
            }

            return DestinationArray;
        }

        private void SpawnNewPiece()
        {
            CurrentPiece = new TetrisPiece();
            CenterNewPiece();

            try
            {
                PlacePiece(CurrentPiece, DO_NOT_ENFORCE_TOP_OF_BOARD_BOUNDARY);
            }
            catch (SquareOffTheBoardException)
            {
                throw new BoardTooNarrowException("The board is too narrow. Unable to spawn new piece.");
            }
        }

        private void CenterNewPiece()
        {
            int ColumnLocation = (int)Math.Round((BOARD_COLUMNS + 0.5) / 2 - 3, 0);
            CurrentPiece.ChangeLocation(new int[2] { -2, ColumnLocation });
        }
        #endregion
    }
}

#region Exceptions
public class SquareOccupiedException : Exception
{
    public SquareOccupiedException()
    {

    }

    public SquareOccupiedException(string message) : base(message)
    {

    }

    public SquareOccupiedException(string message, Exception inner) : base(message, inner)
    {

    }
}

public class SquareOffTheBoardException : Exception
{
    public SquareOffTheBoardException()
    {

    }

    public SquareOffTheBoardException(string message) : base(message)
    {

    }

    public SquareOffTheBoardException(string message, Exception inner) : base(message, inner)
    {

    }
}

public class BoardTooNarrowException : Exception
{
    public BoardTooNarrowException()
    {

    }

    public BoardTooNarrowException(string message) : base(message)
    {

    }

    public BoardTooNarrowException(string message, Exception inner) : base(message, inner)
    {

    }
}
#endregion

Answer 1

Personally, I really hate //TODO statements, because more often than not they are used to make an excuse for a bad code and/or laziness. In a project I am currently working on I recently found the following comment left by one of my coworkers:

//ToDo: add a null check

which dated back to 2014 according to version control. I mean come on. If you need to rename your fields or throw an exception - then for the love of god do so right away. Not only it takes a couple of seconds with modern refactoring tools, but it will also show us, that you actually worked on your code since your last review and improved it accordingly! And to add to the list: constants should be PascalCase in C#. No underscores in-between the words and no capslock.

---

I ended up writing a lot of array manipulation functions, such as FillCharArrayWithChar() and Copy2DCharArray(). Perhaps there are native methods for these already in the C# libraries.

I don't think there is a method which allows you to fill a 2d array. But it is easy enough to write a generic extension method, which will do just that. You have basically implemented it, all that is left is to move FillCharArrayWithChar to separate static class and make it generic.

---

In the past, it has been suggested that I not use if x == true and if x == false. I find it increases code readability though. Thoughts?

Yes, you should omit == true and == false. Those only take space and do not provide any additional information. In C# you can't put anything but bool inside if statement. The only time you might need to write if (x == false) is if x is bool?.

---

I read that using comments is bad, and that code should be written in a way that comments are not needed. Do I have too many comments?

I read that having a bunch of nested loops and conditionals usually means that the method should be broken into more methods. PlacePiece() has a lot of nested loops and conditionals. Does that mean I should break that method into more methods? Any suggestions on how to do that?

Here is the rule of a thumb I use. Whenever I think that I need to write a comment inside method body, I ask myself three questions:

1. Can I extract the section of the code I want to comment on to the separate method with descriptive name?
2. Is my algorithm too complex to understand and maintain and can it be simplified?
3. Does my class do too much work and can I break it down to smaller classes dividing the work and responsibilities between them?

Only if I answer "no" to all of those questions I write a comment.

In your case I would say, that there are places, where you clearly can extract a method. For example, this:

ThrowIfSquareOffTheBoard(...);
if (IsPeiceHangsOffTheTop(...)) continue;
TestBoard[RowToTry, ColumnToTry] = OneLetterName;

looks so much better, than a complex if-else statement you have.

---

Should the exception classes be moved into their own file?

Yes. Unless you are having a bunch of really small classes, you should try to have one class per file.

---

Should the exception classes be shorter? I copied the code from somewhere else, and that code was a bit verbose.

You should probably remove the constructors you don't actually use. Otherwise they look fine. What does not look fine is the way you use exceptions. Exceptions are... "exceptional". They signal that something really bad happened: an invalid state or an error in the workflow which your class can't handle. Exceptions should NOT be used as part of your regular workflow, especially not if you catch those exceptions straight away with an empty catch statements (which are bad enough on their own). In those situations use return instead. Here is a quote from MSDN:

While the use of exception handlers to catch errors and other events that disrupt program execution is a good practice, the use of exception handler as part of the regular program execution logic can be expensive and should be avoided. In most cases, exceptions should be used only for circumstances that occur infrequently and are not expected.. Exceptions should not be used to return values as part of the typical program flow. In many cases, you can avoid raising exceptions by validating values and using conditional logic to halt the execution of statements that cause the problem.

---

I won't comment much on your game logic or design because I don't understand it. All I can say is that it looks weird. You are using a high level programming language with powerful support of OOP, yet you are using... 2d char array to represent a board? 2d int array to represent a piece? Imagine if you, for example, had a LinkedList<Row> Rows (or even regular List) as your board representation. Then your RemoveFullRows method would look like:

while(Rows.First.IsFull)
{
    Rows.RemoveFirst();
    Rows.AddLast(new Row()); 
}

Very simple. No array copying. Major improvement over your current implementation. That is what using higher levels of abstraction can do for you!