Cleanest way in Scala to avoid nested ifs when tra

Question

I have some code for validating ip addresses that looks like the following:

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 the nested ifs because it makes the code less readable. Is there a nice way to linearize this code? In java, I might use return statements, but this is discouraged in scala.

Answer 1

This has some time now but I will add my 2 cents. The proper way to handle this is with Either. You can create a method like:

def checkErrors[T](errorList: Iterable[T], onError: Result) : Either[Result, Unit] = if(errorList.isEmpty) Right() else Left(onError)

so you can use for comprehension syntax

val invalidIpConfigs = getFormatErrors(ipConfigs)

val result = for {
  _ <- checkErrors(invalidIpConfigs, Malformatted(invalidIpConfigs))
  dups = getDuplicates(ipConfigs)
  _ <- checkErrors(dups, Duplicates(dups))
  taken = getAvailability(ipConfigs)
  _ <- checkErrors(taken, Unavailable(taken))
} yield Valid

If you don't want to return an Either use

result.fold(l => l, r => r)

In case of the check methods uses Futures (could be the case for getAvailability, for example), you can use cats library to be able of use it in a clean way: https://typelevel.org/cats/datatypes/eithert.html

Answer 2

I think it's pretty readable and I wouldn't try to improve it from there, except that !isEmpty equals to nonEmpty.

Answer 3

I personally advocate using a match everywhere you can, as it in my opinion usually makes 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)
  }

Note that I've chosen to match on the else part first, since you generally go from specific to generic in matches, since Nil is a subclass of Iterable I put that as the first case, eliminating the need for an i if i.nonEmpty in the other case, since it would be a given if it didn't match Nil

Also a thing to note here, all your vals don't need the type explicitly defined, it significantly declutters the code if you write something like

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

as simply

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

I've also taken the liberty of removing many one-off variables I didn't find remotely necessary, as all they did was add more lines to the code

A thing to note here about using match over nested ifs, is that is that it's easier to add a new case than it is to add a new else if 99% of the time, thereby making it more modular, and modularity is always a good thing.

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

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 has the benefit of splitting it up into smaller more manageable chunks, while keeping to the FP ideal of having functions that only do one thing, but do that one thing well, rather than having god-methods that do everything.

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