1

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?

4
  • 2
    Purely opinion. As a Scala developer, I find the latter easier to read - although I'd much prefer it if your types were wrapped in Option to start with so you didn't have to fake null checks. May 22, 2023 at 21:21
  • 1
    Definitely the second one is more what I'd expect to see. Though I'd more expect to see something before this point, there'd be some type with nulls, com.some.other.package.Foo and then there'd be MyFoo and findBar would take a MyFoo, and before I called findBar I'd have converted the com.some.other.package.Foo to MyFoo. And the reason is, with your way, if I see a Foo as some other parameter of some method, I'll have to do the same rigmarole there too. May 22, 2023 at 21:27
  • 2
    @PhilipKendall "purely opinion" is a valid answer :-) May 22, 2023 at 21:32
  • This is a great example of why Scala is such a crap language. It's almost like it was designed specifically to maximize the obfuscation of code.
    – JimmyJames
    Jun 28, 2023 at 21:09

3 Answers 3

3

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.

1
  • for comprehension seems like the way to go here, because it doesn't have nesting May 23, 2023 at 12:03
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.

2
  • "nothing can be null" - Spark can easily break this assumption because it will carry nulls in underlying datasets through to non-optional case class fields. Spark will break for nulls in Scala types like Int but not for String, BigDecimal or other object references. May 23, 2023 at 12:02
  • 2
    @wrschneider - framework code is not your business logic code; you don't make assumptions about the outside of your application, you make assumptions about your own little bubble the boundary of which you guard, so that you can write more succinct and readable code. May 23, 2023 at 18:57
0

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)
1
  • Is that ternary ?..: expression valid syntax? I think it should be if..else Jul 9, 2023 at 19:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.