String Calculator Kata in Scala

Question

This is my solution of string calulator kata in scala(i'm new of tdd). I'd like a general review of this.

String Calculator

Create a simple String calculator with a method int Add(string numbers) The method can take 0, 1 or 2 numbers, and will return their sum (for an empty string it will return 0) for example “” or “1” or “1,2”

-Allow the Add method to handle an unknown amount of numbers

-Allow the Add method to handle new lines between numbers (instead of commas). the following input is ok: “1\n2,3” (will equal 6) the following input is NOT ok: “1,\n” (not need to prove it - just clarifying)

-Support different delimiters to change a delimiter, the beginning of the string will contain a separate line that looks like this:
“//[delimiter]\n[numbers…]” for example “//;\n1;2” should return three where the default delimiter is ‘;’ .

-Numbers bigger than 1000 should be ignored, so adding 2 + 1001 = 2

-Delimiters can be of any length with the following format: “//[delimiter]\n” for example: “//[***]\n1***2***3” should return 6

package stringcalculator

    import org.scalatest.{BeforeAndAfter, FunSuite}

    class StringCalculetor3 extends FunSuite with BeforeAndAfter {


    private val calculator = new Calculator(new InputParser())

    before {}

    test("sum of empty string is 0") {

      assert(0 === sum(""))

    }

    test("sum of one number  string is number") {

      assert(5 === sum("5"))
      assert(6 === sum("6"))

    }


    test("sum of two  numbers  separated with comma") {

      assert(3 === sum("1,2"))
      assert(5 === sum("1,4"))

    }

    test("sum of 3  numbers separated with comma") {

      assert(6 === sum("1,2,3"))

    }


    test("sum of 2  numbers  separated new line  separetor") {

      assert(3 === sum("1\n2"))
      assert(6 === sum("1\n2,3"))

    }

    test("sum of 2 numbers with another separator") {

      assert(3 === sum("//;\n1;2"))

    }

    test("sum of 2 numbers with multiple separator") {
      assert(3 === sum("//[;]\n1;2"))
      assert(3 === sum("//[--]\n1--2"))
    }

    test("numbers > 1000 do not fall in the sum") {
      assert(3 === sum("1,2,1001"))
    }


    private def sum(in: String): Int = {
      calculator.sum(in)
    }

The calculator

package stringcalculator

/**
 * Created by MarcopTo on 20/10/2015.
 */
class Calculator(inputParser: InputParser) {

  def sum(input: String): Int = {
    val (in, separetor) = inputParser.parse(input)
    val numbers = in.split(separetor).map(_.toInt).filter(_ < 1001)
    numbers.sum
  }

}

The parser

package stringcalculator

/**
 * Created by MarcopTo on 20/10/2015.
 */
class InputParser() {

  private val defaultSeparator: String = ",|\n"
  private val singleSeparatorPosition: Int = 2

  def parse(input: String): (String, String) =
    if (containsSeparator(input)) {
      val endOfHeader: Int = input.indexOf("\n")
      val separetor = extractSeparator(input,endOfHeader)
      val in = extractInput(input,endOfHeader)
      (in, separetor)
    }else
      (defaultInput(input), defaultSeparator)


  private def defaultInput(input: String): String = if (input.isEmpty) "0" else input

  private def containsSeparator(in: String): Boolean = in.contains("//")

  private def extractInput(input: String,endOfHeader: Int): String = input.substring(endOfHeader+1)

  private def extractSeparator(in: String,endOfHeader: Int): String = {
    val header = getHeader(in, endOfHeader)
    if (containsMultipleSeparator(header))
      in.substring(in.indexOf("[") + 1, in.indexOf("]"))
    else
      header.charAt(singleSeparatorPosition).toString
  }

  private def getHeader(in: String, endOfHeader: Int): String = in.substring(0, endOfHeader)
  private def containsMultipleSeparator(in: String): Boolean = in.contains("[")

}

Answer 1

It is nice that you have divided the parsing and the actual calculation over the InputParser and the Calculator.

I think your InputParser can be improved at some points :

• Using a regular expression to extract the separator would reduce the size of your InputParser and the large number of methods needed to get the separator.
• I would also include the splitting of the input in the parser, so you can return only the numbers. The user of the parser does not really need to know which separator was used.
• toInt throws an exception if the string can not be converted to an Int, which you don't handle. More ideomatic Scala would be to return an Option, Try, ...
• If you add more functionality to the InputParser, you probably should also create unit tests for the InputParser class.

My attempt to improve your InputParser :

class Parser {
  private val defaultSeparator: String = ",|\n"

  // parse now returns an Option[List[Int]]
  // -> invalid input now returns None (eg parse("foo"))
  def parse(input: String): Option[List[Int]] = {
    input match {
      case "" => Some(List(0))
      case SeparatorNumbers(sep, numbers) => 
        val separator = sep.map(escapeSeparator).getOrElse(defaultSeparator)
        val stringNumbers = numbers.split(separator).toList
        // stringNumbers.map(...) is of type List[Option[Int]]
        // the sequence function below turns this into Option[List[Int]]
        // (you could also use Scalaz for sequence (or traverse), but 
        // that is probably overkill if you only use it here)
        sequence(stringNumbers.map(i => Try(i.toInt).toOption))
      case _ => None
    }
  }

  // not explicitly mentioned in the requirements
  // make multiple separators possible (eg "//[:|;|,]\n1:2;3.4")
  private def escapeSeparator(sep: String) = 
    sep.split("""\|""").map(java.util.regex.Pattern.quote).mkString("|")

  // an extractor object which uses a regular expression
  // pattern matching with SeparatorNumbers gives an optional separator
  // and the numbers (still as one string)
  object SeparatorNumbers {
    // regular expression with two capture groups for separator and the rest
    private val Extract = """(?s)^(?://\[?(.+)\]?\n)?(.*)""".r

    def unapply(input: String): Option[(Option[String], String)] = 
      input match {
        // if no separator is found, sep is null
        // Option(sep) will then be None 
        case Extract(sep, numbers) => Some(Option(sep), numbers)
        case _ => None
      }
  }
}

// possible implementation to sequence List[Option[A]]
def sequence[A](listO: List[Option[A]]) : Option[List[A]] =
  listO.foldLeft(Option(List.empty[A])) { 
    case (None, _) => None
    case (_, None) => None
    case (Some(list), Some(elem)) => Some(elem :: list)
  }.map(_.reverse)

Your Calculator class could pretty mutch stay the same, you only need to handle Option[List[Int]] instead of List[Int].

val p = new Parser

def add(input: String) = {
  p.parse(input).map(_.filter(_ < 1001).sum) 
  // now Option[Int], depends how you want to handle invalid input
}

Answer 2

Another Solution

class InputParse() {
    val n = """\d+""".r
    def parse(input: String): List[Int] = 
      n.findAllIn(input).map(_.toInt).toList
}

// ...

class Calculator(inputParser: InputParser) {
  def sum(input: String): Int = {
    val ints = inputParser.parse(input)
    (0 :: ints ::: List(0)).filter(_ <= 1000).reduce(_ + _)
  }
}

Review

• I realize that this is a learning exercise and in that regard your solution seems fine to me other than some slight stylistic inconsistencies.

• But if with a bit more Scala under your belt it is clear that your code is more verbose than is necessary.

• Also we can cut out a lot of that work with the realization that it doesn't matter how the delimiter is defined.

• In other words, at a high level the problem (without constraints) could also be stated as:

  Find the sum of all the integers in a given input string.