2
\$\begingroup\$

This is a code that I just wrote is a simple application of a banking system I would like you all to review it all tell me where could I improve it, please feel free to critique it, but be aware that I'm still a beginner in this :D, please help to learn new things and also I would like to know some other mini-project to learn something so if you have something in mind it would be nice if you tell me.

This is the main file of my code. Bank.Java

import java.io.EOFException;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.util.HashMap;
import java.util.InputMismatchException;
import java.util.Scanner;

public class Bank {

static private HashMap<String, User> userAccounts = new HashMap<>();
static private User User;
static private Scanner in = new Scanner(System.in);
static private boolean exit = false;

public static void main(String[] args) throws IOException, ClassNotFoundException {
    
    //Reading the current value-key pair of the hash map to be ready to overwrite with some new changes 
    try {
        FileInputStream fis = new FileInputStream("Accounts.dat");
        ObjectInputStream ois = new ObjectInputStream(fis);
        userAccounts = (HashMap<String, User>) ois.readObject();    
        ois.close();
    } catch (EOFException e) {
        System.out.println("No existing accounts please create one.");
        System.out.println();
    }
    //Showing all the Hash Map for developing purposes only
    for (String userName : userAccounts.keySet()) {
        String key = userName;
        String value = userAccounts.get(userName).toString();
        System.out.println(key + " " + value);
    }

    ///////////////////////////////////////////////////////////////////////////////////////////////////
    //                               First set of options
    //                                  Log in Menu
    System.out.println("\t\tWelome to my Bank System");
    System.out.println();
    System.out.println("1: To open existing account.\t2: To create new account.");
    menuInput(1);
    ///////////////////////////////////////////////////////////////////////////////////////////////////
    //                               Second set of options
    //                                    Main Menu
    // Printed until exit 
    while ( !exit ) {
        System.out.println();
        System.out.println("Chose an action of the folowing options.");
        System.out.println();
        System.out.println("A: Display Account Infrmation.\tB: Deposit.\n" + "C: Withdraw.\t\t\tD: Check Balance.\n" + "E: Change Password.\t\tF: Delete Account.\n" + "G: Exit.");
        menuInput(2);
    }
    
    in.close();
    }

//Handling the input for the menu
static public void menuInput(int set) throws IOException {
            
    //Input for the first set of options
    if ( set == 1 ) {
        int choice;
        
        try {
            System.out.print("Enter: ");
            
            if ( in.hasNextInt() ) {
                
                choice = in.nextInt();
                    if( choice == 1 ) {
                        openAccount();
                    } else if( choice == 2 ) {
                        createAccount();
                    } else {
                        
                        System.out.println();
                        System.out.println("Please enter a valid option.");
                        in.nextLine();
                        menuInput(1);
                    }
                    
            } else if( in.hasNextLine() ) { 
                System.out.println();
                System.out.println("Please enter a valid option.");
                in.nextLine();
                menuInput(1);
                
            }
            
        } catch (InputMismatchException e) {
            e.printStackTrace();
            System.out.println();
            System.out.println("Please enter a valid option.");
            menuInput(1);
        }
    //Input for the second set of options
    } else if ( set == 2 ) {
        
        String choice;
        
        try {
            
            System.out.print("Enter: ");
            choice = in.next();
            choice = choice.toUpperCase();
            
            switch (choice) {
            
                case "A":
                        User.displayInfo();
                        saveHashMap(userAccounts);
                    break;
                case "B":
                        System.out.println();
                        System.out.print("Please enter any amount to deposit: ");
                        double deposit = in.nextDouble();
                        User.deposit(deposit);
                        saveHashMap(userAccounts);
                    break;
                case "C":
                        System.out.println();
                        System.out.print("Please enter any amount to withdraw: ");
                        double withdraw = in.nextDouble();
                        User.withdraw(withdraw);
                        saveHashMap(userAccounts);
                    break;
                case "D":
                        User.checkBalance();
                        saveHashMap(userAccounts);
                    break;
                case "E":
                        System.out.println();
                        System.out.println("To change the password please enter the current password.");
                        in.nextLine();
                        boolean correctPassword = validatePassword(User.getUserName(),0);
                        System.out.println();
                        System.out.println("Please enter the new password: ");
                        String newPassword = in.nextLine();
                        User.changePassword(correctPassword, newPassword);
                        saveHashMap(userAccounts);
                    break;
                case "F":
                        deleteAccount();
                    break;
                case "G":
                    exit = true;
                    break;
                default:
                    System.out.println();
                    System.out.println("Plase chose a valid option.");
                    break;
            
            }
            
            
        } catch( InputMismatchException e ) {
            
            //e.printStackTrace();
            System.out.println();
            System.out.println("Please enter a valid option.");
            menuInput(2);
            
        }
        
    }
    
}

//Ask for a specific account-User object in the already updated HashMap  
static public void openAccount() {
    times++;
    
    System.out.println();
    System.out.print("Please enter the user name of the account: ");
    if( times <= 1 ) {
        in.nextLine();
    }       String userName = in.nextLine();
    if ( !(userAccounts.get(userName) == null) ) {
        
        boolean correctPassword = validatePassword(userName,0);
        if( correctPassword ) {
            User user = userAccounts.get(userName);
            //Set the new User object to a global object to be ready to take action in the account
            User = user;
            
        } else {
            User user = userAccounts.get(userName);
            User = user;
        }
    } else {
        
        System.out.println();
        System.out.println("Please enter an existing account.");
        openAccount();
        
    }
}
static int times = 0;
//Creates a new account-User object and update the file and HashMap with the new account
static public boolean createAccount() throws IOException {
    
    times++;
    
    System.out.println();
    System.out.print("Enter a new user name for the account: ");
    if( times <= 1 ) {
        in.nextLine();
    }
    String userName = in.nextLine();
    
    //Checks for an existing account  
    if( !(userAccounts.get(userName) == null) ) {
            
        System.out.println();
        System.out.println("This user name already exists.");
        createAccount();
        return false;
    }
    
    System.out.println();
    System.out.print("Enter a new password for the account: ");
    String password = in.nextLine();
    System.out.println();
    
    //Creates the account-User object and updates the HashMap
    User user = new User(userName, password);
    userAccounts.put(user.getUserName(),user);

    //Set the new User object to a global object to be ready to take action in the account
    User = user;
    
    saveHashMap(userAccounts);
    
    return true;
}

//Deletes an account-User object from the file and HashMap
static public void deleteAccount() throws IOException {
    
    System.out.println();
    System.out.println("Please enter password to delete account.");
    
    in.nextLine();
    boolean correctPassword = validatePassword(User.getUserName(),0);
    
    if( correctPassword ) {
        
        userAccounts.remove(User.getUserName());
        saveHashMap(userAccounts);
        System.out.println();
        System.out.println("You have successfuly deleted this account: " + User.getUserName() + ".");
        System.exit(0);
        
    }
    
}

static public boolean validatePassword(String userName, int tries) {
    
    User user = userAccounts.get(userName);
    tries++;
    
    //System.out.println(user.getPassword());
    
    if ( tries == 1 ) {
        
        System.out.println();
        System.out.print("Please enter the password: ");
        String password = in.nextLine();
        if ( password.equals(user.getPassword()) ) {
            return true;
        } else {
            System.out.println();
            System.out.println("Incorrect password.");
            validatePassword(userName,tries);
        }
        
    } else if ( tries == 2 ) {
        
        System.out.println();
        System.out.print("Please enter the correct password: ");
        String password = in.nextLine();
        if ( password.equals(user.getPassword()) ) {
            return true;
        } else {
            System.out.println();
            System.out.println("Incorrect password.");
            validatePassword(userName,tries);
        }
    
    } else if ( tries == 3 ) {
        
        System.out.println();
        System.out.print("Please enter the correct password: ");
        String password = in.nextLine();
        if ( password.equals(user.getPassword()) ) {
            return true;
        } else {
            System.out.println();
            System.out.println("Incorrect password.");
            validatePassword(userName,tries);
        }
    
    } else if ( tries == 4 ) {
        System.out.println();
        System.out.println("You have used your 3 tries the app is going to exit.");
        System.exit(0);
    }
    
    return false;
}

static public void saveHashMap(HashMap<String, User> userAccounts) throws IOException {
    
    FileOutputStream fos = new FileOutputStream("Accounts.dat");
    ObjectOutputStream oos = new ObjectOutputStream(fos);
    
    //Saves the HashMap to a file
    oos.writeObject(userAccounts);
    
    oos.close();
}


}

This is another file that the main file uses. User.java

import java.io.Serializable;

public class User implements Serializable {

/**
 * 
 */
private static final long serialVersionUID = 1L;
private String userName;
public String password;
private double balance;
private double lastTransaction;
private char transaction;

User (String userName, String password){
    
    this.userName = userName;
    this.password = password;
    
}

public String getUserName() {
    return userName;
}

public String getPassword() {
    return password;
}

public double getBalance() {
    return balance;
}

//Updates-increases the variable balance according to the input from the user
public void deposit( double deposit ) {
    
    balance += deposit; 
    lastTransaction = deposit;
    transaction = 'D';
    
    System.out.println();
    System.out.printf("You have successfully deposit $%,.2f%n" , deposit);
    
}

//Updates-decreases the variable balance according to the input from the user 
public void withdraw(double withdraw) {
    
    balance -= withdraw; 
    lastTransaction = withdraw;
    transaction = 'W';
    
    System.out.println();
    System.out.printf("You have successfully withdrawn $%,.2f%n" , withdraw);
    
}

//Displays the current balance of the user
public void checkBalance() {
    
    System.out.println();
    System.out.printf("Current Balance: $%,.2f%n" , balance);
    
}

//Displays all the info from the user
public void displayInfo() {
    
    System.out.println();
    System.out.println("User Name: " + userName);
    System.out.println("Password: " + password);
    if ( transaction == 'D' ) {
        System.out.printf("Last Transaction: Deposit, Amount: $%,.2f%n" , lastTransaction);
    } else if ( transaction == 'W' ) {
        System.out.printf("Last Transaction: Withdraw, Amount: $%,.2f%n" , lastTransaction);
    }
    
    
    
}

//Change the current password of the user
public void changePassword (boolean correctPassword, String newPassword) {
    
    if( correctPassword ) {
        
        password = newPassword;
        
    }
    
    
}

}

There is another file that the main class creates. Accounts.dat this file stores all the objects of the user class. This is my first time posting here so if there is something I'm missing please tell me and feel free to ask for any other information. Thank you very much! :D

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Certainly overlapping with Maarten's feedback:

Don't keep all of your Bank members as statics. You should have an actual instance.

Separate Bank from a text user interface class so that the logic and UI work are decoupled.

Don't explicitly ois.close - instead use a try-with-resources.

Destroy Showing all the Hash Map for developing purposes only with prejudice. This kind of code should never exist in an application, and instead if you're curious about the contents of the database, debug your application and look at the contents of memory.

Having an "input set" indicated by an integer 1 or 2 is awkward; instead just write these as separate sections in a menu loop.

Why have input numbers in the first section and input letters later? Better to be consistent.

There's not really a need to in.close; that's stdin and programs nearly always leave that as-is.

Consider writing a utility function that calls one of a given array of functions based on the user's input choice. With a variadic array of Runnable this is made quite easy.

Don't saveHashMap after displayInfo, since nothing has changed.

Rather than printf("$%,.2f%n"), use Java's own locale currency formatting.

You don't nextLine() in the right places - it should be done directly after every nextDouble().

Your times variable, as it's currently written, needs to go away.

Never plaintext-accept a password on the terminal. Java (and basically all other programming languages) have a facility to mask password input on the terminal.

Your validatePassword needs to use a loop instead of copy-pasta-pasta-pasta.

Exiting after three incorrect password attempts is almost certainly not what you want to do. Think about an attacker who walks up to a kiosk, types in a user and three junk passwords to easily terminate the application. Instead, there should be an anti-brute-force sleep() hang and then a return to the main menu.

In all but the construction context, you can hold onto a map reference of weakened type Map rather than HashMap - there's only one place you need to specify the implementation flavour, and that's when you first make the empty map.

Never - not for learning, development, hobby or prototyping purposes - store a password unhashed and unsalted. This is easy to get wrong, and is a whole field of study unto itself; but even a simplistic call to the Java built-in HMAC support is better than nothing.

Suggested

Bank.java

package com.stackexchange.bank;

import java.io.*;
import java.util.HashMap;
import java.util.Map;

public class Bank {
    private final String filename;
    private final Map<String, User> userAccounts;

    public Bank() throws IOException, ClassNotFoundException {
        this("Accounts.dat");
    }

    public Bank(String filename) throws IOException, ClassNotFoundException {
        this.filename = filename;

        Map<String, User> diskAccounts;

        try (
            FileInputStream fis = new FileInputStream(filename);
            ObjectInputStream ois = new ObjectInputStream(fis);
        ) {
            diskAccounts = (Map<String, User>) ois.readObject();
        }
        catch (EOFException | FileNotFoundException e) {
            diskAccounts = new HashMap<>();
        }

        userAccounts = diskAccounts;
    }

    public void addAccount(User user) {
        userAccounts.put(user.getUserName(), user);
    }

    public User getAccount(String username) {
        return userAccounts.get(username);
    }

    public void deleteAccount(String username) {
        userAccounts.remove(username);
    }

    public void saveHashMap() {
        try(
            FileOutputStream fos = new FileOutputStream(filename);
            ObjectOutputStream oos = new ObjectOutputStream(fos);
        ) {
            oos.writeObject(userAccounts);
        }
        catch (IOException e) {
            throw new RuntimeException(e);
        }
    }
}

User.java

package com.stackexchange.bank;

import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.PBEKeySpec;
import java.io.Serial;
import java.io.Serializable;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.KeySpec;
import java.util.Arrays;

public class User implements Serializable {

    @Serial
    private static final long serialVersionUID = 1L;
    private static final SecretKeyFactory secrets;
    private static final SecureRandom rand;
    private static final int keyLength = 512;

    static {
        try {
            secrets = SecretKeyFactory.getInstance(
                String.format("PBKDF2WithHmacSHA%d", keyLength)
            );
            rand = SecureRandom.getInstanceStrong();
        } catch (NoSuchAlgorithmException e) {
            throw new RuntimeException(e);
        }
    }

    private final String userName;
    private final byte[] salt = new byte[16];
    private byte[] passwordHash;
    private double balance;
    private double lastTransaction;
    private char transactionKind;

    User (String userName, char[] password) {
        this.userName = userName;
        this.passwordHash = setPassword(password);
    }

    public String getUserName() {
        return userName;
    }

    public double getBalance() {
        return balance;
    }

    //Updates-increases the variable balance according to the input from the user
    public void deposit(double deposit) {
        balance += deposit;
        lastTransaction = deposit;
        transactionKind = 'D';
    }

    //Updates-decreases the variable balance according to the input from the user
    public void withdraw(double withdraw) {
        balance -= withdraw;
        lastTransaction = withdraw;
        transactionKind = 'W';
    }

    public double getLastTransactionAmount() { return lastTransaction; }

    public char getLastTransactionKind() { return transactionKind; }

    private byte[] setPassword(char[] password) {
        rand.nextBytes(salt);
        return hash(password);
    }

    private byte[] hash(char[] password) {
        final int iterCount = 65536;
        KeySpec spec = new PBEKeySpec(password, salt, iterCount, keyLength);
        byte[] encoded;
        try {
            encoded = secrets.generateSecret(spec).getEncoded();
        }
        catch (InvalidKeySpecException e) {
            throw new RuntimeException(e);
        }

        return encoded;
    }

    public boolean validatePassword(char[] candidate) {
        return Arrays.equals(passwordHash, hash(candidate));
    }

    public void changePassword(char[] newPassword) {
        passwordHash = setPassword(newPassword);
    }
}

BankUI.java

package com.stackexchange.bank;

import java.io.Console;
import java.io.IOException;
import java.text.NumberFormat;
import java.util.InputMismatchException;
import java.util.Scanner;

import static java.lang.System.out;

public class BankUI {
    private final Bank bank = new Bank();
    private final Scanner in = new Scanner(System.in);
    private final Console console = System.console();
    private final NumberFormat currency = NumberFormat.getCurrencyInstance();
    private boolean shouldExit = false;
    private User user;

    public BankUI() throws IOException, ClassNotFoundException {
        if (console == null) {
            throw new UnsupportedOperationException("Refusing to run in an insecure terminal.");
        }
    }

    public static void main(String[] args) throws IOException, ClassNotFoundException {
        new BankUI().mainMenu();
    }

    public void mainMenu() {
        while (!shouldExit) {
            if (user == null) {
                out.printf(
                    "Welcome to my Bank System"
                    + "%n"
                    + "%n1: Open existing account."
                    + "%n2: Create new account."
                    + "%n"
                );
                menuInput(
                    this::openAccount,
                    this::createAccount
                );
            }
            else {
                out.printf(
                    "Choose an action of the following options."
                    + "%n"
                    + "%n1: Display Account Information."
                    + "%n2: Deposit."
                    + "%n3: Withdraw."
                    + "%n4: Check Balance."
                    + "%n5: Change Password."
                    + "%n6: Delete Account."
                    + "%n7: Exit."
                    + "%n"
                );
                menuInput(
                    this::displayUserInfo,
                    this::deposit,
                    this::withdraw,
                    this::checkBalance,
                    this::changePassword,
                    this::deleteAccount,
                    this::exit
                );
            }

            out.println();
        }
    }

    public void menuInput(Runnable... options) {
        int choice;

        while (true) {
            out.print("Enter: ");

            try {
                choice = in.nextInt() - 1;
                in.nextLine();
            }
            catch (InputMismatchException e) {
                out.println("Please enter a valid integer.");
                in.nextLine();
                continue;
            }

            if (choice >= 0 && choice < options.length)
                break;
            out.println("Please enter a valid choice.");
        }

        options[choice].run();
    }

    public void displayUserInfo() {
        out.printf("User Name: %s%n", user.getUserName());

        String kind;
        switch (user.getLastTransactionKind()) {
            case 'D' -> kind = "Deposit";
            case 'W' -> kind = "Withdrawal";
            default -> { return; }
        }

        out.printf(
            "Last Transaction: %s, Amount: %s%n",
            kind, currency.format(user.getLastTransactionAmount())
        );
    }

    public void deposit() {
        out.print("Please enter any amount to deposit: ");
        double deposit = in.nextDouble();
        in.nextLine();
        user.deposit(deposit);
        out.printf("You have successfully deposited %s%n", currency.format(deposit));
        bank.saveHashMap();
    }

    public void withdraw() {
        out.print("Please enter any amount to withdraw: ");
        double withdraw = in.nextDouble();
        in.nextLine();
        user.withdraw(withdraw);
        out.printf("You have successfully withdrawn %s%n", currency.format(withdraw));
        bank.saveHashMap();
    }

    public void checkBalance() {
        out.printf("Current Balance: %s%n", currency.format(user.getBalance()));
    }

    public void changePassword() {
        out.println("To change the password please enter the current password.");
        if (validatePassword()) {
            out.print("Please enter the new password: ");
            user.changePassword(console.readPassword());
            bank.saveHashMap();
        }
        else {
            user = null;
        }
    }

    public void openAccount() {
        out.print("Please enter the user name of the account: ");
        String userName = in.nextLine();

        user = bank.getAccount(userName);
        if (user == null) {
            out.println("Please enter an existing account.");
            return;
        }

        if (!validatePassword())
            user = null;
    }

    public void createAccount() {
        out.print("Enter a new user name for the account: ");
        String userName = in.nextLine();

        if (bank.getAccount(userName) != null) {
            out.println("This user name already exists.");
            return;
        }

        out.print("Enter a new password for the account: ");
        user = new User(userName, console.readPassword());

        bank.addAccount(user);
        bank.saveHashMap();
    }

    public void deleteAccount() {
        out.println("Please enter password to delete account.");
        if (validatePassword()) {
            bank.deleteAccount(user.getUserName());
            out.printf("You have successfuly deleted account '%s'.%n", user.getUserName());
            bank.saveHashMap();
        }

        user = null;
    }

    public boolean validatePassword() {
        try {
            for (int tries = 0; tries < 3; tries++) {
                out.print("Please enter the password: ");
                if (user.validatePassword(console.readPassword()))
                    return true;

                Thread.sleep(1000);
                out.println("Incorrect password.");
            }

            out.println("You have entered too many incorrect passwords.");
            Thread.sleep(3000);
        }
        catch (InterruptedException e) {
            throw new RuntimeException(e);
        }

        return false;
    }

    public void exit() {
        shouldExit = true;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for taking the time to do this and I will consider all of your observations. \$\endgroup\$ Feb 28, 2022 at 2:36
  • \$\begingroup\$ No problem! Consider upvoting Maarten's answer as it has good content. \$\endgroup\$
    – Reinderien
    Feb 28, 2022 at 2:49
2
\$\begingroup\$

Design

First of all, you're doing this for an account. You have a User class, but I don't see any Account class. Why is that? The idea of object oriented programming is to specify these kind of things and encapsulate the data such as the value within the account.

The main method is just there as a starting point. It should not contain any specific functionality other than that required to start up the program. And you'd never use class fields (with the static modifier), only object fields. In principle static should only be used for immutable values such as constants.

In principle you want to stay away from serialization when saving an account. If the serialization ever changes you're in trouble. Instead you should define a specific format to save the account and user data in. Quite often databases are used. You can use lightweight databases directly from Java. Otherwise an XML or JSON file could be an idea.

Some kind of class or classes such as AccountWriter and AccountReader with the appropriate methods would be a good idea; at the very least there should be specific methods reading the accounts. Those methods should not perform other actions such as warning the user that there are no accounts - that code should be split off into another method.

You should also be able to see other methods such as:

  • printWelcomeMessage()
  • printChoices()
  • getChoiceFromUser()

Recursive calls such as menuInput should be avoided as much as possible. Note that at some time the menuInput returns and the rest of the code of the previous calls to menuInput will still be executed. That's not a good thing.

Generic remarks

Don't take time formatting your comments. It's time better spent developing.

If you name your methods well you can get away with a lot fewer comments. There are a lot of issues keeping comments up to date; when you write better code you should require fewer of them.

To a casual reader the value 2 or the parameter name set doesn't say anything.

If you have a function that gets a feedback from a user, then it should return the value. It should not store it in any variable, and certainly not a class variable. This is known as a side effect, and side effects such as those should be avoided.

choice is defined in a higher scope. Variables should only be made available in the scope where they are required, not above.

\$\endgroup\$
4
  • \$\begingroup\$ It's late, I gotta sleep. Hope these remarks are helpful. I'll try and update later. \$\endgroup\$ Feb 26, 2022 at 1:31
  • \$\begingroup\$ Thank you very much for this feedback it helps me know all these parts of my code that can be improved, I can learn a lot from this, and again thank you very much!!! for taking the time to this review. I will read it carefully again and make sure to learn from this. \$\endgroup\$ Feb 26, 2022 at 2:39
  • \$\begingroup\$ Thank you very much!!! :D \$\endgroup\$ Feb 26, 2022 at 2:39
  • \$\begingroup\$ @IsaiasGarrido Consider upvoting this answer. \$\endgroup\$
    – Reinderien
    Feb 28, 2022 at 14:03

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.