1
\$\begingroup\$

I wrote the following Chess logic in Kotlin and am looking for feedback to make the code cleaner and follow good software design principles. I tried to adhere to object-oriented design.

Some notes:

• I ignored special moves like "Castling" and "en pessant" for simplicity.

• I put the last when branch into a separate when statement so that the orthogonal and diagonal Queen moves can add up.

• To reset the game you initialize a new ChessGame object. Would it be better to add a reset method like this instead?

fun resetGame() {
    positionsArray = getStartingPositions()
    currentPlayer = Player.White
    removedPiecesList = mutableListOf()
}

The code itself:

class ChessGame {

    var currentPlayer: Player = Player.White

    private var playingFieldArray = getStartingPositions()
    val playingField: Array<Array<Piece>> = playingFieldArray

    private fun getStartingPositions() = arrayOf<Array<Piece>>(
        arrayOf(
            Piece.Black.Rook1,
            Piece.Black.Knight1,
            Piece.Black.Bishop1,
            Piece.Black.Queen,
            Piece.Black.King,
            Piece.Black.Bishop2,
            Piece.Black.Knight2,
            Piece.Black.Rook2
        ),
        arrayOf(
            Piece.Black.Pawn1,
            Piece.Black.Pawn2,
            Piece.Black.Pawn3,
            Piece.Black.Pawn4,
            Piece.Black.Pawn5,
            Piece.Black.Pawn6,
            Piece.Black.Pawn7,
            Piece.Black.Pawn8
        ),
        arrayOf(
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty
        ),
        arrayOf(
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty
        ),
        arrayOf(
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty
        ),
        arrayOf(
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty,
            Piece.Empty
        ),
        arrayOf(
            Piece.White.Pawn1,
            Piece.White.Pawn2,
            Piece.White.Pawn3,
            Piece.White.Pawn4,
            Piece.White.Pawn5,
            Piece.White.Pawn6,
            Piece.White.Pawn6,
            Piece.White.Pawn8
        ),
        arrayOf(
            Piece.White.Rook1,
            Piece.White.Knight1,
            Piece.White.Bishop1,
            Piece.White.Queen,
            Piece.White.King,
            Piece.White.Bishop2,
            Piece.White.Knight2,
            Piece.White.Rook2
        ),
    )

    private var removedPiecesList = mutableListOf<Piece>()
    val removedPieces: List<Piece> = removedPiecesList

    fun getAvailableMoves(x: Int, y: Int): List<Pair<Int, Int>> {
        if (playingFieldArray[x][y] is Piece.White && currentPlayer == Player.Black ||
            playingFieldArray[x][y] is Piece.Black && currentPlayer == Player.White ||
            isGameOver()
        ) {
            return emptyList()
        }

        val availableMoves = mutableListOf<Pair<Int, Int>>()

        fun isValidPosition(x: Int, y: Int) = x in 0..7 && y in 0..7 && !tileHasPieceOfCurrentPlayer(x, y)

        when (playingFieldArray[x][y]) {
            Piece.Black.Rook1, Piece.Black.Rook2, Piece.White.Rook1, Piece.White.Rook2, Piece.Black.Queen, Piece.White.Queen -> {
                var toXUp = x - 1
                val toYUp = y
                while (isValidPosition(toXUp, toYUp)
                    && !tileHasPieceOfCurrentPlayer(toXUp, toYUp)
                ) {
                    availableMoves.add(Pair(toXUp, toYUp))
                    if (tileHasPieceOfOpponent(toXUp, toYUp)) break
                    toXUp--
                }
                var toXDown = x + 1
                val toYDown = y
                while (isValidPosition(toXDown, toYDown)
                    && !tileHasPieceOfCurrentPlayer(toXDown, toYDown)
                ) {
                    availableMoves.add(Pair(toXDown, toYDown))
                    if (tileHasPieceOfOpponent(toXDown, toYDown)) break
                    toXDown++
                }
                val toXLeft = x
                var toYLeft = y - 1
                while (isValidPosition(toXLeft, toYLeft)
                    && !tileHasPieceOfCurrentPlayer(toXLeft, toYLeft)
                ) {
                    availableMoves.add(Pair(toXLeft, toYLeft))
                    if (tileHasPieceOfOpponent(toXLeft, toYLeft)) break
                    toYLeft--
                }
                val toXRight = x
                var toYRight = y + 1
                while (isValidPosition(toXRight, toYRight)
                    && !tileHasPieceOfCurrentPlayer(toXRight, toYRight)
                ) {
                    availableMoves.add(Pair(toXRight, toYRight))
                    if (tileHasPieceOfOpponent(toXRight, toYRight)) break
                    toYRight++
                }
            }
            Piece.Black.Knight1, Piece.Black.Knight2, Piece.White.Knight1, Piece.White.Knight2 -> {
                val toXUpLeft = x - 2
                val toYUpLeft = y - 1
                if (isValidPosition(toXUpLeft, toYUpLeft)) {
                    availableMoves.add(Pair(toXUpLeft, toYUpLeft))
                }
                val toXUpRight = x - 2
                val toYUpRight = y + 1
                if (isValidPosition(toXUpRight, toYUpRight)) {
                    availableMoves.add(Pair(toXUpRight, toYUpRight))
                }
                val toXDownLeft = x + 2
                val toYDownLeft = y - 1
                if (isValidPosition(toXDownLeft, toYDownLeft)) {
                    availableMoves.add(Pair(toXDownLeft, toYDownLeft))
                }
                val toXDownRight = x + 2
                val toYDownRight = y + 1
                if (isValidPosition(toXDownRight, toYDownRight)) {
                    availableMoves.add(Pair(toXDownRight, toYDownRight))
                }
                val toXLeftUp = x - 1
                val toYLeftUp = y - 2
                if (isValidPosition(toXLeftUp, toYLeftUp)) {
                    availableMoves.add(Pair(toXLeftUp, toYLeftUp))
                }
                val toXRightUp = x - 1
                val toYRightUp = y + 2
                if (isValidPosition(toXRightUp, toYRightUp)) {
                    availableMoves.add(Pair(toXRightUp, toYRightUp))
                }
                val toXLeftDown = x + 1
                val toYLeftDown = y - 2
                if (isValidPosition(toXLeftDown, toYLeftDown)) {
                    availableMoves.add(Pair(toXLeftDown, toYLeftDown))
                }
                val toXRightDown = x + 1
                val toYRightDown = y + 2
                if (isValidPosition(toXRightDown, toYRightDown)) {
                    availableMoves.add(Pair(toXRightDown, toYRightDown))
                }
            }
            Piece.Black.King, Piece.White.King -> {
                val toXUp = x - 1
                val toYUp = y
                if (isValidPosition(toXUp, toYUp)) {
                    availableMoves.add(Pair(toXUp, toYUp))
                }
                val toXDown = x + 1
                val toYDown = y
                if (isValidPosition(toXDown, toYDown)) {
                    availableMoves.add(Pair(toXDown, toYDown))
                }
                val toXLeft = x
                val toYLeft = y - 1
                if (isValidPosition(toXLeft, toYLeft)) {
                    availableMoves.add(Pair(toXLeft, toYLeft))
                }
                val toXRight = x
                val toYRight = y + 1
                if (isValidPosition(toXRight, toYRight)) {
                    availableMoves.add(Pair(toXRight, toYRight))
                }
                val toXUpLeft = x - 1
                val toYUpLeft = y - 1
                if (isValidPosition(toXUpLeft, toYUpLeft)) {
                    availableMoves.add(Pair(toXUpLeft, toYUpLeft))
                }
                val toXUpRight = x - 1
                val toYUpRight = y + 1
                if (isValidPosition(toXUpRight, toYUpRight)) {
                    availableMoves.add(Pair(toXUpRight, toYUpRight))
                }
                val toXDownLeft = x + 1
                val toYDownLeft = y - 1
                if (isValidPosition(toXDownLeft, toYDownLeft)) {
                    availableMoves.add(Pair(toXDownLeft, toYDownLeft))
                }
                val toXDownRight = x + 1
                val toYDownRight = y + 1
                if (isValidPosition(toXDownRight, toYDownRight)) {
                    availableMoves.add(Pair(toXDownRight, toYDownRight))
                }
            }
            Piece.Black.Pawn1, Piece.Black.Pawn2, Piece.Black.Pawn3, Piece.Black.Pawn4, Piece.Black.Pawn5, Piece.Black.Pawn6, Piece.Black.Pawn7, Piece.Black.Pawn8 -> {
                val toXDown = x + 1
                val toY1Down = y
                if (isValidPosition(toXDown, toY1Down) && !tileHasPieceOfOpponent(toXDown, toY1Down)) {
                    availableMoves.add(Pair(toXDown, toY1Down))
                }
                val toXDownRight = x + 1
                val toYDownRight = y + 1
                if (isValidPosition(toXDownRight, toYDownRight)
                    && tileHasPieceOfOpponent(toXDownRight, toYDownRight)
                ) {
                    availableMoves.add(Pair(toXDownRight, toYDownRight))
                }
                val toXDownLeft = x + 1
                val toYDownLeft = y - 1
                if (isValidPosition(toXDownLeft, toYDownLeft)
                    && tileHasPieceOfOpponent(toXDownLeft, toYDownLeft)
                ) {
                    availableMoves.add(Pair(toXDownLeft, toYDownLeft))
                }
            }
            Piece.White.Pawn1, Piece.White.Pawn2, Piece.White.Pawn3, Piece.White.Pawn4, Piece.White.Pawn5, Piece.White.Pawn6, Piece.White.Pawn7, Piece.White.Pawn8 -> {
                val toXUp = x - 1
                val toYUp = y
                if (isValidPosition(toXUp, toYUp) && !tileHasPieceOfOpponent(toXUp, toYUp)) {
                    availableMoves.add(Pair(toXUp, toYUp))
                }
                val toXUpRight = x - 1
                val toYUpRight = y + 1
                if (isValidPosition(toXUpRight, toYUpRight) && tileHasPieceOfOpponent(toXUpRight, toYUpRight)) {
                    availableMoves.add(Pair(toXUpRight, toYUpRight))
                }
                val toXUpLeft = x - 1
                val toYUpLeft = y - 1
                if (isValidPosition(toXUpLeft, toYUpLeft) && tileHasPieceOfOpponent(toXUpLeft, toYUpLeft)) {
                    availableMoves.add(Pair(toXUpLeft, toYUpLeft))
                }
            }
        }
        when (playingFieldArray[x][y]) {
            Piece.Black.Bishop1, Piece.Black.Bishop2, Piece.White.Bishop1, Piece.White.Bishop2, Piece.Black.Queen, Piece.White.Queen -> {
                var toXUpLeft = x - 1
                var toYUpLeft = y - 1
                while (isValidPosition(toXUpLeft, toYUpLeft)
                    && !tileHasPieceOfCurrentPlayer(toXUpLeft, toYUpLeft)
                ) {
                    availableMoves.add(Pair(toXUpLeft, toYUpLeft))
                    if (tileHasPieceOfOpponent(toXUpLeft, toYUpLeft)) break
                    toXUpLeft--
                    toYUpLeft--
                }
                var toXUpRight = x - 1
                var toYUpRight = y + 1
                while (isValidPosition(toXUpRight, toYUpRight)
                    && !tileHasPieceOfCurrentPlayer(toXUpRight, toYUpRight)
                ) {
                    availableMoves.add(Pair(toXUpRight, toYUpRight))
                    if (tileHasPieceOfOpponent(toXUpRight, toYUpRight)) break
                    toXUpRight--
                    toYUpRight++
                }
                var toXDownLeft = x + 1
                var toYDownLeft = y - 1
                while (isValidPosition(toXDownLeft, toYDownLeft)
                    && !tileHasPieceOfCurrentPlayer(toXDownLeft, toYDownLeft)
                ) {
                    availableMoves.add(Pair(toXDownLeft, toYDownLeft))
                    if (tileHasPieceOfOpponent(toXDownLeft, toYDownLeft)) break
                    toXDownLeft++
                    toYDownLeft--
                }
                var toXDownRight = x + 1
                var toYDownRight = y + 1
                while (isValidPosition(toXDownRight, toYDownRight)
                    && !tileHasPieceOfCurrentPlayer(toXDownRight, toYDownRight)
                ) {
                    availableMoves.add(Pair(toXDownRight, toYDownRight))
                    if (tileHasPieceOfOpponent(toXDownRight, toYDownRight)) break
                    toXDownRight++
                    toYDownRight++
                }
            }
        }
        return availableMoves
    }

    fun movePiece(fromX: Int, fromY: Int, toX: Int, toY: Int) {
        if (getAvailableMoves(fromX, fromY).contains(Pair(toX, toY))) {
            if (tileHasPieceOfOpponent(toX, toY)) {
                removedPiecesList.add(playingField[toX][toY])
            }
            playingFieldArray[toX][toY] = playingFieldArray[fromX][fromY]
            playingFieldArray[fromX][fromY] = Piece.Empty
        } else {
            throw IllegalArgumentException("Invalid move coordinates")
        }
        currentPlayer = if (currentPlayer == Player.White) Player.Black else Player.White
    }

    fun tileHasPieceOfCurrentPlayer(x: Int, y: Int) = when (currentPlayer) {
        Player.Black -> {
            playingField[x][y] is Piece.Black
        }
        Player.White -> {
            playingField[x][y] is Piece.White
        }
    }

    private fun tileHasPieceOfOpponent(x: Int, y: Int) = when (currentPlayer) {
        Player.Black -> {
            playingField[x][y] is Piece.White
        }
        Player.White -> {
            playingField[x][y] is Piece.Black
        }
    }

    fun isGameOver() =
        removedPieces.contains(Piece.White.King) || removedPieces.contains(Piece.Black.King)

    sealed class Piece {
        sealed class White : Piece() {
            object Pawn1 : White()
            object Pawn2 : White()
            object Pawn3 : White()
            object Pawn4 : White()
            object Pawn5 : White()
            object Pawn6 : White()
            object Pawn7 : White()
            object Pawn8 : White()
            object Rook1 : White()
            object Knight1 : White()
            object Bishop1 : White()
            object Queen : White()
            object King : White()
            object Bishop2 : White()
            object Knight2 : White()
            object Rook2 : White()
        }

        sealed class Black : Piece() {
            object Pawn1 : Black()
            object Pawn2 : Black()
            object Pawn3 : Black()
            object Pawn4 : Black()
            object Pawn5 : Black()
            object Pawn6 : Black()
            object Pawn7 : Black()
            object Pawn8 : Black()
            object Rook1 : Black()
            object Knight1 : Black()
            object Bishop1 : Black()
            object Queen : Black()
            object King : Black()
            object Bishop2 : Black()
            object Knight2 : Black()
            object Rook2 : Black()
        }

        object Empty : Piece()
    }

    enum class Player {
        Black, White
    }
}
```
\$\endgroup\$
3
4
\$\begingroup\$

There's several parts of your design that can be improved.

There is currently lots of duplicate code regarding color, allowed moves, and Pawn1, Pawn2, Pawn3 etc.

Is there really any difference between Pawn1, Pawn2, Pawn3, etc? I don't think so. A Piece could be a composition between a player color and an enum class PieceType that lists all the options (without numbering them).

Your move logic could use a List<Point> where a Point is a data class Point(val x: Int, val y: Int). For a knight this could then be:

val knightMoves = listOf(
   Point(2, 1), Point(2, -1),
   Point(-1, 2), Point(1, 2),
   Point(-1, -2), Point(1, -2),
   Point(-2, -1), Point(-2, 1)
)

Then iterate through this list and check which moves are allowed.

for (val move in knightMoves) {
    if (isValidPosition(x + move.x, y + move.y)) {
        availableMoves.add(move)
    }
}

It is possible to make more improvements, but... a few things at a time.

\$\endgroup\$
4
  • \$\begingroup\$ Wow, I just realized how bad my code actually is. I must've been too engrossed in getting it to work. Thank you, I will apply your suggestions tomorrow! \$\endgroup\$ – Florian Walther Apr 21 at 20:10
  • 4
    \$\begingroup\$ @FlorianWalther There's definitely room for improvement :) When you've made improvements, I'd recommend posting a follow-up question here on this site. \$\endgroup\$ – Simon Forsberg Apr 21 at 21:12
  • \$\begingroup\$ I am able to extract the Knight and King moves into a List<Point> as you described, but I don't see how I can do this for for example Rook moves, since the increment operator depends on the direction (like toXUp--) Any ideas? \$\endgroup\$ – Florian Walther Apr 22 at 9:43
  • \$\begingroup\$ I posted my new question here: codereview.stackexchange.com/questions/259851/chess-in-kotlin \$\endgroup\$ – Florian Walther Apr 22 at 10:42

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.