Concurrency Bank exercise with multiple accounts

I am doing an exercise in Java concurrency. The problem statement is as follows:


Bank holds an array of Accounts. Client do (in a loop) the following
operations: (1) work, then sleep for random time (2) check account
balance and withdraw random amount if balance>0 (3) work, then sleep
for random time (4) deposit random ammout


Multiple clients may access the same account (which I have implemented
by choosing random account each time).

Could you please take a look at my code and let me know if something is wrong or is it could be simplified?

Bank class

import java.util.ArrayList;

public class Bank {
    private ArrayList accounts;

    public Bank(int accountsCount) {
        accounts = new ArrayList();
        for (int i = 0; i < accountsCount; i++) {
            accounts.add(new Account(i));
        }
    }

    public Account getRandomAccount() {
        return accounts.get((int) (Math.random() * accounts.size()));
    }

    public void printSummary() {
        for (Account a : accounts)
            System.out.println(a.toString());
    }
}

Account class

```
public class Account {
private int balance = 1000;
private int number;

public Account(int number) {
this.number = number;
}

public synchronized int getBalance() {
return balance;
}

public synchronized void withdraw(Client client, int bal) {
try {

if (balance >= bal) {
System.out.println(client.getName() + " "
+ "is trying to withdraw from account " + number);
try {
Thread.sleep(100);
} catch (Exception e) {
e.printStackTrace();
}
balance = balance - bal;
System.out.println(client.getName() + " "
+ "has completed withdrawal from account " + number);
} else {
System.out
.printl

According to this StackOverflow post, you should be using java.util.Random for generating random numbers.

Now, the method getRandomAccount of Bank becomes:

return accounts.get( rand.nextInt( accounts.size() ) + 1 );

Note that rand is not created inside the method itself. rand should be stored as a private field member and should be set to an instance of Random during the creation of an instance of Bank.

That way, rand is only created and seeded once because the number is automatically seeded upon creation of a new Random.

Right now, your method printSummary of Bank is printing the toString of all of it's accounts.

To me, this method sounds like it is creating a toString of itself because it is printing out the information of it's only property: accounts.

Therefore, I recommend that you rename this method to toString.

At these try/catch statements:

try {
    Thread.sleep(100);
} catch (Exception e) {
    e.printStackTrace();
}

and

} catch (Exception e) {
    e.printStackTrace();
}

of the withdraw method of Account(however, the same applies to the deposit method), you are simply catching a general exception.

It is good practice to catch the/a specific exception(s) that could be thrown from your code. This also allows you to choose which ones you'd like to catch, and which you'd like to ignore.

Hint: If you'd like to find out which exceptions could be thrown, read the Java API documentation.

In the methods randAmount and randSleepTime, you are creating a new Random class with every call.

However, as I stated above, this is not a good idea because you are destroying your old seed and attempting to make a new one with every all.

You should create a private field of type Random, set to a new instance of java.util.Random once, and then call nextInt as you please.

In the methods deposit and withdraw of Account, I recommend that you create and throw your own exception.

You could call it something like InsufficientMoneyException that would be thrown when an attempt at a transaction is made, but there is too little money to complete the transaction. For example, when a user attempts to withdraw too much money.

By throwing custom exceptions, if you were to be adding some more functions that automated interaction with banks, accounts, and clients, they could have try/catch statements that were watching for the InsufficientMoneyException to be thrown.

See this StackOverflow post for information on creating custom exceptions.

From what I know about concurrency in Java (which is not the strongest), your code looks absolutely fine in your synchronized sections.

Even though I've never looked too much into concurrency in Java, this code was very easy to follow.

StackExchange Code Review Q#95213, answer score: 3