0
\$\begingroup\$

In one of my services, I have the function as presented below:

private def convertCreateResultsToCreateAllReasult(results: List[CreateResult.Result]): CreateAllResult.Result = {
    val firstConflictResult = results.collectFirst {
      case r: CreateResult.Conflict => r
    } match {
      case Some(cr: CreateResult.Conflict) => Some(CreateAllResult.Conflict(cr.message))

      case None => None
    }

    val firstNotFoundResult = results.collectFirst {
      case r: CreateResult.NotFound => r
    } match {
      case Some(nfr: CreateResult.NotFound) => Some(CreateAllResult.NotFound(nfr.message))

      case None => None
    }

    val firstFailedResult = results.collectFirst {
      case r: CreateResult.Failed => r
    } match {
      case Some(fr: CreateResult.Failed) => Some(CreateAllResult.Failed(fr.message))

      case None => None
    }

    val failedResults = List(firstConflictResult, firstNotFoundResult, firstFailedResult).flatten

    if (failedResults.isEmpty) {

      val createdDefinitions = results.collect {
        case r: CreateResult.Ok => r.definition
      }

      CreateAllResult.Ok(createdDefinitions)

    } else {

      failedResults.head

    }
  }

The argument of this function is a list of result objects, where the result object looks like this:

object CreateResult {

    sealed trait Result

    case class Ok(definition: ChecklistRuleDefinition) extends Result
    case class Conflict(message: String) extends Result
    case class NotFound(message: String) extends Result
    case class Failed(message: String) extends Result

  }

As you can see the result can be Ok (which is considered successful) and everything else (that considered as failed for various reasons).

The function should:

  • Return first fails result converted into a different type in case there is one such result in the list. or
  • Return all successful results converted into CreateAllResult.Ok (if all of them successful).

Here is the CreateAllResult object:

object CreateAllResult {

    sealed trait Result

    case class Ok(definitions: List[ChecklistRuleDefinition]) extends Result
    case class Conflict(message: String) extends Result
    case class NotFound(message: String) extends Result
    case class Failed(message: String) extends Result

  }

The thing is, whatever I posted here, it seems to be working as expected. But, I hate the code. I know know that scala has all the bells and whistles to make the code nicer and more readable. I just didn't find the way to do so.

How can this code be written in a nicer, scala like, way?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Could you provide more context for this question? What is the CreateAllResult class, and how does it differ from CreateResult? \$\endgroup\$ – 200_success Oct 6 '17 at 1:29
  • \$\begingroup\$ @200_success Not, sure if there is more context that I can provide. But, sure, I edited the question with CreateAllResult object \$\endgroup\$ – Shurik Agulyansky Oct 6 '17 at 1:38
  • \$\begingroup\$ Why bother with CreateAllResult, when it looks just like CreateResult? \$\endgroup\$ – 200_success Oct 6 '17 at 1:39
  • \$\begingroup\$ Well, the Ok is different. And, also, that is our convention. That does not simplify anything though. \$\endgroup\$ – Shurik Agulyansky Oct 6 '17 at 1:40
3
\$\begingroup\$

Something like this maybe?

  @tailrec
  def trf(
    results: List[CreateResult.Result],
    conf: Option[CreateAllResult.Conflict] = None,
    nf:   Option[CreateAllResult.NotFound] = None, 
    f:    Option[CreateAllResult.Failed] = None, 
    defs: List[ChecklistRuleDefinition] = Nil
  ): List[CreateAllResult.Result] = results.map { 
    case r: CreateResult.Conflict :: tail if conf.isEmpty => 
         trf(tail, Some(CreateAllResult.Conflict(r)), nt, f, Nil)
    case r: CreateResult.NotFound :: tail if nf.isEmpty  => 
         trf(tail, conf, Some(CreateAllResult.NotFound(r)), f, Nil)
    case r: CreateResult.Failed  if f.isEmpty => 
         trf(tail, conf, nf, Some(CreateAllResult.Failed(r)), Nil)
    case Ok(defn) :: tail if conf ++ nf ++ f isEmpty => 
         trf(tail, None, None, None, dfn :: defs)
    case _ if defs.isEmpty => conf ++ nf ++ f
    case _ =>  CreateAllResult.Ok(defs.reverse)
  }

Oh, wait you are only returning the first failure, are you? I didn't realize that at first, thought you wanted the first of each type. Well, that makes it kinda simpler ... How about this:

object Failure {
   def unapply(res: Result) = res match {
    case r: CreateResult.Conflict  => Some(CreateAllResult.Conflict(r))
    case r: CreateResult.NotFound  => Some(CreateAllResult.NotFound(r))
    case r: CreateResult.Failed => Some(CreateAllResult.Failed(r))
   } 
}

def trf(results: List[CreateResult.Result]) = results
  .collectFirst { case Failure(r) => r }
  .getOrElse(CreateAllResult.Ok(
     results.collect { case r: CreateResult.Ok => r.definition }
   ))
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.