6
\$\begingroup\$

I have created a program to validate a Canadian S.I.N. using Luhn's Algorithm, and was wondering if there was any way to make it more efficient. From what i can tell, it is already extremely efficient, but any tips or advice will be greatly appreciated.

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;


class SinChecker extends JFrame
{ //naming variables
JTextField t1;
private JLabel j, j1, j2, j3;
ButtonListener bl1;
ButtonListener2 bl2;

public SinChecker ()
{ //Get the container
Container c = getContentPane ();

//Set absolute layout
c.setLayout (null);

//Set Background Color
c.setBackground (Color.WHITE);


//Creating label Guess my number text
JLabel j = new JLabel ("S.I.N. Validation Checker");
j.setForeground (Color.BLUE);
j.setFont (new Font ("tunga", Font.BOLD, 24));
j.setSize (270, 20);
j.setLocation (30, 35);

//Creating label Enter a number.....
j1 = new JLabel ("Enter your S.I.N. below.");
j1.setFont (new Font ("tunga", Font.PLAIN, 17));
j1.setSize (270, 20);
j1.setLocation (66, 60);

//Creating a label Instuctions
JLabel j2 = new JLabel ("Enter a 9-digit Social Insurance Number");
j2.setFont (new Font ("tunga", Font.PLAIN, 17));
j2.setSize (270, 20);
j2.setLocation (10, 165);

//Creating a label Instuctions
JLabel j3 = new JLabel ("with no spaces between the digits please.");
j3.setFont (new Font ("tunga", Font.PLAIN, 17));
j3.setSize (270, 20);
j3.setLocation (10, 180);



//Creating TextField for x input guess
t1 = new JTextField (10);
t1.setSize (70, 30);
t1.setLocation (100, 80);

//creating 2 buttons
JButton b1 = new JButton ("Proceed");
b1.setSize (120, 30);
b1.setLocation (70, 200);
bl1 = new ButtonListener ();
b1.addActionListener (bl1);


JButton b2 = new JButton ("Re-enter");
b2.setSize (120, 30);
b2.setLocation (70, 250);
bl2 = new ButtonListener2 ();
b2.addActionListener (bl2);

//Place the components in the pane
c.add (j);
c.add (j1);
c.add (j2);
c.add (j3);

c.add (t1);
c.add (b1);
c.add (b2);

//Set the title of the window
setTitle ("Social Insurance Number Checker");

//Set the size of the window and display it
setSize (300, 350);
setVisible (true);
setDefaultCloseOperation (EXIT_ON_CLOSE);
}


//implement  first action listener
private class ButtonListener implements ActionListener
{

public void actionPerformed (ActionEvent e)
{
    int a = Integer.parseInt (t1.getText ());
    boolean evenDigit = false;   //alternates between true and false
    int sum = 0; //accumulates the sum of the digits (as modified)

    while (a > 0)
    {
        int nextDigit = a % 10; //grab the last digit
        a = a / 10; //discard that digit
        if (evenDigit)
        {
            //double it, then add the two digits of the result
            nextDigit = 2 * nextDigit;
            nextDigit = (nextDigit / 10) + (nextDigit % 10);
        } // if(evenDigit)
        sum = sum + nextDigit;
        evenDigit = !evenDigit; //toggle the flag each time
    } // end while

    if (0 == sum % 10)
    {
        j1.setText ("That is a valid S.I.N.");
    }
    else
    {
        j1.setText ("That is not a valid S.I.N.");

    }
    t1.requestFocus ();
    t1.selectAll ();
}
   }



 private class ButtonListener2 implements ActionListener
 {
   public void actionPerformed (ActionEvent e)
   {
    //reset the label messages
    t1.setText ("");
    j1.setText ("Enter your S.I.N. below.");

    t1.requestFocus ();
    t1.selectAll ();



}
}


public static void main (String[] args)
{
  new SinChecker ();
 } }
\$\endgroup\$
4
\$\begingroup\$

User interface

For portability, you should use a layout manager, not absolutely positioned elements. Here's what it looks like on OS X:

Screenshot on OS X, with truncated words and ellipses

I don't see a reason for two sets of instructions; the instructional text should be placed together.

"Validation Checker" is redundant. It's either a "SIN validator" or "SIN checker".

"Valid SIN" could be construed as meaning a number that has been actually been issued by Service Canada to a person. Perhaps you should clarify that it's merely a "possibly valid SIN".

Why prohibit spaces? Canadian Social Insurance Numbers are conventionally rendered as three groups of three digits, with spaces or hyphens between groups. Your program could easily be made more lenient and human-friendly, accepting and ignoring the superfluous punctuation.

To facilitate data entry, you should allow number keypad-only usage and perform the validation when Enter is pressed. (Currently, you have to press Tab and Space, or click Proceed.) For that matter, you could automatically perform the validation as soon as nine digits have been entered, without even requiring Enter.

Automatically returning focus to the number field and selecting its contents after validating was a nice touch.

Validation quality

"0" is accepted as a valid SIN.

"-9" is accepted as a valid SIN.

"123456789012345" or "hello" cause a NumberFormatException which is invisible in the UI. It just looks like either a valid SIN or an invalid SIN, depending on whether the previous validation passed or failed.

Implementation

You should implement the Luhn algorithm check in its own function (if not its own class) rather than in the event handler. That lets you reuse the code in other contexts such as a text-based UI, a tax-preparation program, or a unit test.

These lines of code are not as clear as they could be:

bl1 = new ButtonListener ();
b1.addActionListener (bl1);

Rather, I would prefer to see the button and text field identified with descriptive variable names. The ActionListener should be simplified and could be made into an anonymous inner class:

proceedButton.addActionListener(new ActionListener() {
    public void actionPerformed(ActionEvent e) {
        boolean valid = SinChecker.isValid(sinTextField.getText());
        SinChecker.this.announceValid(valid);
        sinTextField.requestFocus();
        sinTextField.selectAll();
    }
});

Since your SinChecker class extends JFrame, I would expect its constructor to behave a specialized JFrame. In particular, a JFrame constructor would not make the newly instantiated frame visible, nor would it set exit-on-close. Therefore, your SinChecker constructor shouldn't do those things, either. If your SinChecker is incorporated as a component within some larger program, exit-on-close might not be appropriate.

Speaking of reusability, consider making your SinChecker a JPanel instead of a JFrame, so that you have the flexibility to use it as a widget that is not its own window. In that case, your widget should offer a getSin() method that returns a guaranteed valid SIN.

In accordance with the documentation, Swing code must run in the Event Dispatch Thread, not the main thread.

public class SinChecker extends JPanel {
    public SinChecker() {
        this.setBackground(Color.WHITE);
        …
        this.setPreferredSize(…);  // Ideally automatically determined by layout manager
    }

    …

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                JFrame f = new JFrame("Social Insurance Number Checker");
                f.setLayout(new BorderLayout());
                f.add(new SinChecker());
                f.pack();
                f.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                f.setVisible(true);
            }
        });
    }
}
\$\endgroup\$
3
\$\begingroup\$

@200_success already covered the important points, so I'll focus on readability and style of your code.

Comments

Most of your comments are not all that helpful because they just describe the code. Eg implement first action listener is not needed, ButtonListener implements ActionListener already tells me that. The same goes for Set the title of the window, naming variables, etc.

Your comments should explain why you do something, not what you do. If you feel that you need these comments to separate blocks of code (eg without it, the SinChecker constructor would be one big block of unreadable code), extract some code to a function with a proper name. You can then also add a JavaDoc method comment which can describe in detail what the method does.

Formatting

Your formatting doesn't follow general Java conventions. It's always good to follow conventions, so that people used to Java have an easier time reading your code.

  • use proper indentation
  • use proper spacing (no space before ()).
  • curly brackets go on the same line as the opening statement.
  • it's customary to write sum += nextDigit instead of sum = sum + nextDigit.

The first three points can easily be fixed with any IDE such as NetBeans or Eclipse.

Naming

  • avoid short variable names. One character names are only acceptable in a very limited number of cases, eg as loop variables. a for example is not very clear. sin, insuranceNumber, or even just number would be better.
  • numbered variable names are also almost always bad. Eg j, j1, j2, j3 don't tell me anything about themselves, and neither do bl1 and bl2.

Misc

  • don't import *, make all imports explicit (this increases readability and possibly avoids naming conflicts).
  • make your fields private.
  • always add @Override to increase readability (and possibly avoid bugs, eg a typo and thus declaring a new method instead of implementing an existing one).
\$\endgroup\$
1
\$\begingroup\$

This is for Java Bean Validation but here are my key concepts:

  • For even digits, I look it up from a map rather than do the calculation.
  • I only split once and use it rather than getting the index using toCharArray()
  • I convert to an integer by subtracting by '0'
  • I prevalidate that it is all numbers using regex.

Here's one pass

public class CanadianSinValidator implements
    ConstraintValidator<CanadianSin, String> {

    private static final int[] MAP = {
        0,
        2,
        4,
        6,
        8,
        1 + 0,
        1 + 2,
        1 + 4,
        1 + 6,
        1 + 8
    };

    private boolean stripSpacesAndSymbols;

    @Override
    public void initialize(final CanadianSin constraintAnnotation) {

        stripSpacesAndSymbols = constraintAnnotation.stripSpacesAndSymbols();
    }

    @Override
    public boolean isValid(final String value,
        final ConstraintValidatorContext context) {

        final String valueToProcess;
        if (stripSpacesAndSymbols) {
            valueToProcess = value.replaceAll("\\s|\\-", "");
        } else {
            valueToProcess = value;
        }
        if (!value.matches("\\d{9}")) {
            return false;
        }

        int di = 0;
        int checksum = 0;
        for (final char c : valueToProcess.toCharArray()) {
            final int digit = c - '0';
            if (di % 2 == 0) {
                checksum += digit;
            } else {
                checksum += MAP[digit];
            }
            ++di;
        }
        return checksum % 10 == 0;
    }

}

The problem with the approach above is the regex checks which means I am iterating the string more than once. I eliminated it with this new one in addition, I added the check for first digit is 8 which is not allowed according https://en.wikipedia.org/wiki/Social_Insurance_Number#Geography

@Override
public boolean isValid(final String value,
    final ConstraintValidatorContext context) {

    int di = 0;
    int checksum = 0;
    for (final char c : value.toCharArray()) {
        if (stripSpacesAndSymbols && (c == ' ' || c == '-')) {
            continue;
        }
        ++di;
        final int digit = c - '0';
        if (di > 9 || digit > 9 || di == 1 && digit == 8) {
            return false;
        } else if (di % 2 == 1) {
            checksum += digit;
        } else {
            checksum += MAP[digit];
        }
    }
    return di == 9 && checksum % 10 == 0;
}

I haven't done a micro benchmark but based on gut feel this should be more performant.

This can be improved a bit at the expense of duplicating code by having a separate code path if you assume no spaces or symbols in the input.

In addition you can use the following code to generate valid SIN numbers:

public static String generate() {

    final int firstDigit = new Random().nextInt(10);
    if (firstDigit == 8) {
        return generate(0);
    } else {
        return generate(firstDigit);
    }
}
public static String generate(final int firstDigit) {

    if (firstDigit < 0 || firstDigit > 9 || firstDigit == 8) {
        throw new IllegalArgumentException("Not a valid first digit for SIN: " + firstDigit);
    }
    int checksum = firstDigit;
    // Choose middle digits such that there are 7 sigits, but the first digit may not be 0 or 9
    // the reason the first digit cannot be 9 is to ensure there's room for a carry when the checksum mod 10 is 0
    final int middleDigits = new Random().nextInt(9999999 + 1 - 1000000 - 1000000) + 1000000;
    final int[] digits = new int[] {
        middleDigits / 1000000 % 10,
        middleDigits / 100000 % 10,
        middleDigits / 10000 % 10,
        middleDigits / 1000 % 10,
        middleDigits / 100 % 10,
        middleDigits / 10 % 10,
        middleDigits % 10
    };
    for (int i = 0; i < 7; ++i) {
        if (i % 2 == 1) {
            checksum += digits[i];
        } else {
            checksum += MAP[digits[i]];
        }
    }
    while (checksum % 10 == 0) {
        checksum = checksum - MAP[digits[0]] - digits[1];
        digits[0] = (digits[1] + 1) / 10;
        digits[1] = (digits[1] + 1) % 10;
        checksum = checksum + MAP[digits[0]] + digits[1];
    }
    final int checkDigit = 10 - checksum % 10;
    return String.format("%d%d%d%05d%d", firstDigit, digits[0], digits[1], middleDigits % 100000, checkDigit);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I doubt your regex. Shouldn't it be "\\s|\\-" (either space or hyphen separated), instead of what you've got (space followed by hyphen separated)? \$\endgroup\$ – Tamoghna Chowdhury May 8 '17 at 18:19
  • \$\begingroup\$ Right, I fixed it now. Though I dropped that code for the slightly more performant one. \$\endgroup\$ – Archimedes Trajano May 8 '17 at 18:21

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.