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

Answer 1

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

Answer 2

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