Bxdty

I am trying to implement a bank account with Java in a thread safe way. My code looks like:

import java.math.BigDecimal;

import java.math.RoundingMode;

import java.util.concurrent.locks.ReadWriteLock;

import java.util.concurrent.locks.ReentrantReadWriteLock;



/**

 * Class to represent an account, it also provides with methods to add and withdraw amount from the account.

 *

 * @author Karan Khanna

 * @version 1.0

 * @since 3/17/2019

 */

public class Account {



    private ReadWriteLock accountLock;



    private BigDecimal balance;



    private String accountNumber;



    private String accountHolder;



    public Account(String accountNumber, String accountHolder) {

        this.balance = new BigDecimal(0);

        this.accountNumber = accountNumber;

        this.accountHolder = accountHolder;

        this.accountLock = new ReentrantReadWriteLock();

    }



    public double getBalance() {

        this.accountLock.readLock().lock();

        double balance = this.balance.setScale(2, RoundingMode.HALF_DOWN).doubleValue();

        this.accountLock.readLock().unlock();

        return balance;

    }



    public String getAccountNumber() {

        return accountNumber;

    }



    public String getAccountHolder() {

        return accountHolder;

    }



    public ReadWriteLock getAccountLock() {

        return accountLock;

    }



    public void addAmount(double amount) {

        this.accountLock.writeLock().lock();

        this.balance.add(new BigDecimal(amount));

        this.accountLock.writeLock().unlock();

    }



    public void withdrawAmount(double amount) {

        this.accountLock.writeLock().lock();

        this.balance.subtract(new BigDecimal(amount));

        this.accountLock.writeLock().unlock();

    }

}

I am looking for feedback for the implementation.

In terms of the basic thread locking, it looks like it is doing the right thing, but there are a number of issues in how you are calculating the account balance, and also some escaped locking as well.

Note, your post is titled "Synchronized implementation", but it is not, it is a locked implementation. Synchronization is different, and, in this case, it may be a simpler mechanism.

Locking

Even if you don't catch exceptions, you should always use the try/finally mechanism for locking. Here, for example, it's possible that the addition may throw an exception (even though you don't catch it):

public void addAmount(double amount) {

    this.accountLock.writeLock().lock();

    try {

        this.balance.add(new BigDecimal(amount));

    } finally {

        this.accountLock.writeLock().unlock();

    }

}

In case you think that's extreme, well, the amount could be NaN or infinity, and that would throw a NumberFormatException, etc. Even if it were impossible for the logic to throw an error, you should still use the try/finally mechanism because it makes the logic obvious.

The balance method has the most to gain:

public double getBalance() {

    this.accountLock.readLock().lock();

    try {

        return this.balance.setScale(2, RoundingMode.HALF_DOWN).doubleValue();

    } finally {

        this.accountLock.readLock().unlock();

    }

}

You are also leaking the lock through the public method to get it. You really should not allow other people to manipulate the lock strategy you have in your class. It is intended to be internal for a reason.

Speaking of that lock, you should also make it final...

private final ReadWriteLock accountLock;

Bugs

The most glaring issue is not with your locking, but with the balance management itself. BigDecimals are immutable. They cannot be changed. This does nothing: this.balance.add(new BigDecimal(amount)); .... that should be this.balance = this.balance.add(new BigDecimal(amount));.

The accountNumber and accountHolder should be final as well.

Finally, the getBalance method will not always return a 2-decimal double value. Not all values in binary floating-point are representable in decimal.