Code review by Volodymyr Voityshyn

Tetris.ConsoleUI/ConsoleDrawing.cs

@@ -24,6 +24,9 @@ class ConsoleDrawing : IDrawing
 
         public void DrawScene(Game game)
         {
+            /*
+             * Reivew VV: для чого тут слід використовувати критичну секцію?
+             */
             lock (game)
             {
                 //Current Position
@@ -92,13 +95,19 @@ public void ShowGameOver(Game game)
             }
         }
 
+        /*
+         * Review VV: чому ця функція статична?
+         */
         private static void ShowLines(Game game)
         {
             Console.CursorLeft = game.ActualBoard.Width + 4;
             Console.CursorTop = 7;
             Console.WriteLine("Lines: {0}", game.Lines);
         }
 
+        /*
+         * Review VV: чому ця функція статична?
+         */
         private static void ShowScore(Game game)
         {
             Console.CursorLeft = game.ActualBoard.Width + 4;
@@ -168,6 +177,10 @@ private void DrawArray(int[,] arr, bool border)
             }
         }
 
+
+        /*
+         * Review VV: чому ця функція статична?
+         */
         /// <summary>
         /// Writes a String in the Specified foreground color,
         /// then switches the Color back

Tetris.ConsoleUI/Program.cs

@@ -21,6 +21,10 @@ static int Main(string[] args)
 
             drawer = new ConsoleDrawing();
 
+            /*
+             * Review VV: чому тут використовується статична функція?
+             * Чи можна це замінити на drawer.ShowControls()?
+             */
             ConsoleDrawing.ShowControls();
 
             Console.ReadKey(true);

Tetris.GameEngine.Test/PieceFactoryTest.cs

@@ -41,6 +41,9 @@ public void TestGetRandomPiece()
             Assert.IsNotNull(gen_piece);
         }
 
+        /*
+         * Review VV: цей метод повністю повторює функціональність першого - TestInitialization()
+         */
         [TestMethod]
         public void TestGetCount()
         {

Tetris.GameEngine/Board.cs

@@ -27,6 +27,7 @@ public Board(int width, int height)
             }
             else
             {
+                // Reivew VV: рекомендую валідацію писати на початку методу і брати в регіон "Valiation"
                 throw new ArgumentException("Board must be at least 10x20");
             }
         }
@@ -144,6 +145,8 @@ public int Height
         {
             get
             {
+                // Review VV: чому тут слід використовувати Array.GetUpperBound()? 
+                // Чи є альтернативни цій функції?
                 return _mBoard.GetUpperBound(0) + 1;
             }
         }
@@ -152,6 +155,8 @@ public int Width
         {
             get
             {
+                // Review VV: чому тут слід використовувати Array.GetUpperBound()? 
+                // Чи є альтернативни цій функції?
                 return _mBoard.GetUpperBound(1) + 1;
             }
         }

Tetris.GameEngine/Game.cs

@@ -5,6 +5,7 @@ namespace Tetris.GameEngine
 {
     public class Game: IMovable, IMode
     {
+        // Review VV: рекомендую замінити "static readonly" на "const"
         private static readonly int _board_width = 10;
         private static readonly int _board_height = 20;
 
@@ -21,6 +22,7 @@ public enum GameStatus
         /// <summary>
         /// The Playfield
         /// </summary>
+        // Review VV: невірне іменування: слід замінити на _gameBoard
         private Board _game_board;
         private GameStatus _status;
         private Piece _currPiece;
@@ -66,6 +68,9 @@ public void Start()
 
         public void Pause()
         {
+            /*
+             * Review VV: чи можна викликати цей метод, якщо гра знаходиться у статусах ReadyToStart чи Finished?
+             */
             if (this._status == GameStatus.InProgress)
             {
                 this._status = GameStatus.Paused;
@@ -105,6 +110,11 @@ public int PosY
             }
         }
 
+        /*
+         * Reivew VV: What does this method do?
+         * Is it correct to use Clone()?
+         * Is there more optimal approach?
+         */
         public Board ActualBoard
         {
             get
@@ -253,11 +263,17 @@ public void Rotate()
 
         #region IMode Implementation
 
+        /*
+         * Review VV: what is the purpose of it?
+         */
         public bool NextPieceMode
         {
             get; set;
         }
 
+        /*
+         * Review VV: what is the purpose of it?
+         */
         public bool ShadowPieceMode
         {
             get; set;

Tetris.GameEngine/Interfaces/IDrawing.cs

@@ -2,6 +2,10 @@
 
 namespace Tetris.GameEngine.Interfaces
 {
+    /*
+     * Review VV: думаю, цей інтерфейс не повинен бути у зборці Tetris.GameEngine.
+     * Оскільки ця зборка відповідає тільки за логіку гри і не повинна відповідати за відображення.
+     */
     public interface IDrawing
     {
         void DrawScene(Game game);

Tetris.GameEngine/Piece.cs

@@ -5,7 +5,11 @@ namespace Tetris.GameEngine
     public class Piece: ICloneable
     {
         #region Private Fields
-
+        /*
+         * Review VV: слід додати модифікатор доступу 'private'.
+         * Це питання стилю.
+         * Я рекомендую завжди явно вказувати специфікатор доступу.
+         */ 
         int[,] _piece;
         int _initPosX;
         int _initPosY;
@@ -20,6 +24,7 @@ public Piece(int[,] p)
             {
                 throw new NullReferenceException();
             }
+            // Reive VV: для чого потрібно клонувати цей масив?
             _piece = (int[,])p.Clone();
             _initPosY = (p.GetUpperBound(0) + 1) * -1;
             _initPosX = 0;

Tetris.GameEngine/PieceFactory.cs

@@ -3,6 +3,16 @@
 
 namespace Tetris.GameEngine
 {
+    /*
+     * Review VV: 
+     *      - виділення окремої фабрики для фігурок - правильне рішення
+     *      
+     *      Рекомендую скористатися принципом Dependency Inversion:
+     *          - реалізувати фабрику як нестатичний клас
+     *          - виділити для неї інтерфейс IPieceFactory
+     *          - передавати екземпляр фабрики як параметр конструктора у Game
+     *          - але список _pieces варто залишити статичним
+     */
     public static class PieceFactory
     {
         #region Private Fields
@@ -33,6 +43,10 @@ static PieceFactory()
         /// </summary>
         /// <param name="ID">ID of Piece (0-6)</param>
         /// <returns>the Piece (or null if invalid Value)</returns>
+        /// 
+        /*
+            Review VV: ID повинно бути маленькими літерами - id
+        */
         public static Piece GetPiecebyId(int ID)
         {
             if (_pieces.Count > ID && ID >= 0)