Java concurrency exercise, implementing transfer of amount from one account to another

Question

This is in continuation with my synchronized implementation of a bank account in Java. I am trying to implement bank accounts repository and method to transfer money from one account to another. My implementation for the account and the repository looks like:

Account:

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 final ReadWriteLock accountLock;

    private BigDecimal balance;

    private final String accountNumber;

    private final String accountHolder;

    public Account(String accountNumber, String accountHolder) {
        this.balance = new BigDecimal(0);
        this.accountNumber = accountNumber;
        this.accountHolder = accountHolder;
        this.accountLock = new ReentrantReadWriteLock();
    }

    public String getAccountNumber() {
        return accountNumber;
    }

    public String getAccountHolder() {
        return accountHolder;
    }

    public ReadWriteLock getAccountLock() {
        return accountLock;
    }

    public double getBalance() {
        this.accountLock.readLock().lock();
        try {
            return this.balance.setScale(2, RoundingMode.HALF_DOWN).doubleValue();
        } finally {
            this.accountLock.readLock().unlock();
        }
    }

    public void addAmount(double amount) {
        this.accountLock.writeLock().lock();
        try {
            this.balance = this.balance.add(BigDecimal.valueOf(amount));
        } finally {
            this.accountLock.writeLock().unlock();
        }
    }

    public void withdrawAmount(double amount) {
        this.accountLock.writeLock().lock();
        try {
            this.balance = this.balance.subtract(BigDecimal.valueOf(amount));
        } finally {
            this.accountLock.writeLock().unlock();
        }
    }
}

AccountsRepository:

import static java.lang.String.format;

import java.util.HashMap;
import java.util.Map;
import org.spituk.banking.app.repositories.AccountsRepository;
import org.spituk.banking.app.vo.Account;

/**
 * Map based account repository.
 *
 * @author Karan Khanna
 * @version 1.0
 * @since 3/17/2019
 */
public class AccountsRepositoryImpl implements AccountsRepository {

    private final static String SOURCE_ACCOUNT_DONOT_EXIST = "Source account number %s don't exist.";

    private final static String DESTINATION_ACCOUNT_DONOT_EXIST = "Destination account number %s don't exist.";

    private final static String NOT_ENOUGH_BALANCE = "Account number %s don't have enough balance.";

    private final Map<String, Account> accountMap;


    public AccountsRepositoryImpl() {
        this.accountMap = new HashMap<>();
    }

    @Override
    public void transferAmount(String accountNumberFrom, String  accountNumberTo, double amountToTransfer) throws IllegalArgumentException {
        if (accountMap.containsKey(accountNumberFrom)) {
            if(accountMap.get(accountNumberFrom).getBalance() < amountToTransfer) {
                if(accountMap.containsKey(accountNumberTo)) {
                    Account accountFrom = accountMap.get(accountNumberFrom);
                    Account accountTo = accountMap.get(accountNumberTo);
                    accountFrom.getAccountLock().writeLock().lock();
                    accountTo.getAccountLock().writeLock().lock();
                    try {
                        accountFrom.withdrawAmount(amountToTransfer);
                        accountTo.addAmount(amountToTransfer);
                    } finally {
                        accountFrom.getAccountLock().writeLock().unlock();
                        accountTo.getAccountLock().writeLock().unlock();
                    }
                } else {
                    throw new IllegalArgumentException(format(DESTINATION_ACCOUNT_DONOT_EXIST, accountNumberTo));
                }
            } else {
                throw new IllegalArgumentException(format(NOT_ENOUGH_BALANCE, accountNumberFrom));
            }
        } else {
            throw new IllegalArgumentException(format(SOURCE_ACCOUNT_DONOT_EXIST, accountNumberFrom));
        }
    }
}

I am looking for feedback for the implementation.

Answer 1

Some is personal taste, some is improving performance, some is avoiding deadlocks:

@Override
public void transferAmount(String accountNumberFrom, String  accountNumberTo, double amountToTransfer) throws IllegalArgumentException {
    synchronized(accountMap) {
        Account accountFrom = getAccount(accountNumberFrom, SOURCE_ACCOUNT_DONOT_EXIST);
        if (accountFrom.getBalance() >= amountToTransfer) {
            // BTW: I think that should be only > because balance==amount should be possible
            throw new IllegalArgumentException(format(NOT_ENOUGH_BALANCE, accountNumberFrom));
        }
        Account accountFrom = getAccount(accountNumberTo, DESTINATION_ACCOUNT_DONOT_EXIST);
        accountFrom.withdrawAmount(amountToTransfer);
        accountTo.addAmount(amountToTransfer);
    }
}

private Account getAccount(String accountNumber, String errorReason) {
    Account ret = accountMap.get(accountNumber);
    if (ret == null) {
        throw new IllegalArgumentException(format(errorReason, accountNumberFrom));
    }
    return ret;
}

In lack of knowledge about the details of this program, I changed everything to be synchronized. The reason is that otherwise it will lead to a deadlock, if there are two accounts each sending money to each other. Each thread might be able to aquire the first lock but then both will wait to get the second lock. In other words: You need to redesign your locking mechanism but without knowing more about the whole thing it's hard to come up with usable proposals.

What else have I changed:

• I created a method getAccount that returns the account or throws the IllegalArgumentException with the error message passed by the caller. This avoid code duplication and makes the code more readable by itself
• I personally prefer early returns and throwing exceptions. So instead of a big if-cascade that all need to pass before you do something, I prefer to check the inverted condition and return or throw immediately instead. I found that to be more understandable when looking at it after some time has passed.
• Instead of checking the existence of a key and the retrieval of the value in case it exists, it's simpler (and faster) to try to get the value and check for null unless null is a valid value (which is possible with HashMap but I doubt is the case in your application).

Some more thoughts:

• If accountMap can change (accounts added or deleted), you need to make sure that access to it is synchronized as well (with my change it already is but when you put it back to locks you need to take care of that). Otherwise you might transfer money to an account that stopped existing at that very moment and money "vanishes".
• Your account works with BigDecimal internally but you pass a double to transferAmount. That should be BigDecimal as well, otherwise you still end up with rounding problems. Your implementation of getBalance looks like you've ran into this problem already and your rounding is your workaround to fix the effect.