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
 }

• Simon Forsberg: What is the difference between blacklisted and banned?
• Sami Samiuddin: @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
• Simon Forsberg: I thought that was exactly how banned works...

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.

• Sami Samiuddin: 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

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.

• RedLaser: Just wondering, could you not use switch(false) and avoid using those negations on each case?
• cFreed: @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: @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.
• RedLaser: 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.
• cFreed: @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.