4
\$\begingroup\$

This program is used to create a random bank card number. Card number is created using account type and client location for the first eight digits and the remaining 8 digits are completely random. I'm new to this site, but I believe it's just for code review to get opinions on code. If i'm incorrect please let me know and I can take this down. Otherwise, feel free to give me feedback. Thanks!

import java.util.ArrayList;
import java.util.Scanner;

public class Main {

    private static ArrayList<String> storedCardNumbers = new ArrayList<String>();

    public static void main(String[] args) {

        menu();

    }

    //create menu for checking list of cards or creating a new card
    public static void menu(){
        //used to check if client wants to exit program
        boolean exit = false;

        System.out.println("___________________\n" +
                "Welcome to our bank!\n" + "Choose an option below: \n" +
                "1. Create new bank card.\n" +
                "2. Check existing cards.\n" +
                "3. Exit.");
        Scanner scanner = new Scanner(System.in);
        int choice = scanner.nextInt();

        if(choice == 1) {
            createNewCard();
        } else if(choice == 2){
            System.out.println("-------------------------");
            System.out.println("There are " + storedCardNumbers.size() + " cards in the system");
            for (int i = 0; i < storedCardNumbers.size(); i++) {
                System.out.println(storedCardNumbers.get(i).toString());
            }
            System.out.println("-------------------------");
        } else if(choice == 3) {
            System.out.println("Thank you for coming in!");
            exit = true;
        } else {
                System.out.println("Incorrect choice.\n" +
                        "Please choose a valid option: ");
                menu();
            }

        if(exit == false) {
            System.out.println("Would you like to do anything else?");
            menu();
        } else {

        }
        }


    //create new bank card with user params
    public static String createNewCard() {
//        String accountType = setCardAccountType();
//        String location = setAccountLocation();
        String cardNumber = getCardNumber(setCardAccountType(),setAccountLocation());
        System.out.println("New card successfully created.\n" +
                "Your new bank card number is: \n" +
                cardNumber);
        storedCardNumbers.add(cardNumber);
        return cardNumber;
        }

        public static String setAccountLocation() {

            String location = "other";

            System.out.println("What is your location?\n" +
                    "1. US East\n" +
                    "2. US West\n" +
                    "3. Hawaii\n" +
                    "4. Alaska\n" +
                    "5. Other.");
            Scanner scanner = new Scanner(System.in);
            int choice = scanner.nextInt();

            if (choice == 1) {
                location.equalsIgnoreCase("useast");
            } else if (choice == 2) {
                location.equalsIgnoreCase("uswest");
            } else if (choice == 3) {
                location.equalsIgnoreCase("alaska");
            } else if (choice == 4) {
                location.equalsIgnoreCase("hawaii");
            } else if(choice == 5) {
                location.equalsIgnoreCase("other");
            }else {
                System.out.println("Entered invalid selection.\n" +
                        "Please enter valid selection.");
                setCardAccountType();
            }

            return location;
        }

        //set the card account type for the creatNewCard method to use
        public static String setCardAccountType() {

            String accountType = "other";

            System.out.println("Which type of account do you have?\n" +
                    "1. Checking.\n" +
                    "2. Savings.\n" +
                    "3. Credit.\n" +
                    "4. Other.\n" +
                    "Enter corresponding digit: ");
            Scanner scanner = new Scanner(System.in);
            int choice = scanner.nextInt();

            if (choice == 1) {
                accountType.equalsIgnoreCase("checking");
            } else if (choice == 2) {
                accountType.equalsIgnoreCase("savings");
            } else if (choice == 3) {
                accountType.equalsIgnoreCase("credit");
            } else if (choice == 4) {
                accountType.equalsIgnoreCase("other");
            } else {
                System.out.println("Entered invalid selection.\n" +
                        "Please enter valid selection.");
                setCardAccountType();
            }

            return accountType;
        }

    //create random number for card
    public static int getRandom(int min, int max) {
        return (int) (Math.random()*(max - min)) + min;
    }

    public static String getCardNumber(String accountType, String location) {
        //first set of four
        int accountKeySet;
        //second set of four
        int locationKeySet;
        //third set of four
        int lastEightFirst;
        //fourth set of four
        int lastEightSecond;

        //assign a value to each account type
        if(accountType.equalsIgnoreCase("checking")) {
            accountKeySet = 4400;
        } else if(accountType.equalsIgnoreCase("savings")) {
            accountKeySet = 4300;
        } else if(accountType.equalsIgnoreCase("credit")) {
            accountKeySet = 4200;
        } else {
            accountKeySet = 4000;
        }


        //Assign a value to each location of client
        if(location.equalsIgnoreCase("useast")) {
            locationKeySet = 1011;
        } else if(location.equalsIgnoreCase("uswest")) {
            locationKeySet = 2011;
        } else if (location.equalsIgnoreCase("Alaska")) {
            locationKeySet = 3011;
        } else if (location.equalsIgnoreCase("hawaii")) {
            locationKeySet = 4011;
        } else {
            locationKeySet = 1022;
        }

        //create a randomized number for last of card number
        lastEightFirst = getRandom(1000,9999);
        lastEightSecond = getRandom(1000,9999);

        //construct card number
        StringBuilder key = new StringBuilder();
        key.append(accountKeySet);
        key.append(" ");
        key.append(locationKeySet);
        key.append(" ");
        key.append(lastEightFirst);
        key.append(" ");
        key.append(lastEightSecond);
        return key.toString();
    }
}
\$\endgroup\$
4
\$\begingroup\$

Hi Nicholas and welcome to code review.

A few things that I noticed in your code:

  • The indentation isn't consistent. The closing bracket of menu isn't where it should be. The whole setAccountLocation method is one tab to far right. The readability of the code would be better if you'd fix that. In Eclipse, you can press Ctrl + Shift + F to automatically fix the indentation

  • In my experience, opening multiple Scanners yields problems. I'd recommend to open only one Scanner in your main method and then use it in your other methods

  • In setAccountLocation, you use location.equalsIgnoreCase several times. That does nothing. I think you wanted to assign the strings to the location variable, so you can simply write location = "useast"; etc.

  • Same for the account types in setAccountType.

  • accountType and location are never entered by a user, so you don't need equalsIgnoreCase, but you can simply use equals.

  • I'd recommend to not pass accountType and location as strings, but to create enums for them.

  • in menu, you have the line System.out.println(storedCardNumbers.get(i).toString());. You don't need the toString, because it already is a string. Also the println method implicitly calls the toString method of every Object that you pass.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Awesome thanks for the response! Yea the indentation is fine on intellij so I think it was just when it was copied over. You review was very helpful thank you! \$\endgroup\$ – Nicholas Whitley Oct 24 at 3:05
  • \$\begingroup\$ You're welcome. There's one more thing that I saw, but forgot to mention. Whenever the user entered an option in the main menu, you call menu recursive. That may not become a problem in a small program like this, but in bigger projects you may run out of memory. Also you have to enter 3 for exit multiple times if you chose an incorrect option. You could use a while (!exit) loop instead. (PS: Maybe a silly question, but why did you accept the least helpful answer?) \$\endgroup\$ – Dorian Oct 26 at 8:45
  • \$\begingroup\$ It's been a long time since I've been on here so I really forgot how it works and forgot that you accept only one answer. I accepted yours now. Thanks for the help. \$\endgroup\$ – Nicholas Whitley Oct 27 at 9:17
3
\$\begingroup\$

Always use the base class / interface in the left part of the variable when possible

By setting the java.util.List interface in the variable part, this could make the code more refactorable in the future, since you could easily change the list type without changing everything (inheritance).

Before

private static ArrayList<String> storedCardNumbers = new ArrayList<String>();

After

private static List<String> storedCardNumbers = new ArrayList<String>();

Replace the for loop with an enhanced 'for' loop

In your code, you don’t actually need the index provided by the loop, you can the enhanced version.

Before

for (int i = 0; i < storedCardNumbers.size(); i++) {
   System.out.println(storedCardNumbers.get(i).toString());
}

After

for (String storedCardNumber : storedCardNumbers) {
   System.out.println(storedCardNumber);
}

Also, you can remove the .toString() since it's already a String.

Remove the dead code

In your code, you have multiple places where the code is dead (does nothing) and can be removed.

if (choice == 1) {
   location.equalsIgnoreCase("useast");
} else if (choice == 2) {
   location.equalsIgnoreCase("uswest");
} else if (choice == 3) {
   location.equalsIgnoreCase("alaska");
} else if (choice == 4) {
   location.equalsIgnoreCase("hawaii");
} else if (choice == 5) {
   location.equalsIgnoreCase("other");
} else {
   System.out.println("Entered invalid selection.\n" +
                      "Please enter valid selection.");
   setCardAccountType();
}

For an example, the location.equalsIgnoreCase(...) is dead code in all cases, even if the condition is met (choice >= 1 5 && choice <= 5), the code won’t do anything; you have two similar case like this one.

Simplify the boolean conditions.

Instead of having exit == false, you can simplify to !exit; also I suggest that you remove the empty else under this condition.

Generating random values

This is only a suggestion, but you can also use the java.util.concurrent.ThreadLocalRandom#nextInt method to generate a random number in a defined range (inclusive in this case) with java 7+.

ThreadLocalRandom.current().nextInt(0, 100);

You can read an excellent post on SO

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

This program is used to create a random bank card number. Card number is created using account type and client location for the first eight digits and the remaining 8 digits are completely random.

This isn't a critique of the coding per se, but of the requirements gathering. Bank cards do not allow for a fully random last eight characters. Because the sixteenth character is a checksum. So you should generate seven random digits, not eight. Then calculate the last digit from the first fifteen.

| improve this answer | |
\$\endgroup\$

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.