diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..ec376bb --- /dev/null +++ b/.gitignore @@ -0,0 +1,2 @@ +.idea +target \ No newline at end of file diff --git a/README.md b/README.md index 2e16f33..3ec961f 100644 --- a/README.md +++ b/README.md @@ -27,3 +27,22 @@ Your job is to find bugs and other problems in the implementation. 3. Additionally try to fix or list other potential code quality issues that you see. 4. You are free to create additional classes or files as needed. + +## Solution + +Two main issues in the assignment are having non-thread-safe operations and the usage of `double` for currency calculations. +Deposit and withdraw methods now use `ReentrantLock` class and `tryLock()` to ensure that only one thread can change the account balance at +a time and prevent data corruption. Using `double` for currency calculations introduces precision issues, which are resolved +with the usage of `BigDecimal`. + +Some of the other improvements include: + +- Extracting constants into the configuration file. +- Using a `logger` instead of `System.out.println()`. Trace logging has been added; to enable it, go to the resource/log4j2.xml +configuration file and set the level to trace. +- To ensure the atomicity of the transfer method, both withdraw and deposit are set to return a `boolean`. Both operations, + must be successful in order to transfer money between accounts. +- Removing the `setMoney` method and using deposit instead. +- Amount validation for negative values. +- Using `ThreadLocalRandom` which is generally recommended in a multithreaded environment. +- `JUnit` tests added. \ No newline at end of file diff --git a/pom.xml b/pom.xml index 2ca15ec..b266265 100644 --- a/pom.xml +++ b/pom.xml @@ -7,6 +7,36 @@ ch.engenius accounts 1.0-SNAPSHOT + + + org.apache.logging.log4j + log4j-api + 2.17.1 + + + org.apache.logging.log4j + log4j-core + 2.17.1 + + + org.junit.jupiter + junit-jupiter-api + 5.2.0 + test + + + + + + org.apache.maven.plugins + maven-compiler-plugin + + 8 + 8 + + + + \ No newline at end of file diff --git a/src/main/java/ch/engenius/bank/Account.java b/src/main/java/ch/engenius/bank/Account.java index b9979cb..1f1e02e 100644 --- a/src/main/java/ch/engenius/bank/Account.java +++ b/src/main/java/ch/engenius/bank/Account.java @@ -1,31 +1,44 @@ package ch.engenius.bank; +import ch.engenius.bank.util.ThreadContext; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + import java.math.BigDecimal; public class Account { - private double money; - - public void withdraw(double amount) { - if ((money - amount) < 0) { - throw new IllegalStateException("not enough credits on account"); - } - setMoney(money - amount); - - } - - public void deposit(double amount) { - setMoney(money + amount); - } - - public double getMoney() { - return money; + private BigDecimal balance = new BigDecimal("0"); + private static final Logger log = LogManager.getLogger(Account.class); + private final ThreadContext threadContext = new ThreadContext(); + + public boolean withdraw(BigDecimal amount) throws InterruptedException { + return threadContext.withThreadLock(() -> { + if (balance.compareTo(amount) < 0) { + log.info("There is not enough balance on this account"); + return false; + } + if (amount.compareTo(BigDecimal.ZERO) <= 0) { + log.info("Amount cannot be less than or equal to zero."); + return false; + } + balance = balance.subtract(amount); + return true; + }); } - public void setMoney(double money) { - this.money = money; + public boolean deposit(BigDecimal amount) throws InterruptedException { + return threadContext.withThreadLock(() -> { + try { + balance = balance.add(amount); + return true; + } catch (Exception e) { + log.error("Exception occurred {}", e.getMessage()); + return false; + } + }); } - public BigDecimal getMoneyAsBigDecimal() { - return BigDecimal.valueOf(money); + public BigDecimal getBalance() { + return balance; } } diff --git a/src/main/java/ch/engenius/bank/Bank.java b/src/main/java/ch/engenius/bank/Bank.java index 571ebc7..ef8002f 100644 --- a/src/main/java/ch/engenius/bank/Bank.java +++ b/src/main/java/ch/engenius/bank/Bank.java @@ -1,18 +1,36 @@ package ch.engenius.bank; -import java.util.HashMap; +import java.math.BigDecimal; +import java.util.concurrent.ConcurrentHashMap; public class Bank { - private HashMap accounts = new HashMap<>(); + private final ConcurrentHashMap accounts = new ConcurrentHashMap<>(); + + public void registerAccount(int accountNumber, BigDecimal amount) throws InterruptedException { + if (accounts.containsKey(accountNumber)) { + throw new IllegalArgumentException("Account with number " + accountNumber + " already exits."); + } - public Account registerAccount(int accountNumber, int amount) { Account account = new Account(); - account.setMoney(amount); + account.deposit(amount); accounts.put(accountNumber, account); - return account; } - public Account getAccount( int number) { + public Account getAccount(int number) { return accounts.get(number); } + + public void transfer(Account fromAccount, Account toAccount, BigDecimal amount) throws InterruptedException { + boolean isWithdrawn = fromAccount.withdraw(amount); + if (isWithdrawn) { + boolean isDeposited = false; + try { + isDeposited = toAccount.deposit(amount); + } finally { + if (!isDeposited) { + fromAccount.deposit(amount); + } + } + } + } } diff --git a/src/main/java/ch/engenius/bank/BankRunner.java b/src/main/java/ch/engenius/bank/BankRunner.java index 10b30fd..0942a1c 100644 --- a/src/main/java/ch/engenius/bank/BankRunner.java +++ b/src/main/java/ch/engenius/bank/BankRunner.java @@ -1,71 +1,95 @@ package ch.engenius.bank; +import ch.engenius.bank.config.BankRunnerConstants; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + import java.math.BigDecimal; -import java.util.Random; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeUnit; import java.util.stream.IntStream; public class BankRunner { + final Bank bank = new Bank(); + private static final ExecutorService executor = Executors.newFixedThreadPool(BankRunnerConstants.THREAD_POOL_NUMBER); + private static final Logger logger = LogManager.getLogger(BankRunner.class); + private final ThreadLocalRandom random = ThreadLocalRandom.current(); - private static final ExecutorService executor = Executors.newFixedThreadPool(8); - - private final Random random = new Random(43); - private final Bank bank = new Bank(); - - - public static void main(String[] args) { + public static void main(String[] args) throws InterruptedException { BankRunner runner = new BankRunner(); - int accounts = 100; - int defaultDeposit = 1000; - int iterations = 10000; - runner.registerAccounts(accounts, defaultDeposit); - runner.sanityCheck(accounts, accounts*defaultDeposit); - runner.runBank(iterations, accounts); - runner.sanityCheck(accounts, accounts*defaultDeposit); - + runner.registerAccounts(); + runner.sanityCheck(); + runner.runBank(); + runner.sanityCheck(); } - private void runBank(int iterations, int maxAccount) { - for (int i =0; i< iterations; i++ ) { - executor.submit( ()-> runRandomOperation(maxAccount)); + private void runBank() { + List> randomOperationTasks = new ArrayList<>(); + for (int i = 0; i < BankRunnerConstants.ITERATIONS; i++) { + randomOperationTasks.add(() -> { + runRandomOperation(); + return null; + }); } try { + executor.invokeAll(randomOperationTasks); executor.shutdown(); - executor.awaitTermination(100,TimeUnit.SECONDS); + if (!executor.awaitTermination(100, TimeUnit.SECONDS)) { + logger.error("Executor did not terminate within the specified timeout."); + } } catch (InterruptedException e) { - e.printStackTrace(); + Thread.currentThread().interrupt(); + logger.error("Execution was interrupted", e); + } finally { + if (!executor.isShutdown()) { + executor.shutdownNow(); + } } } - private void runRandomOperation(int maxAccount) { - double transfer = random.nextDouble()*100.0; - int accountInNumber = random.nextInt(maxAccount); - int accountOutNumber = random.nextInt(maxAccount); - Account accIn =bank.getAccount(accountInNumber); - Account accOut =bank.getAccount(accountOutNumber); - accIn.deposit(transfer); - accOut.withdraw(transfer); + private void runRandomOperation() { + BigDecimal transferAmount = BigDecimal.valueOf(random.nextDouble() * 100.0); + int toAccountNumber = random.nextInt(BankRunnerConstants.ACCOUNTS_NUMBER); + int fromAccountNumber = random.nextInt(BankRunnerConstants.ACCOUNTS_NUMBER); + Account toAccount = bank.getAccount(toAccountNumber); + Account fromAccount = bank.getAccount(fromAccountNumber); + + logger.trace("Before transaction - From Account {}: {}", fromAccountNumber, fromAccount.getBalance()); + logger.trace("Before transaction - To Account {}: {}", toAccountNumber, toAccount.getBalance()); + + try { + bank.transfer(fromAccount, toAccount, transferAmount); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + logger.error("Thread interrupted while running random operation", e); + } + + logger.trace("After transaction - From Account {}: {}", fromAccountNumber, fromAccount.getBalance()); + logger.trace("After transaction - To Account {}: {}", toAccountNumber, toAccount.getBalance()); } - private void registerAccounts(int number, int defaultMoney) { - for ( int i = 0; i < number; i++) { - bank.registerAccount(i, defaultMoney); + private void registerAccounts() throws InterruptedException { + for (int i = 0; i < BankRunnerConstants.ACCOUNTS_NUMBER; i++) { + bank.registerAccount(i, BankRunnerConstants.DEPOSIT); + logger.trace("Account {} registered with initial DEPOSIT: {}", i, BankRunnerConstants.DEPOSIT); } } - private void sanityCheck( int accountMaxNumber, int totalExpectedMoney) { - BigDecimal sum = IntStream.range(0, accountMaxNumber) - .mapToObj( bank::getAccount) - .map ( Account::getMoneyAsBigDecimal) - .reduce( BigDecimal.ZERO, BigDecimal::add); + private void sanityCheck() { + BigDecimal sum = IntStream.range(0, BankRunnerConstants.ACCOUNTS_NUMBER) + .mapToObj(bank::getAccount) + .map(Account::getBalance) + .reduce(BigDecimal.ZERO, BigDecimal::add); - if ( sum.intValue() != totalExpectedMoney) { - throw new IllegalStateException("we got "+ sum + " != " + totalExpectedMoney +" (expected)"); + if (sum.compareTo(BankRunnerConstants.TOTAL_EXPECTED_AMOUNT) != 0) { + throw new IllegalStateException("we got " + sum + " != " + BankRunnerConstants.TOTAL_EXPECTED_AMOUNT + " (expected)"); } - System.out.println("sanity check OK"); + logger.info("sanity check OK"); } - } diff --git a/src/main/java/ch/engenius/bank/config/BankRunnerConstants.java b/src/main/java/ch/engenius/bank/config/BankRunnerConstants.java new file mode 100644 index 0000000..3441c28 --- /dev/null +++ b/src/main/java/ch/engenius/bank/config/BankRunnerConstants.java @@ -0,0 +1,11 @@ +package ch.engenius.bank.config; + +import java.math.BigDecimal; + +public class BankRunnerConstants { + public static final BigDecimal DEPOSIT = new BigDecimal("1000"); + public static final int ITERATIONS = 10000; + public static final int ACCOUNTS_NUMBER = 100; + public static final BigDecimal TOTAL_EXPECTED_AMOUNT = DEPOSIT.multiply(BigDecimal.valueOf(ACCOUNTS_NUMBER)); + public static final int THREAD_POOL_NUMBER = 8; +} diff --git a/src/main/java/ch/engenius/bank/util/ThreadContext.java b/src/main/java/ch/engenius/bank/util/ThreadContext.java new file mode 100644 index 0000000..ce02dd5 --- /dev/null +++ b/src/main/java/ch/engenius/bank/util/ThreadContext.java @@ -0,0 +1,21 @@ +package ch.engenius.bank.util; + +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReentrantLock; +import java.util.function.Supplier; + +public class ThreadContext { + ReentrantLock lock = new ReentrantLock(); + + public boolean withThreadLock(Supplier operation) throws InterruptedException { + if (lock.tryLock(1, TimeUnit.SECONDS)) { + try { + return operation.get(); + } finally { + lock.unlock(); + } + } else { + return false; + } + } +} diff --git a/src/main/resources/log4j2.xml b/src/main/resources/log4j2.xml new file mode 100644 index 0000000..32c8f6b --- /dev/null +++ b/src/main/resources/log4j2.xml @@ -0,0 +1,13 @@ + + + + + + + + + + + + + \ No newline at end of file diff --git a/src/test/java/ch/engenius/bank/AccountTest.java b/src/test/java/ch/engenius/bank/AccountTest.java new file mode 100644 index 0000000..8b66b62 --- /dev/null +++ b/src/test/java/ch/engenius/bank/AccountTest.java @@ -0,0 +1,79 @@ +package ch.engenius.bank; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.math.BigDecimal; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +public class AccountTest { + private Account account; + + @BeforeEach + public void setUp() { + account = new Account(); + } + + @Test + public void withdrawFromAccount_whenInsufficientBalance_returnsFalse() throws InterruptedException { + BigDecimal initialBalance = new BigDecimal("100"); + account.deposit(initialBalance); + + BigDecimal withdrawAmount = new BigDecimal("150"); + + boolean result = account.withdraw(withdrawAmount); + + assertFalse(result, "Withdraw should return false when there is insufficient balance"); + assertEquals(initialBalance, account.getBalance(), "Balance should remain unchanged after a failed withdraw"); + } + + @Test + public void whenDepositToAccount_withPositiveAmount_balanceIsIncreased() throws InterruptedException { + BigDecimal initialBalance = account.getBalance(); + + BigDecimal depositAmount = new BigDecimal("100"); + account.deposit(depositAmount); + + BigDecimal expectedBalance = initialBalance.add(depositAmount); + assertEquals(expectedBalance, account.getBalance(), "Deposit should increase the balance"); + } + + @Test + public void whenWithdrawFromAccount_withSufficientAmount_balanceIsDecreased() throws InterruptedException { + BigDecimal initialBalance = new BigDecimal("200"); + account.deposit(initialBalance); + + BigDecimal withdrawAmount = new BigDecimal("50"); + account.withdraw(withdrawAmount); + + BigDecimal expectedBalance = initialBalance.subtract(withdrawAmount); + assertEquals(expectedBalance, account.getBalance(), "Withdraw should decrease the balance"); + } + + @Test + public void whenWithdrawFromAccount_withNegativeAmount_returnFalse() throws InterruptedException { + BigDecimal initialBalance = new BigDecimal("100"); + account.deposit(initialBalance); + + BigDecimal withdrawAmount = new BigDecimal("-50"); + + boolean result = account.withdraw(withdrawAmount); + + assertFalse(result, "Amount should be greater or equal to zero"); + assertEquals(initialBalance, account.getBalance(), "Balance should remain unchanged after a failed withdraw"); + } + + @Test + public void whenWithdrawFromAccount_withZeroAmount_returnFalse() throws InterruptedException { + BigDecimal initialBalance = new BigDecimal("100"); + account.deposit(initialBalance); + + BigDecimal withdrawAmount = BigDecimal.ZERO; + boolean result = account.withdraw(withdrawAmount); + + assertFalse(result, "Amount should be greater or equal to zero"); + assertEquals(initialBalance, account.getBalance(), "Balance should remain unchanged after a failed withdraw"); + } +} diff --git a/src/test/java/ch/engenius/bank/BankTest.java b/src/test/java/ch/engenius/bank/BankTest.java new file mode 100644 index 0000000..3fcce05 --- /dev/null +++ b/src/test/java/ch/engenius/bank/BankTest.java @@ -0,0 +1,66 @@ +package ch.engenius.bank; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.math.BigDecimal; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +public class BankTest { + private Bank bank; + + @BeforeEach + public void setUp() { + bank = new Bank(); + } + + @Test + void givenAccountNumberAndDeposit_whenRegisterAccount_accountShouldBeAddedToBank() throws InterruptedException { + int accountNumber = 1; + BigDecimal initialDeposit = new BigDecimal("500"); + + bank.registerAccount(accountNumber, initialDeposit); + + assertNotNull(bank.getAccount(accountNumber), "Registered account should not be null"); + assertEquals(initialDeposit, bank.getAccount(accountNumber).getBalance(), "Initial deposit should match"); + } + + @Test + public void getNonExistentAccount_returnsNull() { + int nonExistentAccountNumber = 99; + assertNull(bank.getAccount(nonExistentAccountNumber), "Getting a non-existent account should return null"); + } + + @Test + void testTransferWithThreads() throws InterruptedException { + Account account = new Account(); + BigDecimal initialBalance = new BigDecimal("1000"); + account.deposit(initialBalance); + + Thread thread1 = new Thread(() -> { + try { + account.withdraw(new BigDecimal("700")); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + }); + + Thread thread2 = new Thread(() -> { + try { + account.deposit(new BigDecimal("100")); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + }); + + thread1.start(); + thread2.start(); + thread1.join(); + thread2.join(); + + assertEquals(new BigDecimal("400"), account.getBalance()); + } +}