Scala case-match optimization for performance and readability

Question

I wrote the below for pattern matching in scala :

scala> val rowMap = Map("abc" -> null)
rowMap: scala.collection.immutable.Map[String,Null] = Map(abc -> null)

scala> val postgresKey = "abc"
postgresKey: String = abc

scala> val h2Key = "xyz"
h2Key: String = xyz

Suggested Option:

scala> (rowMap.get(postgresKey).map(_.asInstanceOf[String]) , rowMap.get(h2Key).map(_.asInstanceOf[String])) match
     |       {
     |         case (Some(value), _) if (value != null && value.trim.nonEmpty) => value
     |         case (None, Some(value)) if (value != null && value.trim.nonEmpty) => value
     |         case (_ , _) => "0.0"
     |       }
res9: String = 0.0

My Code :

scala> (rowMap.get(postgresKey).map(_.asInstanceOf[String]) , rowMap.get(h2Key).map(_.asInstanceOf[String])) match
     |       {
     |         case (Some(value), _) if (value != null ) => if (value.trim.nonEmpty) value else "0.0"
     |         case (None, Some(value)) if (value != null ) => if (value.trim.nonEmpty) value else "0.0"
     |         case (_ , _) => "0.0"
     |       }  
res10: String = 0.0

Does the suggested option have any performance benefits than my code. I know that && will stop comparing the value.trim.nonEmpty the moment it sees a null. So, we save on 1 comparison.

I do understand that it is cleaner to read as well. Anything else that is better in the suggested approach ?

EDIT 1: I have been told to avoid using null. However it is coming as an input to me from anorm's defaultParser. Since the scenario is difficult to replicate, I gave the example above. In my case, unit tests are in H2 and actual database is postgres.

Below is the snippet from the code:

            val anormQuery = SQL(query)
            // map the anorm Row as per the input params, differentiate into aggregated and group cols
            val tmp = anormQuery.as(anormQuery.defaultParser.*)

            logger.info(" Printing the result set object " + tmp.toString())

            val finalSqlResultset = tmp.map(row ⇒ {

              // Anorm row.asMap has this behaviour that it adds either a leading dot(.) or <tablename>. in front of the map keys (the columns/alias in sql) based on whether it is H2 or Postgres

              val rowMap = for ((k, v) ← row.asMap) yield (k, v)
              logger.info("Print the resultSet as map : " + rowMap.toString())


              val aggregates = expressionsToAggregate.map(input ⇒ {
                val (anormKey, postgresKey) = doesColumnHaveAlias.get(input.alias.getOrElse("UnknownAlias")).getOrElse(("Unknown", "Unknown"))
                //
                val aggResult = AggregatedValue(
                  input.alias.get,
                  (rowMap.get(postgresKey).map(_.asInstanceOf[String]), rowMap.get(anormKey).map(_.asInstanceOf[String])) match {
                    case (Some(value), _) if (value != null && value.trim.nonEmpty) ⇒ value
                    case (None, Some(value)) if (value != null && value.trim.nonEmpty) ⇒ value
                    case (_, _) ⇒ "0.0"
                  })
                logger.info("## TRACE 1 ##" + aggResult)
                aggResult
              })
})

There is no performance difference here. The suggested option is more readable, and that should be more than enough to go for it. — marstran, Jan 23 '20 at 12:41
I would avoid the `asInstanceOf`. You can replace them with `Some(value: String)` in the pattern match. — Thilo, Jan 23 '20 at 12:41
I would also avoid the `value != null` check. You can use `Option(nullableValue)` to avoid creating `Some(null)` in the first place: `rowMap.get(postgresKey).flatMap(Option(_))` — Thilo, Jan 23 '20 at 12:43
The two options are not equivalent. For `(Some(" "), Some("12"))` the first option returns "12", the second one returns "0.0" — Thilo, Jan 23 '20 at 12:46
Yes, you are corect. I am safe as it will always have one of the keys. This method will have (postgesKey, None) when run against postges. Will contain (None, h2Key) for unittests on h2. So , that way I am saved. — ForeverLearner, Jan 23 '20 at 14:16
If you always only have one key, then you don't need the map at all. Just ignore the key, and use the value. — Dima, Jan 23 '20 at 14:33
Sorry for the confusion. I have more than one key in `rowMap`. I have either the `postgresKey` or the `h2Key` based on whether I am in unitest or integration test. That is the reason for the `case-match` — ForeverLearner, Jan 23 '20 at 14:44

Dima · Accepted Answer · 2020-01-23T13:09:14.947

4

First of all, don't use nulls. Ever. Just do val rowMap = Map("abc" -> "").

Or better yet, just not put junk into map at all (you are filtering it out anyway!).

Second, since you mentioned readability, it looks like you are looking for something like this:

 rowMap.get(postgresKey).filterNot(_.trim.isEmpty)
  .orElse(rowMap.get(h2Key).filerNot(_.trim.isEmpty))
  .getOrElse("0.0")

Finally, to answer your question the two versions you showed are not equivalent. The first one does what I did above. The second one is this:

 rowMap.get(postgresKey).orElse(rowMap.get(h2Key))
   .filterNot(_.trim.isEmpty)
   .getOrElse("0.0")

In the former case, setting postgresKey to "" is equivalent to not setting it at all, you'll always get the value for h2Key in that case. In the latter case, if postgresKey is set to "" you will get "0.0", regardless of what h2Key is set.

So, the behaviors are different, it depends on which one you actually need.

If the first type of behavior is what you need, it can also be written like this:

Some(rowMap.filterValues(_.trim.nonEmpty)).flatMap { case m => 
   m.get(postrgesKey) orElse m.get(h2Key)
}.getOrElse("0.0")

Or, if you listen to my advise and decide to not put junk into the map in the first place, you can loose the filter too:

roMap.get(postrgesKey) orElse rowMap.get(h2Key) getOrElse "0.0"

edited Jan 23 '20 at 13:09

answered Jan 23 '20 at 12:46

Dima

39,570
6
44
70

Good answer. A cascade of `orElse` is much more idiomatic in this scenario than a `match` with multiple `case` that you have to reason about (especially if they also have guards on them). – Thilo Jan 23 '20 at 12:49
if that class cast is really necessary (why cannot the Map be properly typed?), you can "upgrade" the `filterNot` to `collect { case x : String if x.trim.nonEmpty => x }` – Thilo Jan 23 '20 at 12:52
@Dima. Thanks for guiding me. I have updated my answer with code snippet. The null value is actually sent from anorm – ForeverLearner Jan 23 '20 at 14:06
2

Doesn't matter where it is sent from. Intercept it as early as possible, and get rid of it: `Option(value).getOrElse("")` – Dima Jan 23 '20 at 14:13
Awesome. Just simplified it to: `val rowMap = for ((k, v) ← row.asMap) yield (k, Option(v).getOrElse("0.0"))` – ForeverLearner Jan 23 '20 at 14:44

Scala case-match optimization for performance and readability

1 Answers1