2
\$\begingroup\$

I wrote a terminal-based TicTacToe game in Kotlin to practice both Kotlin and OOP. Any feedback on structure and code would be appreciated.

Board.kt

package main.com.github.me.tictactoe

class Board {
    val originalBoardConfig = listOf(
        listOf("1", "2", "3"),
        listOf("4", "5", "6"),
        listOf("7", "8", "9")
    )

    val board = originalBoardConfig.map { it.toMutableList() }.toMutableList()

    var boardMap = hashMapOf<String, String>()

    // initialises the boardMap
    init {
        resetBoardMap()
    }

    fun resetBoardMap() {
        boardMap = hashMapOf(
            "1" to board[0][0],
            "2" to board[0][1],
            "3" to board[0][2],
            "4" to board[1][0],
            "5" to board[1][1],
            "6" to board[1][2],
            "7" to board[2][0],
            "8" to board[2][1],
            "9" to board[2][2]
        )
    }

    fun getBoardMap(): MutableMap<String, String> {
        return boardMap
    }

    fun drawBoard() {
        println("Current board:\n")
        for (rowIdx in 0 until 3) {
            println(" ${boardMap["${rowIdx * 3 + 1}"]} | ${boardMap["${rowIdx * 3 + 2}"]} | ${boardMap["${rowIdx * 3 + 3}"]}")
            if (rowIdx < 2) {
                println("-----------")
            }
        }
        println()
    }
}

Turns.kt

package main.com.github.me.tictactoe

import java.util.*

class Turns(val board: Board) {

    val validPositions = listOf("1", "2", "3", "4", "5", "6", "7", "8", "9")

    private fun validateAndPlaceSymbol(symbol: String, position: String): Boolean {
        val boardMap = board.getBoardMap()
        if (boardMap[position] in validPositions) {
            boardMap[position] = symbol
            return true
        }
        return false
    }

    fun makeAPlay(player: Int, symbol: String): String {
        val scanner = Scanner(System.`in`)

        while (true) {
            board.drawBoard()
            println("Player $player, place '$symbol' by selecting a number between 1 and 9 (or enter 'q' to quit): ")
            val input = scanner.nextLine()

            if (input == "q") {
                println("Quitting the game.")
                System.exit(0)
            }

            if (input.length != 1 || !input[0].isDigit()) {
                println()
                println("Invalid input. Please enter a single digit between 1 and 9.")
                println()
                continue
            }

            if (input in validPositions) {
                if (validateAndPlaceSymbol(symbol, input)) {
                    return input
                } else {
                    println()
                    println("Invalid position. The position has already been taken.")
                    println()
                }
            } else {
                println("Invalid input. Please enter a digit between 1 and 9.")
            }
        }
    }

    fun switchPlayer(player: Int, symbol: String): Pair<Int, String> {
        return when {
            player == 1 && symbol == "X" -> {
                Pair(2, "O")
            }
            else -> {
                Pair(1, "X")
            }
        }
    }

    fun playAgain() {
        println("Play again? (y/n)")
        val scanner = Scanner(System.`in`)

        while (true) {
            val input = scanner.nextLine()

            if (input == "n") {
                println("Quitting the game.")
                System.exit(0)
            } else if (input == "y") {
                board.resetBoardMap()
                return
            }
            else {
                println("Invalid input. Play again? (y/n)")
            }
        }
    }
}

WinningConditions.kt

package main.com.github.me.tictactoe

class WinningConditions(val board: Board) {

    private val winningCombinations = listOf(
        listOf("1", "2", "3"),
        listOf("4", "5", "6"),
        listOf("7", "8", "9"),
        listOf("1", "4", "7"),
        listOf("2", "5", "8"),
        listOf("3", "6", "9"),
        listOf("1", "5", "9"),
        listOf("3", "5", "7")
    )

    fun win(player: Int, symbol: String): Boolean {
        val boardMap = board.getBoardMap()

        for (combination in winningCombinations) {
            if (combination.all { position -> boardMap[position] == symbol }) {
                board.drawBoard()
                println("Player $player WINS!")
                return true
            }
        }

        return false
    }
}

main.kt

import main.com.github.me.tictactoe.Board
import main.com.github.me.tictactoe.Turns
import main.com.github.me.tictactoe.WinningConditions

fun main() {
    println("Welcome to TicTacToe!")

    val ticTacToeBoard = Board()
    val turns = Turns(ticTacToeBoard)
    val win = WinningConditions(ticTacToeBoard)

    while (true) {
        var player = 1
        var symbol = "X"
        var round = 0

        while (round < 9) {
            turns.makeAPlay(player, symbol)

            if (win.win(player, symbol)) {
                turns.playAgain()
                break // Exit the current game loop if there's a winner
            }

            val playerSwitch = turns.switchPlayer(player, symbol)
            player = playerSwitch.first
            symbol = playerSwitch.second

            round += 1
        }

        if (round == 9) {
            ticTacToeBoard.drawBoard()
            println("It's a draw.")
            turns.playAgain()
        }
    }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Overall the method names, variable names, indentation and use of whitespace is spot on, which makes it easy to read the code.

Enumerating all the positions is not really required, but it does show good abstract thinking and a clear way to solve the problem.

Board class

If well programmed it should be possible to use no literals in the Board, other than possibly the board dimensions. Let the computer do the counting.


There are some things going on that are not clear, such as why the identification is performed using a string. If a string has to be used then convert a counter to a string.


The Board class seems to be some kind of factory instead of a true OOP class; it only seems to have mutable state, and the entire state is made visible through getBoardMap. You could hold the board state in the Board class and then maybe build some kind of stateful (Game) object on top of it , for instance keeping in mind who's turn it is - see remarks for Turns.


Personally I'd always introduce a Position data class. Then each piece or mark can have a Position. It also allows to check if a position is on the board etc. Note that, given a width and a height you should be able to get a value of 0..8 using multiply with addition from the x and y coordinate. And similarly you can get the x and y coordinate of a number 0..8 using division with remainder!


It's always best to provide an "output" to a print statement. Maybe the game needs to be tested later, and it is hard to read anything back from a console. So providing a PrintStream parameter to drawBoard is highly recommended.

Turns class

This class name immediately raises red flags. Normally a class has a name that is a singular noun. Turns isn't anything specific, and this follows from the fact that it lacks any state. It's better to have a Game class that contains these methods but keeps state.

if it isn't possible to figure out what a class represents then there is something wrong with the design


The Game class can keep state, which means that if Game#switchPlayer is called that the Pair result isn't needed, as it is possible to simply change the state of the ongoing game. That's one of the advantage of fields; it is possible to keep the amount of method arguments and results down.


Some additional counting is going on here for the valid positions. It's not clear why the map that makes up the board is not enough. It already has the strings as hashes after all.


When quitting the game don't forget to print out the final state of the game, and maybe point out a winner (i.e. the other player). Probably best to do this in a separate method, the game may be ended in multiple ways after all (.


Similar remark about checking for valid positions; that's something that should really be in a separate method. To me it makes sense to ask the board if something is a valid position.

WinningConditions class

It's a good idea to have a method or a class that checks if the game is over. Of course, a mathematical method of checking if the game has ended should be preferred; we're all programmers after all. For instance, it is possible to see that the game has rows, columns and 2 diagonals, so simply checking if these are all filled by the pieces / marks of any player would indicate that the game has been won.


In principle it is possible to have a piece that has a color, and the color matches the player. In that case only a single parameter may be required. Or zero, if the end condition simply returns an optional color of who has won. Personally I like enums better than strings; the code is using too many string arguments.

main

The main method is very readable. TicTacToe is of course a simple game, but it needs to be said that this is easy to read and no surprises seem to jump out. This is very important and shows that the design is good enough for this problem.


It's always advisable to put as little gameplay itself in the main method. Generally it pays to have any program run without requiring main to be called. Generally it is also advisable to split the UI elements (in this case the console input / output) from the gameplay itself.


So given a Game (or TicTacToe!) class, the main method could be put in e.g. a Game#play method. When it is required to restart then just create a new Game instead of resetting the board. That way all the state will be reset automatically.


A for ... in ... until is also perfectly possible in Kotlin; that would be better readable than the while loop.

\$\endgroup\$

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