Many nested session verification conditions

Question

Can someone give me advice on cleaning this code? It's more messy then I expected. I got like 10-15 more ifs to be added.

I've thought of adding the error messsages in methods and then just check if method returns true, or false it'd throw error..

    if( isset($_SESSION["access_token"], $_SESSION["user_email"], $_SESSION["username"]) ){
    if($GoogleAccess->does_account_exist){
        if(!$GoogleAccess->is_g_acc_banned()){
            if(!$GoogleAccess->is_ip_banned()){
                if(!$GoogleAccess->is_ip_blacklisted($blacklist_ips)){
                    if($GoogleAccess->is_user_acc_verified()){
                        header("location: ../member.php");
                    }else{
                        $error = "Please verify your account";
                    }
                }else{
                    $error = "This IP's blacklisted.";
                }
            }else{
                $error = "Your IP's been banned";
            }

        }else{
            $error = "This account has been banned";
        }
    }else{
        //mean doesn't have account. Register user and send verification to email
    }

Answer 1

This is indeed quite messy. It's really hard to see which else ends which if.

Just invert the if and return early:

if( !isset($_SESSION["access_token"], $_SESSION["user_email"], $_SESSION["username"]) ){
    return false; // mean doesn't have account. Register user and send verification to email
}

if(!$GoogleAccess->does_account_exist){
    return "This account has been banned";
}

if($GoogleAccess->is_g_acc_banned()){
    return "Your IP's been banned";
}

...

Alternatively, if you don't have a function, you could also phrase it with if-elses:

if( !isset($_SESSION["access_token"], $_SESSION["user_email"], $_SESSION["username"]) ){
    return false; // mean doesn't have account. Register user and send verification to email
} else if(!$GoogleAccess->does_account_exist){
    $error = "This account has been banned";
} else if ($GoogleAccess->is_g_acc_banned()) {
    $error = "Your IP's been banned";
}

...

Personally, I prefer the first approach, as it's a lot more readable.

I'm not familiar with google access, but it seems odd that you have to manually check all the access denied reasons. I would expect that you call their API, and it either logs a user in, or returns the reason why that was not possible. Did you check if something like this exists? It would severely simplify your code.

Answer 2

You can make the code much more readable, taking advantage of the alternate switch() usage mode:

if( isset($_SESSION["access_token"], $_SESSION["user_email"], $_SESSION["username"])) {
  if($GoogleAccess->does_account_exist) {
    switch(true) {
      case !$GoogleAccess->is_g_acc_banned():
        $error = "This account has been banned";
        break;
      case !$GoogleAccess->is_ip_banned():
        $error = "Your IP's been banned";
        break;
      case !$GoogleAccess->is_ip_blacklisted($blacklist_ips):
        $error = "This IP's blacklisted.";
        break;
      case !$GoogleAccess->is_user_acc_verified():
        $error = "Please verify your account";
        break;
      default:
        header("location: ../member.php");
        exit;
    }
  } else {
    //mean doesn't have account. Register user and send verification to email
  }
}

Note that I keeped the first inner if() separate, because it tests a clearly different case.
Also note that I added an exit; after header(), which is a recommended practice.