Simple ATM program with various options

Question

This is not much of a question as more of an practice/experience project and feedback request. First off, this was a project I worked on when I started getting into Java last summer and had an assignment that required us to write a simple ATM application. As I'm looking at the code a year later, and gained a bit more experience, I see where it could have been improved. I'm sure there are going to be quite a bit of suggestions and feedback, but to start it off I'm looking the do while loop in the main program, and feel that the logic can be simplified such that the first or even second if statements can place outside of the loop as I feel that using all 3 can be redundant. Perhaps incorporating a boolean true/false statement.

import java.util.*;

public class ATM {

    public static Scanner kbd = new Scanner(System.in);
    // The checkID method determines if acctNum is a valid account number
    // and pwd is the correct password for the account.  If the account information
    // is valid, the method returns the current account balance, as a string.
    // If the account information is invalid, the method returns the string "error".
    public static String checkID(String acctNum, String pwd)
    {
        String result = "error";

        // Strings a, b, and c contain the valid account numbers and passwords.
        // For each string, the account number is listed first, followed by
        // a space, followed by the password for the account, followed by a space,
        // followed by the current balance.
        String a = "44567-5 mypassword 520.36";
        String b = "1234567-6 anotherpassword 48.20";
        String c = "4321-0 betterpassword 96.74";

        if (acctNum.equals(a.substring(0, a.indexOf(" "))) && 
                pwd.equals(a.substring(a.indexOf(" ")+1,a.lastIndexOf(" "))))
            return result = a.substring(a.lastIndexOf(" ") + 1);

        if (acctNum.equals(b.substring(0, b.indexOf(" "))) && 
                pwd.equals(b.substring(b.indexOf(" ")+1,b.lastIndexOf(" "))))
            return result = b.substring(b.lastIndexOf(" ") + 1);

        if (acctNum.equals(c.substring(0, c.indexOf(" "))) && 
                pwd.equals(c.substring(c.indexOf(" ") + 1,c.lastIndexOf(" "))))
            return result = c.substring(c.lastIndexOf(" ") + 1);

        return result;
    }

    public static int menu()
    {
        int menuChoice;
        do
        { 
            System.out.print("\nPlease Choose From the Following Options:"
                    + "\n 1. Display Balance \n 2. Deposit"
                    + "\n 3. Withdraw\n 4. Log Out\n\n");

            menuChoice = kbd.nextInt();

            if (menuChoice < 1 || menuChoice > 4){
                System.out.println("error");
            }

        }while (menuChoice < 1 || menuChoice > 4);

        return menuChoice;
    }

    public static void displayBalance(double x)
    {
        System.out.printf("\nYour Current Balance is $%.2f\n", x);
    }

    public static double deposit(double x, double y)
    {
        double depositAmt = y, currentBal = x;
        double newBalance = depositAmt + currentBal;

        System.out.printf("Your New Balance is $%.2f\n",  newBalance);

        return newBalance;
    }

    public static double withdraw(double x, double y)
    {
        double withdrawAmt = y, currentBal = x, newBalance;

        newBalance = currentBal - withdrawAmt;
        System.out.printf("Your New Balance is %.2f\n",newBalance);

        return newBalance;  
    }

    public static void main(String[] args) {

        String accNum, pass, origBal = "error";
        int count = 0, menuOption = 0;
        double depositAmt = 0, withdrawAmt = 0, currentBal=0; 
        boolean  foundNonDigit;
        //loop that will count the number of login attempts
        //you make and will exit program if it is more than 3.
        //as long as oriBal equals an error.  
        do{
            foundNonDigit = false;
            System.out.println("Please Enter Your Account Number: ");
            accNum = kbd.next();

            System.out.println("Enter Your Password: ");
            pass = kbd.next();

            origBal = checkID(accNum, pass);

            count++;

            if (count >= 3 && origBal.equals("error")){
                System.out.print("Maximum Login Attempts Reached.");
                System.exit(0);
            }
            if (!(origBal.equals("error"))){
                System.out.println("\nYour New Balance is: $ "+ origBal);
            }
            else
                System.out.println(origBal);


        }while(origBal.equals("error"));

        currentBal=Double.parseDouble(origBal);
        //this loop will keep track of the options that 
        //the user inputs in for the menu. and will 
        //give the option of deposit, withdraw, or logout.

        while (menuOption != 4)
        { 
            menuOption=menu();
            switch (menuOption)
            {
            case 1:
                displayBalance(currentBal);
                break;
            case 2:
                System.out.print("\nEnter Amount You Wish to Deposit: $ ");
                depositAmt = kbd.nextDouble();
                currentBal=deposit(depositAmt, currentBal);
                break;
            case 3:
                System.out.print("\nEnter Amount You Wish to Withdrawl: $ ");
                withdrawAmt = kbd.nextDouble();

                while(withdrawAmt>currentBal){
                    System.out.print("ERROR: INSUFFICIENT FUNDS!! "
                            + "PLEASE ENTER A DIFFERENT AMOUNT: $");
                    withdrawAmt = kbd.nextDouble();
                }

                currentBal = withdraw(currentBal, withdrawAmt);
                break;
            case 4:
                System.out.print("\nThank For Using My ATM.  Have a Nice Day.  Good-Bye!");
                System.exit(0);
                break;
            }
        }
    }
}

Output:

Please Enter Your Account Number: 
44567-5
Enter Your Password: 
mypassword

Your New Balance is: $ 520.36

Please Choose From the Following Options:
 1. Display Balance 
 2. Deposit
 3. Withdraw
 4. Log Out

2

Enter Amount You Wish to Deposit: $ 400
Your New Balance is $920.36

Please Choose From the Following Options:
 1. Display Balance 
 2. Deposit
 3. Withdraw
 4. Log Out

3

Enter Amount You Wish to Withdrawl: $ 1000
ERROR: INSUFFICIENT FUNDS!! PLEASE ENTER A DIFFERENT AMOUNT: $600
Your New Balance is 320.36

Please Choose From the Following Options:
 1. Display Balance 
 2. Deposit
 3. Withdraw
 4. Log Out

1

Your Current Balance is $320.36

Please Choose From the Following Options:
 1. Display Balance 
 2. Deposit
 3. Withdraw
 4. Log Out

4

Thank For Using My ATM.  Have a Nice Day.  Good-Bye!

Answer 1

Storing passwords

The recommended approach for reading passwords is to use a char[] array, instead of a String. The main reason for this, as explained in this SO answer, is that immutable Strings leave your passwords accessible until garbage collection (GC) kicks in, with a malicious process performing a memory dump of your Java process to do so.

Helpfully, Console.readPassword() returns a char[] array while hiding the user input on the console:

char[] password = System.console().readPassword();
// validate password
// replace the char[] array contents immediately after validation
Arrays.fill(password, ' ');

Reading data, and data modeling

Currently, you are hardcoding the account details as mere space-delimited Strings. This is not desirable from a data modeling perspective as it is non-trivial to:

• 'Upgrade' the formatting to support newer or different fields,
• Make edits, and
• Test.

These 'pain points' are evident in the way you are doing validation currently:

if (acctNum.equals(a.substring(0, a.indexOf(" "))) && 
        pwd.equals(a.substring(a.indexOf(" ")+1,a.lastIndexOf(" "))))
    // BTW looks like you have a typo/bug here
    // return result = a.substring(a.lastIndexOf(" ") + 1);
    result = a.substring(a.lastIndexOf(" ") + 1);

• You have to manually identify where each column starts and ends.
• The code assumes the account number is in the first field, the password is the second, and the result is in third and final field.
• (related to the earlier section, and a big security no-no) Your passwords are in clear text, instead of being encrypted (or at least hashed).
• You have hardcorded three accounts into three different variables, which is not scalable.

So how can this be done differently?

Let's start with a class called Account:

public class Account {
    String name;
    String encryptedPassword;
    double balance;

    public class Account(String name, String encryptedPassword, double balance) {
        this.name = Objects.requireNonNull(name);
        this.encryptedPassword = Objects.requireNonNull(encryptedPassword);
        this.balance = balance;
    }

    // create getters and setters too

    public boolean isMatching(String name, String encryptedPassword) {
        return Object.equals(this.name, name) 
                    && Objects.equals(this.encryptedPassword, encryptedPassword);
    }
}

With a class-based design, adding new fields or changing their data types is a lot easier. Now, we need a mechanism that knows how to interact with multiple accounts, and perform validation on picking an account. An AccountManager sounds like one for the job:

public class AccountManager {
    // using a List here for simplicity
    private final List<Account> accounts = getAccounts();

    // no modifiers used as an illustration to show how testing can override this
    List<Account> getAccounts() {
        // read from a source, e.g. database, and return as a List of Account objects
    }

    public Account getAccount(String accountName, char[] password) {
        String encryptedInput = encryptPassword(password);
        Arrays.fill(password, ' ');
        // simple implementation that loops through the accounts
        for (Account account : accounts) {
            if (account.isMatching(accountName, encryptedInput)) {
                return account;
            }
        }
        return null;
    }
}

The use of a List lets us store one, three, 10, or more accounts easily without having to deal with a multitude of variables. We also need to encrypt the password first, then rely on the Account.isMatching() method that lets us easily identify which account matches on the inputs. In this case, we no longer have to worry how to read space-delimited Strings in order to identify the account name and password parts correctly.

In order to help with the testing, you can create a TestAccountManager that override getAccounts() to use some test data:

// assuming same package
public class TestAccountManager extends AccountManager {

    @Override
    List<Account> getAccounts() {
        return Arrays.asList(new Account("acct1", "...", 1),
                                new Account("acct2", "...", 2),
                                new Account("acct3", "...", 3));
    }

    // ...

}

Variable names

public static double deposit(double x, double y)
{
    double depositAmt = y, currentBal = x;
    // ...
}

public static double withdraw(double x, double y)
{
    double withdrawAmt = y, currentBal = x, newBalance;
    // ...
}

I thought it is... mildly amusing that you know what the appropriate variable names should used as the method parameters, and yet you stuck with x and y only to perform the assignment afterwards. They should be rewritten as such:

public static double deposit(double currentBalance, double depositAmount)
{
    // ...
}

public static double withdraw(double newBalance, double withdrawalAmount)
{
    // ...
}

Reducing code duplication

You have very similar ways of printing the balances in the displayBalance(), deposit() and withdrawal() methods, differing only in the output. This ties in with the use of a suitable model class as explained in the earlier section. For example:

public class Account {

    // ...

    public boolean deposit(double amount) {
        // ...
    }

    public boolean withdrawal(double amount) {
        // ...
    }

    @Override
    public String toString() {
        return toString("Current");
    }

    public String toString(String description) {
        return String.format("Your %s Balance is $%.2f", description, balance);
    }
}

With these methods, it simplifies how money is deposited or withdrawn from an Account, and how the balance can be shown:

// show current balance
System.out.println(account);
// do a deposit
boolean isMoneyDeposited = account.deposit(100);
if (isMoneyDeposited) {
    System.out.println(account.toString("New"));
}

try-with-resources

If you are on Java 7 and above, you should use try-with-resources for efficient handling of the underlying I/O resource:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        // ...
    }
}

Consistent bracing style

If you are more comfortable with orphan/standalone braces on each line, then do so consistently to improve the code readability. :) Your class declaration and the main() method deviate from this, and some of your if-else statements are also missing braces.