Is it good to use lots of and condition in if statement?

Question

I've a code something like this, i just want to know is it good habit to use lots of 'and' 'or' in a single if statement

if (array_key_exists(self::SUB_FORM_CONTACT, $data) &&
            array_key_exists('company', $data[self::SUB_FORM_CONTACT]) &&
            ((array_key_exists('salesRegion', $data[self::SUB_FORM_CONTACT]['company']) && !empty($data[self::SUB_FORM_CONTACT]['company']['salesRegion'])) ||
                (array_key_exists('serviceRegion', $data[self::SUB_FORM_CONTACT]['company']) && !empty($data[self::SUB_FORM_CONTACT]['company'][''])))
        ) {

        }

Or is there any better way of doing this?

"Good habit" in terms of readability or performance or ... ? — Fildor, Aug 11 '15 at 07:34
The more conditions you put in, the harder it will be to understand the code, but also the risk of something not going as you planned unless you've done it right and know what you are doing. The more complex the harder it will be, but also more precise :) — Epodax, Aug 11 '15 at 07:35
Good question @user5064303. However, I see responses both from a point of view of readability and performance. It would help to update your question to explain if you are asking for any or both of theses areas. — crafter, Aug 11 '15 at 08:36

score 5 · Answer 1 · answered Aug 11 '15 at 09:35

It's recommended to avoid complex conditional expression because they are difficult to read and understand. However, if the expression uses only one type of logical operators to connect the conditions then it can be easily understood.

For example:

if ($a && $b || $c && ! $d) {
    echo("Yes!");
}

Can you say at the first glance when the code in the if branch (echo()) will execute? Most probably no. The expression contains a mix of all the logical operations and its final value is difficult to evaluate without drawing the table of all possible values of $a, $b, $c and $d.

On the other hand, the expression:

if ($a && $b && $c && $d) {
    echo("Hooray!");
}

is much easier to grasp.
The echo() statement executes when all of $a, $b, $c and $d are simultaneously TRUE.

Refactoring

In your case the condition is more complex than the first one presented above. It contains all the logical operators and parentheses and the sub-expressions are quite long.

In order to keep it readable I would extract the complex condition into a (private or protected) method that returns a boolean value (a predicate).

private function isValidData(array $data)
{
    if (array_key_exists(self::SUB_FORM_CONTACT, $data) &&
        array_key_exists('company', $data[self::SUB_FORM_CONTACT]) &&
        ((array_key_exists('salesRegion', $data[self::SUB_FORM_CONTACT]['company']) && !empty($data[self::SUB_FORM_CONTACT]['company']['salesRegion'])) ||
            (array_key_exists('serviceRegion', $data[self::SUB_FORM_CONTACT]['company']) && !empty($data[self::SUB_FORM_CONTACT]['company'][''])))
    ) {
        return TRUE;
    } else {
        return FALSE;
    }
}

and replace the expression in the if statement:

if ($this->isValidData($data)) {
}

On the next steps I would split the complex conditional expression into smaller conditions, test them one by one and make the function isValidData() return (FALSE) early, when it's possible.

Step 1

Because the expression is complex and the pieces are relatively long it's difficult to work with it as is. This is why, on the first step I would extract the conditions into local variables having shorter names ($a, $b a.s.o. from above):

private function isValidData(array $data)
{
    $a = array_key_exists(self::SUB_FORM_CONTACT, $data);
    $b = array_key_exists('company', $data[self::SUB_FORM_CONTACT]);
    $c = array_key_exists('salesRegion', $data[self::SUB_FORM_CONTACT]['company']);
    $d = empty($data[self::SUB_FORM_CONTACT]['company']['salesRegion']);
    $e = array_key_exists('serviceRegion', $data[self::SUB_FORM_CONTACT]['company']);
    $f = empty($data[self::SUB_FORM_CONTACT]['company']['']);
    // this should probably read 'serviceRegion' -------^

    if ($a && $b && (($c && ! $d) || ($e && ! $f))) {
        return TRUE;
    } else {
        return FALSE;
    }
}

Everything looks much clear now and we even have found out a bug. Great!

Step 2

Let's try to untangle the (now easier to read) condition:

if ($a && $b && (($c && ! $d) || ($e && ! $f))) {
    return TRUE;
} else {
    return FALSE;
}

It may become even easier to understand if we do one test at a time and return (TRUE or FALSE) as soon as the outcome is obvious. The function returns TRUE when $a and $b and the parenthesis that contain the rest of the expression are all TRUE on the same time.

The if statement can be rewritten this way:

if ($a) {
    if ($b) {
        if (($c && ! $d) || ($e && ! $f))) {
            return TRUE;
        }
    }
}

return FALSE;

By inverting the conditions on $a and $b we can return FALSE early:

if (! $a) {
    return FALSE;
}
if (! $b) {
    return FALSE;
}

if (($c && ! $d) || ($e && ! $f))) {
    return TRUE;
}
return FALSE;

The code now looks like this:

private function isValidData(array $data)
{
    $a = array_key_exists(self::SUB_FORM_CONTACT, $data);
    $form = $data[self::SUB_FORM_CONTACT];

    $b = array_key_exists('company', $form);

    $c = array_key_exists('salesRegion', $form['company']);
    $d = empty($form['company']['salesRegion']);

    $e = array_key_exists('serviceRegion', $form['company']);
    $f = empty($form['company']['serviceRegion']);


    if (! $a) {
        return FALSE;
    }
    if (! $b) {
        return FALSE;
    }

    if (($c && ! $d) || ($e && ! $f))) {
        return TRUE;
    }
}

Notice that because all the assignments starting with the second one contained the common sub-expression $data[self::SUB_FORM_CONTACT] we have extracted it into a local variable. The same can be applied to the last 4 assignments (extracting the common sub-expression $form['company']:

// ...
$b = array_key_exists('company', $form);
$company = $form['company'];

$c = array_key_exists('salesRegion', $company);
$d = empty($company['salesRegion']);

$e = array_key_exists('serviceRegion', $company);
$f = empty($company['serviceRegion']);

Step 3

Let's notice that in expression $c && ! $d:

array_key_exists('salesRegion', $company) && ! empty($company['salesRegion'])

the call to array_key_exists() is not really needed. I guess its purpose is to avoid triggering notices when $company['salesRegion'] is accessed and $company does not contain the key 'salesRegion'. The empty() PHP construct will happily return TRUE when the key does not exist in the array and it doesn't trigger notices.

The same happens with $e && ! $f. This allows us to completely remove $c and $e. The condition nows look like this:

if (! $d || ! $f) {
    return TRUE;
}

Or

if (! ($d && $f)) {
    return TRUE;
}

By negating the condition:

if ($d && $f) {
    return FALSE;
} else {
    return TRUE;
}

The complete function now looks like this:

private function isValidData(array $data)
{
    $a = array_key_exists(self::SUB_FORM_CONTACT, $data);
    $form = $data[self::SUB_FORM_CONTACT];

    $b = array_key_exists('company', $form);
    $company = $form['company'];

    $d = empty($company['salesRegion']);
    $f = empty($company['serviceRegion']);


    if (! $a) {
        return FALSE;
    }
    if (! $b) {
        return FALSE;
    }

    if ($d && $f) {
        return FALSE;
    } else {
        return TRUE;
    }
}

Final step

We can now eliminate the local variables $a, $b, $d and $f and move the initialization of $form and $company after we know they are not NULL (each one after the corresponding call to array_key_exists()):

The final form of the function is this:

/**
 * Verify if the provided data is valid.
 *
 * @param array $data the data to validate
 * @return boolean TRUE if the data is valid, FALSE if some field is missing or empty
 */
private function isValidData(array $data)
{
    // $data must contain the contact form information
    if (! (array_key_exists(self::SUB_FORM_CONTACT, $data))) {
        return FALSE;
    }

    // The contact form information exists in $data
    $form = $data[self::SUB_FORM_CONTACT];
    // The form must contain company information
    if (! (array_key_exists('company', $form))) {
        return FALSE;
    }

    // The company information exists in the contact form
    $company = $form['company'];
    // The company must contains information about salesRegion or serviceRegion or both
    if (empty($company['salesRegion']) && empty($company['serviceRegion'])) {
        return FALSE;
    }

    // The data is valid
    return TRUE;
}

Conclusion

The code is several lines longer than before but:

the conditions are easier to read and understand because they are evaluated one after another and not in a big complex expression;
the code is encapsulated in a function; the reader of the if ($this->isValidData($data)) condition now understands at a glance what happens there, without the need to read and understand how it happens; if it works then don't worry about it;
the validation code can be tested independent of what happens when the data is valid or not;
the code can be reused, if needed and appropriate, by calling the method with different data (maybe submitted through a different form).

score 2 · Answer 2 · edited May 23 '17 at 10:26

PHP short circuits the if statement. Therefore if you have a number of if clauses.

   if ($condition1 && $condition2 && $condition3 && $condition4)

then PHP will stop evaluating the expression as soon as the first one is false.

To optimise the code, order the expression so that the most likely ones for failure is evaluated first.

References : http://php.net/manual/en/language.operators.logical.php

Does PHP have short-circuit evaluation?

Is there a short-circuit OR in PHP that returns the left-most value?

kacper3w · Answer 3 · 2015-08-11T08:17:55.140

In terms of readibility you should format the code properly and everything should be fine (formatting in given example is OK). In terms of debugging there might be some problems in defining which statement fails, but still - with right formatting you could just comment out several lines easyli and check the results. In case of performance i think it's even better. I mean you'd have to check all these conditions somehow anyway. If you put them all in separate ifs they'd be checked all one after another (OK, you could write it so that not all of them are checked but I think it's just lengthening your code unnecessarily). If you compose your statements correctly not all of them will have to be checked. Eg with lots of && if the first condition is false the next conditions aren't even checked. You should put less probable, more lightweight (in terms of performance) conditions in the begginning and leave heavy, probable ones at the end. If you want to keep your main code clean you could wrap it in a function as well.

score 0 · Answer 4 · answered Aug 11 '15 at 09:08

This is a poor solution for performance. However, generally I just don't care that much when processing a form if it's the most efficient thing on earth - it's not kernel code. I've just solved a similar problem by throwing 192 gigs of ram at it..... on the basis it'll save me hundreds to thousands of developer hours, which costs a lot more than the ram.

$hasdata            = array_key_exists(self::SUB_FORM_CONTACT, $data);
$hasCompany         = array_key_exists('company', $data[self::SUB_FORM_CONTACT]);
$hasSalesRegion     = array_key_exists('salesRegion', $data[self::SUB_FORM_CONTACT]['company']) 
$hasSalesRegion     = $hasSalesRegion && !empty($data[self::SUB_FORM_CONTACT]['company']['salesRegion']);
$hasServiceRegion   = array_key_exists('serviceRegion', $data[self::SUB_FORM_CONTACT]['company']) 
$hasServiceRegion   = $hasServiceRegion && !empty($data[self::SUB_FORM_CONTACT]['company']);

$evalResult = $hasdata && $hasCompany && ($hasSalesRegion || $hasServiceRegion);

if($evalResult)
{
    // do your thing
}

I would prefer to see something like this than get the benefit of short cutting and hard to read double negatives.