From 717c72d98790816d39086325102adc711415d1d8 Mon Sep 17 00:00:00 2001 From: jelvuc Date: Fri, 2 Feb 2024 14:41:20 +0100 Subject: [PATCH 1/2] fix and refactor bad bank --- .gitignore | 2 + README.md | 19 ++++ pom.xml | 30 ++++++ src/main/java/ch/engenius/bank/Account.java | 51 ++++++---- src/main/java/ch/engenius/bank/Bank.java | 26 +++-- .../java/ch/engenius/bank/BankRunner.java | 95 +++++++++++-------- .../bank/config/BankRunnerConstants.java | 11 +++ .../ch/engenius/bank/util/ThreadContext.java | 21 ++++ src/main/resources/log4j2.xml | 13 +++ .../java/ch/engenius/bank/AccountTest.java | 79 +++++++++++++++ src/test/java/ch/engenius/bank/BankTest.java | 68 +++++++++++++ 11 files changed, 350 insertions(+), 65 deletions(-) create mode 100644 .gitignore create mode 100644 src/main/java/ch/engenius/bank/config/BankRunnerConstants.java create mode 100644 src/main/java/ch/engenius/bank/util/ThreadContext.java create mode 100644 src/main/resources/log4j2.xml create mode 100644 src/test/java/ch/engenius/bank/AccountTest.java create mode 100644 src/test/java/ch/engenius/bank/BankTest.java 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..73917d0 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..64d2c50 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); - + private BigDecimal money = 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 (money.compareTo(amount) < 0) { + log.error("There is not enough money on this account"); + return false; + } + if (amount.compareTo(BigDecimal.ZERO) <= 0) { + log.error("Amount cannot be less than zero."); + return false; + } + money = money.subtract(amount); + return true; + }); } - public void deposit(double amount) { - setMoney(money + amount); + public boolean deposit(BigDecimal amount) throws InterruptedException { + return threadContext.withThreadLock(() -> { + try { + money = money.add(amount); + return true; + } catch (Exception e) { + log.error("Exception occurred {}", e.getMessage()); + return false; + } + }); } - public double getMoney() { + public BigDecimal getMoney() { return money; } - - public void setMoney(double money) { - this.money = money; - } - - public BigDecimal getMoneyAsBigDecimal() { - return BigDecimal.valueOf(money); - } } diff --git a/src/main/java/ch/engenius/bank/Bank.java b/src/main/java/ch/engenius/bank/Bank.java index 571ebc7..39354c0 100644 --- a/src/main/java/ch/engenius/bank/Bank.java +++ b/src/main/java/ch/engenius/bank/Bank.java @@ -1,18 +1,32 @@ 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 Account registerAccount(int accountNumber, int amount) { + public void registerAccount(int accountNumber, BigDecimal amount) throws InterruptedException { 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..614b76c 100644 --- a/src/main/java/ch/engenius/bank/BankRunner.java +++ b/src/main/java/ch/engenius/bank/BankRunner.java @@ -1,71 +1,86 @@ 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.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() { + for (int i = 0; i < BankRunnerConstants.ITERATIONS; i++) { + executor.submit(this::runRandomOperation); } try { executor.shutdown(); - executor.awaitTermination(100,TimeUnit.SECONDS); + boolean terminationStatus = executor.awaitTermination(100, TimeUnit.SECONDS); + + if (!terminationStatus) { + logger.error("Executor did not terminate within the specified timeout."); + executor.shutdownNow(); + } } catch (InterruptedException e) { e.printStackTrace(); + logger.error("InterruptedException occurred: {}", e.getMessage()); } } - 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.getMoney()); + logger.trace("Before transaction - To Account {}: {}", toAccountNumber, toAccount.getMoney()); + + 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.getMoney()); + logger.trace("After transaction - To Account {}: {}", toAccountNumber, toAccount.getMoney()); } - 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::getMoney) + .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..f42a89b --- /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.getMoney(), "Balance should remain unchanged after a failed withdraw"); + } + + @Test + public void whenDepositToAccount_withPositiveAmount_balanceIsIncreased() throws InterruptedException { + BigDecimal initialBalance = account.getMoney(); + + BigDecimal depositAmount = new BigDecimal("100"); + account.deposit(depositAmount); + + BigDecimal expectedBalance = initialBalance.add(depositAmount); + assertEquals(expectedBalance, account.getMoney(), "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.getMoney(), "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.getMoney(), "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.getMoney(), "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..ce763c6 --- /dev/null +++ b/src/test/java/ch/engenius/bank/BankTest.java @@ -0,0 +1,68 @@ +package ch.engenius.bank; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.RepeatedTest; +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; +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).getMoney(), "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.getMoney()); + } +} From 5d2d315b2cd06160bcd671c494153b7d9ec16568 Mon Sep 17 00:00:00 2001 From: jelvuc Date: Fri, 2 Feb 2024 16:46:01 +0100 Subject: [PATCH 2/2] rename variables and check if account exists --- README.md | 18 ++++----- src/main/java/ch/engenius/bank/Account.java | 16 ++++---- src/main/java/ch/engenius/bank/Bank.java | 4 ++ .../java/ch/engenius/bank/BankRunner.java | 37 ++++++++++++------- .../java/ch/engenius/bank/AccountTest.java | 12 +++--- src/test/java/ch/engenius/bank/BankTest.java | 6 +-- 6 files changed, 52 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 73917d0..3ec961f 100644 --- a/README.md +++ b/README.md @@ -30,19 +30,19 @@ Your job is to find bugs and other problems in the implementation. ## 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. +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 +- 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, +- 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. +- 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 +- Using `ThreadLocalRandom` which is generally recommended in a multithreaded environment. +- `JUnit` tests added. \ 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 64d2c50..1f1e02e 100644 --- a/src/main/java/ch/engenius/bank/Account.java +++ b/src/main/java/ch/engenius/bank/Account.java @@ -7,21 +7,21 @@ import java.math.BigDecimal; public class Account { - private BigDecimal money = new BigDecimal("0"); + 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 (money.compareTo(amount) < 0) { - log.error("There is not enough money on this account"); + if (balance.compareTo(amount) < 0) { + log.info("There is not enough balance on this account"); return false; } if (amount.compareTo(BigDecimal.ZERO) <= 0) { - log.error("Amount cannot be less than zero."); + log.info("Amount cannot be less than or equal to zero."); return false; } - money = money.subtract(amount); + balance = balance.subtract(amount); return true; }); } @@ -29,7 +29,7 @@ public boolean withdraw(BigDecimal amount) throws InterruptedException { public boolean deposit(BigDecimal amount) throws InterruptedException { return threadContext.withThreadLock(() -> { try { - money = money.add(amount); + balance = balance.add(amount); return true; } catch (Exception e) { log.error("Exception occurred {}", e.getMessage()); @@ -38,7 +38,7 @@ public boolean deposit(BigDecimal amount) throws InterruptedException { }); } - public BigDecimal getMoney() { - return 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 39354c0..ef8002f 100644 --- a/src/main/java/ch/engenius/bank/Bank.java +++ b/src/main/java/ch/engenius/bank/Bank.java @@ -7,6 +7,10 @@ public class Bank { 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."); + } + Account account = new Account(); account.deposit(amount); accounts.put(accountNumber, account); diff --git a/src/main/java/ch/engenius/bank/BankRunner.java b/src/main/java/ch/engenius/bank/BankRunner.java index 614b76c..0942a1c 100644 --- a/src/main/java/ch/engenius/bank/BankRunner.java +++ b/src/main/java/ch/engenius/bank/BankRunner.java @@ -5,6 +5,9 @@ import org.apache.logging.log4j.Logger; import java.math.BigDecimal; +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; @@ -26,42 +29,48 @@ public static void main(String[] args) throws InterruptedException { } private void runBank() { + List> randomOperationTasks = new ArrayList<>(); for (int i = 0; i < BankRunnerConstants.ITERATIONS; i++) { - executor.submit(this::runRandomOperation); + randomOperationTasks.add(() -> { + runRandomOperation(); + return null; + }); } try { + executor.invokeAll(randomOperationTasks); executor.shutdown(); - boolean terminationStatus = executor.awaitTermination(100, TimeUnit.SECONDS); - - if (!terminationStatus) { + if (!executor.awaitTermination(100, TimeUnit.SECONDS)) { logger.error("Executor did not terminate within the specified timeout."); - executor.shutdownNow(); } } catch (InterruptedException e) { - e.printStackTrace(); - logger.error("InterruptedException occurred: {}", e.getMessage()); + Thread.currentThread().interrupt(); + logger.error("Execution was interrupted", e); + } finally { + if (!executor.isShutdown()) { + executor.shutdownNow(); + } } } - private void runRandomOperation() { + 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.getMoney()); - logger.trace("Before transaction - To Account {}: {}", toAccountNumber, toAccount.getMoney()); + 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) { + } catch (InterruptedException e) { Thread.currentThread().interrupt(); logger.error("Thread interrupted while running random operation", e); } - logger.trace("After transaction - From Account {}: {}", fromAccountNumber, fromAccount.getMoney()); - logger.trace("After transaction - To Account {}: {}", toAccountNumber, toAccount.getMoney()); + logger.trace("After transaction - From Account {}: {}", fromAccountNumber, fromAccount.getBalance()); + logger.trace("After transaction - To Account {}: {}", toAccountNumber, toAccount.getBalance()); } private void registerAccounts() throws InterruptedException { @@ -74,7 +83,7 @@ private void registerAccounts() throws InterruptedException { private void sanityCheck() { BigDecimal sum = IntStream.range(0, BankRunnerConstants.ACCOUNTS_NUMBER) .mapToObj(bank::getAccount) - .map(Account::getMoney) + .map(Account::getBalance) .reduce(BigDecimal.ZERO, BigDecimal::add); if (sum.compareTo(BankRunnerConstants.TOTAL_EXPECTED_AMOUNT) != 0) { diff --git a/src/test/java/ch/engenius/bank/AccountTest.java b/src/test/java/ch/engenius/bank/AccountTest.java index f42a89b..8b66b62 100644 --- a/src/test/java/ch/engenius/bank/AccountTest.java +++ b/src/test/java/ch/engenius/bank/AccountTest.java @@ -26,18 +26,18 @@ public void withdrawFromAccount_whenInsufficientBalance_returnsFalse() throws In boolean result = account.withdraw(withdrawAmount); assertFalse(result, "Withdraw should return false when there is insufficient balance"); - assertEquals(initialBalance, account.getMoney(), "Balance should remain unchanged after a failed withdraw"); + assertEquals(initialBalance, account.getBalance(), "Balance should remain unchanged after a failed withdraw"); } @Test public void whenDepositToAccount_withPositiveAmount_balanceIsIncreased() throws InterruptedException { - BigDecimal initialBalance = account.getMoney(); + BigDecimal initialBalance = account.getBalance(); BigDecimal depositAmount = new BigDecimal("100"); account.deposit(depositAmount); BigDecimal expectedBalance = initialBalance.add(depositAmount); - assertEquals(expectedBalance, account.getMoney(), "Deposit should increase the balance"); + assertEquals(expectedBalance, account.getBalance(), "Deposit should increase the balance"); } @Test @@ -49,7 +49,7 @@ public void whenWithdrawFromAccount_withSufficientAmount_balanceIsDecreased() th account.withdraw(withdrawAmount); BigDecimal expectedBalance = initialBalance.subtract(withdrawAmount); - assertEquals(expectedBalance, account.getMoney(), "Withdraw should decrease the balance"); + assertEquals(expectedBalance, account.getBalance(), "Withdraw should decrease the balance"); } @Test @@ -62,7 +62,7 @@ public void whenWithdrawFromAccount_withNegativeAmount_returnFalse() throws Inte boolean result = account.withdraw(withdrawAmount); assertFalse(result, "Amount should be greater or equal to zero"); - assertEquals(initialBalance, account.getMoney(), "Balance should remain unchanged after a failed withdraw"); + assertEquals(initialBalance, account.getBalance(), "Balance should remain unchanged after a failed withdraw"); } @Test @@ -74,6 +74,6 @@ public void whenWithdrawFromAccount_withZeroAmount_returnFalse() throws Interrup boolean result = account.withdraw(withdrawAmount); assertFalse(result, "Amount should be greater or equal to zero"); - assertEquals(initialBalance, account.getMoney(), "Balance should remain unchanged after a failed withdraw"); + 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 index ce763c6..3fcce05 100644 --- a/src/test/java/ch/engenius/bank/BankTest.java +++ b/src/test/java/ch/engenius/bank/BankTest.java @@ -1,13 +1,11 @@ package ch.engenius.bank; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.RepeatedTest; 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; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -27,7 +25,7 @@ void givenAccountNumberAndDeposit_whenRegisterAccount_accountShouldBeAddedToBank bank.registerAccount(accountNumber, initialDeposit); assertNotNull(bank.getAccount(accountNumber), "Registered account should not be null"); - assertEquals(initialDeposit, bank.getAccount(accountNumber).getMoney(), "Initial deposit should match"); + assertEquals(initialDeposit, bank.getAccount(accountNumber).getBalance(), "Initial deposit should match"); } @Test @@ -63,6 +61,6 @@ void testTransferWithThreads() throws InterruptedException { thread1.join(); thread2.join(); - assertEquals(new BigDecimal("400"), account.getMoney()); + assertEquals(new BigDecimal("400"), account.getBalance()); } }