Count of words in a mutable Map

Question

The function should get some text on input and return a map in a following format:

("word1" -> 1, "word2" -> 2 ...)

The keys are words from text and the values are representing the number of times the word appeared in the text. We take into a count only words which at least 4 characters long. Special chars are ignored, words are case insensitive.

import scala.collection.mutable.Map

def wordCounter(text: String) = {
  var map:Map[String, Int] = Map()

  text.toLowerCase.replaceAll("[^a-z ]", "").split(" ").filter(_.length > 3).foreach(x => addWord(map, x))
  map
}

def addWord(map: Map[String, Int], word: String) = {
  map(word) = map.getOrElse(word, 0) + 1
}

• Is that fine to use mutable in this case?
• Should I always be suspicious about the solutions that require to use mutable and use it only when its problematic to implement it with immutable types?
• I'm doing an extra loop to filter words longer than 3 chars. In my mind this way it looks a bit cleaner than to have a condition in foreach. Should I care more about the performance or readability in my code?

Answer 1

Solution Explanation

So first off we drop everything to lowercase and then filter each Char of the string based on whether it is alphanumeric or a space. We've kept the spaces so that in the next line we may split the String into an Array of sub-strings which we then filter based on your length requirement. In the final line we utilize two more collection methods groupBy and mapValues.

If res1 was equal to Array(abcd, abcd, scala) then res1.groupBy(w => w) would return Map[String, Array[String]](abcd -> Array(abcd, abcd), scala -> Array(scala)).

...mapValues then performs the final transformation to get your desired output.

def wordCounter(text: String): Map[String, Int] = {
  val res0 = text.toLowerCase.filter(c => c.isLetterOrDigit || c == ' ')
  val res1 = res0.split(' ').filter(_.length > 3)
  res1.groupBy(w => w).mapValues(_.length)
}

Regarding your Questions

Is that fine to use mutable in this case?

Should I always be suspicious about the solutions that...

• There are times when it is OK to use mutability. After all it is a part of the language, and the language is just a tool to get stuff done. In this case, however, your function wordCounter leaks mutability by returning a mutable.Map[...]. In other words, if you need to use mutability within a function don't let it escape the function.

... Should I care more about the performance or readability in my code?

• Don't take this the wrong way, but performance and readability aren't mutually exclusive. While it may be the case that performant code ends up being longer, it should still remain as readable as a succinct one-liner.

• But while we're on the subject of readability, note that I added a return type to your wordCounter function :) Fortunately for style related questions Scala has a great style guide. Within the guide the Declarations -> Methods subsection is one place you can find details on idiomatic method declaration style and the reasoning behind it.

Other Details

• When performing operations on common data structures such as a String or Array as we do in the above code, and the overall scope of the program doesn't have a set of common domain specific descriptions I think it is OK to use short value names, e.g. res0, res1, etc. If we wanted to improve readability even further we could always include type signatures with our value declarations. For example:

  val res0: String = ...
  val res1: Array[String] = ...
  

Answer 2

Should I always be suspicious about the solutions that require to use mutable and use it only when its problematic to implement it with immutable types?

I'm new to Scala myself, but I would say yes. Try to use immutable and elegant solutions when possible, but it's OK to resort to mutable alternatives as a second choice.

Is that fine to use mutable in this case?

I would use an immutable algorithm in this case:

• use .groupBy to create groupings
• use .map to convert the groupings to a list of word-count pairs
• a final .toMap call and the map is ready

I'm doing an extra loop to filter words longer than 3 chars. In my mind this way it looks a bit cleaner than to have a condition in foreach. Should I care more about the performance or readability in my code?

If the complexity is on the same order, then prefer readability, even if slightly slower. For example, it's OK to choose an \$O(3n)\$ algorithm instead of an \$O(2n)\$ algorithm. When code is more readable, it tends to be also easier to optimize at a higher level of logic, which can lead to far faster solutions than the original alternatives.

Suggested implementation using immutable logic:

  def wordCounter(text: String) = {
    {
      for {
        word <- text.toLowerCase.replaceAll("[^a-z ]", "").split(" ")
        if word.length > 3
      } yield word
    }.groupBy(identity).map { case (word, list) => (word, list.length) }
  }