Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I had asked questions about my entire class which I'll flag for closing soon. It appeared to be a little too broad for 1 post so I chopped it up and will be asking a little more specific questions separately.


In my Application Layer I have a class AuthService containing the following method with a code structure that has gotten quite complex with if statements that I'm unable to re-factor a better way.

I'd appreciate anybody who could review this piece of code and give me suggestions on improving / optimizing the design of it. Any other suggestions are welcome as well.

public function findLoggedInUser()
{
    if (($userData = $this->httpSession->getParameter('user')) && isset($userData['id'])) {
        // User found in current http session
        return $this->userMapper->findById($userData['id']);
    } elseif ($httpSessionData = $this->httpCookie->getParameter('usess')) {
        if (isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
            if ($user = $this->userMapper->findById($httpSessionData['uid'])) {
                if ($httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno'])) {
                    if (password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
                        // Generate new http session entity tokens
                        $token['raw']  = new Token(CryptoCharGen::numeric(60));
                        $token['hash'] = new TokenHash(password_hash((string) $token['raw'], PASSWORD_BCRYPT, ['cost' => 4]));

                        // Update and persist http session entity with new token
                        $httpSession->setTokenHash($token['hash']);

                        $this->httpSessionMapper->update($httpSession);

                        // Issue new http cookie usess with updated http session recovery data
                        $this->httpCookie->setParameter('usess', [
                            'uid'   => (string) $httpSession->getUser()->getId(),
                            'sno'   => (string) $httpSession->getSeriesNo(),
                            'token' => (string) $token['raw']
                        ], 60 * 60 * 24 * 90);

                        // User has been found
                        return $httpSession->getUser();
                    } else {
                        // Token invalid, sabotage assumed
                        // Invalidate all persisted http sessions of this user
                        $this->httpSessionMapper->deleteAllByUser($user);

                        $this->httpCookie->destroy('usess');
                    }
                } else {
                    // Destroy invalid cookie
                    $this->httpCookie->destroy('usess');
                }
            } else {
                // Destroy invalid cookie
                $this->httpCookie->destroy('usess');
            }
        } else {
            // Destroy invalid cookie
            $this->httpCookie->destroy('usess');
        }
    }
}
share|improve this question
    
What is returned if either main if or elseif conditions aren't met? –  shudder Aug 16 at 13:08
    
@shudder Nothing (null). –  Kid Diamond Aug 16 at 13:10
    
I have to say that it's hard for me to refactor this because while I see some design flaws I can't see the context (don't want to guess). I suggest you to split this method into more smaller ones (do one thing only & mind Command-Query Separation). Some other flaws should reveal themselves. This movie was usefull for me on this matter, so you might check it out: Your code sucks, let's fix it!. –  shudder Aug 16 at 15:14
    
@shudder Very helpful video regarding maintainable and modulair code. –  Kid Diamond Aug 16 at 16:25

3 Answers 3

up vote 6 down vote accepted

Use Guard Expressions

The proper way to do this is to use Guard expressions.

If you have a number of conditions that need to be true before you take an action. You don't check them all in a nested manner but instead check their negations one and one and return/break if you find any condition that is not met.

Example

Instead of having:

if(a){
  if(b){
    if(c)
      ...
      return SUCCESS;
    else
      return FAIL;
  else
    return FAIL;
}
else
  return FAIL;

You can invert the checks and collapse the nesting like this:

if(!a)
   return FAIL;
if(!b)
   return FAIL;
if(!c)
   return FAIL
...
return SUCCESS;

Or if the consequence of failing the checks are the same, you can collapse the ifs like this:

if(!a || !b || !c)
   return FAIL;
...
return SUCCESS;

Which method to choose depends on how long the expressions a, b and c are and which one you find more readable.

Your code

Use with caution! I might have missed something as my PHP is rusty.

if (!isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
   $this->httpCookie->destroy('usess');
   return;
}
$user = $this->userMapper->findById($httpSessionData['uid'])
if (!$user) {
    $this->httpCookie->destroy('usess');
   return;
}

$httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno'])
if (!$httpSession) {
    $this->httpCookie->destroy('usess');
   return;
}

if (password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
    // ...
    return $httpSession->getUser();
} else {
    // Token invalid, sabotage assumed
    // Invalidate all persisted http sessions of this user
    $this->httpSessionMapper->deleteAllByUser($user);
    $this->httpCookie->destroy('usess');
}
share|improve this answer
    
Am I correct that this is the same expression strategy as in George's answer? And I appreciate your constructive answer. –  Kid Diamond Aug 16 at 10:49
    
Yeah, we wrote our answers simultaneously and I didn't see theirs. :s –  Emily L. Aug 16 at 10:51
    
It's okay. You did it just a little more thoroughly, which is always helpful. :) –  Kid Diamond Aug 16 at 10:52
1  
A note about the example code, I would recommend a terminateSession() function and call it instead of httpCookie->destroy(...). You know, to help with DRY. :) –  Emily L. Aug 16 at 10:56

Two suggestions on how to keep this code maintainable:

  • use early exits

  • keep conditions atomic (one if checks for one thing)

Example:

function findLoggedInUser()
{
    if (($userData = $this->httpSession->getParameter('user')) && isset($userData['id'])) {
        // User found in current http session
        return $this->userMapper->findById($userData['id']);
    }

    $httpSessionData = $this->httpCookie->getParameter('usess');
    if(!$httpSessionData) {
        $this->httpCookie->destroy('usess');
        return null;
    }

    if(!isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
        $this->httpCookie->destroy('usess');
        return null;
    }

    $user = $this->userMapper->findById($httpSessionData['uid']);
    if(!$user) {
        $this->httpCookie->destroy('usess');
        return null;
    }

    $httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno']));
    if(!$httpSessionData) {
        $this->httpCookie->destroy('usess');
        return null;
    }

    if (!password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
        $this->httpSessionMapper->deleteAllByUser($user);
        $this->httpCookie->destroy('usess');
        return null;
    }

    // EVERYTHING'S FINE, DO THE JOB

    // Generate new http session entity tokens
    $token['raw']  = new Token(CryptoCharGen::numeric(60));
    $token['hash'] = new TokenHash(password_hash((string) $token['raw'], PASSWORD_BCRYPT, ['cost' => 4]));

    // Update and persist http session entity with new token
    $httpSession->setTokenHash($token['hash']);

    $this->httpSessionMapper->update($httpSession);

    // Issue new http cookie usess with updated http session recovery data
    $this->httpCookie->setParameter('usess', [
        'uid'   => (string) $httpSession->getUser()->getId(),
    'sno'   => (string) $httpSession->getSeriesNo(),
    'token' => (string) $token['raw']
        ], 60 * 60 * 24 * 90);

    // User has been found
    return $httpSession->getUser();
}

This way, the code remains readable and flexible for future modifications.

share|improve this answer

In the deeply nested code, ultimately you either return a user, or else destroy the usess cookie if something fails at any point. So instead of this:

if (isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
    if ($user = $this->userMapper->findById($httpSessionData['uid'])) {
        if ($httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno'])) {
            if (password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
                // ...

                return $httpSession->getUser();
            } else {
                // Token invalid, sabotage assumed
                // Invalidate all persisted http sessions of this user
                $this->httpSessionMapper->deleteAllByUser($user);

                $this->httpCookie->destroy('usess');
            }
        } else {
            // Destroy invalid cookie
            $this->httpCookie->destroy('usess');
        }
    } else {
        // Destroy invalid cookie
        $this->httpCookie->destroy('usess');
    }
} else {
    // Destroy invalid cookie
    $this->httpCookie->destroy('usess');
}

You could do like this:

if (isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])) {
    if ($user = $this->userMapper->findById($httpSessionData['uid'])) {
        if ($httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno'])) {
            if (password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
                // ...

                return $httpSession->getUser();
            }
            // Token invalid, sabotage assumed
            // Invalidate all persisted http sessions of this user
            $this->httpSessionMapper->deleteAllByUser($user);
        }
    }
}
// Destroy invalid cookie
$this->httpCookie->destroy('usess');

And instead of such deeply nested if, it might be more readable to chain conditions together with &&:

// note: remember to use if (($x = val) && ...) { ... }
//       instead of just if ($x = val && ...) { ... }
if (isset($httpSessionData['uid'], $httpSessionData['sno'], $httpSessionData['token'])
    && ($user = $this->userMapper->findById($httpSessionData['uid']))
    && ($httpSession = $this->httpSessionMapper->findByUserAndSeriesNo($user, $httpSessionData['sno']))) {

    if (password_verify($httpSessionData['token'], (string) $httpSession->getTokenHash()) {
        // ...

        return $httpSession->getUser();
    }
    // Token invalid, sabotage assumed
    // Invalidate all persisted http sessions of this user
    $this->httpSessionMapper->deleteAllByUser($user);
}
// Destroy invalid cookie
$this->httpCookie->destroy('usess');
share|improve this answer
    
Would it be possible to have those 3 chained expressions in that if statement stored in an array $expressions and use !in_array(false, $expressions) as the expression for the if statement for better readability? –  Kid Diamond Aug 16 at 8:53
    
Probably it's possible, but I don't think it would help readability. There is a "sweet spot" where it's good to stop trying to optimize. –  janos Aug 16 at 9:03
    
Yeah, but I think readability preferences are probably different for everyone. I'm the only one that has to read this code later again. :P –  Kid Diamond Aug 16 at 9:12
    
You never know. You see, I'm reading your code right now. Another problem with in_array is that it would evaluate all expressions, but with a && b if a is false then b should not be evaluated. Maybe you can find another solution instead of in_array, but I think it's going to be hack-ish, obscure. Also remember Occam's Razor: the simple solutions are often better –  janos Aug 16 at 9:17
1  
Why the } else { block? You don't need it since you have the return inside the if (to reduce nesting) –  Marco Acierno Aug 16 at 10:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.