Scala: improves readability and style of this piece of code

Question

The following is a fairly common play 2 platform controller:

def save(ideaId : Long) = CORSAction { request => Idea.findById(ideaId).map { idea => request.body.asJson.map { json => json.asOpt[Comment].map { comment => comment.copy(idea = idea).save.fold( errors => JsonBadRequest(errors), comment => Ok(toJson(comment).toString) ) }.getOrElse (JsonBadRequest("Invalid Comment entity")) }.getOrElse (JsonBadRequest("Expecting JSON data")) }.getOrElse (JsonBadRequest("Could not find idea with id '%s'".format(ideaId))) } 

I find it a little annoying with all of these nested .maps, and I also find it a little tedious that each error handling is at the bottom

How would you improve it to make it more readable and at the same time remain a functional idiomatic scala code?

I thought maybe something like this (this is seudo code, still not compiling)

 def save(ideaId : Long) = CORSAction { request => val idea = Idea.findById(ideaId).getOrElse( return JsonBadRequest("Could not find idea with id '%s'".format(ideaId))) val json = request.body.asJson.getOrElse( return JsonBadRequest("Expecting JSON data")) val comment = json.asOpt[Comment].getOrElse( return JsonBadRequest("Invalid Comment entity")) comment.copy(idea = idea).save.fold( errors => JsonBadRequest(errors), comment => Ok(toJson(comment).toString) ) } 

ps: I know it would be much better to avoid the return statement ...

Answer 1

Simplify first. Suppose I have three methods that take a String and return Option[String] :

 def foo(s: String): Option[String] = if (s.size >= 4) Some(s + "1") else None def bar(s: String): Option[String] = if (s(0) != 'A') Some(s + "2") else None def baz(s: String): Option[String] = if (s toSet ' ') Some(s + "3") else None 

I need a method that passes a string through them and returns the corresponding error message if I get None along the way. I could write this:

 def all(s: String): Either[String, String] = foo(s).map { x => bar(x).map { y => baz(y).map { z => Right(z) } getOrElse Left("Doesn't contain a space!") } getOrElse Left("Starts with an A!") } getOrElse Left("Too short!") 

But right, it's ugly. We can use the for -comprehension and toRight on Option to write a clearer version:

 def all(s: String): Either[String, String] = for { x <- (foo(s) toRight "Too short!" ).right y <- (bar(x) toRight "Starts with an A!" ).right z <- (baz(y) toRight "Doesn't contain a space!").right } yield z 

Calling toRight(msg) on Option gives us Left(msg) if it is empty, and a Right(whatever) otherwise. Then we must take the right projection of Either with .right , since Scala Either not right.

The equivalent in your case would be something like this:

 def save(ideaId: Long) = CORSAction { request => val saveResult = for { idea <- (Idea.findById(ideaId) toRight "Could not find id" ).right json <- (request.body.asJson toRight "Invalid Comment entity").right comment <- (json.asOpt[Comment] toRight "Expecting JSON data" ).right result <- comment.copy(idea = idea).save().right } yield result saveResult.fold( error => JsonBadRequest(error), comment => Ok(toJson(comment).toString) ) } 

Not as concise as your desired syntax, but error messages appear in a more logical place, and we got rid of the ugly attachment.