-1
\$\begingroup\$

My method counts the number of times an error occurs.

I have a feeling my snippet isn't the best practice way of doing this. So when one of the validation methods fails I want to stop the method.

public static function validate($data)
{
    $errors = [
        'settings_mismatch' => 0,
        'name_not_set' => 0,
        'price_not_set' => 0,
        'payment_qty_not_set' => 0,
        'payment_qty_zero' => 0,
        'payment_unit_not_set' => 0,
        'payment_unit_not_allowed' => 0
    ];


    if (!isset($data['name'])) {
        $errors['name_not_set']++;
        return $errors;
    }

    if (!isset($data['price'])) {
        $errors['price_not_set']++;
        return $errors;
    }

    if (!isset($data['payment_qty'])) {
        $errors['payment_qty_not_set']++;
        return $errors;
    }

    if (!isset($data['payment_unit'])) {
        $errors['payment_unit_not_set']++;
        return $errors;
    }

    if (isset($data['payment_qty']) && $data['payment_qty'] == 0) {
        $errors['payment_qty_zero']++;
        return $errors;
    }

    if (isset($data['payment_unit']) && !in_array(strtolower(self::transformUnit($data)['payment_unit']), self::$allowedMeasurements)) {
        $errors['payment_unit_not_allowed']++;
        return $errors;
    }

    return $errors;
}
\$\endgroup\$
  • \$\begingroup\$ There are mutual eexclusive statements in this question: First it states that "My method counts the number of times an error occurs." and then " when one of the validation methods fails I want to stop the method" - so it doesn't actually count but return immediately. Can't you please make your mind first? \$\endgroup\$ – Your Common Sense Jan 23 '18 at 12:11
1
\$\begingroup\$

If you are looking for a shorter code you can do this :

public static function validate($data)
{
    $errors = [
        'settings_mismatch' => 0,
        'name_not_set' => 0,
        'price_not_set' => 0,
        'payment_qty_not_set' => 0,
        'payment_qty_zero' => 0,
        'payment_unit_not_set' => 0,
        'payment_unit_not_allowed' => 0
    ];

    if (!isset($data['name']))
        return array_merge($errors,['name_not_set'=>1]);

    if (!isset($data['price']))
        return array_merge($errors,['price_not_set'=>1]);

    if (!isset($data['payment_qty']))
        return array_merge($errors,['payment_qty_not_set'=>1]);

    if (!isset($data['payment_unit']))
        return array_merge($errors,['payment_unit_not_set'=>1]);

    if (isset($data['payment_qty']) && $data['payment_qty'] == 0)
        return array_merge($errors,['payment_qty_zero'=>1]);

    if (isset($data['payment_unit']) && !in_array(strtolower(self::transformUnit($data)['payment_unit']), self::$allowedMeasurements))
        return array_merge($errors,['payment_unit_not_allowed'=>1]);

    return $errors;
}

But using function is always better, so for a cleaner code :

public static function validate($data)
{
    $methods = ['settings_mismatch','name_not_set','price_not_set','payment_qty_not_set','payment_unit_not_set','payment_qty_zero','payment_unit_not_allowed'];
    $errors = array();
    foreach($methods as $method)
        $errors[$method] = 0;
    foreach($methods as $method)
        if(self::$method($data))
        {
            $errors[$method] = 1;
            break;
        }
    return $errors;
}

public static function settings_mismatch(&$data)
{
    # TODO
}

public static function name_not_set(&$data)
{
    return !isset($data['name']);
}

public static function price_not_set(&$data)
{
    return !isset($data['price']);
}

public static function payment_qty_not_set(&$data)
{
    return !isset($data['payment_qty']);
}
public static function payment_qty_zero(&$data)
{
    return isset($data['payment_qty']) && $data['payment_qty'] == 0;
}

public static function payment_unit_not_set(&$data)
{
    return !isset($data['payment_unit']);
}
public static function payment_unit_not_allowed(&$data)
{
    return isset($data['payment_unit']) && !in_array(strtolower(self::transformUnit($data)['payment_unit']), self::$allowedMeasurements);
}

Then I wonder why would you break on the first error instead of returning all errors (a better practice if you are implementing a Web service) :

public static function validate($data)
{
    $methods = ['settings_mismatch','name_not_set','price_not_set','payment_qty_not_set','payment_unit_not_set','payment_qty_zero','payment_unit_not_allowed'];
    $errors = array();
    foreach($methods as $method)
        $errors[$method] = (int)self::$method($data);
    return $errors;
}
# same other methods as above

Please note that in the 2 last codes the order of $methods is not the same as the function order, to keep your original code behavior. In the last code since the order is not relevant, you could reflect the class to get the list of $methods.

Also as a code review there is a wrong logic (too me) : instead of isset && ==0 or isset && !in_array I would have put !isset || ==0 and !isset || !in_array, to imply that a not set quantity is as not allowed as a zero quantity, and that a not set unit is not allowed too, but you can keep it this way there is no error here.

\$\endgroup\$

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.