TetrisPiece Class in C#

I am writing a Tetris program in C# for programming practice, and I've got it working. This will be the first post in a multi-part series. Future posts will cover the TetrisBoard (the game logic), 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:

• I chose to use a 5x5 char array to represent each piece. 0 is empty, 1 is filled.

• For piece rotation, the center square of the 5x5 array is used as the pivot point.

• The top two rows of the 5x5 array start hanging off the top of the board, and are only used if the piece gets rotated.

• I particularly like the simple solution I came up with to rotate the 5x5 board in the RotateSquareArray() method. I was reading another answer on StackOverflow and they were using trigonometry (sine, cosine) to rotate array contents. That just seemed way too complicated. If you plot out the coordinates in a table, the number changing pattern is clear, and has been captured in my RotateSquareArray() method.

• The piece letters are short for their colors. I had the colors in here before, but I moved them to the graphics class so that the graphics class can choose the color of each piece.

Feedback

Specific areas that might need improvement include:

• I'm still getting used to C# styling conventions. Things like camelCase vs PascalCase, and if ( conditional ) vs if(conditional).

• Those CLOCKWISE and RIGHT/LEFT constants might need a better data structure. Enum?

• Any advice on the code in TetrisPiece(TetrisPiece PieceToCopy) that deep copies the class? I'm surprised there isn't a "copy" keyword or something in the C# language.

• I might delete the Orientation variable. The TetrisBoard class never needs it or uses it.

```
using System;

namespace MillenniumTetris
{
public class TetrisPiece
{
#region Variables, Constants, Literals
private string FullName;
private char OneLett

Let's start with small stuff.

Casing is indeed wrong - it should be lower (Pascal) case for variables, parameters and private fields.

So, eg. instead of

for (int ColNum = 0; ColNum < columns; ColNum++)

should be:

for (int colNum = 0; colNum < columns; colNum++)

Refer to https://msdn.microsoft.com/en-us/library/ms229043%28v=vs.100%29.aspx for more info.

In C# it is also a bit alien to use Get... methods, because C# has a thing called properties. More info: https://msdn.microsoft.com/en-us/library/x9fsa0sw.aspx

Properties allow to replace

private string fullName;

public string GetFullName()
{
    return FullName;
}

with:

public string FullName
{
    // "get" is implicitly public, because FullName itself is public
    get;

    // this mirrors your implementation, but you can use any access modifiers in properties
    private set; 
}

That's idiomatic C#.

You are correct that constants could be replaced with enums.

I have to say I don't particularly like the design of TetrisPiece. It feels very wrong to me that the shape is chosen at random shape in the constructor. TetrisPiece represents a single piece - and randomizing pieces isn't a responsibility of a single piece. This violates Single Responsibility Principle and Open/Closed Principle.

Here's how I would tackle it, thinking out loud - obviously there isn't one right (or wrong) solution, I'll just try to sort of pitch my OOD approach to you.

I'd make TetrisPiece an abstract class, in which Shape, FullName and OneLetterName are abstract properties.

Then I would define concrete pieces (such as Stick) as its subclasses, and create a multiton - https://en.wikipedia.org/wiki/Multiton_pattern - containing all possible pieces.

(If you used Java, you could use an enum - Java enums are more powerful than their C# counterpart, see https://stackoverflow.com/questions/469287/c-sharp-vs-java-enum-for-those-new-to-c - but we can emulate it in C# with multiton pattern).

I would keep them immutable (read-only): these are our Platonic ideas of pieces, if you will, they themselves don't ever change. A stick is always the same stick. Only a single copy of a Stick is ever allowed to exist.

Let's rename it from TetrisPiece to TetrisShape to acknowledge that.

Now, pieces in the sense of multiple pieces located on the screen, these can be numerous. That would be our TetrisPieces. Each TetrisPiece has a readonly Shape property, set to one of predefined TetrisShapes.

A TetrisPiece knows its Shape, and it knows its Location and Roration. The former two can be mutable.

And the TetrisPiece exposes a Sprite matrix (as in https://en.wikipedia.org/wiki/Sprite_%28computer_graphics%29). The Sprite takes the matrix from the Shape, and transforms it accordingly to the piece's Rotation value.

The result could be cached, or always calculated on demand - it's not computationally expensive in this case.

It kind of solves your deep-copying problems. Shapes don't need to be copied: there's only 7 of them in existence (the multiton), and each piece only points to one of them.

I would move randomization of pieces into another class - let's call it PieceProvider. Objects galore, but, well, that's consistent with Single Responsibility Principle. What if one day you wanted to adjust probabilities for different shapes? You shouldn't have to meddle with the implementation of the TetrisPiece itself. What if you wanted these probabilities different for various difficulty levels, for instance? Would you then supply the TetrisPiece constructor with yet another parameter? That's an increasingly clever piece we have, whereas it would be much simplier to just switch between different PieceProviders.

And one final remark: note I didn't name it TetrisPieceProvider. Indeed, I would drop "Tetris" from the all class names.

It's not so jarring when you only have one class (in which, in my opinion, you've shoved too many responsibilities), but it's redundant anyway. We're in the "MillenniumTetris" namespace already - obviously we're not talking chess pieces here.

See http://blog.codinghorror.com/new-programming-jargon/ :)

Smurf Naming Convention

When almost every class has the same prefix. IE, when a user clicks on the button, a SmurfAccountView passes a SmurfAccountDTO to the SmurfAccountController. The SmurfID is used to fetch a SmurfOrderHistory which is passed to the SmurfHistoryMatch before forwarding to either SmurfHistoryReviewView or SmurfHistoryReportingView. If a SmurfErrorEvent occurs it is logged by SmurfErrorLogger to ${app}/smurf/log/smurf/smurflog.log

So it's Piece, Shape, PieceProvider(Dealer?) etc. that they're Tetris-related goes without saying at this point : )

StackExchange Code Review Q#124601, answer score: 3