current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
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
up vote 2 down vote favorite

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♦ 5 hours ago
    
@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 2 hours ago
    
I thought that was exactly how banned works... – Simon Forsberg♦ 2 hours ago
up vote 6 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 2 hours ago
up vote 3 down vote

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 4 hours ago
    
@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 4 hours ago
    
@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 4 hours ago
    
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 4 hours ago
    
@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 4 hours ago

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.