Improving my Tic Tac Toe Solution in Scala

Question

I'm relatively new to Scala and made a small TicTacToe in a functional Style. Any improvement options would be highly appreciated.

There are some things which I am unsure of if they are made in an optimal way.

1. Error Handling in the readMove Function
2. If i should incorporate types like (type Player = Char, type Field = Int) and if they would benefit the code
3. The printBoard function looks unclean and I can't yet figure out how to best change it.
4. board(nextMove) = nextPlayer(board) seems to break the style of immutable values.

This is already an improvement from my first version

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

object TicTacToeOld {
  val startBoard: Array[Char] = Array('1', '2', '3', '4', '5', '6', '7', '8', '9')
  val patterns: Set[Set[Int]] = Set(
    Set(0, 1, 2),
    Set(3, 4, 5),
    Set(6, 7, 8),
    Set(0, 3, 6),
    Set(1, 4, 7),
    Set(2, 5, 8),
    Set(0, 4, 8),
    Set(2, 4, 6)
  )

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

  def startGame(): Unit ={
    println("Welcome to TicTacToe!")
    println("To play, enter the number on the board where you want to play")
    printBoard(startBoard)
    nextTurn(startBoard)
  }

  @tailrec
  private def nextTurn(board: Array[Char]): Unit = {
    val nextMove = readMove(board)
    board(nextMove) = nextPlayer(board)
    printBoard(board)
    if (!isWon(board)) {
      nextTurn(board)
    }
  }

  @tailrec
  private def readMove(board: Array[Char]): Int ={
    try {
      val input = readLine("Input next Turn: ").toInt-1
      if(input<0 || input>8 || !board(input).toString.matches("[1-9]")) {
        throw new Exception
      }
      input
    } catch {
      case _: Exception => readMove(board)
    }
  }

  private def nextPlayer(board: Array[Char]): Char = {
    val remainingTurns = board.count(_.toString.matches("[1-9]"))
    if(remainingTurns%2 == 0) 'x' else 'o'
  }

  private def printBoard(board: Array[Char]): Unit = {
    print(
      0 to 2 map { r =>
        0 to 2 map { c =>
          board(c + r*3)
        } mkString "|"
      } mkString ("__________\n", "\n------\n", "\n")
    )
    println("Next Player is " + nextPlayer(board))
  }
  
  private def isWon(board: Array[Char]): Boolean = {
    patterns.foreach(pattern=>{
      if(pattern.forall(board(_) == board(pattern.head))) {
        print("Winner is " + board(pattern.head))
        return true
      }
    })
    false
  }
}

Answer 1

1. you can remove the main method and use extends App. It's free 3 lines of code.
2. replace Array('1', '2', '3', '4', '5', '6', '7', '8', '9') on ('1' to '9').toArray
3. I always prefer to use type annotation if it is not clear. E.g.: val nextMove: Int = readMove(board)
4. use Intellij autoformat (ctrl + alt + L)
5. it is good idea to say user what is wrong with his input before prompt new one
6. [Char].toString.matches("[1-9]") may be replaced on [Char].isDigit (PS: may be it is not correct, because 0 is also digit)
7. don't use return keyword, especially in foreach, map and so on. It is not what you want, at least in this cases. foreach + forall + return should be replaced on contains + forall or something.
8. usually list map f used only for non-chained calls, but not for list map m filter filter collect g, because it is unclear for reader. UPDATE: also this syntax is used for symbol-named functions, like +, ::, ! (in akka).
9. you can use special type for positive digit, for example, case class PositiveDigit(v: Int) {require(v > 0 && v <= 9, s"invalid positive digit: $v")}
10. there is no essential reason to pass the board as an argument. Commonly if an argument is passed, it is not changed. In your code it is not.

UPDATE for 10. In functional programming the clear way is to pass an immutable collection to the function and return new one if you need. It is programming without side effects. In OOP there is a way to use a mutable collection of class (or class's object). Scala is both OOP and FP language.

You can learn mode on the official Scala style web site

Answer 2

1. You are right that the readMove could be made better. There's no need for throwing an exception, especially since you're catching it immediately. A simple if/else can do the job here:

   @tailrec
   private def readMove(board: Array[Char]): Int = {
     val input = readLine("Input next Turn: ").toInt-1
     if(input<0 || input>8 || !board(input).toString.matches("[1-9]")) {
       readMove(board)
     } else {
       input
     }
   

   However, this means that an exception will be thrown if the input is not an integer. To fix that, regex can be used:

   @tailrec
   private def readMove(board: Array[Char]): Int = {
     val input = readLine("Input next Turn: ")
     if (input.matches("[1-9]")) {
       val move = input.toInt - 1
       if (board(move).isDigit) {
         move
       } else {
         println("That location is already taken.")
         readMove(board)
       }
     } else {
       println("The input must be an integer between 1 and 9.")
       readMove(board)
     }
   }
   

   I added a message telling the user what they did wrong, since simply asking for input again may be confusing. I also used isDigit as Mikhail Ionkin's answer suggested because it's cleaner.

2. Since Char is a pretty simple type, I don't think you need a type alias for it. Array[Char] is also rather simple, but it's up to you if you want it. Personally, I wouldn't use a type alias (yes, I know I recommended it in my last answer, but that was nearly a year ago).

3. printBoard looks more or less okay to me (though I may be a bit biased :) ), although here's an alternative using grouped to take advantage of the fact that the board is a single 1D array and not a collection of moves.

   private def printBoard(board: Array[Char]): Unit = {
     println(
       board.grouped(3)
         .map(_.mkString("|"))
         .mkString("__________\n", "\n------\n", "")
     )
     println(s"Next Player is ${nextPlayer(board)}")
   }
   

   Like the original, this creates new collections each time, but it shouldn't make a difference with a board of just 9 cells.

4. board(nextMove) = nextPlayer(board) is indeed not idiomatic Scala, where one usually avoids mutable collections such as Array. I'd like to suggest, once again, a List of moves that you can easily prepend to each time you add a move instead of modifying it directly.

---

Some more things I'd like to point out:

• The nextPlayer method doesn't feel right. You're traversing the array each time to find the next player. I'd suggest keeping track of the current player (as a local variable that's passed around, not as a global variable).

• isWon is an effectful method. You should either rename it so that it's clear that you're also going to have side effects there, not just return a Boolean, or you should return a value indicating whether there's a winner, there's a tie, or the game can continue. That value should be used by the calling function to print the results.

• Also, I don't see any way for the game to end should a tie occur. This seems to be a pretty big problem.

• In the isWon method, you use return true. Using return from a lambda is a bad idea (in fact, using return at all isn't a good idea). It throws a NonLocalReturnControl exception to pretend to work like a normal return. It can seriously mess things up if you call the lambda somewhere else and there's nothing to catch the exception. Instead, you can use find here to obtain an Option possibly containing a Player, and then act on that.

• I agree with all of Mikhail Ionkin's points except the last one. Passing the board around is better than keeping it as a global variable. I'd suggest continuing to do that.

• Your code's formatted pretty well, but some places lack spaces. Make sure you put a space between if and the opening parenthesis, between operators like < and their operands (input < 0 instead of input<0), and between a lambda parameter and the arrow (pattern =>). Also, the lambda in isWon can be rewritten like this:

  patterns.foreach { pattern =>
    ...
  }