3
\$\begingroup\$

I recently built an improved version of a bank account program I made. It has no GUI or any interaction with users, but I simply built it to test it and play with it via main(). I'll post all my classes below and you can run the program yourself or just critique my code. I'll take any and all suggestions, so please leave corrections that you see (this code will most likely be rough, as I'm a beginner programmer altogether, but I'm still learning a lot).

Bank Class:

package improvedBank;
import java.util.*;

public class Bank {

    public static ArrayList<Account> bankAccounts = new ArrayList<Account>();
    static int time = 0; 
    int bankCash = 0;

    public ArrayList<Account> addAcc(Account acc) {
        bankAccounts.add(acc);
        return bankAccounts;
    }

    public static void passTime(Account acc) {
        time += 1;
        acc.getInterest();
    }

    // testing code, not sure how to implement user control yet :/
    public static void main(String[] args) {
        Bank myBank = new Bank();

        BusinessAccount billsAccount = new BusinessAccount("Bill's Donut Shop", "Bill", 372);
        BusinessAccount joesAccount = new BusinessAccount("Joe's Italian Restaurant", "Joe", 568);
        SavingsAccount dillonsAccount = new SavingsAccount("Dillon", 57);
        SavingsAccount jessicasAccount = new SavingsAccount("Jessica", 72);

        myBank.addAcc(billsAccount);
        myBank.addAcc(joesAccount);
        myBank.addAcc(dillonsAccount);
        myBank.addAcc(jessicasAccount);

        billsAccount.deposit(37);
        System.out.println(bankAccounts);

        dillonsAccount.withdraw(12);
        jessicasAccount.deposit(12);
        billsAccount.withdraw(4);
        System.out.println(bankAccounts);
    }
}

Account class (abstract class):

package improvedBank;

public abstract class Account {

    public String accountType;
    public String owner;
    public int balance;
    public int timeOpened;

    public int getInterest() {
        int newBal = (int) (balance * 0.15);
        balance += newBal;
        timeOpened += 1;
        return balance & timeOpened;
    }

    public int deposit(int amount) {
        Bank.passTime(this);
        return balance += amount;
    }

    public int withdraw(int amount) {
        Bank.passTime(this);
        return balance -= amount;
    }

    public String toString() {
        return "Account Type: " + this.accountType + " | Account owner: " + this.owner + " | Account balance: $" + this.balance + " | Account active for: " + timeOpened + " days";
    }   
}

SavingsAccount class (extends Account class):

package improvedBank;

public class SavingsAccount extends Account {

    public SavingsAccount(String own, int cash) {
        this.accountType = "Savings Account";
        this.owner = own;
        this.balance = cash;
        System.out.println("Savings account created.");
        Bank.passTime(this);
        return;
    }

}

BusinessAccount class (also extends Account class):

package improvedBank;


public class BusinessAccount extends Account {

    String businessName;

    public BusinessAccount(String bizName, String own, int cash) {
        this.accountType = "Business Account";
        this.businessName = bizName;
        this.owner = own;
        this.balance = cash;
        System.out.println("Business account created");
        Bank.passTime(this);
        return;
    }
}
\$\endgroup\$
14
\$\begingroup\$

Over-all, you say you are a beginner programmer, but you are off to a very nice start! All my suggestions focus around one concept: the Interface. You are definitely ready to read Joshua Bloch's "Effective Java 2nd Edition" which is all about creating great interfaces.

+1 using generics with collections

-1 import wildcard - Though it's a pain, you should really specify each import.

Prefer interfaces for return types:

OK:

public ArrayList<Account> addAcc(Account acc)...

Better:

public List<Account> addAcc(Account acc)...

Now you can use any kind of List without breaking client code. Heck if you can get away with it, it gives you even more freedom to use:

public List<Collection> addAcc(Account acc)...

Careful with Static...

Static means this variable exists on the Bank class instead of having one for each object (instance). I think you want one list of accounts for each Bank which means you don't want Static.

Prefer Private to Public

Public means all clients can access it.

Not so good:

public static ArrayList<Account> bankAccounts = new ArrayList<Account>();

Private means you have to provide get/set methods for users of your API to access this variable. Final means you can never create a new ArrayList for bankAccounts.

private final List<Account> bankAccounts = new ArrayList<Account>();

Favor Interfaces over Inheritance

They are a little more work up front, but inheritance forces you to make variables and constructors protected instead of private. But if you want to use inheritance, then give Account a constructor to take on some of the work:

public abstract class Account {
    private String owner; // person or business
    private int balance;

    protected Account(String own, int cash) {
        owner = own;  balance = cash;
    }

    public String getOwner() { return owner; }
    public void setOwner(String own) { owner = own; }

    public int getBalance() { return balance; }
    public void setBalance(int cash) { balance = cash; }

    public abstract String getAccountType();

    public String toString() {
        return new StringBuilder("Account Type: ")
                .append(this.getAccountType())
                .append(" | Account owner: ").append(this.owner)
                .append(" | Account balance: $").append(this.balance)
                .append(" | Account active for: ").append(timeOpened)
                .append(" days").toString();
    }
    ...

You notice I use a StringBuilder instead of string concatenation with plus. This can make a big speed and memory difference if the concatenation is ever done in a loop. In some situations what you did is OK, but I'm not sure about this one. Better safe than sorry.

Then:

public class SavingsAccount extends Account {
    public final static String SAVINGS_ACCOUNT = "Savings Account";
    public SavingsAccount(String own, int cash) {
        // Call account constructor...
        super(own, cash);
        ...
    }
    public String getAccountType() { return SAVINGS_ACCOUNT; }
}

This assumes you are going to add functionality to the individual Account sub-classes. If not, just make AccountType an enum inner class of Account:

public abstract class Account {
    public enum AccountType {
        PERSONAL("Savings Account"), 
        BUSINESS("Business Account");
        private final String name;
        AccountType(String n) { name = n; }
        public String getName() { return name; }
    }
    private String owner;
    private int balance;
    private AccountType type;
    protected Account(String own, int cash, AccountType t) {
        owner = own;  balance = cash; type = t;
    }
    ...
}

Interest Calculation

I think your interest calculation is done for fun, but having worked for a bank, it is a little disturbing. :-) For one thing, I can get very rich by just making repeated withdrawls and deposits to increment the time counter. For another, I can create a second bank Bank getRich = new Bank(); Every transaction I make there, affects all accounts at myBank because time is static, thus has a single copy on the Bank class, not on the individual Bank object instances.

Worse yet, Bank.time is public, so without making a new bank, I can just call Bank.time = (Integer.MAX_VALUE - 1) - (myAccount.balance * 0.15); Then call myAccount.getInterest(); myAccount.withdraw(myAccount.balance);

Conclusion

Over-all, you are well on your way to becoming a great programmer. A code review necessarily focuses on the things you should change, but this is an excellent beginner effort. Actually, I think you are being humble calling yourself a beginner.

To take it to the next level, I'd recommend you read Josh's book or find other sources for API design. The more you can make private, the less you can expose for clients of your API, the easier it is to make changes to your implementation down the road, and the fewer problems they'll have with it. Good luck!

P.S.

I'd recommend you spend some time with JavaDoc. Not only is it good to comment your code in general, but it makes you focus on the way someone else will view your classes - makes you view your work as an API. When I read my own generated JavaDocs, I always have, "Oh, that's public? That's accessible?" moments that end up making my code better.

\$\endgroup\$
  • \$\begingroup\$ Wow, thanks for the great advice! You have no idea how much that means to me as a programmer. All the things you recommended are things that I'm learning about in the books I'm reading, so I'll make sure to bookmark this and take a look at your advice If I'm confused or need help. Thanks again, really. :D \$\endgroup\$ – hasherr Apr 9 '13 at 9:33
  • \$\begingroup\$ Oh, and yes, I know how crappy the actual 'simulation of a bank' is implemented, but I know very little about finance, so I did five minutes of research on banks and used that. Thanks again. \$\endgroup\$ – hasherr Apr 9 '13 at 10:18
  • \$\begingroup\$ Glad I could help. Please notice I added a P.S. section in my answer suggesting that you spend some time playing with JavaDoc. \$\endgroup\$ – GlenPeterson Apr 10 '13 at 12:09

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.