3
\$\begingroup\$

I've just started my journey into functional programming and tried to implement a TicTacToe game Recursively without State in Scala.

What I dislike is the JOptionPane but I don't know if i can solve this more elegantly.

Does anyone know if i can solve this in a more "Scala" like way?

import javax.swing.JOptionPane

import scala.annotation.tailrec

class TicTacToe {
}

object TicTacToe {
  val l1: List[Int] = List(0, 1, 2)
  val l2: List[Int] = List(3, 4, 5)
  val l3: List[Int] = List(6, 7, 8)
  val c1: List[Int] = List(0, 3, 6)
  val c2: List[Int] = List(1, 4, 7)
  val c3: List[Int] = List(2, 5, 8)
  val d1: List[Int] = List(0, 4, 8)
  val d2: List[Int] = List(2, 4, 6)
  val patterns: List[List[Int]] = List(l1, l2, l3, c1, c2, c3, d1, d2)
  val winConditions: List[String] = List("xxx", "ooo")
  val startBoard = Array('#', '#', '#', '#', '#', '#', '#', '#', '#')

  def main(args: Array[String]): Unit = {
    startGame
  }

  def startGame: Unit ={
    @tailrec
    def playGameAt(game: List[Array[Char]], atPosition: Int): Unit = {
      val board: Array[Char] = game.head
      board.update(atPosition, nextPlayer(board))
      printBoard(board)
      if (!isWon(board)) {
        playGameAt(List(board) ++ game, Integer.parseInt(JOptionPane.showInputDialog()))
      }
    }
    playGameAt(List(startBoard), Integer.parseInt(JOptionPane.showInputDialog()))
  }

  def nextPlayer(board: Array[Char]): Char = {
    val turnNumber = board.count(_ == '#')
    if(turnNumber%2 == 0) 'x' else 'o'
  }

  def isWon(board: Array[Char]): Boolean = {
    patterns.foreach(pattern=>{
      val rowValues = pattern.foldLeft("")(_+board(_))
      if (winConditions.contains(rowValues)){
        println("Winner is " + rowValues)
        return true
      }
    })
    false
  }

  def printBoard(board: Array[Char]): Unit = {
    List(l1, l2, l3).foreach(row => println("" + board(row(0)) + board(row(1)) + board(row(2))))
    println("------")
  }
}

New, Improoved but still imperfect version

\$\endgroup\$
2
  • 1
    \$\begingroup\$ By the way, you should look up ScalaFX if you don't like Swing \$\endgroup\$ – user Jul 6 '20 at 20:58
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Jul 16 '20 at 18:05
5
\$\begingroup\$

Edit: Sorry for the ridiculously long answer. I've added an alternative (better) way of doing this at the bottom.

Here are a few things you could improve:

class TicTacToe

There's no need for this class. You can get (probably) rid of it.

Spaces

Overall, your code is well-formatted, but there a few instances like these, where you missed spaces:

if(turnNumber%2 == 0)    |>   if (turnNumber % 2 == 0)
pattern=>{               |>   pattern => {
def startGame: Unit ={   |>   def startGame(): Unit = {

Type aliases

I don't know about you, but I like using type aliases because they help me remember what each type represents. It's also handy when you have to refactor your code, e.g., if you want to represent moves using tuples representing the row and column ((Int, Int)) or you want to make a Board class instead of just using an array of characters, you don't have to change your method signatures - they can still return and accept objects of type Player and Board.

type Player = Char
type Board = Array[Player]

Unnecessary braces and the main method

Instead of

def main(args: Array[String]): Unit = {
    startGame
}

you could make it a one-liner

def main(args: Array[String]) = startGame

However, it'd be much more helpful to announce instructions before starting the game (I know those instructions don't match your own game, but bear with me).

def main(args: Array[String]): Unit = {
    println("Welcome to Tic Tac Toe!")
    println("To play, enter the row and column of the cell where you want to move when prompted")
    println("Both the row and column must be numbers from 1 to 3")

    runGame()
}

startGame

Rather than startGame, I feel like you should name it runGame, but that's entirely subjective, and you should pick whatever feels more intuitive to you.

More importantly, I think the startGame should be a nilary method rather than a nullary method, i.e., it should have an empty parameter list so that it looks like a proper method call rather than a property access. Currently, it looks very confusing when you just have startGame to run the entire game, since it looks like an unused expression.


Storing the board

Using a 1-D array of characters to represent a board is fine for now, although not very good functional programming style. There are a lot of other issues here, though.

board.update

You can use board(atPosition) = nextPlayer(board) instead of board.update(atPosition, nextPlayer(board)), since the update method is one of Scala's special methods that let you use syntactic sugar.

Adding the current board to the game

Currently, you use List(board) ++ game, which creates a new list and then concatenates game to it. A better approach would be board :: game, which is more idiomatic and simpler.

Why using a List[Array[Char]] is bad

First of all, there is absolutely no reason to maintain a list of all the past boards. You don't use the game variable everywhere. You can just have a single Array[Char] to keep track of the current board. Even if you do need to be able to go back to a previous move, you can just maintain a List[Move] and use it to reconstruct the board at a specific time.

Second, every single board you store in game refers to the exact same object! You're not copying board anywhere, so when you write the below code, you're getting the head of game, updating it, and then prepending that same board back onto game.

val board: Array[Char] = game.head
board.update(atPosition, nextPlayer(board))

A better solution would be to use tuples or a case class of your making. I'm going to just use tuples for now, because case classes would bloat the code.

type Row = (Player, Player, Player)
type Board = (Row, Row, Row)

Now that the board's 2-D, let's also make our moves 2-D, actually. They'll represent the row and column of each move. I made them 1-indexed in my code because I also make the user pick a number from 1 to 3 instead of 0 to 2 because I feel it'd be easier. By also internally using 1 instead of 0, we'll reduce off-by-one errors.

type Move = (Int, Int)

Variables

At the top of your object, you have a ton of variables, of which you need only one - patterns (which I'm going to rename to winCases, because that is more descriptive to me). There's no need to make separate public variables for l1, l2, etc. You can just do it like this (I used Set because the order doesn't matter):

val winCases = Set(
    Set((1, 1), (1, 2), (1, 3)),
    Set((2, 1), (2, 2), (2, 3)),
    Set((3, 1), (3, 2), (3, 3)),
    Set((1, 1), (2, 1), (3, 1)),
    Set((1, 2), (2, 2), (3, 2)),
    Set((1, 3), (2, 3), (3, 3)),
    Set((1, 1), (2, 2), (3, 3)),
    Set((1, 3), (2, 2), (3, 1)),
  )

startBoard can just be a local variable in runGame and doesn't have to be accessible by everyone.

winConditions we won't need because the whole isWon method can be refactored. There's absolutely no need to make a new string for each element of patterns/winCases.


Finding the winner

I don't like the fact that your isWon method prints the winner instead of only returning whether or not someone has won the game. A better way to do it would be to return a player and let the calling method decide what to display to the user. For that, I made this method:

def findWinner(board: Board): Either[Unit, Option[Player]] =
    if (isWinner(player1, board)) Right(Some(player1))
    else if (isWinner(player2, board)) Right(Some(player2))
    else if (isTie(board)) Right(None)
    else Left(())

Having an Either[Unit, Option[Player]] lets us encode multiple things into that single value. If it's a Left, we know the game will continue. If it's a Right, the game's ended. If it's the latter, containing a Some, there's a winner, and if it's a None, there's a tie.


Back to startGame/runGame

Among other things, I've renamed the playGameAt function to playRound. I also changed the signature. It takes the current board and player, and outputs the winner of the game. If there's a winner, it's a Some. If there's a tie, it's a None.

def playRound(board: Board, curr: Player): Option[Player]

Here's how the new function looks:

@tailrec
def playRound(board: Board, curr: Player): Option[Player] = {
  printBoard(board)
  println(s"Player $curr's move")
  val move = nextMove(board)
  val newBoard = moveTo(curr, move, board)
  findWinner(newBoard) match {
    case Right(possWinner) => possWinner
    case Left(_)           => playRound(newBoard, nextPlayer(curr))
  }
}

I've put the call to printBoard at the very top, because even though all tic-tac-toe boards are the same (unless you're playing a special variant), I personally would like to see the board I'm going to be moving on before I actually select a move. It's totally subjective, but I prefer it this way.

The print statement lets you know which player is supposed to move. The way you have it now, the users have to scroll up to see whose move it is currently, and this seems more helpful.

After that, it gets the next move with the nextMove function (which I'll put in later), creates a new board using that move, and tries to find the winner (see above for how the findWinner method works). If the game's ended, it returns the winner (or None if it's a tie). If not, it plays another round using a new board and the other player.

nextPlayer is implemented like this, by the way:

def nextPlayer(curr: Player): Player =
    if (curr == player1) player2
    else player1

The entire runGame function looks like this:

def runGame() = {
    @tailrec
    def playRound(board: Board, curr: Player): Option[Player] = ...

  val startBoard = (
    (default, default, default),
    (default, default, default),
    (default, default, default)
  )

  val winner = playRound(startBoard, player1)
  winner match {
    case Some(player) => println(s"Player $player won!")
    case None         => println("Tie")
  }
}

Here, startBoard is just a local variable, because I don't think there's any reason for anybody outside this method to know about it. default is a Char that represents a cell where no one's moved. player1 and player2 are used to mark where Player 1 and Player 2 moved, respectively.

val default = ' '
val player1 = 'x'
val player2 = 'o'

I also moved the print statement from isWon to here, so that there could be a customized message. Otherwise, if there was a tie, nothing would happen.


User input

It doesn't make sense to me that you're using a JOptionPane and displaying everything in the terminal. Why not make the user input also come from the console? We can write our nextMove method like this:

@tailrec
def nextMove(board: Board): Move = {
  val move = (nextRowOrCol("Row"), nextRowOrCol("Column"))

  if (isValid(move, board)) {
    move
  } else {
    println("That move is already taken. Please enter a different move.")
    nextMove(board)
  }
}

The above code reads a row and column using the helper function nextRowOrCol, then checks if it's a valid move, i.e., no one's moved there already. If it is, it just returns it, and if not, it gets the user to re-enter a move. I'll add the isValid function a little below.

nextRowOrCol is implemented like this (prompt can be either "Row" or "Column"). It uses regex to ensure that the input is a number between 1 and 3. Like the nextMove function, if the row or column inputted is valid, it returns it directly, otherwise, it prompts the user(s) again.

private def nextRowOrCol(prompt: String): Int = {
  val input = readLine(s"$prompt: ")

  if (input.matches("[1-3]")) {
    input.toInt
  } else {
    println("Please enter a number from 1 to 3")
    nextRowOrCol(prompt)
  }
}

The entire code

As the heading says, here's the entire code. You'll notice that it's wayyy longer than what you have currently, mostly because I used tuples, which complicated everything, but also because your original solution didn't have a lot of functionality.

I'm sure you can find a way to make it shorter, especially if you use your own case classes to represent everything. I've made almost every function that deals with tiny details like destructuring tuples private, but the public functions shouldn't need to be changed much even if you do decide to make a case class Board or something like that.

import scala.io.StdIn.readLine
import scala.annotation.tailrec

object TicTacToe {

  type Player = Char
  type Move = (Int, Int)
  type Row = (Player, Player, Player)
  type Board = (Row, Row, Row)

  val winCases = Set(
    Set((1, 1), (1, 2), (1, 3)),
    Set((2, 1), (2, 2), (2, 3)),
    Set((3, 1), (3, 2), (3, 3)),
    Set((1, 1), (2, 1), (3, 1)),
    Set((1, 2), (2, 2), (3, 2)),
    Set((1, 3), (2, 3), (3, 3)),
    Set((1, 1), (2, 2), (3, 3)),
    Set((1, 3), (2, 2), (3, 1)),
  )

  val default = ' '
  val player1 = 'x'
  val player2 = 'o'

  def main(args: Array[String]) = {
    println("Welcome to TicTacToe!")
    println("To play, enter the row and column of the cell where you want to move when prompted")
    println("Both the row and column must be numbers from 1 to 3")

    runGame()
  }

  def runGame() = {
    @tailrec
    def playRound(board: Board, curr: Player): Option[Player] = {
      printBoard(board)
      println(s"Player $curr's move")
      val move = nextMove(board)
      val newBoard = moveTo(curr, move, board)
      findWinner(newBoard) match {
        case Right(possWinner) => possWinner
        case Left(_)           => playRound(newBoard, nextPlayer(curr))
      }
    }

    val startBoard = (
      (default, default, default),
      (default, default, default),
      (default, default, default)
    )

    val winner = playRound(startBoard, player1)
    winner match {
      case Some(player) => println(s"Player $player won!")
      case None         => println("Tie")
    }
  }

  def findWinner(board: Board): Either[Unit, Option[Player]] =
    if (isWinner(player1, board)) Right(Some(player1))
    else if (isWinner(player2, board)) Right(Some(player2))
    else if (isTie(board)) Right(None)
    else Left(())

  def moveTo(player: Player, move: Move, board: Board): Board = {
    val (row0, row1, row2) = board
    val (r, c) = move

    def updateTuple[T](tup: (T, T, T), ind: Int)(f: T => T): (T, T, T) = 
      ind match {
        case 1 => tup.copy(_1 = f(tup._1))
        case 2 => tup.copy(_2 = f(tup._2))
        case 3 => tup.copy(_3 = f(tup._3))
      }

    updateTuple(board, r) {
      row => updateTuple(row, c)(_ => player)
    }
  }

  def isWinner(player: Player, board: Board): Boolean =
    winCases.exists(winCase =>
      winCase.forall(move => playerAt(move, board) == player)
    )

  def isTie(board: Board): Boolean = !board.productIterator.exists {
    row => row.asInstanceOf[Row].productIterator.contains(default)
  }

  def playerAt(move: Move, board: Board): Player = {
    val (r, c) = move
    elementAt(elementAt(board, r), c)
  }

  private def elementAt[T](tup: (T, T, T), ind: Int): T =
    ind match {
      case 1 => tup._1
      case 2 => tup._2
      case 3 => tup._3
    }

  @tailrec
  def nextMove(board: Board): Move = {
    val move = (nextRowOrCol("Row"), nextRowOrCol("Column"))

    if (isValid(move, board)) {
      move
    } else {
      println("That move is already taken. Please enter a different move.")
      nextMove(board)
    }
  }

  private def nextRowOrCol(prompt: String): Int = {
    val input = readLine(s"$prompt: ")

    if (input.matches("[1-3]")) {
      input.toInt
    } else {
      println("Please enter a number from 1 to 3")
      nextRowOrCol(prompt)
    }
  }

  def isValid(move: Move, board: Board): Boolean = 
    playerAt(move, board) == default

  def nextPlayer(curr: Player): Player =
    if (curr == player1) player2
    else player1

  def printBoard(board: Board): Unit =
    print(
      "__________________\n" + 
      tup2String(
        mapTuple(board) {row => tup2String(row, "|")},
        "------\n"
      )
    )

  private def tup2String[T](tup: (T, T, T), sep: String): String =
    s"${tup._1}$sep${tup._2}$sep${tup._3}\n"

  private def mapTuple[T, R](tup: (T, T, T))(f: T => R): (R, R, R) =
    (f(tup._1), f(tup._2), f(tup._3))
}

Edit:

An alternative way of storing the board

Working with tuples is really annoying, and even with case classes, you'd have to define your own methods and stuff. A nicer way to store the board would be to just maintain a list of all the moves anyone's ever made. Every move should contain where that move was made and the player who made that move, so let's makes these two types. By the way, the Move from before is more like Coord here.

type Coord = (Int, Int)
type Move = (Coord, Player)

Now everywhere we use board: Board, we just replace that with moves: List[Move].

playRound doesn't change all that much. The variable move now has to include the current player because of how we defined Move above, and the newMoves variable (analogous to newBoard) is constructed by prepending moves to the pre-existing list of moves, which is a lot easier than creating a moveTo function that does all sorts of crazy stuff inside. Everywhere else, just remember that board has been replaced with moves.

@tailrec
def playRound(moves: List[Move], curr: Player): Option[Player] = {
  println(s"Player $curr's move")
  val move = (nextMove(moves), curr)
  val newMoves = move :: moves
  printBoard(newMoves)
  findWinner(newMoves) match {
    case Right(possWinner) => possWinner
    case Left(_)           => playRound(newMoves, nextPlayer(curr))
  }
}

runGame only has 1 change: instead of manually creating a variable called startBoard filled with the default character (' '), you can use List.empty (or Nil):

val winner = playRound(List.empty, player1)

playerAt is a lot simpler now. It tries to find a move with the given coordinates, and if no move with those coordinates is found in our List[Move], then default is chosen.

def playerAt(coord: Coord, moves: List[Move]): Player =
    moves.find(move => move._1 == coord).map(_._2).getOrElse(default)

isTie is also a lot simpler - just check if 9 moves have been made!

def isTie(moves: List[Move]): Boolean = moves.size == 9

printBoard is the only one with big-ish changes (the good kind). You can just use map and mkString now that we're not using tuples.

def printBoard(moves: List[Move]): Unit =
    print(
      1 to 3 map { r =>
          1 to 3 map { c =>
            playerAt((r, c), moves)
          } mkString "|"
      } mkString ("__________\n", "\n------\n", "\n")
    )

The entire code:

import scala.io.StdIn.readLine
import scala.annotation.tailrec

object TicTacToe2 {

  type Player = Char
  type Coord = (Int, Int)
  type Move = (Coord, Player)

  val winCases: Set[Set[Coord]] = Set(
    Set((1, 1), (1, 2), (1, 3)),
    Set((2, 1), (2, 2), (2, 3)),
    Set((3, 1), (3, 2), (3, 3)),
    Set((1, 1), (2, 1), (3, 1)),
    Set((1, 2), (2, 2), (3, 2)),
    Set((1, 3), (2, 3), (3, 3)),
    Set((1, 1), (2, 2), (3, 3)),
    Set((1, 3), (2, 2), (3, 1))
  )

  val default = ' '
  val player1 = 'x'
  val player2 = 'o'

  def main(args: Array[String]) = {
    println("Welcome to TicTacToe!")
    println(
      "To play, enter the row and column of the cell where you want to move when prompted"
    )
    println("Both the row and column must be numbers from 1 to 3")
    printBoard(List.empty)

    runGame()
  }

  def runGame() = {
    @tailrec
    def playRound(moves: List[Move], curr: Player): Option[Player] = {
      println(s"Player $curr's move")
      val move = (nextMove(moves), curr)
      val newMoves = move :: moves
      printBoard(newMoves)
      findWinner(newMoves) match {
        case Right(possWinner) => possWinner
        case Left(_)           => playRound(newMoves, nextPlayer(curr))
      }
    }

    val winner = playRound(List.empty, player1)
    winner match {
      case Some(player) => println(s"Player $player won!")
      case None         => println("Tie")
    }
  }

  def findWinner(moves: List[Move]): Either[Unit, Option[Player]] =
    if (isWinner(player1, moves)) Right(Some(player1))
    else if (isWinner(player2, moves)) Right(Some(player2))
    else if (isTie(moves)) Right(None)
    else Left(())

  def isWinner(player: Player, moves: List[Move]): Boolean =
    winCases.exists { winCase =>
      winCase.forall(move => playerAt(move, moves) == player)
    }

  def isTie(moves: List[Move]): Boolean = moves.size == 9

  def playerAt(coord: Coord, moves: List[Move]): Player =
    moves.find(move => move._1 == coord).map(_._2).getOrElse(default)

  @tailrec
  def nextMove(moves: List[Move]): Coord = {
    val coord = (nextRowOrCol("Row"), nextRowOrCol("Column"))

    if (isValid(coord, moves)) {
      coord
    } else {
      println("That move is already taken. Please enter a different move.")
      nextMove(moves)
    }
  }

  private def nextRowOrCol(prompt: String): Int = {
    val input = readLine(s"$prompt: ")

    if (input.matches("[1-3]")) {
      input.toInt
    } else {
      println("Please enter a number from 1 to 3")
      nextRowOrCol(prompt)
    }
  }

  def isValid(coord: Coord, moves: List[Move]): Boolean =
    playerAt(coord, moves) == default

  def nextPlayer(curr: Player): Player =
    if (curr == player1) player2
    else player1

  def printBoard(moves: List[Move]): Unit =
    print(
      1 to 3 map { r =>
          1 to 3 map { c =>
            playerAt((r, c), moves)
          } mkString "|"
      } mkString ("__________\n", "\n------\n", "\n")
    )
}

By the way, here's a small change you can make concerning the isValid method. Instead of returning a boolean, return an Option with which you can do getOrElse. If you write a validate function like this:

def validate(coord: Coord, moves: List[Move]): Option[Coord] =
    Option.when(playerAt(coord, moves) == default)(coord)

you can use it in nextMove like this, which looks much more idiomatic. The only thing is that you'll have to drop the tailrec annotation.

def nextMove(moves: List[Move]): Coord = {
  val coord = (nextRowOrCol("Row"), nextRowOrCol("Column"))

  validate(coord, moves).getOrElse {
    println("That move is already taken. Please enter a different move.")
    nextMove(moves)
  }
  }

Let me know if there's anything I did wrong or can improve.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ keep up the good work! wow \$\endgroup\$ – Roman Jul 9 '20 at 6:43
  • \$\begingroup\$ link Follow up, also yeah I need to change it to private. Thanks I'll look more into it \$\endgroup\$ – elauser Jul 16 '20 at 18:20
  • 1
    \$\begingroup\$ I changed the Win Conditions function in the new Post, what do you think about it? And I totally agree the win conditions bugged me because the logic was twisted. \$\endgroup\$ – elauser Jul 16 '20 at 18:46

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.