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("..."))

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"))
  }