13
\$\begingroup\$
import java.math.BigInteger;

/**
 * Created by lungisani on 2017/02/25.
 */
public class Luhn {
    public Boolean getIdentitySummation(BigInteger identities){
        String identify = String.valueOf(identities);
        int sumOdd = 0, sumEven = 0, _doubled;
        int summation;
        try {
            String identity = String.valueOf(identify);
            int length = identity.length() - 1;
            char[] chars = identity.toCharArray();
            String check = identity.substring(12);
            for(int x  = 0; x < length; x++){
                if(x % 2 == 0 ){
                        String numString = String.valueOf(chars[x]);
                        int numbers = Integer.valueOf(numString);
                        sumOdd += numbers;
                }else if(x % 2 != 0){
                        String numString = String.valueOf(chars[x]);
                        int numbers = Integer.valueOf(numString);
                        int doubled = numbers * 2;
                        if(doubled > 9){
                            _doubled = doubled - 9;
                        }else{
                            _doubled = doubled;
                        }
                        sumEven += _doubled;
                }
            }
            summation = sumOdd + sumEven;
            int checksum = Integer.valueOf(check);
            if((summation * 9) % 10 == checksum)
                return Boolean.TRUE;
            else
                return Boolean.FALSE;
        } catch (Exception e) {
            e.printStackTrace();
            return Boolean.FALSE;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Of course there are obvious flaws like the returns \$\endgroup\$ – EastXWest Mar 6 '17 at 17:07
  • 1
    \$\begingroup\$ What do you mean by "obvious flaws"? Does this code work correctly, to the best of your knowledge? \$\endgroup\$ – 200_success Mar 6 '17 at 17:13
  • \$\begingroup\$ yes it works perfectly. Any south african ID number i can prove to be valid or invalid. \$\endgroup\$ – EastXWest Mar 6 '17 at 17:20
  • \$\begingroup\$ My reference to obvious flaw is to the return statements, which im not happy about. I created the code myself. And i have tested on more than 100, ID numbers \$\endgroup\$ – EastXWest Mar 6 '17 at 17:21
  • \$\begingroup\$ For future reference, please see Simon's checklist for how to write a good Code Review question. There is absolutely no information at all here except the code. I have no choice but to downvote this. \$\endgroup\$ – Simon Forsberg Mar 7 '17 at 14:01
18
\$\begingroup\$

Let's work through some of the simpler issues, like code redundancy, and some helper-functions in the core library that help a lot, and will make your code simpler.

Double-checking else

When you have an either-or check in an if-condition, there's no need to double-check the else side of things.

you check for odd-positioned digits, which implies every other digit is even-positioned... This code:

if(x % 2 == 0 ){
    .....
}else if(x % 2 != 0){
    .....
}

can be just:

if(x % 2 == 0 ){
    .....
} else {
    .....
}

Try-Catch

There's no code in your function that throws an explicit exception - why do you have a try/catch? All integer parsing comes from digits in a BigInteger, so there can be no illegal characters, etc.

Character-to-digit-value

Your code has a lot of this type of logic:

String numString = String.valueOf(chars[x]);
int numbers = Integer.valueOf(numString);

but that can be simplified to just:

int numbers = Character.getNumericValue(chars[x]);

Separated sums

You have both sumOdd and sumEven, but there's no need for both. You can have one sum accumulator and use it in each side.

Simplifying the doubled digits

The algorithm requires doubles that are larger than 9 to be reduced by 9. Your code is:

int doubled = numbers * 2;
if(doubled > 9){
    _doubled = doubled - 9;
}else{
    _doubled = doubled;
}
sumEven += _doubled;

Using a "ternary" expression, and a little bit of manipulation on the math, you can reduce that to just:

sum += digit < 5 ? digit * 2 : digit * 2 - 9;

Autoboxing and conditionals

Java will "autobox" primitive variables like boolean to their full class types Boolean when needed, without any explicit handling. Let's take this exit segment:

if((summation * 9) % 10 == checksum)
    return Boolean.TRUE;
else
    return Boolean.FALSE;

This should have braces on the 1-liners, to be:

if((summation * 9) % 10 == checksum) {
    return Boolean.TRUE;
} else {
    return Boolean.FALSE;
}

but really, that's all unnecessary, because autoboxing comes to the rescue:

return (summation * 9) % 10 == checksum

That's all you need.

Conclusion

Putting all these suggestions together, you can significantly reduce the complexity of the code, to something like:

public static Boolean getIdentitySummationRL(BigInteger identities){
    char[] idchars = identities.toString().toCharArray();
    int sum = 0;
    // loop over each digit, except the check-digit
    for (int i = 0; i < idchars.length - 1; i++) {
        int digit = Character.getNumericValue(idchars[i]);
        if ((i % 2) == 0) {
            sum += digit;
        } else {
            sum += digit < 5 ? digit * 2 : digit * 2 - 9;
        }
    }
    int checkdigit = Character.getNumericValue(idchars[idchars.length - 1]);
    int compdigit = (sum * 9) % 10;

    return checkdigit == compdigit;
}

(I have checked that using my ID number, and it's OK)

Update

Note I have been doing a little more reading on Luhn's algorithm because of @Molvalio's comment and also I remember doing it a couple of decades ago for other numbers (not ID numbers) and I remember the checking algorithm to be different to this implementation. I was right that checking the number is simpler than your code. See the algorithm here: Luhn's Algorithm Verification

The point is that your code is computing a check-digit and comparing it with the existing digit, but you're missing the fact that the digit is designed to be incorporated in to the same calculations as the checksum, and a valid number has a resulting digit of 0.

Additionally, the Luhn's algorithm is computed right-to-left. In your case, because the length of SA ID numbers is 13, you're OK (both left and right digits are odd) but you should implement the algorithm more closely....

So, the code can be simplified further:

public static Boolean checkLuhn(BigInteger identities){
    char[] idchars = identities.toString().toCharArray();
    int sum = 0;
    // loop over each digit right-to-left, including the check-digit
    for (int i = 1; i <= idchars.length; i++) {
        int digit = Character.getNumericValue(idchars[idchars.length - i]);
        if ((i % 2) != 0) {
            sum += digit;
        } else {
            sum += digit < 5 ? digit * 2 : digit * 2 - 9;
        }
    }
    return (sum % 10) == 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I am grateful. I will start perusing the javadocs as there is much to learn \$\endgroup\$ – EastXWest Mar 6 '17 at 18:11
  • 1
    \$\begingroup\$ I started writing an answer, looked back and realised you'd already covered everything I was writing about. +1 Would be preempted again. \$\endgroup\$ – Pharap Mar 6 '17 at 21:37
  • 1
    \$\begingroup\$ I like double checking the else sometimes because it makes the code more explicit and readable, but a comment would do the same without a possible performance cut. On the otherhand you have to be sure that the conditions are exactly the same when leaving it out, in this case it's easy, but it can lead to bugs, especially if a new else if is introduced later. \$\endgroup\$ – HopefullyHelpful Mar 7 '17 at 9:44
  • 1
    \$\begingroup\$ I fabricated the ID numbers 8901165050080 and it checks OK with both our codes \$\endgroup\$ – rolfl Mar 7 '17 at 12:58
  • 1
    \$\begingroup\$ @EastXWest - no problem. Note that I have added a section to my answer as well. Your process can be simplified further. My brother is like that too, and it can be useful... sometimes. Bliksem \$\endgroup\$ – rolfl Mar 7 '17 at 13:22
10
\$\begingroup\$

In addition to rolfl's answer about the implementation itself, let's look at the signature of the method:

public Boolean getIdentitySummation(BigInteger identities)

This declares a method taking a BigInteger and returning a Boolean. This raises comments:

  • Can the South African ID truly be considered an integer, or is it really a more of a String that happens to only be made of digits? This thing probably cannot really change so it should be fine for South Africa, but some countries, like Spain, have characters in it, which you wouldn't be able to represent with a BigInteger. Note that the rest of the method treats it like a String anyway, so perhaps taking a String to begin with is simpler, both for the method and the code using it.
  • Returning Boolean says that null could be a valid outcome. And what would it mean for this method to return null? In fact, the implementation never returns that. While you could document in Javadoc, and say that null will never be returned, prefer to force it in the code by returning a boolean. This saves the headache of the caller trying to determine if they really should check for null (because the documentation may be out of date, or because they're just paranoid).
  • The method is named getIdentitySummation, which implies that it returns some sort of identity summation. But it is not the case, it checks the validity of the given identification number, and returns whether it is valid or not. Sure, the process of doing that requires checking the digits by performing a summation, but the caller doesn't need to know; they just want to know whether the ID is valid or not.

With all this, I find a better signature would be

public boolean isIdentificationValid(String identification)
\$\endgroup\$
  • \$\begingroup\$ Great points about the signature, especially the name. \$\endgroup\$ – rolfl Mar 6 '17 at 17:55
  • \$\begingroup\$ Hi Tunaki, I appreciate the answer and thank you for the response, however the Luhn algorithm is only just a part of checking whether an ID is valid of not. There are other tests, for one there is a rule about the the digits (7 - 10), which determine gender. And the 11th digit determines nationality, south african born = 0, naturalised = 1. \$\endgroup\$ – EastXWest Mar 6 '17 at 18:08
  • \$\begingroup\$ I have other classes that do this. And they take the same ID_Number. I hope you understand. And thank you again for helping me \$\endgroup\$ – EastXWest Mar 6 '17 at 18:10
  • 4
    \$\begingroup\$ Also consider making it a static function. \$\endgroup\$ – 200_success Mar 6 '17 at 20:14

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.