# UPC-A Validation

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:

And the checkdigit derivation algorithm is described here:

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

-
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 '14 at 15:31
Just so people know then, see the accepted answer for a bug-fix in the above. – deed02392 Feb 6 '14 at 16:31

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;
}

-
Very good, surprised no one else spotted that. Many thanks. – deed02392 Feb 6 '14 at 9:15
I think your odd and even are backwards. $i % 2 will evaluate to 0 if the number is even, which is considered false. Your "even" condition is actually running when it's odd. You should also multiply the even sum by 3 ,not the odd sum. These errors together are probably producing the correct result, but is not doing what you think it is. You should put $i%2===0 in your if condition because that will evaluate to a boolean, and more importantly, be true when the number is even, as intended. – Nick Dec 27 '14 at 2:29

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

-
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

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.

-
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
@PeterKiss: Please see my comment on the accepted answer above, i%2 in the if condition is working backwards. – Nick Dec 27 '14 at 2:32