Adding two values in a map, if they exist

My code looks like imperative style code. I want to make my code more functional. How I can rewrite my program so it become functional style code?

val _map = getMap();
    if(_map.contains(x) && _map.contains(y)) Right(_map(x) + _map(y))
    else if(_map.contains(x + y)) Right(_map(x + y))
    else if(!_map.contains(x)) Left("%d or %d not found".format(x, x + y))
    else if(!_map.contains(y)) Left("%d or %d not found".format(y, x + y))
    else Left("What is it?")

OK, so I taught myself a little scala and here is something a bit more functional than your example:

val _map = Map(1 -> 2, 2 -> 8, 4 -> 13 )

def pickSum(x:Int, y:Int): Either[String,Int] = {

  /* If the map contains the key k, return Right(value)
   * else return Left(key)
   */
  def pick(k:Int): Either[Int,Int] = {
    _map.get(k) match {
      case Some(v) => Right(v)
      case _ => Left(k)
    }
  }

  List(x, y) map { pick } match {
    case List(Right(a), Right(b)) => Right(a + b) // Two matching values: return the sum
    case l:List[Either[Int,Int]] => pick(x + y) match { // At least one failure: try  x+y
      case Right(i) => Right(i)  // x+y is in the map.  Return the value
      // Otherwise, return the list of keys not found in the map
      case e:Left[_,_] => Left( e :: l collect { case Left(i) => i } mkString("Not found in map: ", ",", "") ) 
    }
  } 
}

Testing this in the Scala repl...

scala>  pickSum(1,2)
res0: Either[String,Int] = Right(10)

scala> pickSum(2,4)
res1: Either[String,Int] = Right(21)

scala> pickSum(2,3)
res2: Either[String,Int] = Left(Not found in map: 5,3)

scala> pickSum(1,3)
res3: Either[String,Int] = Right(13)

scala> pickSum(3,7)
res4: Either[String,Int] = Left(Not found in map: 10,3,7)

This does more or less what your code does; it either returns a Right() with the desired Integer value or a Left() containing a string explaining which keys could not be found in the map. Personally, in the case of failure I had rather just return a Left containing a list of the failed keys, to be handled as some other part of the code saw fit.

The core function, here, is to look for a key in the map and either return the matching value wrapped in a Right() or, if there is no such key, the rejected key wrapped in a Left(). I went straight for Map.get rather than checking with .contains() because the former returns an Option, which allows more concise and expressive code.

You should notice that nowhere in the code do I re-assign a value (hell, I don't even create any vals, let alone vars). Instead, I return a series of immutable collections, each one derived from the previous one. This is one aspect of functional programming.

So...

List(x, y) map { pick }

This makes a list out of the given keys and then creates from that a list of the corresponding values or the rejected keys. I then do some pattern matching

case List(Right(a), Right(b)) => Right(a + b)

If this matches, we have successfully retrieved two values. Return the sum. And we're done here. Otherwise...

case l:List[Either[Int,Int]] => pick(x + y)

So this matches a list but doesn't care what it contains, because we know there was at least one failure or the previous match would have succeeded. Matching all the permutations of failure in one line is less fragile than listing them all, if there's only one action to take. So now we look for _map(x + y) and match the result...

case Right(i) => Right(i)

If this matches, _map(x + y) exists so we return it and that's it. Otherwise...

case e:Left[_,_] =>

OK, (x + y) is not a valid key. So what the rest of that line does is

• Adds Left(x + y) to the head of the list: e :: l

• Collects all the invalid keys from the list: collect { case Left(i) => i }

• Builds a string containing the invalid keys: mkString

• Wraps the resulting string in a Left()

I'm sure this could be done better, since I only learned enough Scala to do this in the last 24 hours. However, it is (like Yuushi's answer) more idiomatic while also using some functional programming techniques (however modestly).

To genuinely improve my example code, it would help to know why you're doing this slightly odd thing.

To summarise the differences between your example and mine:

• All the else if statements make yours very fragile.

• Each individual else if statement is almost the same (i.e. using _map.contains()), giving room for error with all that duplication.

• You could easily miss a permutation.

• You might (in fact you do) have code paths that can never be evaluated.

• I avoid duplication almost entirely.

• Pattern matching makes it almost certain I catch all the permutations.

StackExchange Code Review Q#29946, answer score: 4