12
\$\begingroup\$

I was hoping someone could provide some feedback with regards to this:

import java.util.InputMismatchException;
import java.util.Scanner;


public class Bank {

    protected double myBalance = 10.40;
    protected double bankBalance = 1000000;
    protected double creditRating = 0.1;
    private static final int PIN = 1234;
    boolean access = false;
    protected Scanner s = new Scanner(System.in);

    public Bank() {

        int pin = 0000;
        int option;

        System.out.print("Please enter your 4 digit pin code: ");
        try {
            pin = s.nextInt();
        } catch (InputMismatchException e) {
            System.out.print("Invalid character entered, bye bye...");
        }

        if (pin != PIN) {
            System.err.println("Incorrect PIN entered, please try again later.");
            System.exit(0);
        }
        access = true;
        System.out.println("Welcome to JCBank please choose from the following options");
        System.out.println();
        System.out.println("Withdraw (1)\tDeposit (2)\tCheck Balance (3)\tApply for Loan(4) \tQuit (5)");
        System.out.println();

        do {
            option = s.nextInt();
        } while(!selectOption(option));
    }

    public boolean selectOption(int option) {
        switch (option) {
            case 1:
                System.out.println("You would like to withdraw");
                System.out.println();
                break;
            case 2:
                System.out.println("You would like to Deposit");
                System.out.println();
                break;
            case 3:
                System.out.println("Your account balance is £" + myBalance);
                System.out.println();
                break;
            case 4:
                System.out.println("Please wait whilst we perform a credit check.");
                try {
                    checkCreditRating();
                } catch (InterruptedException e) {
                    System.out.println("How embarrassing, the system is currently unavailable. Try again later.");
                }
                System.out.println();
                break;
            case 5:
                System.out.println("Successfully logged out, see you soon!");
                System.out.println();
                return true;
            default:
                System.out.println("Invalid option, please choose another option.");
                System.out.println();
                return false;
        }
            System.out.println("Please choose another option.");
            return false;
    }

    public void deposit(double amount) {
        myBalance += amount;
    }

    public void withdraw(double amount) {
        if (myBalance > amount) {
            System.out.println("Your funds have been successfully withdrawn.");
        } else {
            System.out.println("You don't have the funds to complete this transaction");
        }
    }

    public void checkCreditRating() throws InterruptedException {
        Thread.sleep(1000);
        if (creditRating <= 0.0) {
            System.err.println("Sorry, we can't lend you any money at this time.");
            return;
        }

        System.out.print("Please enter your loan amount: ");
        double amount = s.nextDouble();
        if (amount > bankBalance) {
            System.err.println("We are unable to loan you such an amount.");
        }
        double amountToPayBack = (20/100)*amount;
        System.out.println("The amount to payback is " + amountToPayBack);
    }

    public double checkAccountBalance() {
        return myBalance;
    }
}
\$\endgroup\$
  • \$\begingroup\$ BTW, you can format stuff as source code by adding four spaces of indentation. \$\endgroup\$ – FUZxxl Nov 22 '11 at 19:48
  • \$\begingroup\$ First your stackexchange code formatting is a little messed up as the first part of the code is not formatted. There's room for a ton of improvement. A couple things: Your code is tightly coupled to the UI, ideally you want to decouple it so you could easily switch between a console app, a windowed GUI, or a GUI-less app used in scripting. 2. Don't do work in the constructor like that, make a separate method. 3. Don't try to put everything into one class like you are here. I know this is just a test but a "real" java class would have way more classes. \$\endgroup\$ – User Nov 22 '11 at 20:00
16
\$\begingroup\$

A number of points:

  • Do not use floating-point data types (float or double) for amounts of money. Floating-point data types have limited precision, they cannot represent all decimal values exactly, and you're going to get roundoff errors sooner or later. Use integers instead (int or long) and store cents instead (for example, 1040 cents) or use BigDecimal.
  • Make member variables private by default; only make them protected (or another access level) if there is a good reason to do so.
  • Prefer local variables above member variables. Is there a good reason why s is a member variable instead of a local variable?
  • Don't write the main part of a program in the constructor of a class. You have the main part of your program in the constructor of the Bank class. The constructor is meant to initialize the state of a new object, so the only thing that the constructor should do is initialize member variables.
  • In your constructor, you catch an InputMismatchException and print an error message ("bye bye...") but you are not actually returning from the constructor. So execution will continue and print "Incorrect PIN entered ..." which is not what you meant.
  • The program doesn't actually do what you'd expect it to do; withdrawal, deposit etc. functionality never gets called anywhere.
  • You need a public static void main(String[] args) method to be able to run this as a stand-alone program.
\$\endgroup\$
  • \$\begingroup\$ Thank you both very much some good feedback, really appreciated !! \$\endgroup\$ – John Crossley Nov 22 '11 at 20:11
  • 1
    \$\begingroup\$ In vein about floating point precision (or lack thereof), the amount .1 cannot be accurately represented. In other words, you can't add exactly one dime (say, of interest) to an account. It might be close (especially with doubles), but it will add up over time. \$\endgroup\$ – Clockwork-Muse Nov 23 '11 at 0:32
10
\$\begingroup\$

Jesper has already made comments on the code itself, so I will address your organization. You only have one big class that does everything, and it does things in many areas. For instance, your class handles customers, accounts, and security. You should split these out into separate classes. Perhaps Bank would be your top level class, with the main method. Then you would have an Account class, with the withdraw, deposit, and checkBalance methods, a Customer class with a list of accounts that customer has as well as methods for checking their credit rating, etc.

Security - your model is weak, but obviously this is an exercise so let's keep it simple (and I'm no security expert either!). I'd create a file with the credentials for each customer, and have a class used for authenticating the user before you do anything, say Authenticator.

Authenticator auth = new Authenticator();
Customer cust = null;

while (cust == null) {
   System.out.println("Username: ");
   String username = s.nextLine();
   System.out.println("PIN: ");
   int pin = s.nextInt();

   cust = auth.authenticateCredentials(username, pin);
}

The authenticateCredentials() method would return null if the credentials provided did not match any on record. If a match was found, it would return the customer records that match the login credentials.

Obviously I'm leaving a lot out, like how to save the customer and login information, and organizing the relationship between customers and accounts. I'd recommend that you take a peek at the ArrayList class for keeping lists of accounts. You've got a good start here, with a lot you can do to improve and add new features. Have fun on the new project!

\$\endgroup\$
3
\$\begingroup\$

I would say instead of:

if (pin != PIN) {
        System.err.println("Incorrect PIN entered, please try again later.");
        System.exit(0);
    }
    access = true;

use :

if(pin == PIN){
   access = true;
    System.out.println("Welcome to JCBank please choose from the following options");
    System.out.println();
    System.out.println("Withdraw (1)\tDeposit (2)\tCheck Balance (3)\tApply for Loan(4) \tQuit (5)");
    System.out.println();
}
else{
    System.err.println("Incorrect PIN entered, please try again later.");
    System.exit(0);
}

its much more readable I guess.

\$\endgroup\$
  • \$\begingroup\$ That's not good. You have an unneccessary conditional in your code. Why not just access |= pin == PIN; if (access) ...? \$\endgroup\$ – FUZxxl Nov 23 '11 at 7:39
  • 3
    \$\begingroup\$ Why not just pin == PIN...it's not like access is used somewhere else anymore. \$\endgroup\$ – Bobby Nov 23 '11 at 10:18
  • \$\begingroup\$ I assumed he would use access later in the development of his code. If not,you could also delete the variable access. \$\endgroup\$ – Feras Nov 23 '11 at 21:08

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.