Csdrhrt

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.

double is not a good choice to use for currency in Java. The better option is BigDecimal (which you are using for the internal balance, but not for the parameters passed to the addAmount and withdrawAmount methods). A better approach would be to make those methods take a BigDecimal parameter instead (and to use BigDecimal everywhere in your code that deals with currency amounts).

If for some reason those methods need to take a double parameter then you should not convert it to a BigDecimal with the new BigDecimal(double) constructor - this will give an inaccurate conversion and an unexpected (and incorrect) value for the balance after the add/withdraw operation. For example, the following test fails:

    @Test

    public void demonstrateBigDecimalRoundingErrorsFromDouble() {

        BigDecimal balance = BigDecimal.ZERO;

        balance = balance.add(new BigDecimal(0.1));

        assertThat(balance, is(new BigDecimal("0.1")));

    }

with the error

java.lang.AssertionError: 

Expected: is <0.1>

     but: was <0.1000000000000000055511151231257827021181583404541015625>

The correct way to convert from a double to a BigDecimal is to use BigDecimal.valueOf(double). For example, changing the middle line in the above test will make it pass:

    @Test

    public void demonstrateBigDecimalRoundingErrorsFromDouble() {

        BigDecimal balance = BigDecimal.ZERO;

        balance = balance.add(BigDecimal.valueOf(0.1));

        assertThat(balance, is(new BigDecimal("0.1")));

    }