Idiomatic Scala try option code block

Question

Basically, this function returns a list of flights for an airline. But if the airline doesn't exist, I want to throw some custom runtime exception. If that airline does exist, I want to return a list of flights (which could potentially be empty).

Is this is the standard way of doing things in Scala, or is there a better way to write this code?

Coming from a Java background, I'm still trying to get to grips with the Scala way of doing things.

def getFlightsForAirline(id:Long):Try[Option[List[Flight]]] = {
 val maybeAirline:Option[Airline] = Airline.getById(id)
 maybeAirline match {
 case Some(airline) => {
 Success(Flight.getFlightsByAirline(airline))
 }
 case None => Failure(new RecordNotFoundException("Couldnt find airline"))
 }
 }

Answer 1

1. The maybeAirline variable isn't necessary. Note that the type annotation in the declaration is optional, and can easily be inferred by the compiler. So I would change that to:

   def getFlightsForAirline(id: Long): Try[Option[List[Flight]]] =
    Airline.getById(id) match {
    case Some(airline) => Success(Flight.getFlightsByAirline(airline))
    case None => Failure(new RecordNotFoundException("Couldnt find airline"))
    }
   

   Notice that the Scala style guide suggests putting a space after the colon in a type annotation – name: Type instead of name:Type.

2. Every time you destructure an Option with match/case, they kill a puppy because you should have probably used map. Well, almost every time, because the Failure is an important part of this function's behaviour. But this still is somewhat of a code smell.

3. The return type Try[Option[List[Flight]]] strikes me as rather weird: What is the difference between Success(Some(List())) and Success(None())? I guess the latter might occur if getFlightsByAirline isn't passed a valid airline.

   Try, Option and List are essentially the same thing (either you get a result, or there isn't anything there: Failure, None or empty List). Therefore it does not make much sense to nest them, and it would be better to return a Try[List[Flight]]. This could be implemented along the lines of:

   def getFlightsForAirline(id: Long): Try[List[Flight]] =
    Airline.getById(id) flatMap { Flight.getFlightsByAirline(_) } match {
    case Some(flights) => Success(flights)
    case None => Failure(new RecordNotFoundException("Couldnt find airline"))
    }
   

   This way, the Failure is returned if either of the getXByY methods returns None.

   Interestingly, there is no builtin way to transform an Option to a Try (along the lines of maybe toTry { MyException("Reason") }) while there is a try.toOption method.

4. Your RecordNotFoundException will have a much more Scala-ish vibe if you use a case class here. This way, you can omit the new to create a new instance, and just write: Failure(RecordNotFoundException("..."))

• Akos Krivachy: +1 for the Option[List[...]] type, seen it too many times where it doesn't make sense :). Also how about considering Seq instead of List?
• ScalaWilliam: Good. You might want to comment on the clarity of getFlightsForAirline and getFlightsByAirline as well. It is not obvious which you would like to use and how either of those would behave.
• scand1sk: You can use a map here instead of a flatMap, cannot you?
• amon: @scand1sk No I cannot, that is the point of this exercise. Using Haskell's notation: map :: (A → B) → M[A] → M[B] and fmap :: (A → M[B]) → M[A] → M[B]. Translated into Scala, the former produces two nested Options: Option[Flight].map(Airline => Option[List[Flight]]): Option[Option[List[Flight]]], whereas Option[Flight].flatMap(Airline => Option[List[Flight]]): Option[List[Flight]]. In map, the thing inside the output container is returned. In flatMap, instances of the output container are returned, which are then concatenated to produce the final output.
• scand1sk: You are right, I did not realized that getFlightsForAirLine returned an Option. I have seen cases where Some(Nil) have not the same semantics as None, but this one ought not to be such a case.

Answer 2

The most idiomatic way of using Option is to use map/flatMap methods:

def getFlightsForAirline(id: Long): Try[List[Flight]] =
 Airline.getById(id) flatMap Flight.getFlightsByAirline map Success.apply getOrElse {
 Failure(new RecordNotFoundException("Couldnt find airline"))
 }

Note: this works, if you simplify returned type, e.g strip Option.

Otherwise, you need just change flatMap to map:

def getFlightsForAirline(id: Long): Try[Option[List[Flight]]] =
 Airline.getById(id) map Flight.getFlightsByAirline map Success.apply getOrElse {
 Failure(new RecordNotFoundException("Couldnt find airline"))
 }