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;
 }
}
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Sep 11, 2012 at 10:30
\$\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().

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
answered Sep 11, 2012 at 11:21
\$\endgroup\$
2
  • \$\begingroup\$ btw, I made length as a String for removing the whitespaces. Can you give me an alternative? \$\endgroup\$ Commented 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\$ Commented Sep 11, 2012 at 11:40

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.