HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

If statement with 2 conditions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withconditionsstatement

Problem

On my webform, there are two kinds of address-fields a user can give. But since it's a complex webform (Drupal) it's kind of hard to achieve. Every time a user adds an extra address, the form is regenerated and there will be a new item added to the $address_afl and $address_bev-array. Is it possible to compress this code so the foreach-loop starts looping with the highest value (either $address_afl or$address_bev )

$address_afl = $form['field_afl_dienst_adressen']['und'];
$i = 0;
foreach($address_afl as $value) {
    if (is_array($value)) {
        $test = array_keys($value);
    if(is_array($test)) {
        foreach ($test as $z) {
            if (strpos($z, '#') === 0) continue;
            if (isset($form['field_afl_dienst_adressen']['und'][$i][$z])) {
                $form['field_afl_dienst_adressen']['und'][$i][$z]['#prefix'] = "";
                $form['field_afl_dienst_adressen']['und'][$i][$z]['#suffix'] = "";
            }
        }
    }
    }
    $i++;
}

$address_bev = $form['field_bevoegde_adressen']['und'];
$i = 0;
foreach($address_bev as $value) {
    if (is_array($value)) {
        $test = array_keys($value);
    if(is_array($test)) {
        foreach ($test as $z) {
            if (strpos($z, '#') === 0) continue;
            if (isset($form['field_afl_dienst_adressen']['und'][$i][$z])) {
                $form['field_bevoegde_adressen']['und'][$i][$z]['#prefix'] = "";
                $form['field_bevoegde_adressen']['und'][$i][$z]['#suffix'] = "";
            }
        }
    }
    }
    $i++;
}

Solution

I don't quite exactly understand the logic, but you could extract out at least one method which removes lots of code duplication. I've inverted some conditions (which improves code readability) and placed some comments into the code too.

function processValue(&$form, $value, $i, &$form_sub) {
    if (!is_array($value)) {
        return;
    }

    // TODO: rename test to $keys or something more meaningful
    $test = array_keys($value);
    if (!is_array($test)) {
        // TODO: is it possible? according to the documentation 
        // array_keys() always returns array 
        return;
    }
    foreach ($test as $z) {
        if (strpos($z, '#') === 0) {
            continue;
        }
        if (!isset($form['field_afl_dienst_adressen']['und'][$i][$z])) {
            continue;
        }
        $form_sub[$i][$z]['#prefix'] = "";
        $form_sub[$i][$z]['#suffix'] = "";
    }
}

$address_afl = $form['field_afl_dienst_adressen']['und'];
$i = 0;
foreach($address_afl as $value) {
    processValue($form, $value, $i, $form['field_afl_dienst_adressen']['und']);
    $i++;
}

$address_bev = $form['field_bevoegde_adressen']['und'];
$i = 0;
foreach($address_bev as $value) {
    processValue($form, $value, $i, $form['field_bevoegde_adressen']);
    $i++;
}


A possible bug: both foreach has the following condition with the same $form index:

if (isset($form['field_afl_dienst_adressen']['und'][$i][$z])) ...


Maybe you want to change the second one to

if (isset($form['field_bevoegde_adressen']['und'][$i][$z])) ...


It would make possible some other refactorings (fewer parameters etc.), as Victor T. suggested.

Code Snippets

function processValue(&$form, $value, $i, &$form_sub) {
    if (!is_array($value)) {
        return;
    }

    // TODO: rename test to $keys or something more meaningful
    $test = array_keys($value);
    if (!is_array($test)) {
        // TODO: is it possible? according to the documentation 
        // array_keys() always returns array 
        return;
    }
    foreach ($test as $z) {
        if (strpos($z, '#') === 0) {
            continue;
        }
        if (!isset($form['field_afl_dienst_adressen']['und'][$i][$z])) {
            continue;
        }
        $form_sub[$i][$z]['#prefix'] = "<div class='address-publicatie-$z'>";
        $form_sub[$i][$z]['#suffix'] = "</div>";
    }
}


$address_afl = $form['field_afl_dienst_adressen']['und'];
$i = 0;
foreach($address_afl as $value) {
    processValue($form, $value, $i, $form['field_afl_dienst_adressen']['und']);
    $i++;
}

$address_bev = $form['field_bevoegde_adressen']['und'];
$i = 0;
foreach($address_bev as $value) {
    processValue($form, $value, $i, $form['field_bevoegde_adressen']);
    $i++;
}
if (isset($form['field_afl_dienst_adressen']['und'][$i][$z])) ...
if (isset($form['field_bevoegde_adressen']['und'][$i][$z])) ...

Context

StackExchange Code Review Q#7031, answer score: 2

Revisions (0)

No revisions yet.