2
\$\begingroup\$

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
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ What is the difference between blacklisted and banned? \$\endgroup\$ – Simon Forsberg Jan 16 '16 at 20:23
  • \$\begingroup\$ @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 \$\endgroup\$ – verax Jan 16 '16 at 23:00
  • \$\begingroup\$ I thought that was exactly how banned works... \$\endgroup\$ – Simon Forsberg Jan 16 '16 at 23:11
8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – verax Jan 16 '16 at 23:06
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Just wondering, could you not use switch(false) and avoid using those negations on each case? \$\endgroup\$ – RedLaser Jan 16 '16 at 20:52
  • \$\begingroup\$ @RedLaser Good insight, you're right! I'm (too) used to employ this form with more complex cases, so I automatically initiate with false. \$\endgroup\$ – cFreed Jan 16 '16 at 20:56
  • \$\begingroup\$ @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. \$\endgroup\$ – cFreed Jan 16 '16 at 20:59
  • \$\begingroup\$ 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. \$\endgroup\$ – RedLaser Jan 16 '16 at 21:01
  • \$\begingroup\$ @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. \$\endgroup\$ – cFreed Jan 16 '16 at 21:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.