3
\$\begingroup\$

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?")
\$\endgroup\$
  • 1
    \$\begingroup\$ You realise that your error messages are inaccurate and incomplete? Firstly, it should be "%d and %d not found" (because a "not found" message will only be generated if _map.contains(x + y) => false and either x or y is not in _map). Secondly, it's possible for you to report "x and x + y not found" when _map.contains(y) => true and when _map.contains(y) => false (so you're not reporting the state where neither x, nor y, nor x+y are in the map). Thirdly, this code will never return Left("What is it?"). Your errors do demonstrate why the functional approach is often superior. \$\endgroup\$ – itsbruce Aug 19 '13 at 15:52
  • \$\begingroup\$ how you will log the x and x + y if you had tested for nonexistence before? \$\endgroup\$ – Joao Polo Oct 29 '16 at 0:12
4
\$\begingroup\$

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

  1. Adds Left(x + y) to the head of the list: e :: l
  2. Collects all the invalid keys from the list: collect { case Left(i) => i }
  3. Builds a string containing the invalid keys: mkString
  4. 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.
\$\endgroup\$
  • \$\begingroup\$ Since writing this answer, I discovered that the braces in "List(x, y) map { pick }" can be left out and that "List(x, y) map pick" is more idiomatic. \$\endgroup\$ – itsbruce Aug 25 '13 at 11:54
2
\$\begingroup\$

In this case, I'd utilize pattern matching. As an example:

def getMap(): Map[String, String] =
  Map[String, String]("foo" -> "bar", "baz" -> "quux", "foobar" -> "bazquux")

// Note that I've simplified this to take Strings
def getOption(x:String, y: String): Either[String, Any] = {
  val map = getMap
  val (xOption, yOption, both) = (map.get(x), map.get(y), map.get(x + y))

  (xOption, yOption, both) match {
    case (Some(_), Some(_), _) => Right(xOption.get, yOption.get)
    case (_, _, Some(_)) => Right(both.get) 
    case (None, _, _) => Left("%s and %s not found".format(x, x + y))
    case (_, None, _) => Left("%s and %s not found".format(y, x + y))
  }

}

This has a few benefits. For example, say we commented out the last line:

// case (_, None, _) => Left("%s or %s not found".format(y, x + y))

The compiler will then complain at us:

warning: match may not be exhaustive. It would fail on the following input: (Some(_), None, None).

Likewise if we add in matches that are unreachable, we'll also get a warning:

case (None, None, None) => Left("What is it?")

warning: unreachable code case (None, None, None) => Left("What is it?")

You may want to modify this to be more specific:

(xOption, yOption, both) match {
  case (Some(_), Some(_), _) => Right(xOption.get, yOption.get)
  case (_, _, Some(_)) => Right(both.get) 
  case (None, Some(_), None) => Left("%s and %s not found".format(x, x + y))
  case (Some(_), None, None) => Left("%s and %s not found".format(y, x + y))
  case (None, None, None) => Left("Nothing at all found!")
}

We get no compiler errors or warning with this, so we can be sure that all the code is reachable and there will not be any unmatched patterns.

\$\endgroup\$
  • \$\begingroup\$ This is much more idiomatic scala and addresses the objections I raised to the original example. But using pattern matching as the entire solution means you are calculating x+y before you need to. The two "%s and %s not found" lines are also nearly identical in function. So while more idiomatic, I do not think this is more "functional". I know how I would do this in Scheme but barely know Scala. Hmm \$\endgroup\$ – itsbruce Aug 20 '13 at 10:50
  • \$\begingroup\$ I'm not too fussed about the calculation of x + y - that falls under worrying about premature optimization in my eyes. The string formatting I left "as-is", but I agree that it could likely be improved upon. \$\endgroup\$ – Yuushi Aug 20 '13 at 13:18
  • \$\begingroup\$ It doesn't answer the question, is my main point. Pattern matching is a feature of most modern functional languages because it has been shown to aid in the creation of concise and elegant code, but it is not a requirement for functional code nor a defining characteristic of it (that is, a functional language need not have pattern matching as a feature to qualify as functional). Pattern matching can simply the functions used to solve a problem, but this is not what you are doing. More idiomatic, yes; more functional, no. \$\endgroup\$ – itsbruce Aug 21 '13 at 12:32

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.