Which is more readable?

[Joe San]

The following functions that operates on two option types, which one from your point of view is readable?

def mapFn[A, B, C](a: Option[A], b: Option[B]) (f: (A, B) => C): Option[C] = (a, b) match {
  case (None, None) | (_, None) | (None, _) => None
  case (Some(aa), Some(bb))                 => Some(f(aa, bb))
}

OR

def map2[A,B,C](a: Option[A], b: Option[B])(f: (A, B) => C): Option[C] =
  a flatMap (aa => b map (bb => f(aa, bb)))

From my point of view the first one is more readable while the second one if more elegant! What do you think?

[Sidharth Khattri]

I guess second one is more elegant as well as readable.

[Joe San]

Sidharth Khattri wrote:I guess second one is more elegant as well as readable.

Only if you already have some experience in Scala to understand what a flatMap does. If not the first version is always readable from my point of view!

[Jesper de Jong]

I find it easier to see what this method does in the first version: it takes two options and a function f and applies the function only when the two options are both not empty.

[Tim Cooke]

I find the first one easier to read, but that's probably only because I don't naturally 'see' how flatMap functions. Otherwise it's likely that the second would be the preferred choice.

But, even the first option looks a little odd to me. I would write it with the cases the other way round, which would negate the need for the OR operator. For example:

def mapFn[A, B, C](a: Option[A], b: Option[B]) (f: (A, B) => C): Option[C] = (a, b) match {  
  case (Some(aa), Some(bb))  => Some(f(aa, bb))
  case (_, _)                => None  
}

[Tim Cooke]

Also, the function name 'mapFn' is a terrible name. If we're on the topic of readability then a meaningful name would be the most effective improvement to make in this example.

[chris webster]

Another approach is to use a for comprehension over your two input Options:

  def applyToAandB[A, B, C](a: Option[A], b: Option[B])(f: (A, B) => C): Option[C] =
    for (aa <- a; bb <- b) yield  f(aa, bb)       

  /* Try it out */
  val x = Some(1)                                
  val y = Some(3)                                 
  def add(n1: Int, n2: Int) = n1 + n2           

  /* Applying your original function... */
  mapFn(x, y)(add)                                
  //> res0: Option[Int] = Some(4)
  /* gives the same result as mine */
  applyToAandB(x, y)(add)         
  //> res2: Option[Int] = Some(4)

This avoids the map stuff but is a lot easier to read than the explicit case checks on the Options. YMMV.

[Edit:]
In case you're curious:

This works because you can apply a for-comprehension to any class that provides map, flatMap, withFilter and foreach functions (see Chapter 23 of "Programming In Scala (2nd edition)" by Martin Odersky).

Here we are looking at Options, and Options extend the Iterable trait, which supplies these functions. That's partly why Options behave in many ways like a collection that contains precisely one item or none.

So you can run a for-comprehension over an Option and it will yield Some(result) for Some(input), or None if the input Option was None.

I really like this trick!

[chris webster]

Tim Cooke wrote:I find the first one easier to read, but that's probably only because I don't naturally 'see' how flatMap functions. Otherwise it's likely that the second would be the preferred choice.

But, even the first option looks a little odd to me. I would write it with the cases the other way round, which would negate the need for the OR operator. For example:

def mapFn[A, B, C](a: Option[A], b: Option[B]) (f: (A, B) => C): Option[C] = (a, b) match {  
  case (Some(aa), Some(bb))  => Some(f(aa, bb))
  case (_, _)                => None  
}

You can make the default case even simpler because we don't care what we received if it wasn't a pair of Some options:

  def mapFn[A, B, C](a: Option[A], b: Option[B])(f: (A, B) => C): Option[C] = (a, b) match {
    case (Some(aa), Some(bb)) => Some(f(aa, bb))
    case (_) => None
  }

[Joe San]

Wow! It was worth the discussion!