CODEWITHSUNDEEP
scalafunctional-programmingrefactoringtype-conversion
Hanfei Sun
Hanfei Sun

Reputation: 47051

In Scala, how to refactor codes with Option class like this?

In the file com.typesafe.play/play_2.11/srcs/play_2.11-2.3.8-sources.jar!/play/api/data/Form.scala, I saw the definition of a function like this:

  protected def addPrefix(prefix: String) = {
    Option(prefix).filterNot(_.isEmpty).map(p => p + Option(key).filterNot(_.isEmpty).map("." + _).getOrElse(""))
  }

I think these codes may need a few seconds to understand and could be improved to be more clear. Does anyone have ideas about what might be a better way to rewrite it?

Upvotes: 3

Views: 151

Answers (2)

elm
elm

Reputation: 20415

A way to check whether prefix is an empty string,

Option(prefix).filterNot(_.isEmpty)

If not, append a (non empty string) key, else return an empty string. Many ways to rewrite this in several lines, although not necessarily so succinct, for instance,

protected def addPrefix(prefix: String) = {
  (prefix,key) match {
    case ("",_) => ""
    case (p,"") => p
    case (p,k)  => p + "." + k
  }
}

Note that the original code handles null strings,

Option(null: String).filterNot(_.isEmpty)
res5: Option[String] = None

whereas the suggested code here would require additional checks, namely for instance

def getStr(s: String) = Option(s).filterNot(_.isEmpty).getOrElse("")

and thus match the cases above against (getStr(prefix), getStr(key)) as follows,

protected def addPrefix(prefix: String) = {
  (getStr(prefix), getStr(key)) match {
    case ("",_) => ""
    case (p,"") => p
    case (p,k)  => p + "." + k
  }
}

The original code is terse yet readable and most significantly requires no additional elaboration as to handling null values or covering all the possible matching cases, namely, the semantics is well-defined.

A possible improvement on readability would be to define a function such as getStr(s: String) (proposed here), to shorten up the otherwise long one-liner.

The use of for comprehensions as suggested by Justin proves a robust alternative.

Upvotes: 1

Justin Pihony
Justin Pihony

Reputation: 67075

You could try turning it into a for-comprehension?

def addPrefix(prefix: String) =
  for{
    safePrefix <- Option(prefix)
    if !safePrefix.isEmpty
    safeKey <- (for{
      innerKey <- Option(key)
      if !innerKey.isEmpty
    } yield s".$innerKey").orElse(Option("")) 
  } yield s"$safePrefix$safeKey"

However, the inner for is messy due to the need for orElse. So, it is an option, and some do consider for comprehensions better....up to personal preference maybe. Realistically, it would work to wrap that inner for in a method and that would increase readability.

I was interested in elm's approach of using matching and even if you take the same approach, it still looks messy due to that inner condition:

def addPrefix(prefix: String) =  
  Option(prefix) match {
    case Some(prefix) if !prefix.isEmpty => Option(prefix + 
      (Option(key) match{
        case Some(key) if !key.isEmpty => s".$key"
        case _ => ""
      }))
    case _ => None
    }

Upvotes: 1

Related Questions