3
\$\begingroup\$

I really don't like the so many return statements. Anyone think that this could be refactored more?

public function validate($value, Constraint $constraint)
{
    if('' === $value || null === $value) {
        return;
    }

    if(!preg_match('/^[0-9]{7}([0-9]{3})[0-9]{1}$/', $value, $matches)) {
        $this->context->addViolation($constraint->message, array(
            '{{ value }}' => $value
        ));
        return;
    }

    // Fast Luhn check
    $vatin = (string) $matches[0];

    $luhn = array(0, 2, 4, 6, 8, 1, 3, 5, 7, 9);
    $sum  = 0;

    for($i = 0; $i < strlen($vatin); $i++) {
        $dgt  = (int) substr($vatin, $i, 1);
        $sum += (0 === $i % 2) ? $dgt : $luhn[$dgt];
    }

    if(0 !== ($sum % 10)) {
        $this->context->addViolation($constraint->message, array(
            '{{ value }}' => $value
        ));
        return;
    }

    // Province check
    $provinceCode = $matches[1];

    if(!in_array((int) $provinceCode, range(1, 121) + array(888, 999))) {
        $this->context->addViolation($constraint->provinceMessage, array(
            '{{ value }}' => $provinceCode
        ));
    }
}
\$\endgroup\$
2
\$\begingroup\$

Ah, the yoda syntax, don't see that too often. Anyways, on to the subject at hand. Here are a few ways you can refactor it, I'll address your specific question towards the end.

You can use the empty() function in your first comparison, it will return TRUE if the value is any FALSE value ('', 0, NULL, FALSE, "false", etc...), so it does the same thing but is a little cleaner.

if( empty( $value ) ) {
    return;
}

Another possible refactor, is to not use typecasting unless necessary. For instance, below you have a string typecast that is converting the element of an array to a string. This is unnecessary, at least if I'm reading the doc page on preg_match() properly. The 0th element will already contain a string, so this is redundant. Below that is another typecast that converts a string to an integer. This also seems unnecessary. PHP is a very loosely typed language, meaning transitioning from a integer to a string, or vice-versa, doesn't require any special type-casting or syntax. BTW: below each example is also another way to refactor the statement that may be a little better.

$vatin = (string) $matches[0];
$vatin = $matches[ 0 ];//the same
$vatin = array_shift( $matches );//avoids magic numbers, even though most will know where the 0 comes from

$dgt  = (int) substr($vatin, $i, 1);
$dgt = substr( $vatin, $i, 1 );//the same
$dgt = $vatin[ $i ];//string array syntax avoids the unnecessary function call

Avoid calling functions in for or while loop parameter lists. Both of these loops call any parameter functions on every iteration, meaning more overhead. Set the value before the loop. I'd also suggest doing the same in foreach loops, but its not as important.

$length = strlen($vatin);
for( $i = 0; $i < $length; $i++ ) {

Another problem you have here is that you are violating "Don't Repeat Yourself" (DRY) by using the same, or almost the same values in the addViolation() method so many times. A better way would be to set up a default, then log it once. There are probably a few different ways to go about this, but here's an example of one.

//in class
$violation = array(
    $constrain->message,
    $value
);

if( ! preg_match() ) {
    return $violation;
}
//repeat or return different violation as necessary

//implementation
$violation = $instance->validate( $value, $constraint );
if( is_array( $violation ) ) {
    list( $message, $value ) = $violation;

    $instance->context->addViolation( $message, array(
        '{{ value }}' => $value
    ) );
}

Now, the question you are asking about is more or less a style choice. The one-return vs many. At least if I am understanding your question correctly. I used to be of the first camp, but I think I'm leaning a little more towards the other now. Sometimes it makes sense to use a single return, but its a matter of context. For instance, if I found what I was looking for in the middle of a loop, its better to return from the loop early rather than finish it and then test a few other things. Your code can quite easily get skewed or convoluted or redundant when trying to maintain a single return.

So, while it may seem undesirable, your code is actually benefiting from these multiple return statement. I wouldn't try refactoring them out, but if you absolutely must, you can modify the $violation array I showed you above so that the value is only set if an error occured, then, at the end of your function, if it has been set you can return the array.

The only other solution I can think of, and perhaps a better one, would be to not use returns at all, but to throw errors instead. That appears to be very close to what you are doing anyways, so you might want to seriously contemplate this switch.

\$\endgroup\$
2
  • \$\begingroup\$ Regarding strnlen function inside the loop, I'm pretty sure that the upper condition is computed just one time, at least in foreach. Going to investigate further about for loop. \$\endgroup\$ – gremo Sep 29 '12 at 6:42
  • \$\begingroup\$ @Gremo: Corbin and I have already had this argument and both of us walked away having learned something. If you are interested in the results, take a look at these comments. I believe I said in the above review that foreach was not the same, but it does still do something extra when using the function in the loop instead of defining it before hand. Usually that something extra is ignored, but its still a good idea to remove it, IMHO. \$\endgroup\$ – mseancole Sep 29 '12 at 13:11
0
\$\begingroup\$
  • Why are you passing the Constraint instance to the function, it it is there only for you to extract $constraint->message from it?

    This use violates the LoD and you seem to be leaking the encapsulation too.

  • It seems to me that validation process consists of multiple independent stages. Each of those should be in a separate method.

    You might find this lecture relevant.

  • The process seems very convoluted.

    You are manipulating some other $this->context object and terminating the execution after first manipulation. Why not instead return the array and execute the method directly?

    $response = $foobar->validate( $value );
    if ( ! empty( $repsones ) )
    {
        $context->addViolation( $constraint->message, $respone );
    }
    
\$\endgroup\$
1
  • \$\begingroup\$ 1) because I'm extending Validation class by Symfony 2 framework and Constraint class holds the options. 2) Good one, i can do this! 3) See point 1 :P \$\endgroup\$ – gremo Sep 29 '12 at 6:37

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.