4
\$\begingroup\$

I have a couple questions about this program

  1. Should I implement runnable instead of using thread class? I like the idea of the consumer and producer being thread objects, it seems simpler to me, but I have seen people argue the benefits if runnable interface.

  2. Are static methods in the main class considered good/bad convention?

  3. For a factory pattern like this, is the parent of the two bank accounts the best place to put the multithreading logic?

  4. Should setters be included always? Or should they be left out if the constructor is the only method that should set values? Is one of these approaches bad practice?

  5. Does an arraylist of accounts count as tight coupling and if so should it be abandoned? If so, what is the best way for a bank object to have access to each account?

Thank you in advance

bank class:

    package Model;
    import java.util.ArrayList;

    public class Bank {
        private ArrayList<BankAccount> accounts;
        private static int numberOfBanks;
        private int routingNumber;
    
        public Bank() {
            this.accounts = new ArrayList<BankAccount>();
            this.routingNumber = ++numberOfBanks;
        }
    }

BankAccount.java:


    package Model;

    import java.util.Date;

    abstract class BankAccount {
    static private int numberOfAccounts;
    private int accountNumber;
    private double balance;
    private String accountHolder;
    private Date accountOpened;
    private int withdrawsLeft;
    
    public BankAccount(String name) {
        this.accountNumber = ++numberOfAccounts;
        this.balance = 0;
        this.accountHolder = name;
        this.accountOpened = new Date();
    }
    
    public BankAccount(String name, double initialBalance) {
        this.balance = initialBalance;
        this.accountHolder = name;
        this.accountOpened = new Date();
    }

    public static int getNumberOfAccounts() {
        return numberOfAccounts;
    }

    public static void setNumberOfAccounts(int numberOfAccounts) {
        BankAccount.numberOfAccounts = numberOfAccounts;
    }

    public int getAccountNumber() {
        return accountNumber;
    }

    public void setAccountNumber(int accountNumber) {
        this.accountNumber = accountNumber;
    }

    public synchronized double getBalance() {
        return balance;
    }

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

    public String getAccountHolder() {
        return accountHolder;
    }

    public void setAccountHolder(String accountHolder) {
        this.accountHolder = accountHolder;
    }

    public Date getAccountOpened() {
        return accountOpened;
    }

    public void setAccountOpened(Date accountOpened) {
        this.accountOpened = accountOpened;
    }

    public int getWithdrawsLeft() {
        return withdrawsLeft;
    }

    public void setWithdrawsLeft(int withdrawsLeft) {
        this.withdrawsLeft = withdrawsLeft;
    }

    public synchronized void deposit(double d) {
        try {
            Thread.sleep(300);
        }
        catch(InterruptedException e) {
            e.getMessage();
        }
        balance += d;
        printReceipt();
    }
    
    public synchronized void withdraw(double w) {
        while (w > balance) {
            try {
               wait(); // wait for funds
            } catch (InterruptedException ie) {
               System.err.println(ie.getMessage());
            }
        }
        if (balance > 0) {
            balance -= w;
            printReceipt();
        }
        else System.out.println("ERROR: You are way too broke for that!");
    }

    public void printReceipt() {
        System.out.println(getAccountHolder() + "\'s balance is " + getBalance() + "0 on " + accountOpened.toString());
        
    }
    }

Main.java:


    package Model;

    public class Main {

    public static void main(String[] args) {
        Bank bankOfJames = new Bank();
        BankAccountFactory fact = new BankAccountFactory();
        BankAccount james = fact.getAccount("savings account","james", 1000.0);
        Thread[] users = new Thread[10];
        int[] threadNames = new int[10];
        for(int i = 0; i < users.length; i++) {
            if(i % 2 == 0) users[i] = new Producer(james);
            else users[i] = new Consumer(james);
        }
        for(int i = 0; i < users.length; i++) {
            System.out.println("Starting thread " + users[i].getName());
            users[i].start();
        }
        for(int i = 0; i < users.length; i++) {
            try {
                System.out.println("Joining thread " + users[i].getName());
                users[i].join();
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
            
        }
                
    }
    
    static class Producer extends Thread{
        BankAccount a;
        Producer(BankAccount ba){
            this.a = ba;
        }
        public void run() {
            a.deposit(100);
        }
        
    }
    
    static class Consumer extends Thread{
        BankAccount a;
        Consumer(BankAccount ba){
            this.a = ba;
        }
        public void run() {
            a.withdraw(50);
        }
    }

    }

CheckingAccount.java:

    package Model;

    public class CheckingAccount extends BankAccount {

    public CheckingAccount(String name) {
        super(name);
        // TODO Auto-generated constructor stub
    }

    public CheckingAccount(String name, double initialBalance) {
        super(name, initialBalance);
        // TODO Auto-generated constructor stub
    }
    
    public void writeCheck() {
        System.out.println("writing check");
    }

}

SavingsAccount.java:

    package Model;

    public class SavingsAccount extends BankAccount {
    private double interestRate;

    public double getInterestRate() {
        return interestRate;
    }

    public void setInterestRate(double interestRate) {
        this.interestRate = interestRate;
    }

    public SavingsAccount(String name) {
        super(name);
        // TODO Auto-generated constructor stub
    }

    public SavingsAccount(String name, double initialBalance) {
        super(name, initialBalance);
        // TODO Auto-generated constructor stub
    }
    
    public void compoundInterest() {
        double current = getBalance();
        current *= interestRate;
        setBalance(current);
    }

}

BankAccountFactory.java

    package Model;

    public class BankAccountFactory {
    
    
    public BankAccount getAccount(String account, String name) {
        if(account.equalsIgnoreCase("checkingaccount") || account.equalsIgnoreCase("checking account")) {
            return new CheckingAccount(name);
        }
        else if(account.equalsIgnoreCase("savingsaccounts") || account.equalsIgnoreCase("savings account")) {
            return new SavingsAccount(name);
        }
        else{
            System.out.println("Please indicate savings or checking account");
            return null;
        }
    }

    public BankAccount getAccount(String account, String name, double balance) {
        if(account.equalsIgnoreCase("checkingaccount") || account.equalsIgnoreCase("checking account")) {
            return new CheckingAccount(name, balance);
        }
        else if(account.equalsIgnoreCase("savingsaccounts") || account.equalsIgnoreCase("savings account")) {
            return new SavingsAccount(name, balance);
        }
        else{
            System.out.println("Please indicate savings or checking account");
            return null;
        }
    }

}
\$\endgroup\$
1
  • \$\begingroup\$ Any feedback would be very helpful! \$\endgroup\$ – James Jun 22 '20 at 22:51
1
\$\begingroup\$

Naming

I'm sure you already know this, but naming matters. The few keystrokes that you save using w are paid for every time somebody reads the code. Consider always using more meaningful names withdrawalAmount etc.

Be cautious about reusing names of classes from the standard library. Rather than Consumer, consider giving it a more specific name to reflect what it's consuming. This will both be more descriptive and remove the name conflict with Consumer<T>.

Scenario

The scenario you've constructed feels a bit forced, which makes it more difficult to assess if the code really makes sense. Bank accounts don't really a producer/consumer relationship. Spinning up a new thread, in order to perform a single transaction feels very inefficient. On the face of it, your producer creates money and your consumer removes money. I can't help but feel like really, they are both producing transactions that would be better served by a TransactionConsumer.

Factory

Consider using an enum rather than a raw String for the account. Perhaps you need the flexibility of a string, however if you can use an enum it makes typo's less likely and removes the need for your 'else' conditions.

Generally speaking, when you can't handle a situation you want to throw an exception. The error printed if an unsupported account is specified suggests that the caller does something else do you really want this method to return null in that scenario? If you do want to support 'possibly' creating the account, consider returning an Optional instead to give the caller a hint that it could fail. Currently your main doesn't check if null is returned, so you'll end up with NullPointerExceptions from your Producer/Consumers.

If the intention is to only have the accounts created by your factory, I would consider dropping the non-balance constructor from your accounts. This would make it easier to overload the construction methods and chain them:

public BankAccount getAccount(String account, String name) {
    return getAccount(account, name, 0);
}

BankAccount

To answer question 4, no you shouldn't always include setters. If you don't want the value to be changed from outside the class after construction, don't include setters. In the future, people with less context may decide that since it's there, it must be ok to use it. setBalance is a good example of a confusing setter. Why would you want an external actor to be able to set the balance of this account? But what makes it stand out is that whilst getBalance, deposit and withdrawal are synchronised, setBalance isn't. So, somebody could do a deposit and have a receipt for a different balance entirely printed...

Money is one of those things where people start getting upset if rounding errors start impacting their balance. Generally speaking you should be using BigDecimal for money, rather than relying on double.

I'm not a huge fan of overly commented code, however in your deposit method you sleep for 300. Consider adding a comment as to why you're sleeping, or extracting the logic into a private method that indicates what the sleep is about, something like simulateSlowProcessing perhaps.

To me, withdraw has a bug. If a withdrawal comes in that is more than the balance, the thread can't escape until after the balance goes above that value. This might be ok, if you were waiting for a message to process, however it doesn't really seem to make sense for a withdrawal. If you don't have money, you expect it to tell you that and move on. Indeed, if you escape the while loop, you perform a check that suggests you want to be able to fail transactions if there isn't sufficient funds. Because the scenario isn't entirely clear, it's difficult to know what you expect the behaviour to be. Changing Main.Consumer to withdraw 500, for example can result in the program never exiting.

It also looks like there is an AccountNumber bug. Again, this is probably caused by the scenario, however you're using a static to set the account number for created accounts. However, you only do this through the 'name' constructor. You don't do it through the 'name' and 'initial balance' constructor, which is the one you're actually using.

SavingsAccount

compoundInterest has a bug in my view. You're not currently calling it, however it makes use of setBalance on the base class. You get the balance, perform some calculation and then set the balance. It's possible for deposit/withdrawal to be called between the getBalance and the setBalance which meaning that money can be lost or gained as a consequence. If you want to do stuff like this, then it would be better to use the atomic adjustment methods in the base class. So, for example you could use deposit to add a calculated amount of interest to the balance...

Your questions

  1. Whether or not to use runnable or thread class depends on what you're doing, at the moment I don't think it makes a lot of difference either way. As I've said, I'm not sure that spinning up a thread for each transaction really makes sense to me.

  2. static methods in your main class are fine, however, you want your main to be responsible for one thing, typically bootstrapping your application. With that in mind, the scope for having many static methods should be quite small. I think the same applies for static classes, they aren't 'bad', however they may be a sign that the Main class is doing too much.

  3. For multi-threading, control performed at the right level. Typically, you want to minimise the amount of time when objects are locked, so protecting balance in the base object seems to make sense. However, as I've already indicated you need to be careful about what you do in derived classes to ensure you don't accidentally break that encapsulation.

  4. Setters aren't always required, fields that you don't expect at all (open date, account number...) should also be marked as final. Some serialisation may require setters, but then you might want to reconsider your logic location.

  5. An arraylist of accounts sounds OK in principal, however you've not done anything with it the code. Whether or not it's tightly coupled really depends on what you do with it and how/if you expose it to other classes. A HashMap of accountNumber to Account might make more sense... but consider how/if your Bank needs to know the type of account and how it will identify them.

\$\endgroup\$
1
  • \$\begingroup\$ I really appreciate all the time you put into this response. thank you. Your comments/answers/critiques are very helpful. \$\endgroup\$ – James Jun 30 '20 at 21:03

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.