UPC-A validation

Question

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 Wikipedia).

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

• Bobby: 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).
• deed02392: Just so people know then, see the accepted answer for a bug-fix in the above.

Answer 1

Version incorporating all fixed bugs from various answers, with my own take on how to deal with the case where the intermediate result is 10 (aka M from the definition @Wikipedia) by adding another mod 10 operation to the check digit calculation, results in 0 if the sum mod 10 is 10, or the check digit otherwise. I also deleted the conversion to string because the reality is you have to handle UPCs as a string. Using Integer to represent UPC is bogus because UPCs can have leading 0's which are lost upon conversion to Integer.

At this point in time, all of the above have one or more bugs.

 function valid_upc_a($upc) {
 if(!isset($upc[11])) {
 return FALSE;
 }
 $odd_sum = $even_sum = 0;
 for($i = 0; $i < 11; ++$i) {
 if ($i % 2 == 0) {
 $even_sum += $upc[$i];
 }
 else {
 $odd_sum += $upc[$i];
 }
 }
 $total_sum = $even_sum + $odd_sum * 3;
 $modulo10 = $total_sum % 10;
 $check_digit = (10 - $modulo10) % 10;
 return $upc[11] == $check_digit;
 }

• IEatBagels: "with my own take on how to deal with the case where the intermediate result is 10" could you explain in more detail what your take is? On Code Review we're looking for explanations on how to make the code better, not just better code! :) The key point is to explain why you've made some changes to OP's code
• CodeWriter23: 10 % 10 = 0, 10 % non-zero check digit = non-zero check digit. Submission updated.
• IEatBagels: I understood it, I meant you should add it in your answer :) Comments aren't meant to hold valuable information
• CodeWriter23: Was doing while you were typing :)
• deed02392: @CodeWriter23 You're welcome :-) Although Nick's comment applies to this code codereview.stackexchange.com/questions/19438/… - the $even_sum/$odd_sum logic is backwards.

Answer 2

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

• deed02392: Very good, surprised no one else spotted that. Many thanks.
• Nick: 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.

Answer 3

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.

• deed02392: 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?
• Peter Kiss: 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()).
• Nick: @PeterKiss: Please see my comment on the accepted answer above, i%2 in the if condition is working backwards.

Answer 4

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

• Paul: 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 ==.