MarsRover in Scala

Question

This is my first attempt at Scala programming. I tried to be functional but I'm not sure if I have achieved that.

DataTypes.scala

object DataTypes {
 case class Position(xPos: Int, yPos: Int)
 sealed trait Direction
 case object N extends Direction
 case object S extends Direction
 case object E extends Direction
 case object W extends Direction
 case class RoverState(direction: Direction, position: Position)
 val leftRotation = Map[Direction, Direction](N -> W, W -> S, S -> E, E -> N)
 val rightRotation = Map[Direction, Direction](N -> E, W -> N, S -> W, E -> S)
}

RoverMachine.scala:

import DataTypes._
object RoverMachine {
 def initialRoverState(direction: Direction, position: Position): RoverState = {
 RoverState(direction, position)
 }
 def move(roverState: RoverState): RoverState = {
 roverState.direction match {
 case N => RoverState(N, Position(roverState.position.xPos, roverState.position.yPos + 1))
 case S => RoverState(S, Position(roverState.position.xPos, roverState.position.yPos - 1))
 case E => RoverState(E, Position(roverState.position.xPos + 1, roverState.position.yPos))
 case W => RoverState(W, Position(roverState.position.xPos - 1, roverState.position.yPos))
 }
 }
 def leftTurn(roverState: RoverState): RoverState = {
 RoverState(leftRotation.get(roverState.direction).get, roverState.position)
 }
 def rightTurn(roverState: RoverState): RoverState = {
 RoverState((rightRotation.get(roverState.direction)).get, roverState.position)
 }
}

RoverApp.scala

import DataTypes._
import RoverMachine._
import scala.io.StdIn.readLine
object RoverApp extends App {
 println("Enter Direction")
 def parseToDirection(direction: String): Direction = {
 direction match {
 case "N" => N
 case "S" => S
 case "E" => E
 case "W" => W
 }
 }
 val initialDirection: Direction = parseToDirection(readLine())
 println("Enter XPosition")
 val initialXPosition: Int = readLine().toInt
 println("Enter YPosition")
 val initialYPosition: Int = readLine().toInt
 println("Enter Instructions")
 val instructionList: Array[Char] = readLine().toCharArray
 var roverState = initialRoverState(N, Position(initialXPosition, initialYPosition))
 for (instruction <- instructionList) {
 instruction match {
 case 'M' => roverState = move(roverState)
 case 'L' => roverState = leftTurn(roverState)
 case 'R' => roverState = rightTurn(roverState)
 }
 }
 println(roverState.direction + " " + roverState.position.xPos + ", " + roverState.position.yPos)
}

Answer 1

Suggestions

1. For all your direction case object hierarchy, you are basically trying to duplicate an enum, which Scala provides no language-level support for. However, Scala does provide support for enumerations at the API level via the Enumeration class, which should be extended by new enums.

   You could try something like this:

   object Directions extends Enumeration {
    type Direction = Value
    val N, S, E, W = Value
   }
   

   and then your parseToDirection(String) method becomes unnecessary and can be replaced with a Direction.withName(String), a method for retrieving enum constants by name defined in Enumeration. You will also need to update your method signatures involving Direction to Directions.Direction, and add an import Directions._ before every piece of code in a given scope which uses a Direction, i.e., one import before declaring the rotation Maps, one in RoverMachine (this makes the get() call on the value unnecessary in the rotation methods), and one in RoverApp.

   However, this might complicate the code a bit and reduce the type safety offered by exhaustive pattern matching, however.

   TL;DR Your approach with a sealed trait and case objects is the preferred one in Scala, this is just an alternative. Note, you could also as well used a Java enum for this purpose too.

2. The one var in main() clouds the functional-ness of the code a bit. You could replace the loop with a recursive inner method in main, but there's always a better solution. Read to the end for a cohesive correction of your code.

3. String in Scala can be treated to inherit Seq[Char], as String has an implicit conversion to StringOps, which is a subclass of Seq[Char], so you don't need to convert the instructionList to an Array[Char] before looping over it, you can use it directly as a String.

4. Map.apply() is semantically the same as Map.get(), so use the former as it allows the syntactic sugar of making the entire call look like the following:

   def leftTurn(roverState: RoverState): RoverState = {
    RoverState(leftRotation(roverState.direction), roverState.position)
   }
   

   (similarly for rightTurn(RoverState))

5. Rely on type inference: In Scala, it is unnecessary to declare the types of most values, as they can be inferred by the compiler, and this does not decrease the readability of your code much unless the declaration is extremely complicated or the inferred type is unintuitive/incorrect.

   This makes your value declarations in main() look like (incorporating some of my other suggestions:

   val initialDirection = Directions.withName(readLine())
   val initialXPosition = readLine().toInt
   val initialYPosition = readLine().toInt
   val instructionList = readLine()
   

   Also, in DataTypes, you can skip the generic type specification for the Map[Direction,Direction] and write just Map, the compiler will infer the generic types.

6. The code to move the rover should be a part of the RoverMachine, not RoverApp. Also, it should be defined as a function like this:

7. Drop the surrounding braces in one-line functions, it makes the code look more functional.

8. No exception handling - What to do if the user enters illegal characters? Scala is infamous for its confusing stack traces, it would be good to define code to handle exceptional conditions - a functional (recursive) equivalent of running the entire main() block in a while loop which breaks only on legal input. Also, consider throwing proper exceptions in exceptional cases, an example is in point 6.

9. Imports should be declared closest to their site of use.

10. There is code duplication between leftTurn() and rightTurn(). You could probably use a turn() function accepting a type of turn, be it indicated by a Boolean flag, an enum constant (I use this approach in the code below), or a case object hierarchy. Pattern match on the type of turn to choose the correct rotations Map to use.

Style

Naming:

You name your side-effect-free functions in a way which makes users think that they have side-effects. Instead of naming them with present-tense verbs, name them with past-tense verbs, like turned instead of turn, moved instead of move, etc.

Your indentation and bracket usage are spot-on unless otherwise noted.

Errors

1. In main(), you initialize roverState as

   var roverState = initialRoverState(N, Position(initialXPosition, initialYPosition))

   The initial direction is always N; although you accept the initial direction as an input, you never use it. That line should be

   var roverState = initialRoverState(initialDirection, Position(initialXPosition, initialYPosition))

Rant

You hadn't tried much to be functional. If you had, then not even a single var, mutable data structure or loop would have turned up in your code, but all 3 did, so...

Most programmers hate global state, and for good reason. if there were any vars in your DataTypes object, it would have been a mess in the making. Thankfully, it is just a declaration of constants, so no problem there.

My take on your code

The final code, which is very functional and handles errors using Option (real error handling should use Either to retain error information, but AFAIK it's provided by scalaz, which I don't have), follows. I put it all into one file, as Scala allows that.

object DataTypes {
 case class Position(xPos: Int, yPos: Int)
 object Directions extends Enumeration {
 type Direction = Value
 val N, S, E, W = Value
 }
 case class RoverState(direction: Directions.Direction, position: Position)
 import Directions._
 val leftRotation = Map(N -> W, W -> S, S -> E, E -> N)
 val rightRotation = Map(N -> E, W -> N, S -> W, E -> S)
 object Turns extends Enumeration {
 type Turn = Value
 val Left, Right = Value
 }
}
object RoverMachine {
 import DataTypes._
 def initialRoverState(direction: Directions.Direction, position: Position) = Some(RoverState(direction, position))
 import Directions._
 def moved(state: Option[RoverState]) = state match {
 case Some(roverState) => roverState.direction match {
 case N => Some(RoverState(N, Position(roverState.position.xPos, roverState.position.yPos + 1)))
 case S => Some(RoverState(S, Position(roverState.position.xPos, roverState.position.yPos - 1)))
 case E => Some(RoverState(E, Position(roverState.position.xPos + 1, roverState.position.yPos)))
 case W => Some(RoverState(W, Position(roverState.position.xPos - 1, roverState.position.yPos)))
 }
 case None => None
 }
 import Turns._
 def turned(state: Option[RoverState], turn: Turns.Turn) = state match {
 case Some(roverState) => Some(RoverState(turn match {
 case Left => leftRotation(roverState.direction)
 case Right => rightRotation(roverState.direction)
 }, roverState.position))
 case None => None
 }
 def travelled(state: Option[RoverState], instructions: Seq[Char]): Option[RoverState] = state match {
 case Some(roverState) => instructions match {
 case Seq() => state
 case head +: tail => travelled(head match {
 //also added support for lower case characters in the instructions String
 case 'M'|'m' => moved(state)
 case 'L'|'l' => turned(state, Turns.Left)
 case 'R'|'r' => turned(state, Turns.Right)
 case other => None
 }, tail)
 }
 case None => None
 }
}
object RoverApp extends App {
 import DataTypes._
 import RoverMachine._
 import scala.io.StdIn.readLine
 println("Enter Direction")
 val initialDirection = Directions.withName(readLine())
 println("Enter XPosition")
 val initialXPosition = readLine().toInt
 println("Enter YPosition")
 val initialYPosition = readLine().toInt
 println("Enter Instructions")
 val instructions = readLine()
 val state = travelled(initialRoverState(initialDirection, Position(initialXPosition, initialYPosition)), instructions)
 state match {
 case Some(roverState) => println(roverState.direction + " " + roverState.position.xPos + ", " + roverState.position.yPos)
 case None => throw new IllegalArgumentException("There were illegal characters in the Command String")
 }
}

I have left off the return types of non-recursive functions as the compiler can infer them, and they are not unintuitive enough to hurt readability.

Edit:

In response to @leoOrion's comment about pattern matching and value bindings, i think I should explain what the following piece of code actually does.

 instructions match {
 case Seq() => ... //matches the empty _Seq_uence
 case head :+ tail => ... //binds the head of the _Seq_uence to `head` and the tail to `tail` (similarly to pattern matching on lists). Read below.
 ...
 }

Pattern matching in functional languages is a very powerful construct, much more so than the simple switch-case in most imperative languages. The clauses in a pattern matching expression (a match expression in Scala), match the patterns in the clause against the input argument(s) (and, optionally, also bind values to the results of the match).

In Scala, pattern matching clauses are denoted by the starting case keyword, which often occludes beginners from the power of pattern matching. Hence, both Seq() and head :+ tail denote patterns to matched against the argument, which in this case is the instructions Seq. Now, Seq() just matches the empty Seq, as would Nil for an empty List, and head :+ tail for Seqs is the exact equivalent of head :: tail for Lists. This pattern not only matches the "head" (the first element of a list or sequence) and "tail" (the rest of that list or sequence), it also binds the values head and tail to the results of that matching, in respective order. So, head :+ tail would cause head to contain the first element of the instructions Seq, and tail to contain the rest of that Seq, allowing for easily described recursion over the sequence.

Note:

The case class :+ for pattern matching on Seqs in general is only available in Scala 2.10+. I use 2.11, and I guess the OP uses that or higher too. For a lower Scala version, this behaviour can be emulated by converting the Seq to a List before the pattern match with .toList, or by making a custom implicit wrapper for Seq containing the appropriate extractor (unapply()) method.

• Nathan Davis: You could also handle errors using exceptions.
• Nathan Davis: Also, I don't find the recursive call to travelled to be a very readable. It's difficult to tell where the first argument ends and the second one begins.
• Nathan Davis: Ah, I missed that. Although I disagree with the implication that exceptions are any less functional that Option or similar alternatives.
• leoOrion: case head +: tail => travelled(head match {. In this line what does 'head' and 'tail' mean ??
• Tamoghna Chowdhury: @leoOrion head and tail are variable to which the results of a pattern match on the instructions Seq are bound. (Pattern matching is more than a glorified switch-case, as you will soon realize if you continue with Scala). head contains the first element of the instructions Seq, while tail will contain the rest of the Seq (everything in the Sequence other than the first element).