HiveBrain v1.2.0
Get Started
← Back to all entries
patternModerate

Code with many "early returns (exits)" into the functional style

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
exitsthewithintofunctionalstyleearlyreturnsmanycode

Problem

I have some Scala code that uses Internet to authorize a user. Therefore, it can throw Exceptions like IOException in the method.

The original code was written in Java. So, yes, it uses variables, not val. Also it "returns early" many times if some condition is not satisfied.

The following is an example. (The actual code is, of course, a little more complicated.)

def connect(): (IdInfo, PasswordInfo, ConnectionInfo) = {
  var idInfo: IdInfo = null
  var pwdInfo: PasswordInfo = null
  var connInfo: ConnectionInfo = null

  try {
    idInfo = calcIdInfo()
  } catch {
    case ex: IOException => return null
  }

  try {
    pwdInfo = calcPwdInfo(idInfo)
  } catch {
    case ex: AuthorizationException => return null
  }

  try {
    connInfo = calcConnInfo(idInfo, pwdInfo)
  } catch {
    case ex: IOException => return null
  }

  try {
    verify(idInfo, pwdInfo, connInfo)
  } catch {
    case ex: IOException => return null
    case ex: AuthorizationException => return null
  }

  (idInfo, pwdInfo, connInfo)
}


I'm trying to rewrite the code in the functional style. What I did was

  • Use vals instead of vars



  • Remove "return early"



  • Use None instead of null



I tried to use methods like fold, but at each stage, the types of input or the types of output are different - IdInfo, (IdInfo, PasswordInfo), (IdInfo, PasswordInfo, ConnectionInfo) are all different types - and, what each stage does is also diffrent from each other, so I couldn't use such method.

```
def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
val idInfoOpt = try {
Some(calcIdInfo())
} catch {
case ex: IOException => None
}

idInfoOpt map (idInfo => {
val pwdInfoOpt = try {
Some(calcPwdInfo(idInfo))
} catch {
case ex: AuthorizationException => None
}

pwdInfoOpt map (pwdInfo => {
val connInfoOpt = try {
Some(calcConnInfo(idInfo, pwdInfo))
} catch {
case ex: IOException => None
}

Solution

After reading the code, I assume these are the signatures of the building blocks of your code

class IdInfo
class PasswordInfo
class ConnectionInfo

def calcIdInfo:IdInfo=???
def calcPwdInfo(idInfo:IdInfo):PasswordInfo=???
def calcConnInfo(idInfo:IdInfo,pwdInfo:PasswordInfo):PasswordInfo=???


I left out the implementations for the calc as they don't really matter.

There are several possible variations to improve on the current code. Let's have a look at the last call to map in connect :

connInfoOpt map (connInfo => {
    try {
      verify(idInfo, pwdInfo, connInfo)
      Some(idInfo, pwdInfo, connInfo)
    } catch {
      case ex: IOException => None
      case ex: AuthorizationException => None
    }
  }) getOrElse None


The try expression has type Option[(IdInfo, PasswordInfo, ConnectionInfo)] which is the type you want as the output of the function connect. As the name connInfoOpt suggests, it is also an option. Map on option has a signature roughly equivalent to :

map(f:A=>B):Option[B]


Where A is the type which is wrapped in the original option, and B is the type wrapped in the resulting option.

Thus let's say connInfoOpt has type Option[ConnectionInfo] we get

A = ConnectionInfo
B = Option[(IdInfo, PasswordInfo, ConnectionInfo)]


Therefore the return type of the map expression is Option[B] which can be expanded to

Option[Option[(IdInfo, PasswordInfo, ConnectionInfo)]]


Here lies the first problem : to get the Option[(IdInfo, PasswordInfo, ConnectionInfo)] out, you use getOrElse None. A slightly shorter way to express that is to call flatten.

The original expression can thus be rewritten as:

connInfoOpt map (connInfo => {
    try {
      verify(idInfo, pwdInfo, connInfo)
      Some(idInfo, pwdInfo, connInfo)
    } catch {
      case ex: IOException => None
      case ex: AuthorizationException => None
    }
  }) flatten


which has type Option[(IdInfo, PasswordInfo, ConnectionInfo)]. The next step is to study the option type and see that it has a flatMap method which has the following signature :

flatMap(f:A=>Option[B]):Option[B]
// for comparison with map which was 
map(f:A=>B):Option[B]


Then notice that each intermediary is step is of the form :

option map { A=> Option[B] } flatten


for instance:

connInfoOpt map (connInfo => {
  try {
    verify(idInfo, pwdInfo, connInfo)
    Some(idInfo, pwdInfo, connInfo)
  } catch {
    case ex: IOException => None
    case ex: AuthorizationException => None
  }
}) flatten


can be rewritten as :

connInfoOpt flatMap (connInfo => {
  try {
    verify(idInfo, pwdInfo, connInfo)
    Some(idInfo, pwdInfo, connInfo)
  } catch {
    case ex: IOException => None
    case ex: AuthorizationException => None
  }
})


Which means the whole function can now be simplified to :

def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
    val idInfoOpt = try {
      Some(calcIdInfo())
    } catch {
      case ex: IOException => None
    }

    idInfoOpt flatMap (idInfo => {
      val pwdInfoOpt = try {
        Some(calcPwdInfo(idInfo))
      } catch {
        case ex: AuthorizationException => None
      }

      pwdInfoOpt flatMap (pwdInfo => {
        val connInfoOpt = try {
          Some(calcConnInfo(idInfo, pwdInfo))
        } catch {
          case ex: IOException => None
        }

        connInfoOpt flatMap (connInfo => {
          try {
            verify(idInfo, pwdInfo, connInfo)
            Some(idInfo, pwdInfo, connInfo)
          } catch {
            case ex: IOException => None
            case ex: AuthorizationException => None
          }
        })
      })
    })
  }


We got rid of the 3 getOrElse but this still isn't so nice. Let's extract a few methods next to make the code easier to read :

def safeIdInfo():Option[IdInfo]={
  try {
    Some(calcIdInfo())
  } catch {
    case ex: IOException => None
  }
}

def verifyConnInfo(idInfo: IdInfo, pwdInfo: PasswordInfo, connInfo: ConnectionInfo) :Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
  try {
    verify(idInfo, pwdInfo, connInfo)
    Some(idInfo, pwdInfo, connInfo)
  } catch {
    case ex: IOException => None
    case ex: AuthorizationException => None
  }
}

def safeConnInfo(idInfo: IdInfo, pwdInfo: PasswordInfo): Option[ConnectionInfo] = {
  try {
    Some(calcConnInfo(idInfo, pwdInfo))
  } catch {
    case ex: IOException => None
  }
}

def safePwdInfo(idInfo: IdInfo): Option[PasswordInfo] = {
  try {
    Some(calcPwdInfo(idInfo))
  } catch {
    case ex: AuthorizationException => None
  }
}


then the connect method looks like :

def connect(): Option[(IdInfo, PasswordInfo, ConnectionInfo)] = {
  safeIdInfo() flatMap (idInfo =>
    safePwdInfo(idInfo) flatMap (pwdInfo=>
      safeConnInfo(idInfo, pwdInfo) flatMap (connInfo =>
        verifyConnInfo(idInfo, pwdInfo, connInfo)
      )
    )
  )
}


Which is better but not perfect. Let's apply some scala syn

Code Snippets

class IdInfo
class PasswordInfo
class ConnectionInfo

def calcIdInfo:IdInfo=???
def calcPwdInfo(idInfo:IdInfo):PasswordInfo=???
def calcConnInfo(idInfo:IdInfo,pwdInfo:PasswordInfo):PasswordInfo=???
connInfoOpt map (connInfo => {
    try {
      verify(idInfo, pwdInfo, connInfo)
      Some(idInfo, pwdInfo, connInfo)
    } catch {
      case ex: IOException => None
      case ex: AuthorizationException => None
    }
  }) getOrElse None
map(f:A=>B):Option[B]
A = ConnectionInfo
B = Option[(IdInfo, PasswordInfo, ConnectionInfo)]
Option[Option[(IdInfo, PasswordInfo, ConnectionInfo)]]

Context

StackExchange Code Review Q#38689, answer score: 19

Revisions (0)

No revisions yet.