2
\$\begingroup\$

How can I make this more readable, faster, and make the GUI more appealing?

CreditCard class:

import java.util.*;
public class CreditCards {

    private String prefix;
    private String length;

    public CreditCards(String p, String l) {

        prefix = p;
        length = l;

    }

    public String getPrefix() {
        return prefix;
    }

    public String getLength() {
        return length;
    }
}

MasterCard class:

public class MasterCard extends CreditCards {

    public MasterCard(String prefix,String length) {
        super(prefix,length);
    }

    public boolean isMaster() {
        boolean pre = (getPrefix().equals("55") || getPrefix().equals("51"))? true:false;
        boolean len = getLength().equals("16")? true:false;

        if(pre && len) {
            return true;
        }
        else {
            return false;
        }
    }
}

VisaCard class:

public class VisaCard extends CreditCards {

    public VisaCard(String pre,String len) {
        super(pre,len);
    }

    public boolean isVisa() {
        boolean pre = getPrefix().equals("4") ? true:false;
        boolean len = (getLength().equals("16") || getLength().equals("13"))? true:false;

        if(pre && len) {
            return true;
        }
        return false;
    }
}

GUI:

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

public class cardValidatorGUI{
    JFrame frame;
    JTextField cardNumField;
    JComboBox creditCardBox;
    JButton checkButton;
    JButton clearButton;
    String[] cards = {"Visa","Mastercard"};
    String selected = null;

    public static void main(String[] args) {

        cardValidatorGUI r1 = new cardValidatorGUI();
        r1.go();
    }

    public void go() {
        frame = new JFrame();
        JPanel cardPanel = new JPanel();
        JPanel buttonPanel = new JPanel();
        cardPanel.setLayout(new GridLayout(0,2));
        buttonPanel.setLayout(new GridLayout(0,2));

        creditCardBox = new JComboBox(cards);

        cardNumField = new JTextField("",20);

        checkButton = new JButton("check");
        checkButton.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent ae) {
                String selected = creditCardBox.getSelectedItem().toString();
                if (checkCriteria(selected)){
                    if (checkSum(cardNumField.getText())) {
                        JOptionPane.showMessageDialog(null, "Your Credit Card is Valid");
                    }
                    else {
                        JOptionPane.showMessageDialog(null, "Your Credit Card is Not Valid");

                    }
                }
                else {
                    JOptionPane.showMessageDialog(null, "Your Credit Card is Not Valid");
                }
            }
        });

        clearButton = new JButton("clear");

        cardPanel.add(creditCardBox);
        cardPanel.add(cardNumField);

        buttonPanel.add(checkButton);
        buttonPanel.add(clearButton);

        frame.getContentPane().add(BorderLayout.CENTER,cardPanel);
        frame.getContentPane().add(BorderLayout.SOUTH,buttonPanel);
        frame.setSize(250, 200);
        frame.setVisible(true);
        frame.pack();
        frame.setDefaultCloseOperation(frame.EXIT_ON_CLOSE);
    }

    public static boolean checkSum(String check){
        int sum = 0;
        String trimCheck = check.replaceAll(" ", "");
        String reverse = new StringBuffer(trimCheck).reverse().toString();
        for(int i =0;i<trimCheck.length();i++) {
            int checkThis = Character.digit(reverse.charAt(i), 10);

            if(i%2==0) {
                sum+=checkThis;
            }
            else {
                sum += checkThis *2;
                if(checkThis >=5) {
                    sum-=9;
                }
            }
        }
        if(sum%10==0) {
            return true;
        }
        else {
            return false;
        }
    }

    public boolean checkCriteria(String c) {
        int length = cardNumField.getText().replaceAll(" ","").length();
        String len = Integer.toString(length);

        if (c.equals("Mastercard")) {
            String pre = cardNumField.getText().substring(0,2);
            MasterCard masters = new MasterCard(pre,len);

            if(masters.isMaster()) {
                return true;
            }
        }

        if (c.equals("Visa")) {
            String pre = cardNumField.getText().substring(0,1);
            VisaCard visa = new VisaCard(pre,len);

            if (visa.isVisa()) {
                return true;
            }
        }

        return false;
    }
}
\$\endgroup\$

1 Answer 1

7
\$\begingroup\$
  1. Construction like

    boolean pre = getPrefix().equals("4") ? true:false;
    

    is redundant. The following is ok:

    boolean pre = getPrefix().equals("4");
    

    just as

    if(pre && len) {
        return true;
    }
    return false;
    

    this could be just:

    return pre && len;
    
  2. Try to avoid 'magic numbers' like 55 or 4. Extract them as constants and give them a proper name.

  3. I don't think that you need to operate with length as a String.

  4. Use error dialog for invalid credit card number:

    JOptionPane.showMessageDialog(frame, "Your Credit Card is Valid", "Error", JOptionPane.ERROR_MESSAGE);
    

    and also use a parent frame as an argument, not null.

  5. I'd suggest to remove methods isViza() and isMaster() and add abstract isValid() method to CreditCard class and override it in both subclasses.

  6. Text on your buttons should start with the capital letter.

  7. Avoid method names like go(). You could name it like initUI().

\$\endgroup\$
2
  • \$\begingroup\$ btw, I made length as a String for removing the whitespaces. Can you give me an alternative? \$\endgroup\$ Sep 11, 2012 at 11:31
  • \$\begingroup\$ just try to parse it before using in other methods. By the way, you can restrict user to input letters - only digits. Here is the sample code for custom JTextField: devx.com/tips/Tip/14311 \$\endgroup\$ Sep 11, 2012 at 11:40

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.