Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've just written a validation routine for a UPC-A, which ensures the check digit matches the given UPC number (according to rules I found on Wiki, mind!).

Below is the code, what do you think?

function valid_upc_a($value)
{
    $odd_sum = $even_sum = 0;
    if(strlen($value) != 12) return FALSE;
    $chars = str_split($value);
    for($i=0;$i<11;$i++)
    {
        $odd_sum += $i%2==0?$chars[$i]:0;
        $even_sum += $i%2==1?$chars[$i]:0;
    }
    $total_sum = $even_sum + $odd_sum*3;
    $modulo10 = $total_sum % 10;
    $check_digit = 10 - $modulo10;
    return (int)$chars[11] === $check_digit;
}

It works for a couple of cases I made up, here it is on CodePad:

http://codepad.viper-7.com/QEqpFX

And the checkdigit derivation algorithm is described here:

http://en.wikipedia.org/wiki/Universal_Product_Code#Check_digits

share|improve this question
    
I took the liberty to roll back the changes. Please do not change the code after posting it except for errors which should not have been there in the first place (like typos when copying the code into the question). –  Bobby Feb 6 at 15:31
    
Just so people know then, see the accepted answer for a bug-fix in the above. –  deed02392 Feb 6 at 16:31
add comment

3 Answers 3

up vote 2 down vote accepted

Good function, only when $modulo10 is 0 it should not be substracted from 10, so it would be something like this:

function valid_upc_a($value) {
    $upc = strval($value);

    if(!isset($upc[11])) {
        return FALSE;
    }

    $odd_sum = $even_sum = 0;

    for($i = 0; $i < 11; ++$i) {
        if ($i % 2) {
            $even_sum += $upc[$i];
        } else {
            $odd_sum += $upc[$i];
        }
    }

    $total_sum = $even_sum + $odd_sum * 3;
    $modulo10 = $total_sum % 10;

    if ($modulo10 > 0)
        $check_digit = 10 - $modulo10;
    else 
       $check_digit = 0;        

    return $upc[11] == $check_digit;
}
share|improve this answer
    
Very good, surprised no one else spotted that. Many thanks. –  deed02392 Feb 6 at 9:15
add comment

Two things I noticed.

  1. The return statement if the length is invalid should be on a separate line, and wrapped in braces
  2. What purpose does the int cast solve? I thought php was typeless.

Other than that, the only other thing I noticed is to space out your operators. However, all the actual function seems to work fine

share|improve this answer
1  
PHP does have types and provides type juggling. The Identical Operator === assures that the variables are equal and of the same type, so a cast is required to make them the same type. Another option would have been to use the Equivalent Operator ==. –  Paul Dec 11 '12 at 1:18
add comment

I have made a cleaner version of yours put something in and get rid of other things:

<?php

function valid_upc_a($value) {
    $upc = strval($value);

    if(!isset($upc[11])) {
        return FALSE;
    }

    $odd_sum = $even_sum = 0;

    for($i = 0; $i < 11; ++$i) {
        if ($i % 2) {
            $even_sum += $upc[$i];
        } else {
            $odd_sum += $upc[$i];
        }
    }

    $total_sum = $even_sum + $odd_sum * 3;
    $modulo10 = $total_sum % 10;
    $check_digit = 10 - $modulo10;

    return $upc[11] == $check_digit;
}

It's not a huge modification but for example to me is more readable.

share|improve this answer
    
I have to ask why have you used strval against the value instead of str_split, to get it into an array of characters? Can we rely on strval to always provide us with a string that we can access with indicies? –  deed02392 Dec 30 '12 at 16:27
    
strval() will not always return a string, if the value is an object without __toString() it will raise a Fatal Error if the parameter is an array it will return 'Array' and with parameter type resource it will return 'Resource id #2'. str_split can be a better choice becouse it will trigger an error if it's parameter is not a string (or stringable) which means array/resource can not be the parameter (beside objects without __toString()). –  Peter Kiss Dec 31 '12 at 13:13
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.