CODEWITHSUNDEEP
scalaeither
Christian Rodemeyer
Christian Rodemeyer

Reputation: 2058

How can I convert a list of Either to a list of Right values?

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}

Upvotes: 8

Views: 4217

Answers (3)

Xavier Guihot
Xavier Guihot

Reputation: 61766

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.

Upvotes: 2

Dima
Dima

Reputation: 40508

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

Upvotes: 5

James Whiteley
James Whiteley

Reputation: 3464

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

Upvotes: 0

Related Questions