The cleanest way in Scala is to avoid nested ifs when converting collections and checking error conditions at every step

Question

I have code for checking IP addresses that looks like this:

sealed abstract class Result case object Valid extends Result case class Malformatted(val invalid: Iterable[IpConfig]) extends Result case class Duplicates(val dups: Iterable[Inet4Address]) extends Result case class Unavailable(val taken: Iterable[Inet4Address]) extends Result def result(ipConfigs: Iterable[IpConfig]): Result = { val invalidIpConfigs: Iterable[IpConfig] = ipConfigs.filterNot(ipConfig => { (isValidIpv4(ipConfig.address) && isValidIpv4(ipConfig.gateway)) }) if (!invalidIpConfigs.isEmpty) { Malformatted(invalidIpConfigs) } else { val ipv4it: Iterable[Inet4Address] = ipConfigs.map { ipConfig => InetAddress.getByName(ipConfig.address).asInstanceOf[Inet4Address] } val dups = ipv4it.groupBy(identity).filter(_._2.size != 1).keys if (!dups.isEmpty) { Duplicates(dups) } else { val ipAvailability: Map[Inet4Address, Boolean] = ipv4it.map(ip => (ip, isIpAvailable(ip))) val taken: Iterable[Inet4Address] = ipAvailability.filter(!_._2).keys if (!taken.isEmpty) { Unavailable(taken) } else { Valid } } } } 

I don't like nested ifs because it makes the code less readable. Is there a good way to linearize this code? In java, I can use return statements, but this is not recommended in scala.

Answer 1

I personally advocate using the match wherever you can, because in my opinion it usually makes the code very readable

 def result(ipConfigs: Iterable[IpConfig]): Result = ipConfigs.filterNot(ipc => isValidIpv4(ipc.address) && isValidIpv4(ipc.gateway)) match { case Nil => val ipv4it = ipConfigs.map { ipc => InetAddress.getByName(ipc.address).asInstanceOf[Inet4Address] } ipv4it.groupBy(identity).filter(_._2.size != 1).keys match { case Nil => val taken = ipv4it.map(ip => (ip, isIpAvailable(ip))).filter(!_._2).keys if (taken.nonEmpty) Unavailable(taken) else Valid case dups => Duplicates(dups) } case invalid => Malformatted(invalid) } 

Please note that I first decided to match with the else part, since you usually go from specific to generic in matches, since Nil is a Iterable subclass. I put this as the first case, eliminating the need for i if i.nonEmpty in another case , since he would be asked if he did not match Nil

It should also be noted that for all your val do not need an explicitly defined type, it significantly rejects the code if you write something like

 val ipAvailability: Map[Inet4Address, Boolean] = ipv4it.map(ip => (ip, isIpAvailable(ip))) 

how simple

 val ipAvailability = ipv4it.map(ip => (ip, isIpAvailable(ip))) 

I also took the liberty of removing many one-time variables that I did not find remotely necessary, since all they did was add more lines to the code

It should be noted here that the use of match over nested if s is that it is easier to add a new case than to add 99% of the time of the new else if , thereby making it more modular, and modularity is always good.

Alternatively, as Nathaniel Ford suggested, you can break it down into several smaller methods, in which case the above code would look like this:

 def result(ipConfigs: Iterable[IpConfig]): Result = ipConfigs.filterNot(ipc => isValidIpv4(ipc.address) && isValidIpv4(ipc.gateway)) match { case Nil => wellFormatted(ipConfigs) case i => Malformatted(i) } def wellFormatted(ipConfigs: Iterable[IpConfig]): Result = { val ipv4it = ipConfigs.map(ipc => InetAddress.getByName(ipc.address).asInstanceOf[Inet4Address]) ipv4it.groupBy(identity).filter(_._2.size != 1).keys match { case Nil => noDuplicates(ipv4it) case dups => Duplicates(dups) } } def noDuplicates(ipv4it: Iterable[IpConfig]): Result = ipv4it.map(ip => (ip, isIpAvailable(ip))).filter(!_._2).keys match { case Nil => Valid case taken => Unavailable(taken) } 

This allows you to split it into smaller, more manageable pieces, while preserving the FP ideal of having functions that do only one thing, but do it only well, instead of having divine methods that do everything.

Which style you prefer, of course, is up to you.

Answer 2

I think this is pretty readable, and I would not try to improve it, except that !isEmpty nonEmpty is nonEmpty .