TetrisBoard Class in C#

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 o

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:

• Can I extract the section of the code I want to comment on to the separate method with descriptive name?

• Is my algorithm too complex to understand and maintain and can it be simplified?

• 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 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

StackExchange Code Review Q#124650, answer score: 2