Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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
    }
share|improve this question
1  
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
up vote 8 down vote accepted

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.

share|improve this answer
    
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:
        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.

share|improve this answer
    
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

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.