# Many nested session verification conditions

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 }  - What is the difference between blacklisted and banned? – Simon Forsberg Jan 16 at 20:23 @SimonForsberg blacklisted is a field in database with list of ip's that are to blacklist. it's basically bunch of ips you want to disallow – sami Jan 16 at 23:00 I thought that was exactly how banned works... – Simon Forsberg Jan 16 at 23:11 ## 2 Answers 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()){
}

...


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. - I'm storing user email to manage users later on where i can ban, unban or do other stuff with them. when user logs in next time im checking him against db whether he's previlidges and whatnot – sami Jan 16 at 23:06 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:
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.

-
Just wondering, could you not use switch(false) and avoid using those negations on each case? – RedLaser Jan 16 at 20:52
@RedLaser Good insight, you're right! I'm (too) used to employ this form with more complex cases, so I automatically initiate with false. – cFreed Jan 16 at 20:56
@RedLaser According to your remark I was to edit my answer, but I suddenly hesitate: while it's technically irreproachable, I feel it less obvious for readability. – cFreed Jan 16 at 20:59
Good point, I just felt that it would be better to reduce operations although it's not going to majorly impact performance so i guess it would be better to optimize for readability. – RedLaser Jan 16 at 21:01
@RedLaser Yes, I feel the same. Your post was the occasion to step back from my current practice to meditate about it, and I actually think that it's probably good to consider it like a kind of ready-made "tool": always begin with switch(true) then put every case: you want, which may be of any complexity. – cFreed Jan 16 at 21:23