6

I do some data conversion on a list of string and I get a list of Either where Left represents an error and Right represents a successfully converted item. 

val results: Seq[Either[String, T]] = ...

I partition my results with:

val (errors, items) = results.partition(_.isLeft)

After doing some error processing I want to return a Seq[T] of valid items. That means, returning the value of all Right elements. Because of the partitioning I already knew that all elements of items Right. I have come up with five possibilities of how to do it. But what is the best in readability and performance? Is there an idiomatic way of how to do it in Scala?

// which variant is most scala like and still understandable?
items.map(_.right.get)
items.map(_.right.getOrElse(null))
items.map(_.asInstanceOf[Right[String, T]].value)
items.flatMap(_.toOption)
items.collect{case Right(item) => item}
Xavier Guihot
  • 54,987
  • 21
  • 291
  • 190
Christian Rodemeyer
  • 2,001
  • 1
  • 19
  • 22
  • 1
    `items.collect ...` seems like a good option here. – Dima Nov 07 '18 at 13:32
  • Can you avoid the intermediate `Right`? It seems cumbersome to `collect` and it seems dangerous/unnecessary to `get`. – erip Nov 07 '18 at 13:39
  • 2
    You might be interested in using Cats Validated for accumulating errors or results (https://typelevel.org/cats/datatypes/validated.html) - you can avoid the boilerplate that `Either` requires, like partitioning and unnecessary `.get` calls. – Eric Nov 07 '18 at 16:08
  • 1
    `items.flatMap(_.toOption)` works well in my tests and is a clean and understandable solution. – jwvh Nov 07 '18 at 18:21

3 Answers3

5

Using .get is considered "code smell": it will work in this case, but makes the reader of the code pause and spend a few extra "cycles" in order to "prove" that it is ok. It is better to avoid using things like .get on Either and Option or .apply on a Map or a IndexedSeq.

.getOrElse is ok ... but null is not something you often see in scala code. Again, makes the reader stop and think "why is this here? what will happen if it ends up returning null?" etc. Better to avoid as well.

.asInstanceOf is ... just bad. It breaks type safety, and is just ... not scala.

That leaves .flatMap(_.toOption) or .collect. Both are fine. I would personally prefer the latter as it is a bit more explicit (and does not make the reader stop to remember which way Either is biased).

You could also use foldRight to do both partition and extract in one "go": 

 val (errors, items) = results.foldRight[(List[String], List[T])](Nil,Nil) { 
    case (Left(error), (e, i)) => (error :: e, i)
    case ((Right(result), (e, i)) => (e, result :: i)
 }
Dima
  • 39,570
  • 6
  • 44
  • 70
1

Starting in Scala 2.13, you'll probably prefer partitionMap to partition.

It partitions elements based on a function which returns either Right or Left. Which in your case, is simply the identity:

val (lefts, rights) = List(Right(1), Left("2"), Left("3")).partitionMap(identity)
// val lefts:  List[String] = List(2, 3)
// val rights: List[Int]    = List(1)

which let you use lefts and rights independently and with the right types.

Xavier Guihot
  • 54,987
  • 21
  • 291
  • 190
0

Going through them one by one:

items.map(_.right.get)

You already know that these are all Rights. This will be absolutely fine.

items.map(_.right.getOrElse(null))

The .getOrElse is unnecessary here as you already know it should never happen. I would recommend throwing an exception if you find a Left (somehow) though, something like this: items.map(x => x.right.getOrElse(throw new Exception(s"Unexpected Left: [$x]")) (or whatever exception you think is suitable), rather than meddling with null values.

items.map(_.asInstanceOf[Right[String, T]].value)

This is unnecessarily complicated. I also can't get this to compile, but I may be doing something wrong. Either way there's no need to use asInstanceOf here.

items.flatMap(_.toOption)

I also can't get this to compile. items.flatMap(_.right.toOption) compiles for me, but at that point it'll always be a Some and you'll still have to .get it.

items.collect{case Right(item) => item}

This is another case of "it works but why be so complicated?". It also isn't exhaustive in the case of a Left item being there, but this should never happen so there's no need to use .collect.

Another way you could get the right values out is with pattern matching:

items.map {
  case Right(value) => value
  case other => throw new Exception(s"Unexpected Left: $other")
}

But again, this is probably unnecessary as you already know that all values will be Right.

If you are going to partition results like this, I recommend the first option, items.map(_.right.get). Any other options either have unreachable code (code you'd never be able to hit through Unit tests or real-life operation) or are unnecessarily complicated for the sake of "looking functional".

James Whiteley
  • 3,363
  • 1
  • 19
  • 46