Is it good style to nest for-comprehensions in Scala?

Question

I just found myself writing a piece of code that looks like this:

  def language(frequencies: Array[String], text: Array[String]) = {
    val allText = text.mkString.replace(" ", "")

    val emaps = for {
      fset <- frequencies
      devs = for {
        i <- 'a' to 'z'
        p = fset.indexOf(i) match {
          case -1 => 0d
          case x  => fset.substring(x + 1, x + 3).toDouble / 100 * allText.size
        }
        a = allText.count(i ==)
        dev = math.pow(p - a, 2)
      } yield dev
    } yield devs.sum

    emaps.min
  }

As you can see, the value emaps is an array of Doubles created from an array of Strings. It works fine. I just haven't seen for-comprehensions nested like this before. Is it OK or should I refactor somehow?

Answer 1

It's generally more standard to use map and friends than to write long blocks of code in the looping part of the for construct. And since allText doesn't depend on the frequencies, you can do that once at the beginning:

val lcounts = 'a' to 'z' map {i => i -> allText.count(i==)} toMap
val emaps = frequencies.map { fset =>
  val devs = 'a' to 'z' map { i =>
    val p = fset.indexOf(i) match {
      case -1 => 0d
      case x  => fset.substring(x+1, x+3).toDouble / 100 * allText.size
    }
    math.pow(p - lcounts(i), 2)
  }
  devs.sum
}

(Also, are you sure you want to square negative values, i.e. where allText.count(i==) is nonzero, but fset.indexOf(i) is -1? That seems strange.)

Answer 2

As soon as I use a match statement or something other then simple , sustinct if/ else I would use a method there. With good naming the code would become clearer to read IMO.