Return to Revisions

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 : )