Complex if statements in authentication method

Question

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');
        }
    }
}

What is returned if either main if or elseif conditions aren't met? — shudder, Aug 16, 2014 at 13:08
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, 2014 at 15:14
@shudder Very helpful video regarding maintainable and modulair code. — Kid Diamond, Aug 16, 2014 at 16:25

Community · Accepted Answer · 2020-06-10 13:24:26Z

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');
}

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, 2014 at 10:49
Yeah, we wrote our answers simultaneously and I didn't see theirs. :s — Emily L., Aug 16, 2014 at 10:51
It's okay. You did it just a little more thoroughly, which is always helpful. :) — Kid Diamond, Aug 16, 2014 at 10:52
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, 2014 at 10:56

georg · Accepted Answer · 2014-08-16 10:33:55Z

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.

janos · Accepted Answer · 2014-08-16 13:33:51Z

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');

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, 2014 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, 2014 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, 2014 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, 2014 at 9:17
Good points. I'll just stick with your solution, unless somebody else has a better idea. — Kid Diamond, Aug 16, 2014 at 9:19

Stack Exchange Network

Complex if statements in authentication method

3 Answers 3

Use Guard Expressions

Example

Your code

Your Answer

Not the answer you're looking for? Browse other questions tagged
php
optimization
object-oriented
php5
authentication
or ask your own question.

Hot Network Questions

Complex if statements in authentication method

3 Answers 3

Use Guard Expressions

Example

Your code

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
php
optimization
object-oriented
php5
authentication
or ask your own question.