4
\$\begingroup\$

I am trying to improve my Java skill, so I wrote a simple ATM program.

I would like a review, to help me make the code more effective.

Main

public static void main(String[] args) {
    ATM a = new ATM("A", "S", 20000);
    Scanner in = new Scanner(System.in);
    while(true){
        System.out.println("Please choose: " + "\n" +
                "1. Deposit" + "\n" + "2. Withdraw" + "\n" + "3. Info" + "\n" + "4. Exit");
        int option = in.nextInt();
        switch (option) {
            case 1:
                System.out.println("How much would you like to Deposit?");
                double depositValue = in.nextDouble();
                a.deposit(depositValue);
                System.out.println("Your current balance is : " + a.getBalance() + "\n") ;
                break;

            case 2:
                System.out.println("How much would you like to Withdraw?");
                double withdrawValue = in.nextDouble();
                a.withdraw(withdrawValue);
                break;

            case 3:
                System.out.println(a.toString() + "\n" );
                break;

            case 4:
           System.exit(0);

        }
    }

}

ATM

private String name;
private String lastName;
private double balance;

public ATM(String name, String lastName, double balance) {
    this.balance = balance;
    this.lastName = lastName;
    this.name = name;
}

public String getName(){
    return this.name;
}

public String getLastName(){
    return this.lastName;
}

public double getBalance(){
    return this.balance;
}

public void setBalance(int newValue){
    this.balance = newValue;
}

public void deposit(double value){
    balance += value;
}

public void withdraw(double value){
    if(value > balance){
        System.out.println("You can't withdraw that amount of money." +"\n"+ "Your balance is: " + getBalance() + "\n");
    } else {
        balance -= value;
        System.out.println("You withdraw " + value +"from your account!" + "\n");
    }
}
public String toString(){
    return "First name: " + getName() +"\n" + "Last name: " + getLastName() + "\n" + "Your balance: " +getBalance();
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Are you missing one or more import statements? You'll probably get better reviews if you post the complete code of the program. \$\endgroup\$ Mar 7 at 17:13
  • 1
    \$\begingroup\$ What exactly do you mean by "more effective"? \$\endgroup\$ Mar 7 at 20:48
5
\$\begingroup\$

This is fairly good for a program in this category for being well formatted and having variable names that mostly make sense. I.e. I have no trouble figuring out what the code tries to do.

ATM a = new ATM("A", "S", 20000);

Here you create an ATM with a name and balance. If you think about what an ATM does, this does not reflect real life. ATM's have a balance as in how much money they contain but they do not have a first and last name. You may have been thinking of a BankAccount when implementing the code. This is supported by the code not having a step where the user tells the machine who they are and what account to access. But then again, bank accounts are identified by account number, not by the account owner name.

System.out.println("Please choose: " + "\n" +
    "1. Deposit" + "\n" + "2. Withdraw" + "\n" + "3. Info" + "\n" + "4. Exit");

This is just hard to read and maintain. In these situations I prefer reflecting the end result in the formatting so that the intended output is more clear.

System.out.println("Please choose:\n" +
    "1. Deposit\n" +
    "2. Withdraw\n" +
    "3. Info\n" +
    "4. Exit");

Now it is also instantly clear to the code maintainer what the valid input options are.

int option = in.nextInt();

Because you have not written any error handling, your program will show an ugly error message to the user and it will immediately exit if they make a wrong choice, such as enterin a letter instead of a number.

switch (option) {
    ....

Switch statements should always have a default-case. This is especially important as you are handling user input.

    default:
        System.err.println("Invalid input " + option + ". Please choose a value between 1 and 4.");
        break;

This also holds true if the swich handles an enumeration and you have handled all the cases because a change to the enumeration might add a new value and your code would then be unable to handle it. In such cases the default case should throw an IllegalArgumentException with a message describing the problem.

case 4:
    System.exit(0);

It is pretty much never Ok to exit from a Java program using System.exit(int). It's akin to pulling the power cord from the computer to exit. It exits your program but also takes out everything else that might have been running in the JVM. In this case the correct choice would be to set the loop condition in the while-loop to false.

boolean exitRequested = false;
while (! exitRequested) {
    ...
    switch (option) {
    ...
    case 4:
        exitRequested = true;
        break;
    }
}

Now that you have a variable controlling the exit, you can use it to search for the code that makes the program exit. This is a useful feature when the programs that you work with are larger.

public ATM(String name, String lastName, double balance) {

Money should never be handled using floating point numbers. Use either integers and divide the result with 100 to obtain euros and cents or use BigDecimal. For classroom code we can let it pass, but it's really not a good idea to learn that habit. I have seen it in production and it is neither nice to fix the issues nor explain to customers why our software stole a cent of their money.

public void withdraw(double value){
    if(value > balance){
        System.out.println("You can't withdraw that amount of money." +"\n"+ "Your balance is: " + getBalance() + "\n");

You've placed a user interface inside the ATM class (if we call it "BankAccount", it might be more obvious why this is a bad idea, bank accounts don't have a user interface). Instead of placing error messages in the withdraw method, you should throw an exception that describes the error and let the user interface code in your main class to decide what message to show to the user.

\$\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.