7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ FWIW, you can always implement AutoCloseable in your lock and do try-with-resources on your operations - IMVHO, that would make both the intention more obvious, the code would be shorter, and the risk of omitting the unlock (either as a typo or after throwing in method) would be mitigated. I dunno why Java doesn't provide this functionality as default... \$\endgroup\$ – vaxquis Mar 18 at 11:42
12
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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")));
    }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.