Account Balance doesnt update - thread synchronization Java

Question

So I have a task to create a program, that has 1 bank account with a starting balance. Then I create a set of cards that can access that account, deposit or withdraw an amount, but not at the same time. I have created a number of threads and then use these threads to access the bank account, withdraw or deposit a random amount, then exit.

My card class:

public class card implements Runnable {
private int id;
private account account;
private int count = 1;


card(int id, account acc1){
    this.id = id;
    this.account = acc1;

}

public int getid(){
    return this.id;
}

public int localBalance(){
    return account.getBalance();
}


public void run() {
    for(count = 0; count <= 5; count++) { //withdraw and deposit random amounts
        int transactionamount = (int)(Math.random()*10);
        if (Math.random() > 0.5) {
        System.out.printf("%-12s%-12s%-12s%-12s\n","card id:(" + getid() + ")"," ",transactionamount, account.getBalance());
        account.deposit(transactionamount);
        } else {
        System.out.printf("%-12s%-12s%-12s%-12s\n","card id:(" + getid() + ")",transactionamount," ",account.getBalance());
        account.withdraw(transactionamount);

        }
        }
        }
}

my account class:

public class account {

private int balance = 5000;


public account(int balance){
    this.balance = balance;
}

public synchronized void withdraw(int amount) {
    balance =   getBalance() - amount;
}

public synchronized void deposit(int amount) {
    balance =   getBalance() + amount;
}

public synchronized int getBalance() {
    return this.balance; 
}


public synchronized void setNewBalance(int localBalance) { //no longer in use
    balance = localBalance;
}

}

my program class

public class Program {

public static void main(String[] args ) throws InterruptedException {
    // TODO Auto-generated method stub

    int numberofcards = 5;
    int accountbalance = 5000;


    card cardArray[] = new card[numberofcards];

    account acc1 = new account(accountbalance); //pass into account the starting balance.

    System.out.printf("\n%-12s%-12s%-12s%-12s\n","Transaction","Withdrawal","Deposit","Balance");
    System.out.printf("%-12s%-12s%-12s%-12s\n"," "," "," ",accountbalance + "\n");

    Thread[] threads = new Thread[numberofcards];

    for(int i = 1; i < numberofcards; i++ ){ 
       cardArray[i] = new card(i, acc1);
       threads[i] = new Thread(cardArray[i]);
       threads[i].start();
    }
    for(int i = 1; i < threads.length; i++ ){
       threads[i].join();
    }
    System.out.printf("\n%-12s%-12s%-12s%-12s\n","finished"," "," ","Final Balance: " + acc1.getBalance()); 


}

}

example of output:

http://puu.sh/lvoE5/05ebcd4c74.png

As you can see, it works however for each different card it goes back to the 5000 and then deposits or withdraws a random amount. I cant seem to figure out why my code doesnt constantly update the amount. Any help would be amazing, im amazed ive got this far by myself anyway.

edit: got rid of the not needed new thread .join code & got rid of synchronized in my run method, output is still the same however, please keep pointing out the problems though, teaching myself about threads is really difficult so ill remember. thank you in advance

edit 2: still not working, directly using withdraw and deposit now. output attached in link

Answer 1

There are multiple problems with your code, first of which is that your testing code will never interweave, as I suspect you wish it to.

Consider the following code:

  for(int i = 1; i < numberofcards; i++ ){ //create amount of cards in array of cards
      cardArray[i] = new card(i, acc1);
      new Thread(cardArray[i]).start();
      new Thread(cardArray[i]).join();
  }

For each loop, you create two thread objects (see new keyword), and for one you start the thread, and the other you call join. Since the second thread is not running, it simply returns and does nothing.

Furthermore, since your run method is synchronized, any new synchronized methods in that class will never run at the same time, which makes no sense whatsoever. See: Should you synchronize the run method? Why or why not?

This is probably what you want:

Thread threads = new Threads[numberofcards];
for(int i = 1; i < numberofcards; i++ ){ 
   cardArray[i] = new card(i, acc1);
   threads[i] = new Thread(cardArray[i]);
   threads[i].start();
}
for(int i = 1; i < threads.length; i++ ){
   threads[i].join();
}

Also, you have a race condition:

You initialize each card with $5000 this.cardBalance = account.getBalance() and you update this.cardBalance. You then update the account at account.setNewBalance(cardBalance) however during the time you set the local variable, and the time you wrote to the account, other threads have also fetched and updated the account's card balance. Your modifications to the account are not atomic.

You should not be using a local variable to represent the balance of the account in the card. This violates the separate entity rule, and has no legitimate purpose.

Answer 2

This should work, notice proper synchronization in Account class

Account:

public class Account {
    private int bankBalance;

    public Account(int initialBankBalance) {
        this.bankBalance = initialBankBalance;
    }

    public synchronized int withdraw(int cardId, int amount) {
        bankBalance = bankBalance - amount;
        System.out.println("Card: " + cardId + "; withdraw: " + amount + "; after withdrawal: " + bankBalance);
        return bankBalance;
    }

    public synchronized int deposit(int cardId, int amount) {
        bankBalance = bankBalance + amount;
        System.out.println("Card: " + cardId + "; deposit: " + amount + "; after deposit: " + bankBalance);
        return bankBalance;
    }

    public synchronized int getBankBalance() {
        return bankBalance;
    }
}

Card:

public class Card implements Runnable {
    private int id;
    private Account account;

    public Card(int id, Account account) {
        this.account = account;
        this.id = id;
    }

    @Override
    public void run() {
        double depositTotal = 0;
        double withdrawTotal = 0;
        for (int i = 0; i < 5; i++) {
            if (Math.random() > 0.5) {
                int deposit = (int) (Math.random() * 10);
                depositTotal = depositTotal + deposit;
                account.deposit(id, deposit);
            } else {
                int withdraw = (int) (Math.random() * 10);
                withdrawTotal = withdrawTotal + withdraw;
                account.withdraw(id, withdraw);
            }
            try {
                Thread.sleep(2000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

        System.out.println("Card: " + id + "; withdrawal total: " + withdrawTotal);
        System.out.println("Card: " + id + "; deposit total: " + depositTotal);
    }
}

Program:

    public class Program {

    public static void main(String[] args) throws InterruptedException {
        int numCards = 2;
        int initialBankBalance = 1000;

        Account account = new Account(initialBankBalance);
        System.out.println("Starting balance: " + initialBankBalance);

        Thread[] threads = new Thread[numCards];

        for (int i = 0; i < numCards; i++) {
            Thread thread = new Thread(new Card(i+1, account));
            thread.start();
            threads[i] = thread;
        }

        for (int i = 0; i < threads.length; i++) {
            threads[i].join();
        }

        System.out.println("End balance is : " + account.getBankBalance());

    }
}

Output from a sample run:

Starting balance: 1000
Card: 1; deposit: 3; after deposit: 1003
Card: 2; withdraw: 1; after withdrawal: 1002
Card: 2; deposit: 6; after deposit: 1008
Card: 1; deposit: 3; after deposit: 1011
Card: 1; withdraw: 7; after withdrawal: 1004
Card: 2; deposit: 1; after deposit: 1005
Card: 1; deposit: 0; after deposit: 1005
Card: 2; withdraw: 7; after withdrawal: 998
Card: 1; withdraw: 2; after withdrawal: 996
Card: 2; withdraw: 7; after withdrawal: 989
Card: 2; withdrawal total: 15.0
Card: 1; withdrawal total: 9.0
Card: 2; deposit total: 7.0
Card: 1; deposit total: 6.0
End balance is : 989