Scala Option vs. conditional branches

Question

If I'm writing Scala functions and have to check for nulls in situations where I can't avoid it (say, working with Spark UDFs and some legacy Java types), is it better to turn things into Option or to write conditional branches?

Example:

case class Foo (
  bars: Seq[Bar]
)

case class Bar(s: String)

def findBarWithConditionals(foo: Foo, barToFind: String): Option[Bar] = {
   if (foo == null || foo.bars == null) {
     None
   } else {
     foo.bars.find(_.s == barToFind)
   }
}

def findBarWithOptions(foo: Foo, barToFind: String): Option[Bar] = {
   Option(foo).flatMap(f => Option(f.bars).map(_.find(_.s == barToFind)))
}

Is there a rule of thumb of how to weigh readability vs. maintaining functional programming principles? Or is this purely opinion?

Answer 1

Don't get me wrong, optional is great. Use it when it makes sense.

However, if you want to compare it honestly to conditional code then you need better conditional code. I think De Morgan can help here.

if (foo != null && foo.bars != null) {
  foo.bars.find(_.s == barToFind)
} else {
  None
}

Conversely, I prefer whitespace that shows structure over fish gills.

Option(foo).flatMap(
  f => Option(f.bars).flatMap( //See note below
    _.find(_.s == barToFind)
  )
)

As a bonus, when error messages give you line numbers you have a better idea what blew up.

My rule of thumb

Learn to write readable code in either style before you decide which one is more readable. Otherwise, as you get better at just one of them, you're simply entrenching your own bias.

---

Note

While checking that my rewrite hadn't broken anything I discovered that this code was already broken. It seems both maps should be flat.⁼

for-comprehention

And here's what Gastón Schabas thinks this looks like as a for-comprehention:

for {
  f <- Option(foo)
  bar <- Option(f.bars)
  barFound <- bar.find(_.s == barToFind)
} yield {
  barFound
}

Which I like, apart from the meaningless names. If you can think of good names, great. Use them. If you can't, please avoid meaningless names.

Answer 2

You've already gotten some tips on how to improve the readability of your Option-based code in this specific situation. Another way, btw, is reducing the nesting level as follows:

Option(foo)
  .flatMap(f => Option(f.bars))
  .flatMap(_.find(_.s == barToFind))

In general, as a Scala developer I strongly prefer this style, and in fact find it more, not less, readable than your first option. The reasons are:

• using null is not idiomatic, to the point where I don't think I've written a single null check in my entire time working in Scala
• chaining functor/monad/etc. operations is the bread and butter of functional Scala and becomes more and more legible as you get used to it
• on the flip side, as chained operations often end up replacing what would be control flow statements like if/else in imperative code, you become less used to needing to parse multiple possible code paths and it becomes more of a mental overhead

I've noticed people coming from a more imperative/OO background often make assumptions about what constitutes readable Scala code that do not line up with my experience. I'm pretty sure this sort of thing is why.

Of note: both ways of writing this are still very surprising in a Scala mindset, because they violate one of the core assumptions a lot of us make: that nothing can be null! If your business logic is bigger or Foo and Bar get used in multiple places, I'd strongly suggest making a facade layer of some kind that will filter out null values, automatically transform Foo with null bars into Foo with empty bars, or map the whole thing to a custom class, so that your business logic doesn't have to worry about this at all.

Answer 3

I have same dilema and I couldn't find an answer I can cope with yet. Nevertheless following are some opinions maybe it helps.

Code using Option is mid ground between object oriented and functional programming. Talking principles in object oriented is the implementation of abstraction principle while functional seems to be disciplined state and from a wider perspective is the implementation of YAGNI principle. Otherwise is a fail-fast technique, is a way to avoid the billion-dollar mistake in software development following software development principles.

Talking readability either of the versions seems equally readable

    if (foo == null || foo.bars == null) {
        None
    } else {
        foo.bars.find(_.s == barToFind)
    }

    foo == null ? None : foo.bars == null ? None : foo.bars.find( _.s == barToFind)

    Option(foo).map(_.bars)
               .getOrElse(Seq.empty)
               .find(_.s == barToFind)