Too many IFs?

[halben]

Can someone tell me if this is excessive use of IF statement?

 

Thanks.

if ($role == 'admin') {

    // Check if the user exist
    if (isset($_POST['user_email']) && !empty($_POST['user_email'])) {

        // Sanitize the data
        $user_email_data = trim(strip_tags(stripslashes($_POST['user_email'])));

        // Now use PHP to check for validation
        if (filter_var($user_email_data, FILTER_VALIDATE_EMAIL)) {
            if (false == get_user_by('email', $user_email_data)) {
                // the user doesn't exist
            } else {
                // the user exists
                update_user_meta($userID, 'wp_user_roles', '10');
                // check wp if the new value has been stored
                if (get_user_meta($userID, 'wp_user_roles', true) != '10') {
                    wp_die('An error occured!');
                }
            }
        }
    }
} else {
    // Do nothing, exit
    exit ;
}

[requinix]

IMO no, but you could very easily clean some of them up:

if ($role != 'admin') {
    // Do nothing, exit
    exit ;
}

// Check if the user exist
if (isset($_POST['user_email']) && !empty($_POST['user_email'])) {

    // Sanitize the data
    $user_email_data = trim(strip_tags(stripslashes($_POST['user_email'])));

    // Now use PHP to check for validation
    if (filter_var($user_email_data, FILTER_VALIDATE_EMAIL) && get_user_by('email', $user_email_data)) {
        // the user exists
        update_user_meta($userID, 'wp_user_roles', '10');
        // check wp if the new value has been stored
        if (get_user_meta($userID, 'wp_user_roles', true) != '10') {
            wp_die('An error occured!');
        }
    }
}

[vinny42]

the fact that you think it might be too much means it is too much.

PHP can handle it, no problem, but the code is hard to read. it would be better to split the IF's up. and maybe split the code into re-usable functions.

 

Nested IF statements generally become easier to read if you test the 'fail' state first, because the amount of code to handle the fail state is usually much shorter than the success code, which means that the ELSE will be close to the IF and that helps to keep track of the nesting.

 

 

By the way, your code itself has a few holes :-)

 

$user_email_data = trim(strip_tags(stripslashes($_POST['user_email'])));

 

There are no slashes in the $_POST unless you are using the *extremely* evil magic_quotes, in which case you should probably get rid of them asap.

There are also no HTML tags in the address. If there are then it's not an emailaddress so removing the tags will not magically create an email adress, you might aswell stop there.

 

I'm not familiar with wp but I'm sure update_user_meta() will return some value to indicate success and failure. running an extra query to see if it works sounds unnecessary (and just returning "something went wrong" doesn't tell the user much (or you).

[halben]

Thanks for helping, kudos to the both!

[AbraCadaver]

Not my favorite, but fewer ifs for illustration.  Short-circuit and fail on the first false: 

if ($role == 'admin' &&
    isset($_POST['user_email']) &&
    filter_var($_POST['user_email'], FILTER_VALIDATE_EMAIL) &&
    get_user_by('email', $_POST['user_email'])
{
        update_user_meta($userID, 'wp_user_roles', '10');

        if (get_user_meta($userID, 'wp_user_roles', true) != '10') {
            wp_die('An error occured!');
        }
}

Edited September 17, 2013 by AbraCadaver