5
\$\begingroup\$

please comment on game flow, Solid principle and OOP concepts.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Chess
{
    class Program
    {
        static void Main(string[] args)
        {
            ChessGame game = new ChessGame();
        }
    }

    public class ChessGame
    {
        public ChessGame()
        {
            ChessPlayer blackPlayer = new ChessPlayer();
            ChessPlayer whitePlayer = new ChessPlayer();
            Board board = new Board(blackPlayer, whitePlayer);

            while (!board.IsGameOver())
            {
                Console.WriteLine("white player move");

                board.PositionPiece(whitePlayer.GetPieceAtLocation(1, 0), new Position(2, 0));
                if (board.IsGameOver())
                {
                    break;
                }
                Console.WriteLine("black player move");
                board.PositionPiece(blackPlayer.GetPieceAtLocation(7, 0), new Position(6, 0));
            }
        }
    }

    public class ChessPlayer
    {
        public List<ChessPiece> PiecesList { get; private set; }

        public ChessPlayer()
        {
            PiecesList = new List<ChessPiece>();
        }
        public ChessPiece GetPieceAtLocation(int h, int w)
        {
            var tempPiece = PiecesList.FirstOrDefault(x => x.Position.Height == h && x.Position.Width == w);
            if (tempPiece == null)
            {
                Console.WriteLine("no piece in location h:" + h + "w:" + w);
                return tempPiece;
            }
            return tempPiece;
        }
    }

    /// <summary>
    /// the board should position the pieces
    /// the board knows where all the piece are and decides if the move is not being block by other pieces
    /// </summary>
    public class Board
    {
        private readonly ChessPlayer _blackPlayer;
        private readonly ChessPlayer _whitePlayer;
        private Rect[,] _rects;
        public readonly int BoardSize = 8;
        public Board(ChessPlayer blackPlayer, ChessPlayer whitePlayer)
        {
            _blackPlayer = blackPlayer;
            _whitePlayer = whitePlayer;
            InitBoardColors();
            //init the position of the troops at the top of the board
            InitBlackPieces();
            InitWhitePieces();
        }

        private void InitWhitePieces()
        {
            Bishop whiteBishopLeft = new Bishop(new Position(0, 0));
            Knight whiteKnightLeft = new Knight(new Position(0, 1));
            Rook whiteRookLeft = new Rook(new Position(0, 2));
            King whiteKing = new King(new Position(0, 3));
            Queen whiteQueen = new Queen(new Position(0, 4));
            Rook whiteRookRight = new Rook(new Position(0, 5));
            Knight whiteKnightRight = new Knight(new Position(0, 6));
            Bishop whiteBishopRight = new Bishop(new Position(0, 7));

            _whitePlayer.PiecesList.Add(whiteBishopLeft);
            _whitePlayer.PiecesList.Add(whiteKnightLeft);
            _whitePlayer.PiecesList.Add(whiteRookLeft);
            _whitePlayer.PiecesList.Add(whiteKing);
            _whitePlayer.PiecesList.Add(whiteQueen);
            _whitePlayer.PiecesList.Add(whiteKing);
            _whitePlayer.PiecesList.Add(whiteRookRight);
            _whitePlayer.PiecesList.Add(whiteKnightRight);
            _whitePlayer.PiecesList.Add(whiteBishopRight);

            for (int i = 0; i < BoardSize; i++)
            {
                WhitePawn blackPawn = new WhitePawn(new Position(1, i));
                _whitePlayer.PiecesList.Add(blackPawn);
            }

            foreach (var temp in _blackPlayer.PiecesList)
            {
                _rects[temp.Position.Height, temp.Position.Width].PieceType = temp.PieceType;
            }
        }

        private void InitBlackPieces()
        {
            Bishop blackBishopLeft = new Bishop(new Position(7,0));
            Knight blackKnightLeft  = new Knight(new Position(7,1));
            Rook blackRookLeft = new Rook(new Position(7,2));
            King blackKing = new King(new Position(7,3));
            Queen blackQueen = new Queen(new Position(7,4));
            Rook blackRookRight = new Rook(new Position(7, 5));
            Knight blackKnightRight = new Knight(new Position(7, 6));
            Bishop blackBishopRight = new Bishop(new Position(7, 7));

            _blackPlayer.PiecesList.Add(blackBishopLeft);
            _blackPlayer.PiecesList.Add(blackKnightLeft);
            _blackPlayer.PiecesList.Add(blackRookLeft);
            _blackPlayer.PiecesList.Add(blackKing);
            _blackPlayer.PiecesList.Add(blackQueen);
            _blackPlayer.PiecesList.Add(blackKing);
            _blackPlayer.PiecesList.Add(blackRookRight);
            _blackPlayer.PiecesList.Add(blackKnightRight);
            _blackPlayer.PiecesList.Add(blackBishopRight);

            for (int i = 0; i < BoardSize; i++)
            {
                BlackPawn blackPawn = new BlackPawn(new Position(6, i));
                _blackPlayer.PiecesList.Add(blackPawn);
            }

            foreach (var temp in _blackPlayer.PiecesList)
            {
                _rects[temp.Position.Height, temp.Position.Width].PieceType = temp.PieceType;
            }
        }

        private void InitBoardColors()
        {
            _rects = new Rect[BoardSize, BoardSize];
            //init the colors of the rects on the board
            var temp = RectColor.White;
            for (int h = 0; h < BoardSize; h++)
            {
                for (int w = 0; w < BoardSize; w++)
                {
                    _rects[h, w] = new Rect(temp,PieceType.Empty);
                    if (temp == RectColor.White)
                    {
                        temp = RectColor.Black;
                    }
                    else
                    {
                        temp = RectColor.White;
                    }
                }
            }
        }

        public bool PositionPiece(ChessPiece chessPiece, Position dest)
        {
            if (!chessPiece.IsValidMove(dest))
            {
                return false;
            }
            if (_rects[dest.Height, dest.Width].PieceType == PieceType.Empty)
            {
                _rects[dest.Height, dest.Width].PieceType = chessPiece.PieceType;
                chessPiece.Position.Width = dest.Width;
                chessPiece.Position.Height = dest.Height;
                return true;
            }
            else
            {
                //check friend or enemy
                if (_blackPlayer.PiecesList.Contains(chessPiece))
                {
                    if (_blackPlayer.GetPieceAtLocation(dest.Height, dest.Width) != null)
                    {
                        //we are trying to put the piece on another black piece
                        return false;
                    }
                    else
                    {
                        var tempWhite = _whitePlayer.GetPieceAtLocation(dest.Height, dest.Width);
                        if (tempWhite != null)
                        {
                            //killed white walker!
                            _whitePlayer.PiecesList.Remove(tempWhite);
                            _rects[dest.Height, dest.Width].PieceType = chessPiece.PieceType;
                            chessPiece.Position.Width = dest.Width;
                            chessPiece.Position.Height = dest.Height;
                            return true;
                        }
                        //error
                    }
                }
                //error
            }
            return false;
        }

        public bool IsGameOver()
        {
            // can the king of the black or the white still move?
            return false;
        }
    }

    public class Rect
    {
        public RectColor RectColor { get; set; }
        public PieceType PieceType { get; set; }
        public Rect(RectColor rectColor, PieceType pieceType)
        {
            RectColor = rectColor;
            PieceType = pieceType;
        }
    }

    public enum RectColor
    {
        White,
        Black
    }

    public enum PieceType
    {
       Empty,
       King,
       Queen,
       Rook, //runner
       Knight, //horse
       Bishop, //castle
       BlackPawn,
       WhitePawn
    }

    public abstract class ChessPiece
    {
        public PieceType PieceType { get; set; }
        public Position Position { get; set; }
        protected ChessPiece(Position poisition, PieceType pieceType)
        {
            Position = poisition;
            PieceType = pieceType;
        }
        public abstract bool IsValidMove(Position dest);

        protected bool IsOnBoard(Position origin, Position dest)
        {
            if (origin.Height < 0 || origin.Height > 7 || origin.Width < 0 || origin.Width > 7 || dest.Height < 0 || dest.Height > 7 || dest.Width < 0 || dest.Width > 7)
            {
                Console.WriteLine("position piece out or range");
                return false;
            }
            return true;
        }
    }

    public class Knight : ChessPiece
    {
        public Knight(Position poisition)
            : base(poisition, PieceType.Knight)
        {
        }

        public override bool IsValidMove(Position dest)
        {
            if (!IsOnBoard(this.Position,dest))
            {
                return false;
            }

            if (dest.Height == Position.Height + 2 && dest.Width == Position.Width + 1 || dest.Width == (Position.Width - 1))
            {
                return true;
            }
            if (dest.Height == Position.Height - 2 && dest.Width == Position.Width + 1 || dest.Width == (Position.Width - 1))
            {
                return true;
            }
            if (dest.Width == Position.Width - 2 && dest.Height == Position.Height + 1 || dest.Height == Position.Height - 1)
            {
                return true;
            }
            if (dest.Width == Position.Width + 2 && dest.Height == Position.Height + 1 || dest.Height == Position.Height - 1)
            {
                return true;
            }
            return false;
        }
    }

    public class Bishop : ChessPiece
    {
        public Bishop(Position poisition)
            : base(poisition, PieceType.Bishop)
        {
        }

        public override bool IsValidMove(Position dest)
        {
            if (!IsOnBoard(this.Position, dest))
            {
                return false;
            }

            if (Position.Height == dest.Height) //same row
            {
                return true;
            }
            if (Position.Width == dest.Width) //same column
            {
                return true;
            }
            return false;
        }
    }

    public class Rook : ChessPiece
    {
        public Rook(Position poisition)
            : base(poisition, PieceType.Rook)
        {
        }

        public override bool IsValidMove(Position dest)
        {
            if (!IsOnBoard(this.Position, dest))
            {
                return false;
            }
            if (dest.Width - Position.Width == dest.Height - Position.Height) // diagonal
            {
                return true;
            }
            return false;
        }
    }

    public class Queen :ChessPiece
    {
        public Queen(Position poisition) : base(poisition, PieceType.Queen)
        {

        }

        public override bool IsValidMove(Position dest)
        {
            if (!IsOnBoard(this.Position, dest))
            {
                return false;
            }
            if (Position.Height == dest.Height) //same row
            {
                return true;
            }
            if (Position.Width == dest.Width) //same column
            {
                return true;
            }
            if (dest.Width - Position.Width == dest.Height - Position.Height) // diagonal
            {
                return true;
            }
            return false;
        }
    }

    public class King : ChessPiece
    {
        public King(Position poisition): base(poisition, PieceType.King)
        {
        }

        public override bool IsValidMove(Position dest)
        {
            //can't move to a blocked location - the board should do it
            //can't move more than one to each side including diagonal
            if (!IsOnBoard(this.Position, dest))
            {
                return false;
            }
            //one down
            if (Position.Height == dest.Height - 1 && Position.Width == dest.Width)
            {
                return true;
            }
            //one up
            if (Position.Height + 1 == dest.Height && Position.Width == dest.Width)
            {
                return true;
            }
            //one up one right
            if (Position.Height + 1 == dest.Height && Position.Width + 1 == dest.Width)
            {
                return true;
            }
            //one up one left
            if (Position.Height + 1 == dest.Height && Position.Width - 1 == dest.Width)
            {
                return true;
            }
            //one down
            if (Position.Height - 1 == dest.Height && Position.Width == dest.Width)
            {
                return true;
            }
            //one left
            if (Position.Height == dest.Height && Position.Width - 1 == dest.Width)
            {
                return true;
            }

            //one right
            if (Position.Height == dest.Height && Position.Width + 1 == dest.Width)
            {
                return true;
            }

            //one down one left
            if (Position.Height - 1 == dest.Height && Position.Width - 1 == dest.Width)
            {
                return true;
            }

            //one down one right
            if (Position.Height - 1 == dest.Height && Position.Width + 1 == dest.Width)
            {
                return true;
            }
            return false;
        }
    }
    public class BlackPawn : ChessPiece
    {
        public BlackPawn(Position poisition) : base(poisition, PieceType.BlackPawn)
        {

        }

        public override bool IsValidMove(Position dest)
        {
            if (!IsOnBoard(this.Position, dest))
            {
                return false;
            }
            if (Position.Height == dest.Height - 1 && Position.Width == dest.Width)
           {

                return true;
            }
            if (Position.Height == dest.Height && Position.Width == dest.Width - 1)
            {
                return true;
            }
            return false;
        }
    }


    public class WhitePawn : ChessPiece
    {
        public WhitePawn(Position poisition)
            : base(poisition, PieceType.WhitePawn)
        {

        }

        public override bool IsValidMove(Position dest)
        {
            if (!IsOnBoard(this.Position, dest))
            {
                return false;
            }
            if (Position.Height == dest.Height + 1 && Position.Width == dest.Width)
            {
                return true;
            }
            if (Position.Height == dest.Height && Position.Width == dest.Width + 1)
            {
                return true;
            }
            return false;
        }
    }

    public class Position
    {
        public int Height { get; set; }
        public int Width { get; set; }

        public Position(int h, int w)
        {
            Height = h;
            Width = w;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ color is incidental and has no bearing on the general behavior of pieces and players. So make color a property in these classes. Also easy to tell if a piece at a given location is black or white player's with PiecesList.Contains(thisPiece); So construction is "color agnostic" until a set of instantiated pieces is passed to a player at which time the player & pieces color can be set in a nice, tight loop - in the Game.BlackPlayer & Game.WhitePlayer property setters; which very nicely encapsulates all that color that's currently spilled all over the code. \$\endgroup\$ – radarbob Oct 3 '17 at 6:44
  • \$\begingroup\$ Differentiating black/white pieces can be genericised with simple methods like isOpponent, to help test capturing for example. Thus all the functional code, like the construction code (above), can be color agnostic. And so the code as written and read conveys that chess game objects behave the same regardless of color. And the reader is not distracted with trying to find out how and why white pieces move different from black - they must or there wouldn't be so much color-differentiated code everywhere in what otherwise would be duplicate/redundant code. \$\endgroup\$ – radarbob Oct 3 '17 at 6:59
8
\$\begingroup\$
using System.Text;
using System.Threading.Tasks;

What are these doing there? (The obvious answer would be that you used the default template in VS and didn't bother to clean up, but I hope that you have a better explanation than that).


        static void Main(string[] args)
        {
            ChessGame game = new ChessGame();
        }

When I compile this I get a warning.

Generally the idea of a constructor is to construct, not to implement the logic of an entire program.


            ChessPlayer blackPlayer = new ChessPlayer();
            ChessPlayer whitePlayer = new ChessPlayer();
            Board board = new Board(blackPlayer, whitePlayer);

            while (!board.IsGameOver())
            {
                Console.WriteLine("white player move");

                board.PositionPiece(whitePlayer.GetPieceAtLocation(1, 0), new Position(2, 0));
                if (board.IsGameOver())
                {
                    break;
                }
                Console.WriteLine("black player move");
                board.PositionPiece(blackPlayer.GetPieceAtLocation(7, 0), new Position(6, 0));
            }

There are so many things I don't understand here.

  • Why are the players properties of the board?
  • Why is there debug output to stdout?
  • Why is there no input?
  • PositionPiece? GetPieceAtLocation?

            Bishop whiteBishopLeft = new Bishop(new Position(0, 0));
            Knight whiteKnightLeft = new Knight(new Position(0, 1));
            Rook whiteRookLeft = new Rook(new Position(0, 2));
            King whiteKing = new King(new Position(0, 3));
            Queen whiteQueen = new Queen(new Position(0, 4));
            Rook whiteRookRight = new Rook(new Position(0, 5));
            Knight whiteKnightRight = new Knight(new Position(0, 6));
            Bishop whiteBishopRight = new Bishop(new Position(0, 7));

            _whitePlayer.PiecesList.Add(whiteBishopLeft);
            _whitePlayer.PiecesList.Add(whiteKnightLeft);
            _whitePlayer.PiecesList.Add(whiteRookLeft);
            _whitePlayer.PiecesList.Add(whiteKing);
            _whitePlayer.PiecesList.Add(whiteQueen);
            _whitePlayer.PiecesList.Add(whiteKing);
            _whitePlayer.PiecesList.Add(whiteRookRight);
            _whitePlayer.PiecesList.Add(whiteKnightRight);
            _whitePlayer.PiecesList.Add(whiteBishopRight);

All of those variables are used precisely once. Why not inline them?

What is the identity of a chess man? I would say it's colour and type. Its location at any given time is a property of the game, not the man, so I don't understand why locations are being passed into the constructors, nor why the men are not being added to a 2D board structure (perhaps encapsulated by the Board class).


                WhitePawn blackPawn = new WhitePawn(new Position(1, i));
                _whitePlayer.PiecesList.Add(blackPawn);

Copy-paste naming?


            foreach (var temp in _blackPlayer.PiecesList)
            {
                _rects[temp.Position.Height, temp.Position.Width].PieceType = temp.PieceType;
            }

We're still in InitWhitePieces, so this looks like more copy-paste which either means that it's unnecessary code in both Init***Pieces methods or that there's a bug.

temp as a name implies to me that the variable is being used in a swap, or maybe as a factored out part of a complex expression. For a loop variable, why not man? (Or piece, since that's what the base class is called?)


        private void InitBoardColors()

Why? What are they ever used for? This looks like 21 lines of NOP.


        public bool PositionPiece(ChessPiece chessPiece, Position dest)

Ok, so it seems this should be called Move (or MovePiece if you prefer). To be honest, I think that should be a method of ChessGame rather than Board, but that's by the by.

I see that it has a return value. Why did the main loop completely ignore it?

            if (_rects[dest.Height, dest.Width].PieceType == PieceType.Empty)
            {
                _rects[dest.Height, dest.Width].PieceType = chessPiece.PieceType;
                chessPiece.Position.Width = dest.Width;
                chessPiece.Position.Height = dest.Height;
                return true;
            }

Spot the bug. Hint:

The number of pieces on the board seems to increase over time.

                if (_blackPlayer.PiecesList.Contains(chessPiece))

See previous comments about the identity of a man. You shouldn't need to do a linear search to find out whether a man is black or white.

Also, what about the case where a white man tries to move to an occupied square? Not handled at all.


So far I think I've found at least five bugs, and I'm only 40% of the way through the code. No interviewer would have stuck with it as long as I have, and I'm not going any further.

\$\endgroup\$
  • \$\begingroup\$ +1 for actually addressing the code review from how we'd view it from an interviewer perspective. The init white/black methods alone would be a red flag. Followed by the multi-nested if statement in the PositionPiece method which should be inverted a couple of times. The PieceType enum is inconsistent with the royalty being "color-blind", but the pawns are not. Anyhow, solid code review. \$\endgroup\$ – Metro Smurf Oct 3 '17 at 0:20
  • \$\begingroup\$ @Peter Taylor. Thank you very very much for the code review. I hope to get better by it. \$\endgroup\$ – Gilad Oct 3 '17 at 4:45
  • \$\begingroup\$ if (_blackPlayer.PiecesList.Contains(chessPiece)) "...shouldn't need ... search to find out...". Just take that line and wrap it in a method, Player.hasPiece(chessPiece). The principle of least knowledge at work. \$\endgroup\$ – radarbob Oct 4 '17 at 19:17
  • \$\begingroup\$ Likewise, if the board must be searched for a particular piece do exactly the same thing: Board.hasPiece(chessPiece) \$\endgroup\$ – radarbob Oct 4 '17 at 19:20
  • \$\begingroup\$ @radarbob, that's just moving the problem elsewhere. If the piece owns its colour then the player object is unnecessary for testing whether two pieces are of the same colour. \$\endgroup\$ – Peter Taylor Oct 4 '17 at 20:51
5
\$\begingroup\$

I am looking at this with the eyes of an interviewer, overall this is a start but it could use some refinement.

There is a typo in your code, even though you spelled the class Position correctly the variable poisition is misspelled. Be careful when you release code. As an employer I don't want to have to hunt for typos in code. While this is a minor problem, its something that can be seen without looking at the structure of the code therefore it might already put you at a disadvantage before the reviewer has seen what you can do.

Design

The choice of only providing a function to check for a valid move to a specific coordinate is not ideal. You model a game between two "human" opponents that look at the board and make a move, when it's legal. Using your design it would be very inconvenient to implement a computer based chess player, it would need a list of all available moves, to generate that with your design one would have to query each piece, over all squares. A better approach here would be to be able to query each piece for a list of available moves. To check a validity of a move just see if the move you want to make is in the list.

There are some rules that are hard to implement in your design because there is no representation of the pieces on the board. That means your tests for valid moves have to be split between the piece itself and the PositionPiece function. That is not ideal. A pawn moving diagonally is only valid if there is another piece on the target square (besides the fact that you don't have this move available) you would have to test against the pieces rules and then in the PositionPiece function for the pawn do a specific test. If you gave the pieces access to the board this could be avoided.

GetPieceAtLocation is not implemented for the player, but if you wrote it, it would have to test all the pieces and see which is at that location. That's not really a good pattern, a 2D array on the board with references to all the pieces and access to each square would make things a lot smoother.

It is important to know the domain, the devil is in the detail, look at your desing and look up the chess rules, how would you implement "en passant", "castleing", "piece promotion", "check", "checkmate". Think about how you could rearrange things so that there isn't a need for too many exception paths ... and that functions can be accessed from multiple places. E.g. checkmate happens when the king is in check at the beginning of the move, but a move that exposes the king to check by their own player is illegal. If you feel that your design can cover those cases well go ahead and implement them, if not go back and figure out something else.

Names

Usually classes in OOP design are part of a certain domain, its not necessary to repeat the domain or other classnames in names, A ChessPiece can easily be a Piece, the PieceType should just be Type. This make code easier to read, the more repetitive information in the code, the sooner you'll start glossing over stuff.

I don't know if english is your first or second language, height and width would be used in an extent or a size, for chess row and column would be more appropriat, x, y would work better as well.

Little used Temporaries

InitWhitePieces and InitBlackPieces both introduce a lot of temporary variables just for what looks like readibility. If you refactor these calls to

_whitePlayer.PiecesList.Add(new King(new Position(0, 3)));

for example, the size of these function drops by half, again less code, more readibility.

Repetition in a derived function call

IsValidMove() calls IsOnBoard() for every piece, that's not really a good way of implementing this, IsValidMove() should at least call the superclass version of it self, or even better implement a mechanism where the public concrete function in the base class calls the private abstract function (e.g https://stackoverflow.com/questions/3747711/call-base-function-then-inherited-function). This way base functionality can be executed every time.

Other

What's the point of marking the squares as white and black ? On the rules level there is nothing that needs the state of a square. And it could easily be queried from the coordinates.

\$\endgroup\$
  • \$\begingroup\$ Nice answer aside from the advice on the game piece not requiring the color. You would need that information to determine if a move is legal. For example you can remove an opponents piece by moving into their position, but moving two of your pieces onto the same square is illegal. \$\endgroup\$ – Nick Savage Oct 2 '17 at 23:58
  • 1
    \$\begingroup\$ @NickSavage each square has a color, yes the pieces need to be of a color but the squares don't \$\endgroup\$ – Harald Scheirich Oct 3 '17 at 11:22
5
\$\begingroup\$

In the chess variant I know best, the rooks are in the corners, not the bishops. Oh, I see. You just confused their names, but the moves are correct. The bishop is the one that moves diagonally.

How can your program ever work correctly if IsGameOver always returns false?

To check the valid knight moves, you can combine all the conditions into one:

var dx = dest.Width - Position.Width;
var dy = dest.Height - Position.Height;
return dx * dx + dy * dy == 5;

You should rename the Position fields to Rank and File since these words are more common in chess.

There are several more mistakes in the code:

  • Both players start with 2 kings in their inventory, but only one on the board (count how often Add(whiteKing) appears in the code).
  • Bishops (what you call rooks) can only move along one of the diagonals, but not the others.
  • Same for the queens.
  • White pawns can move sideways, but only to the right. They capture in the same way as they move, unlike in standard western chess.
  • Pawns cannot move 2 squares with their initial move.
  • Castling is not allowed.
  • Pawns can enter the last rank without being promoted.
  • The queens start in the e file; they should start in the d file.
\$\endgroup\$
  • \$\begingroup\$ Roland the main point of the excercise was the OOP design not the full flawless implementation of the end game goal. I will post a more updated version with this part in it as well. \$\endgroup\$ – Gilad Oct 2 '17 at 18:09
  • \$\begingroup\$ Thanks I come from an image processing enviroment so I used width ad height automatically \$\endgroup\$ – Gilad Oct 2 '17 at 18:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.