Simple ATM program with various options

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.s

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 accounts = getAccounts();

    // no modifiers used as an illustration to show how testing can override this
    List 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 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)
{

StackExchange Code Review Q#109831, answer score: 6