0
\$\begingroup\$

I refactored 2 seperate functions that I found here into a single function:

<?
    function isValidBarcode($barcode) {

        $barcode = trim($barcode);

        //If value is not numeric, it is not valid.
        if ((!is_numeric($barcode)) && ((strlen($barcode) == 12) || (strlen($barcode) == 13))) return false;

        //set empty variables
        $even_sum = $odd_sum = 0;

        //array key value to count to, doesn't include the last 2 numbers of barcode
        $countTo = strlen($barcode) - 2;

        //loop through every digit in barcode, count even and odds based on $countTo set before.
        for ($x = 0; $x <= $countTo; $x++) {
            if ($x % 2 == 0) {
                $even_sum += $barcode[$x];
            } else {
                $odd_sum += $barcode[$x];
            }
        }

        //Multiply even_sum by 3, and then add odd_sum to it.
        $total_sum   = $even_sum * 3 + $odd_sum;

        //get highest multiple of 10 of total_sum.
        $next_ten    = ceil($total_sum / 10) * 10;

        //Subtract highest multiple of 10 from total_sum to get check_digit.
        $check_digit = $next_ten - $total_sum;

        //if check_digit is equal to the last digit in the barcode, it is valid.
        if ($check_digit == $barcode[$countTo + 1]) return true;

        //if check_digit is not equal to the last digit in the barcode, it is invalid.
        return false;
    }

?>

But, I feel as though I can make this even better. I wanted some other opinions on this, and I wanted to see what other people could come up with in terms of a solution.

My main question is, how efficient is this code, and is it possible for me to make it more efficient?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Some thoughts:

  • Why would you silently trim() and potentially transform an invalid bar code value into a valid one? This would seem to give unexpected results. If a string value needs to be trimmed, let the caller do it before calling this method.
  • What happens if you are not passed a string? To me this might be an exceptional event and worthy of throwing exception from this function, as this is something the developer writing code calling this function needs to be aware of.
  • Do you really want to use is_numeric() to validate the input format? This can lead to false positives (for example +123.45678e9 would pass your current format check). If what you are expecting are 12 or 13 digits in this string, then precisely test for that. In addition to your questionable use of is_numeric(), you also have your length conditions set inappropriately. You should be using OR between the digit criteria and length criteria as either cause failure. Right now a string like foobar will actually pass your conditional because it does not satisfy both conditions.

For example:

if (!preg_match('/^[0-9]{12,13}$/', $barcode)) {
    return false;
}
// rest of code

or

if (!ctype_digit($barcode) || $bcLen < 12 || $bcLen > 13) {
    return false;
}
// rest of code
  • I think the logic to determine check digit can be done in a more simplified manner. Part of this simplificatoin could be to explictly convert the barcode string to an array to make it easier to work with. I have done a function rewrite below which I hope demonstrates some simplification.

Example:

function isValidBarcode($barcode)
{
    if (!is_string) {
        throw new InvalidArgumentException('String value expected.');
    }
    if (!preg_match('/^[0-9]{12,13}$/', $barcode)) {
        return false;
    }
    $barcodeArray = str_split($barcode);
    $lastDigit = (int)array_pop($barcodeArray);
    $sum = 0;
    for($i = 0; $i < count($barcodeArray); $i++) {
        $sum += (int)$barcode[$i];
        if ($i % 2 === 0) {
            $sum += (int)$barcode[$i] * 2;
        }
    }
    $checkDigit = 10 - ($sum % 10);
    return $lastDigit === $checkDigit;
}
  • Be careful using loose comparisons as your default means for comparing. I would suggest you get in the habit of using exact comparisons as default unless there is good reason to do loose comparison. I don't see any actual bug in this code (since PHP's built-in type juggling should take care of string to integer conversions in your specific use case here). Bu,t you might not always be so lucky. PHP's loose typing can be useful, but it can also be a source of hard-to-identify bugs. To me when reading code, I would like the code writer to explicitly demonstrate their understanding of types being applied to the variables.
  • Style-wise, I have a few concerns. You are mixing camelCase and snake_case in your code. Ideally you should pick one style and stick to it (though PHP as a language is not very good about this). There are too many comments, most of which are not really necessary. You also have inconsistent spacing around your = assignments near the end of code. Trying to horizontally-align assignment operators can be a bit of a fool's errand if you try to do this throughout your codebase. Every time you want to make changes to variable names and such, you end up having to redo spacing. Is it worth it?
  • Why are you concerned with the "efficiency" of this code? Are you planning to execute this code thousands of time in any one script execution such that every millisecond spent in this function gets compounded? If not, you might be prematurely optimizing this bit of code. Sure, you want to make sure it runs with reasonable efficiency (which I think your code does since it is such a simple data structure - a string - that you are working with), but I don't think you want to necessarily squeeze out every millisecond out of this function if it means making the function hard to read or maintain. That code quality aspect should likely be you primary concern with a function such as this which you are not likely to spend a large percentage of your overall processing time in.
\$\endgroup\$
1
  • \$\begingroup\$ The comments were to explain what the code was doing for StackExchange - they weren't in the production version of the function. And thank you for your other notes :) \$\endgroup\$ Jul 19, 2017 at 18:46
1
\$\begingroup\$

It took me like 3 seconds to read this line because of all the unnecessary perens...

if ((!is_numeric($barcode)) && ((strlen($barcode) == 12) || (strlen($barcode) == 13))) return false;

It's PHP, not LISP. Consider not wrapping every thing in parentheses. ((( :P )))

if (!is_numeric($barcode) && (strlen($barcode) == 12 || strlen($barcode) == 13)) return false;

This comment is a lie

//array key value to count to, doesn't include the last 2 numbers of barcode

Your loop is testing $x <= $countTo so your're only omitting the last char, not the last two. If you want to omit the last two the loop needs to test < instead of <=.

The entire for loop can be shortened into 3 lines...

preg_match_all('/(\w)(\w)/',$barcode,$oddEven);
$even_sum = array_sum($oddEven[2]);
$odd_sum = array_sum($oddEven[1]);

Note: If you have an odd number of chars in the barcode, this regex will not account for the last char. If you have an even number of chars, you will still need to pop the last char off manually.

If you don't want to include the last digit or two digits just trim them off the barcode first. See here for an example.


Here's a complete working example with my suggested changes.

function isValidBarcode($barcode) {

    $barcode = trim($barcode);

    $bcLen = strlen($barcode);

    //If value is not numeric, it is not valid.
    if (!is_numeric($barcode) && ($bcLen == 12 || $bcLen == 13)) return false;

    //set empty variables
    $even_sum = $odd_sum = 0;

    preg_match_all('/(\w)(\w)/',$barcode,$oddEven);
    $even_sum = array_sum($oddEven[1]);

    // If you have an odd number of chars in the barcode, this regex will not account for the last char. 
    // If you have an even number of chars, you will still need to pop the last char off manually.
    if($bcLen % 2 === 0) array_pop($oddEven[2]);
    $odd_sum = array_sum($oddEven[2]);

    //Multiply even_sum by 3, and then add odd_sum to it.
    $total_sum   = $even_sum * 3 + $odd_sum;

    //get highest multiple of 10 of total_sum.
    $next_ten    = ceil($total_sum / 10) * 10;

    //Subtract highest multiple of 10 from total_sum to get check_digit.
    $check_digit = $next_ten - $total_sum;

    //if check_digit is equal to the last digit in the barcode, it is valid.
    return $check_digit == substr($barcode, $bcLen-1);
}
\$\endgroup\$
6
  • \$\begingroup\$ How does your preg_match_all code work? What does that pattern do? And thank you for the advice about the parenthesis \$\endgroup\$ Jul 17, 2017 at 19:01
  • \$\begingroup\$ \w matches any single alpha numeric char. Using twice effectively separates even and odd chars in the 1st and 2nd index. \$\endgroup\$ Jul 17, 2017 at 19:30
  • \$\begingroup\$ I see. This actually seems to break the code. I used substr to cut off the last digit of the barcode substr($barcode,0,-1), and now this no longer returns properly in my test cases PHP Sandbox The top 2 responses should be true and the bottom 2 should be false \$\endgroup\$ Jul 17, 2017 at 19:36
  • \$\begingroup\$ The code in your sandbox is not equivelent to your original code. it looks like you have an off-by-one issue here.. i'll take a closer look when i get off work and see if i can spot the mistake. \$\endgroup\$ Jul 17, 2017 at 20:14
  • \$\begingroup\$ Except it works when I switch back to the for loop. I changed a few more things but it works until I change to preg_match_all \$\endgroup\$ Jul 17, 2017 at 20:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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