Scala: Convert procedural code to functional

Question

I've written code similar to the one given below. It works as per the (bit confusing) requirements but something tells me it can be written differently in Scala using 'pattern matching' or something like that to make it look more functional. Any suggestions?

  def xyzMethod(map: util.HashMap[Any, List[SomeObject]]) : Option[xyzObject] = {
    var myObject: Option[xyzObject] = None
    val cCount: Int = map.get("c").size
    if (cCount != 10) {
      val bCount: Int = map.get("b").size
      if (bCount + cCount == 20) {
        myObject = buildXyz("b")
      } else {
        val aCount: Int = map.get("a").size
        if (aCount != 0) {
          val dCount: Int = map.get("d").size
          if (dCount > 10) {
            myObject = buildXyz("a")
          }
        }
      }
    }
    myObject
  }

As per the request here are some test cases:

Test Case 1:

map =>
"c" => Objectc1, Objectc2... Objectc10

This should return None
-----------
Test Case 2:

map =>
"c" => Objectc1, Objectc2
"b" => Objectb1, Objectb2..... Objectb18

This should return buildXyz("b")

-----------
Test Case 3:

map =>
"c" => Objectc1
"b" => Objectb1, Objectb2

This should return None

-----------
Test Case 4:

map =>
"c" => Objectc1
"b" => Objectb1, Objectb2
"a" => Objecta1
"d" => Objectd1

This should return None

-----------
Test Case 5:

map =>
"c" => Objectc1
"b" => Objectb1, Objectb2
"a" => Objecta1
"d" => Objectd1, Objectd2......Objectd10, Objectd11

This should return buildXyz("a")

yes who ever told you this is correct, but it doesn't mean that we should not use if else, its totally fine to use if statement, if there are not may cases to match with. tho uses of var is not recommended so you might need to get rid of that. on which field you want to do pattern matching as i can see your method doesn't take any parameters, do you have any global variable? — Raman Mishra, Aug 26 '20 at 17:10
Yes, map is a global variable. It doesn't have to be global. Not sure what you mean by "yes who ever told you this is correct". How would I get rid of 'var' in this case? Anyway, coding sample would be great! — DilTeam, Aug 26 '20 at 17:16
What exactly you want to do? you want to check the length of values what is the type of your map please update the question accordingly doesn't has the sufficient information. — Raman Mishra, Aug 26 '20 at 17:22
map is of type: map: util.HashMap[Any, List[SomeObject]] so .get().size would return number of entries in the List. Anyway, added argument to the method to make it more clear. — DilTeam, Aug 26 '20 at 17:33
`.get("d").size` does **not** return the number of entries in the `List`. And why is the number of key->value pairs of interest if only keys `a`,`b`,`c`, and `d` are accessed? — jwvh, Aug 26 '20 at 17:38
I stepped thru the debugger and confirmed that map.get("somevalue").size indeed returns number of elements in the List that correspond to "somevalue". In any case, can we please focus on converting this to *functional* code as opposed to validity of this code? I can fix bugs later by writing test cases. Thanks. — DilTeam, Aug 26 '20 at 18:22
Added Test cases. You can use them to create the dummy input. Hope this helps. If not, let me know. Thanks. — DilTeam, Aug 26 '20 at 21:12

score 1 · Answer 1 · answered Aug 26 '20 at 22:12

First of all var could be replaced with val

  def xyzMethod1(map: java.util.HashMap[Any, List[SomeObject]]): Option[xyzObject] = {
    val cCount: Int = map.get("c").size
    val myObject: Option[xyzObject] = if (cCount != 10) {
      val bCount: Int = map.get("b").size
      if (bCount + cCount == 20) {
        buildXyz("b")
      } else {
        val aCount: Int = map.get("a").size
        if (aCount != 0) {
          val dCount: Int = map.get("d").size
          if (dCount > 10) {
            buildXyz("a")
          } else {
            None
          }
        } else {
          None
        }
      }
    } else {
      None
    }

    myObject
  }

Overall this huge if-else looks a bit complex. We can see there's only three possible results buildXyz("a"), buildXyz("b") and None So, this will look much better with just three if branches

  def xyzMethod1(map: java.util.HashMap[Any, List[SomeObject]]): Option[xyzObject] = {
    val aCount: Int = map.get("a").size
    val bCount: Int = map.get("b").size
    val cCount: Int = map.get("c").size
    val dCount: Int = map.get("d").size

    val myObject: Option[xyzObject] = if (cCount != 10 && bCount + cCount == 20) {
      buildXyz("b")
    } else if (cCount != 10 && aCount != 0 && dCount > 10) {
      buildXyz("a")
    } else {
      None
    }

    myObject
  }

This will look even better with couple of helper methods:

  def isBuildA(map: java.util.HashMap[Any, List[SomeObject]]): Boolean = {
    val aCount: Int = map.get("a").size
    val cCount: Int = map.get("c").size
    val dCount: Int = map.get("d").size

    cCount != 10 && aCount != 0 && dCount > 10
  }

  def isBuildB(map: java.util.HashMap[Any, List[SomeObject]]): Boolean = {
    val bCount: Int = map.get("b").size
    val cCount: Int = map.get("c").size

    cCount != 10 && bCount + cCount == 20
  }

  def xyzMethod1(map: java.util.HashMap[Any, List[SomeObject]]): Option[xyzObject] = {
    if (isBuildA(map)) {
      buildXyz("a")
    } else if (isBuildB(map)) {
      buildXyz("b")
    } else {
      None
    }
  }

And then java map could be converted to scala map. As we only care about counts we can map lists to sizes right away. Then also change helpers to be more functional.

  def isBuildA(map: scala.collection.Map[Any, Int]): Boolean = {
    map.exists(v => v._1 == "c" && v._2 != 10) &&
      map.exists(v => v._1 == "a" && v._2 != 0) &&
      map.exists(v => v._1 == "d" && v._2 > 10)
  }

  def isBuildB(map: scala.collection.Map[Any, Int]): Boolean = {
    map.exists(v => v._1 == "c" && v._2 != 10) &&
      map.filterKeys(k => k == "b" || k == "c").values.sum == 20
  }

  def xyzMethod1(map: java.util.HashMap[Any, List[SomeObject]]): Option[xyzObject] = {
    import scala.collection.JavaConverters._
    val map1 = map.asScala.mapValues(_.size)

    if (isBuildA(map1)) {
      buildXyz("a")
    } else if (isBuildB(map1)) {
      buildXyz("b")
    } else {
      None
    }
  }

3rd variant is better then 4th. but it looks 'less functional', so you couldn't impress your java mates with this elven scripts — Artem Sokolov, Aug 26 '20 at 22:18
I liked the 3rd variant so it seems we can't avoid using 'if else' in this case. Can't use pattern matching etc which is fine. Just wanted to learn. — DilTeam, Aug 26 '20 at 23:44

jwvh · Accepted Answer · 2020-08-26T23:57:34.133

Your code, as posted, doesn't pass all your tests, but this does.

import scala.collection.immutable.HashMap

//just enough stubs to make everything compile
case class XyzObject(x:String)
class SomeObject
def buildXyz(xyz:String) = Option(XyzObject(xyz))

def xyzMethod(map: HashMap[Any, List[SomeObject]]) :Option[XyzObject] = {
  val cCount: Int = map.getOrElse("c", Nil).size
  if (cCount == 10)                                    None
  else if (map.getOrElse("b",Nil).size + cCount == 20) buildXyz("b")
  else if (map.getOrElse("a",Nil).isEmpty ||
           map.getOrElse("d",Nil).size < 11)           None
  else                                                 buildXyz("a")
}

The test suite:

//  Test Case 1:
xyzMethod(HashMap("c" -> List.fill(10)(new SomeObject)))
//This should return None

//Test Case 2:
xyzMethod(HashMap("c" -> List.fill(2)(new SomeObject)
                 ,"b" -> List.fill(18)(new SomeObject)))
//This should return Some(XyzObject("b"))

//Test Case 3:
xyzMethod(HashMap("c" -> List.fill(1)(new SomeObject)
                 ,"b" -> List.fill(2)(new SomeObject)))
//This should return None

//Test Case 4:
xyzMethod(HashMap("c" -> List.fill(1)(new SomeObject)
                 ,"b" -> List.fill(2)(new SomeObject)
                 ,"a" -> List.fill(1)(new SomeObject)
                 ,"d" -> List.fill(1)(new SomeObject)))
//This should return None

//Test Case 5:
xyzMethod(HashMap("c" -> List.fill(1)(new SomeObject)
                 ,"b" -> List.fill(2)(new SomeObject)
                 ,"a" -> List.fill(1)(new SomeObject)
                 ,"d" -> List.fill(11)(new SomeObject)))
//This should return Some(XyzObject("a"))

Nice. Thanks. It seems, though, we have to use 'if else' in this case. Can't do this with 'pattern matching' etc. That's fine just wanted to know. — DilTeam, Aug 26 '20 at 23:45

Scala: Convert procedural code to functional

2 Answers2