When to use Option/None return types

Question

I understand the whole principle and concept behind the Some/None/Option and I can certainly appreciate its merits. My question is more of best practice. When does it become overkill and when does it make sense to use it religiously. I think (and i could be wrong) but it makes sense to use it as much as possible since it's a safer way to pass around nothing (as opposed to null). The thing I see myself doing a lot though is having some functions littered with map, getOrElse, get, match and even sometimes nesting them which tends to look ugly. Is there some concept I'm missing or some best practice to do with a function that receives multiple Optional values. For example:

  def updateJobs(id: Int) = withAuth {
    request => {

      User.userWithToken(request.headers.get("token").get).map {
        user =>
          Job.jobsAfterIdForForeman(id.toString, user.id.toString) match {
            case Some(json) => Ok(json)
            case _ => NoContent
          }
      }.getOrElse(BadRequest)
    }
  }

or even worse for example:

  def addPurchaseRequest(json: JsValue) = {
    (json \ "jobId").asOpt[Long].map {
      jobId => JobDAO.jobWithId(jobId).map {
        job => PurchaseRequestDAO.insert(new PurchaseRequest(json, job)).map {
          model =>
            val request = model.asInstanceOf[PurchaseRequest]
            (json \ "items").asOpt[List[JsObject]].map {
              list => {
                if (PurchaseItemAssociationDAO.bulkInsert(PurchaseItemAssociation.itemsFromJsonArray(list, request.id))) Option(request.addResponseJson) else None
              }
            }.getOrElse(None)
        }.getOrElse(None)
      }.getOrElse(None)
    }.getOrElse(None)
  }

I managed to refactor some to not look so crazy, but is there a better way to refactor this so it doesn't look so crazy? Am I missing something or do you get used to things just looking like this? Seems like there should certainly be a cleaner practice.

Answer 1

Since the Option class is monadic, you should use for comprehensions to make that code look cleaner. For example, your second example can be rewritten as:

def addPurchaseRequest(json: JsValue) = 
  for {
    jobId <- (json \ "jobId").asOpt[Long]
    job <- JobDAO.jobWithId(jobId)
    model <- PurchaseRequestDAO.insert(new PurchaseRequest(json, job))
    request = model.asInstanceOf[PurchaseRequest]
    list <- (json \ "items").asOpt[List[JsObject]]
      if PurchaseItemAssociationDAO.bulkInsert(PurchaseItemAssociation.itemsFromJsonArray(list, request.id))
  } yield request.addResponseJson

Answer 2

Use for-comprehensions or at least use flatMap instead of map / getOrElse(None) to make things more compact. Also, it's customary to put the new variable at the end of the previous line instead of on its own line. This would clean up your worst case tremendously.

Alternatively, with particularly long chains that require intermediate logic, I find an exception-like mechanism works even better than for-comprehensions (based on the same principle as nonlocal returns):

trait Argh extends scala.util.control.ControlThrowable
implicit class GetOrArgh[A](val underlying: Option[A]) extends AnyVal {
  def or(a: Argh) = underlying match {
    case None => throw a
    case _ => underlying.get
  }
}
def winnow[A](f: Argh => A): Option[A] = {
  val argh = new Argh { }
  try { Some(f(argh)) }
  catch { case x: Argh if (x eq argh) => None }
}

Which you then use like this:

def addPurchaseRequest(json: JsValue) = winnow { fail =>
  val jobId = (json \ "jobId").asOpt[Long] or fail
  val job = JobDAO.jobWithId(jobId) or fail
  val model = PurchaseRequestDAO.insert(new PurchaseRequest(json, job)) or fail
  val request = model match {
    case pr: PurchaseRequest => pr
    case _ => throw fail
  }
  val list = (json \ "items").asOpt[List[JsObject]]
  if (!PurchaseItemAssociationDAO.bulkInsert(
    PurchaseItemAssociation.itemsFromJsonArray(list, request.id)
  )) throw fail
  request.addResponseJson
}

Whether this approach or the for comprehension works better depends (in my experience) on how much intermediate processing you have to do. If you can make everything one-liners, the for-comprehension is nice and clean. Once you require something a little more tangled, I like this approach better.

Note that for comprehensions are canonical Scala constructs, whereas this will require some learning for new users. Thus, favor for comprehensions or flatMaps in code that people may need to get up to speed on rapidly.