Skip to main content
added 1 character in body
Source Link
  • Using a regular expression to extract the separator would reduce the size of youyour 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.
  • Using a regular expression to extract the separator would reduce the size of you 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.
  • 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.
added 173 characters in body
Source Link
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 separtorseparator
  // 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)
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
}
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 separtor
  // 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)
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)
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
}
Source Link

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 you 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 separtor
  // 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].