Concurrency Bank exercise with multiple accounts

Question

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<Account> accounts;
 public Bank(int accountsCount) {
 accounts = new ArrayList<Account>();
 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
 .println(client.getName()
 + " "
 + "doesn't have enough money for withdraw from account "
 + number);
 }
 System.out.println(client.getName() + " withdrawal value: "
 + balance + " from account " + number);
 } catch (Exception e) {
 e.printStackTrace();
 }
 }
 public synchronized void deposit(Client client, int bal) {
 try {
 if (bal > 0) {
 System.out.println(client.getName() + " "
 + "trying to deposit on account " + number);
 try {
 Thread.sleep(100);
 } catch (Exception e) {
 e.printStackTrace();
 }
 balance = balance + bal;
 System.out.println(client.getName() + " "
 + "has completed the deposit on account " + number);
 } else {
 System.out.println(client.getName() + " "
 + " doesn't have enough money for deposit on account "
 + number);
 }
 System.out.println(client.getName() + " deposited " + balance
 + " on account " + number);
 } catch (Exception e) {
 e.printStackTrace();
 }
 }
 @Override
 public String toString() {
 return "Account " + number + " balance: " + balance;
 }
}

Client class

public class Client {
 private String name;
 public Client(String name) {
 this.name = name;
 }
 public String getName() {
 return name;
 }
 public void setName(String name) {
 this.name = name;
 }
 @Override
 public String toString() {
 return name;
 }
}

Main class

import java.util.Random;
import java.util.logging.Level;
import java.util.logging.Logger;
public class Main extends Thread implements Runnable {
 private static final int CLIENT_COUNT = 5;
 private static final int CLIENT_ITERATIONS = 5;
 private static final int ACCOUNTS_COUNT = 2;
 private static final int RAND_MIN = 10;
 private static final int RAND_MAX = 10;
 private static final int RAND_MIN_AMOUNT = 10;
 private static final int RAND_MAX_AMOUNT = 200;
 private static Bank bank = new Bank(ACCOUNTS_COUNT);
 private Client client;
 public Main(Client c) {
 client = c;
 }
 private int randAmount() {
 return (new Random()).nextInt((RAND_MAX_AMOUNT - RAND_MIN_AMOUNT) + 1)
 + RAND_MIN_AMOUNT;
 }
 private int randSleepTime() {
 return (new Random()).nextInt((RAND_MAX - RAND_MIN) + 1) + RAND_MIN;
 }
 public static void main(String[] args) {
 for (int i = 0; i < CLIENT_COUNT; i++) {
 Main ts1 = new Main(new Client("Person " + i));
 ts1.start();
 }
 }
 private void clientWork() {
 try {
 Thread.sleep(randSleepTime());
 } catch (InterruptedException ex) {
 Logger.getLogger(Main.class.getName()).log(Level.SEVERE, null, ex);
 }
 }
 @Override
 public void run() {
 for (int i = 0; i < CLIENT_ITERATIONS; i++) {
 Account acc = bank.getRandomAccount();
 clientWork();
 acc.withdraw(client, randAmount());
 clientWork();
 acc.deposit(client, randAmount());
 }
 System.out.println("#Account balance: " + Account.getBalance());
 }
}

• JS1: I'm confused about why your balance is a static variable. Are all the accounts sharing the same balance? Shouldn't there be one balance per account?
• syntagma: @JS1 You're right, it's a mistake.
• JimmyB: And what's public static Account account; in Account for?
• syntagma: @HannoBinder Thanks, I should also delete it.
• JimmyB: In return (Account) accounts.get(... the cast (Account) is unnecessary. But my points are merely cosmetic. Functionally your code seems correct, and the style follows best practices.

Answer 1

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.