Scala - avoid too complex nested pattern matching

Question

I try to simplify validation process for serving responses for HTTP requests in Spray (I use Slick for database access). Currently, I check in one query if I should go further to the following query or not (return error). This end up with nested pattern matching. Every validation case can return different error so I can not use any flatMap.

class LocationDao {
  val db = DbProvider.db

  // Database tables
  val devices = Devices.devices
  val locations = Locations.locations
  val programs = Programs.programs
  val accessTokens = AccessTokens.accessTokens

def loginDevice(deviceSerialNumber: String, login: String, password: String): Either[Error, LocationResponse] = {
    try {
      db withSession { implicit session =>
        val deviceRowOption = devices.filter(d => d.serialNumber === deviceSerialNumber).map(d => (d.id, d.currentLocationId.?, d.serialNumber.?)).firstOption
        deviceRowOption match {
          case Some(deviceRow) => {
            val locationRowOption = locations.filter(l => l.id === deviceRow._2.getOrElse(0L) && l.login === login && l.password === password).firstOption
            locationRowOption match {
              case Some(locationRow) => {
                val programRowOption = programs.filter(p => p.id === locationRow.programId).firstOption
                programRowOption match {
                  case Some(programRow) => {
                    val program = Program(programRow.name, programRow.logo, programRow.moneyLevel, programRow.pointsForLevel,
                      programRow.description, programRow.rules, programRow.dailyCustomerScansLimit)
                    val locationData = LocationData(program)
                    val locationResponse = LocationResponse("access_token", System.currentTimeMillis(), locationData)
                    Right(locationResponse)
                  }
                  case None => Left(ProgramNotExistError)
                }
              }
              case None => Left(IncorrectLoginOrPasswordError)
            }
          }
          case None => Left(DeviceNotExistError)
        }
      }
    } catch {
      case ex: SQLException =>
        Left(DatabaseError)
    }
  }
}

What is a good way to simplify this? Maybe there is other approach..

Eckstein

Your code sample is too specific to what your'e doing, try to be more generic. But for your question if you change your xxxRowOption methods to return Either you'll be able to use for comprehension. – Preston

Use a for comprehension. – Waynant

I refactored this code with for-comprehension. Thanks. – Eckstein

Answer 1

Generally, you can use a for-comprehension to chain together a lot of the monadic structures you have here (including Try, Option, and Either) without nesting. For example:

for {
  val1 <- Try("123".toInt)
} yield for {
  val2 <- Some(val1).map(_ * 2)
  val3 = Some(val2 - 55)
  val4 <- val3
} yield val4 * 2

In your style, this might otherwise have looked like:

Try("123".toInt) match {
    case Success(val1) => {
        val val2 = Some(val1).map(_ * 2)
        val2 match {
            case Some(val2value) => {
                val val3 = Some(val2value - 55)
                val3 match {
                    case Some(val4) => Some(val4)
                    case None => None
                }
            }
            case None => None
        }
    case f:Failure => None
    }
}

Kanchenjunga

This case is wrong: case Failure => None. Either use case f: Failure => None or case Failure(_) => None. – Barrera

you're right - my bad. I haven't run any of this code, I was just trying to give the flavor of for-comprehensions versus nested matches and maps. Feel free to edit the answer if you spot any other small mistakes. – Kanchenjunga

Answer 2

You can define a helper method for Eiterising your control flow.

The benefit of doing this is that you will have great control and flexibility in your flow.

def eitherMe[ I, T ]( eitherIn: Either[ Error, Option[ I ] ],
                      err: () => Error,
                      block: ( I ) => Either[ Error, Option[ T ] ]
                    ): Either[ Error, Option[ T ] ] = {
  eitherIn match {
    case Right( oi ) => oi match {
      case Some( i ) => block( i )
      case None => Left( err() )
    }
    case Left( e ) => Left( e )
  }

}

def loginDevice(deviceSerialNumber: String, login: String, password: String): Either[Error, LocationResponse] = {
  try {
    db withSession { implicit session =>
      val deviceRowOption = devices.filter(d => d.serialNumber === deviceSerialNumber).map(d => (d.id, d.currentLocationId.?, d.serialNumber.?)).firstOption

      val locationRowEither = eitherMe(
        Right( deviceRowOption ),
        () => { DeviceNotExistError },
        deviceRow => {
          val locationRowOption = locations.filter(l => l.id === deviceRow._2.getOrElse(0L) && l.login === login && l.password === password).firstOption
          Right( locationRowOption )
        }
      )

      val programRowEither = eitherMe(
        locationRowEither,
        () => { IncorrectLoginOrPasswordError },
        locationRow => {
          val programRowOption = programs.filter(p => p.id === locationRow.programId).firstOption
          Right( programRowOption )
        }
      )

      val locationResponseEither = eitherMe(
        programRowEither,
        () => { ProgramNotExistError },
        programRow => {
          val program = Program(programRow.name, programRow.logo, programRow.moneyLevel, programRow.pointsForLevel,
            programRow.description, programRow.rules, programRow.dailyCustomerScansLimit)
          val locationData = LocationData(program)
          val locationResponse = LocationResponse("access_token", System.currentTimeMillis(), locationData)
          Right(locationResponse)
        }
      )

      locationResponseEither

    }
  } catch {
    case ex: SQLException =>
      Left(DatabaseError)
  }
}

Valdemar

I took different aproach with for-comprehension. I think this solution is good but what if we want to return more than one error? – Eckstein

For compressions have the problem of fine management of errors. In this case you can manage your errors any way you want. As of now... this code will return different errors for different problems exactly same as your original code. – Valdemar

Answer 3

For me, when I sometime can't avoid nested complexity, I would extract out section of the code that make sense together and make it into a new method and give it a meaningful name. This will both document the code and make it more readable, and reduce the complexity inside each individual method. And usually, once I have done that, I am able to see the flow better and might be able to refactor it to make more sense (after first writing the tests to cover the behavior I want).

e.g. for your code, you can do something like this:

class LocationDao {
  val db = DbProvider.db

  // Database tables
  val devices = Devices.devices
  val locations = Locations.locations
  val programs = Programs.programs
  val accessTokens = AccessTokens.accessTokens

  def loginDevice(deviceSerialNumber: String, login: String, password: String): Either[Error, LocationResponse] = {
    try {
      db withSession { implicit session =>
        checkDeviceRowOption(deviceSerialNumber, login, password)
      }
    } catch {
      case ex: SQLException =>
        Left(DatabaseError)
    }
  } 

  def checkDeviceRowOption(deviceSerialNumber: String, login: String, password: String): Either[Error, LocationResponse] = {
    val deviceRowOption = devices.filter(d => d.serialNumber === deviceSerialNumber).map(d => (d.id, d.currentLocationId.?, d.serialNumber.?)).firstOption
    deviceRowOption match {
      case Some(deviceRow) => {
        val locationRowOption = locations.filter(l => l.id === deviceRow._2.getOrElse(0L) && l.login === login && l.password === password).firstOption
        locationRowOption match {
          case Some(locationRow) => { checkProgramRowOption(locationRow) }
          case None => Left(IncorrectLoginOrPasswordError)
        }
      }
      case None => Left(DeviceNotExistError)
    }
  }

  def checkProgramRowOption(locationRow: LocationRowType): Either[Error, LocationResponse] = {
    val programRowOption = programs.filter(p => p.id === locationRow.programId).firstOption
    programRowOption match {
      case Some(programRow) => {
        val program = Program(programRow.name, programRow.logo, programRow.moneyLevel, programRow.pointsForLevel,
          programRow.description, programRow.rules, programRow.dailyCustomerScansLimit)
        val locationData = LocationData(program)
        val locationResponse = LocationResponse("access_token", System.currentTimeMillis(), locationData)
        Right(locationResponse)
      }
      case None => Left(ProgramNotExistError)
    }
  }

}

Note that this is just an illustration and probably won't compile because I don't have your lib, but you should be able to tweak the code for it to compile.

Dependence