diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..6322372
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,3 @@
+.idea/*
+target/*
+
diff --git a/ClassDiagram.JPG b/ClassDiagram.JPG
new file mode 100644
index 0000000..163a883
Binary files /dev/null and b/ClassDiagram.JPG differ
diff --git a/README.md b/README.md
index 2e16f33..9f0d66c 100644
--- a/README.md
+++ b/README.md
@@ -27,3 +27,38 @@ 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.
+Corrections:
+1) The setMoney() was removed from the Account class to promote encapsulation and ensure proper handling of account balance modifications.
+By removing the setMoney method, we prevent direct access to the money variable from outside the class, ensuring that all modifications
+to account balances must go through the deposit and withdraw methods. This approach helps maintain data integrity and prevents misuse
+of the Account class. Instead of using the setMoney method to set the initial balance during account registration, we use the deposit
+method, which is more appropriate in this context. By making these changes, we ensure that the Account class follows the principle of
+encapsulation and provides a cleaner and more robust implementation.
+
+2) Using double for currency calculations can lead to precision issues. It's better to use BigDecimal for currency-related calculations.
+This change eliminates any potential issues with floating-point arithmetic inaccuracies.
+
+3) It's better to use Loggers instead of System.out.println(). Loggers can be configured to output log messages to various destinations,
+such as console, files, or remote logging services. This makes it easy to collect and manage logs from different parts of application or
+even multiple applications. Logging frameworks usually handle log messages more efficiently than System.out.println() particularly when
+writing to file or other output streams.
+
+4) When the InterruptedException is caught, it's generally a good practise to either re-interrupt the current thread or propagate the
+exception further. The Thread.currentThread().interrupt() method call re-interrupts the current thread, allowing other parts of code to
+handle interruption if necessary. Also, we log the exception using LOGGER instead of calling e.printStackTrace().
+
+5) Exception in thread "main" java.lang.IllegalStateException: we got 106524.836545496162378 != 100000 (expected)
+ at ch.engenius.bank.BankRunner.sanityCheck(BankRunner.java:65)
+ at ch.engenius.bank.BankRunner.main(BankRunner.java:26)
+This issue is solved with introducing lock method using Reentrant lock in deposit and withdraw methods. This ensures that only one thread
+can modify the balance of an account at a time, and that the threads can access the transfer method in parallel. This will prevent the balance
+from being corrupted and ensure thread safety.
+
+6) In original code for runRandomOperation() the deposit method was first called, and then withdraw, but the correct way is to first withdraw money
+and then do the deposit. The transfer logic was also moved to a separate method and put in the Bank class.
+
+7) For ExecutorService part in runBank() method it is better to add the tasks in a list through iteration and then use invokeAll() method that
+ensure us that all the tasks will be finished before we shutdown the service.
+
+8) Added JUnit tests, class diagram, .gitignore, javadoc. Names of some variables and methods are changed to be more descriptive.
+
diff --git a/pom.xml b/pom.xml
index 2ca15ec..5baf266 100644
--- a/pom.xml
+++ b/pom.xml
@@ -7,6 +7,32 @@
ch.engeniusaccounts1.0-SNAPSHOT
+
+
+
+ org.apache.maven.plugins
+ maven-compiler-plugin
+
+ 8
+ 8
+
+
+
+
+
+
+ org.junit.jupiter
+ junit-jupiter-engine
+ 5.8.1
+ test
+
+
+ org.junit.jupiter
+ junit-jupiter
+ 5.8.1
+ test
+
+
\ 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..6b891ff 100644
--- a/src/main/java/ch/engenius/bank/Account.java
+++ b/src/main/java/ch/engenius/bank/Account.java
@@ -1,31 +1,71 @@
package ch.engenius.bank;
import java.math.BigDecimal;
+import java.util.concurrent.locks.ReentrantLock;
public class Account {
- private double money;
+ private BigDecimal balance = BigDecimal.ZERO;
+ private final ReentrantLock lock = new ReentrantLock();
- public void withdraw(double amount) {
- if ((money - amount) < 0) {
- throw new IllegalStateException("not enough credits on account");
+ /**
+ * Withdraws the specified amount from this account.
+ * @param amount the amount to withdraw
+ * @throws IllegalArgumentException if the amount is null or non-positive
+ * @throws IllegalStateException if there is not enough balance to cover the withdrawal
+ */
+ public void withdraw(BigDecimal amount) {
+ validateAmount(amount);
+ lock.lock();
+ try {
+ if (!hasEnoughBalance(amount)) {
+ throw new IllegalStateException("Not enough credits on account!");
+ }
+ balance = balance.subtract(amount);
+ } finally {
+ lock.unlock();
}
- setMoney(money - amount);
-
}
- public void deposit(double amount) {
- setMoney(money + amount);
+ /**
+ * Deposits the specified amount into this account.
+ * @param amount the amount to deposit
+ * @throws IllegalArgumentException if the amount is null or non-positive
+ */
+ public void deposit(BigDecimal amount) {
+ validateAmount(amount);
+ lock.lock();
+ try {
+ balance = balance.add(amount);
+ } finally {
+ lock.unlock();
+ }
}
- public double getMoney() {
- return money;
+ /**
+ * Returns the current balance of this account.
+ * @return the current balance
+ */
+ public BigDecimal getBalance() {
+ return balance;
}
- public void setMoney(double money) {
- this.money = money;
+ /**
+ * Validates the specified amount.
+ * @param amount the amount to validate
+ * @throws IllegalArgumentException if the amount is null or non-positive
+ */
+ private void validateAmount(BigDecimal amount) {
+ if (amount == null || amount.compareTo(BigDecimal.ZERO) <= 0) {
+ throw new IllegalArgumentException("Amount must be positive and not null.");
+ }
}
- public BigDecimal getMoneyAsBigDecimal() {
- return BigDecimal.valueOf(money);
+ /**
+ * Checks if this account has enough balance to cover the specified amount.
+ * @param amount the amount to check
+ * @return true if there is enough balance, false otherwise
+ */
+ private boolean hasEnoughBalance(BigDecimal amount) {
+ return balance.compareTo(amount) >= 0;
}
}
diff --git a/src/main/java/ch/engenius/bank/Bank.java b/src/main/java/ch/engenius/bank/Bank.java
index 571ebc7..1b8e460 100644
--- a/src/main/java/ch/engenius/bank/Bank.java
+++ b/src/main/java/ch/engenius/bank/Bank.java
@@ -1,18 +1,67 @@
package ch.engenius.bank;
-import java.util.HashMap;
+import java.math.BigDecimal;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.ReentrantLock;
public class Bank {
- private HashMap accounts = new HashMap<>();
+ private final ConcurrentHashMap accounts = new ConcurrentHashMap<>();
+ private final ConcurrentHashMap locks = new ConcurrentHashMap<>();
+
+ /**
+ * Registers a new account with the bank.
+ *
+ * @param accountNumber the account number to register.
+ * @param amount the initial deposit amount.
+ * @return the newly created account.
+ * @throws IllegalArgumentException if the account number is already registered or if the initial deposit amount is negative.
+ */
+ public Account registerAccount(int accountNumber, BigDecimal amount) {
+ if (accounts.containsKey(accountNumber)) {
+ throw new IllegalArgumentException("Account number " + accountNumber + " is already registered.");
+ }
- 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) {
- return accounts.get(number);
+ /**
+ * Retrieves an account by its number.
+ *
+ * @param number the account number to retrieve.
+ * @return the account with the given number.
+ * @throws IllegalArgumentException if no account with the given number exists.
+ */
+ public Account getAccount(int number) {
+ Account account = accounts.get(number);
+ if (account == null) {
+ throw new IllegalArgumentException("Account number " + number + " does not exist.");
+ }
+
+ return account;
+ }
+
+ /**
+ * Transfers an amount of money from one account to another.
+ *
+ * @param fromAccount the number of the account to withdraw money from.
+ * @param toAccount the number of the account to deposit money into.
+ * @param amount the amount of money to transfer.
+ * @throws IllegalArgumentException if the source and destination accounts are the same, or if either account does not exist,
+ * or if the source account has insufficient funds.
+ */
+ public void transfer(int fromAccount, int toAccount, BigDecimal amount) {
+ if (fromAccount == toAccount) {
+ throw new IllegalArgumentException("The source and destination accounts cannot be the same.");
+ }
+
+ Account accOut = getAccount(fromAccount);
+ Account accIn = getAccount(toAccount);
+
+ accOut.withdraw(amount);
+ accIn.deposit(amount);
}
+
}
diff --git a/src/main/java/ch/engenius/bank/BankRunner.java b/src/main/java/ch/engenius/bank/BankRunner.java
index 10b30fd..acdb008 100644
--- a/src/main/java/ch/engenius/bank/BankRunner.java
+++ b/src/main/java/ch/engenius/bank/BankRunner.java
@@ -1,15 +1,23 @@
package ch.engenius.bank;
import java.math.BigDecimal;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Random;
+import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
-import java.util.concurrent.TimeUnit;
+import java.util.logging.Level;
import java.util.stream.IntStream;
+import java.util.logging.Logger;
public class BankRunner {
- private static final ExecutorService executor = Executors.newFixedThreadPool(8);
+ private static final ExecutorService EXECUTOR = Executors.newFixedThreadPool(8);
+ private static final Logger LOGGER = Logger.getLogger(BankRunner.class.getName());
+ private static final int NUM_ACCOUNTS = 100;
+ private static final BigDecimal INITIAL_DEPOSIT = BigDecimal.valueOf(1000);
+ private static final int NUM_ITERATIONS = 10000;
private final Random random = new Random(43);
private final Bank bank = new Bank();
@@ -17,55 +25,73 @@ public class BankRunner {
public static void main(String[] args) {
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(NUM_ACCOUNTS, INITIAL_DEPOSIT);
+ runner.verifyTotalBalance(NUM_ACCOUNTS, INITIAL_DEPOSIT.multiply(BigDecimal.valueOf(NUM_ACCOUNTS)));
+ runner.runBank(NUM_ITERATIONS, NUM_ACCOUNTS);
+ runner.verifyTotalBalance(NUM_ACCOUNTS, INITIAL_DEPOSIT.multiply(BigDecimal.valueOf(NUM_ACCOUNTS)));
}
+ /**
+ * Executes a number of iterations of random transfers between accounts in the bank.
+ * @param iterations The number of iterations to run
+ * @param maxAccount The maximum number of accounts in the bank
+ */
private void runBank(int iterations, int maxAccount) {
- for (int i =0; i< iterations; i++ ) {
- executor.submit( ()-> runRandomOperation(maxAccount));
+ List> transferTasks = new ArrayList<>();
+ for (int i = 0; i < iterations; i++) {
+ transferTasks.add(() -> {
+ performRandomTransfer(maxAccount);
+ return null;
+ });
}
try {
- executor.shutdown();
- executor.awaitTermination(100,TimeUnit.SECONDS);
+ EXECUTOR.invokeAll(transferTasks);
+ EXECUTOR.shutdown();
} catch (InterruptedException e) {
- e.printStackTrace();
+ LOGGER.log(Level.SEVERE, "Execution was interrupted", e);
+ Thread.currentThread().interrupt();
+ } finally {
+ EXECUTOR.shutdownNow();
}
}
- private void runRandomOperation(int maxAccount) {
- double transfer = random.nextDouble()*100.0;
+ /**
+ * Performs a random transfer between two random accounts in the bank.
+ * @param maxAccount The maximum number of accounts in the bank
+ */
+ private void performRandomTransfer(int maxAccount) {
+ BigDecimal transferAmount = BigDecimal.valueOf(random.nextDouble()).multiply(BigDecimal.valueOf(100));
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);
+ bank.transfer(accountOutNumber, accountInNumber, transferAmount);
}
- private void registerAccounts(int number, int defaultMoney) {
- for ( int i = 0; i < number; i++) {
+ /**
+ * Registers a specified number of accounts in the bank, each with a default amount of money.
+ * @param number The number of accounts to register
+ * @param defaultMoney The default amount of money for each account
+ */
+ private void registerAccounts(int number, BigDecimal defaultMoney) {
+ for (int i = 0; i < number; i++) {
bank.registerAccount(i, defaultMoney);
}
}
- private void sanityCheck( int accountMaxNumber, int totalExpectedMoney) {
+ /**
+ * Verifies that the total balance of all accounts in the bank matches an expected value.
+ * @param accountMaxNumber The maximum number of accounts in the bank
+ * @param totalExpectedMoney The expected total balance of all accounts
+ */
+ private void verifyTotalBalance(int accountMaxNumber, BigDecimal totalExpectedMoney) {
BigDecimal sum = IntStream.range(0, accountMaxNumber)
- .mapToObj( bank::getAccount)
- .map ( Account::getMoneyAsBigDecimal)
- .reduce( BigDecimal.ZERO, BigDecimal::add);
+ .mapToObj(bank::getAccount)
+ .map(Account::getBalance)
+ .reduce(BigDecimal.ZERO, BigDecimal::add);
- if ( sum.intValue() != totalExpectedMoney) {
+ if (sum.compareTo(totalExpectedMoney) != 0) {
throw new IllegalStateException("we got "+ sum + " != " + totalExpectedMoney +" (expected)");
}
- System.out.println("sanity check OK");
+ LOGGER.info("Total balance is OK.");
}
-
-
}
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..543be2a
--- /dev/null
+++ b/src/test/java/ch/engenius/bank/AccountTest.java
@@ -0,0 +1,45 @@
+package ch.engenius.bank;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.math.BigDecimal;
+
+class AccountTest {
+
+ private Account account;
+
+ @BeforeEach
+ void setUp() {
+ account = new Account();
+ account.deposit(new BigDecimal("1000"));
+ }
+
+ @Test
+ void testWithdraw() {
+ account.withdraw(new BigDecimal("500"));
+ assertEquals(new BigDecimal("500"), account.getBalance());
+ }
+
+ @Test
+ void testWithdrawInvalidAmount() {
+ assertThrows(IllegalArgumentException.class, () -> account.withdraw(new BigDecimal("-100")));
+ }
+
+ @Test
+ void testWithdrawInsufficientBalance() {
+ assertThrows(IllegalStateException.class, () -> account.withdraw(new BigDecimal("1500")));
+ }
+
+ @Test
+ void testDeposit() {
+ account.deposit(new BigDecimal("500"));
+ assertEquals(new BigDecimal("1500"), account.getBalance());
+ }
+
+ @Test
+ void testDepositInvalidAmount() {
+ assertThrows(IllegalArgumentException.class, () -> account.deposit(new BigDecimal("-100")));
+ }
+}
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..4faedf2
--- /dev/null
+++ b/src/test/java/ch/engenius/bank/BankTest.java
@@ -0,0 +1,94 @@
+package ch.engenius.bank;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.math.BigDecimal;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class BankTest {
+
+ private Bank bank;
+
+ @BeforeEach
+ void setUp() {
+ bank = new Bank();
+ }
+
+ @Test
+ void testRegisterAccount() {
+ Account account = bank.registerAccount(1, new BigDecimal("1000"));
+ assertEquals(new BigDecimal("1000"), account.getBalance());
+ }
+
+ @Test
+ void testRegisterDuplicateAccount() {
+ bank.registerAccount(1, new BigDecimal("1000"));
+ assertThrows(IllegalArgumentException.class, () -> bank.registerAccount(1, new BigDecimal("500")));
+ }
+
+ @Test
+ void testRegisterNegativeAmount() {
+ assertThrows(IllegalArgumentException.class, () -> bank.registerAccount(1, new BigDecimal("-500")));
+ }
+
+ @Test
+ void testGetAccount() {
+ Account account = bank.registerAccount(1, new BigDecimal("1000"));
+ assertEquals(account, bank.getAccount(1));
+ }
+
+ @Test
+ void testGetNonExistentAccount() {
+ assertThrows(IllegalArgumentException.class, () -> bank.getAccount(1));
+ }
+
+ @Test
+ void testTransfer() {
+ bank.registerAccount(1, new BigDecimal("1000"));
+ bank.registerAccount(2, new BigDecimal("500"));
+ bank.transfer(1, 2, new BigDecimal("500"));
+ assertEquals(new BigDecimal("500"), bank.getAccount(1).getBalance());
+ assertEquals(new BigDecimal("1000"), bank.getAccount(2).getBalance());
+ }
+
+ @Test
+ void testTransferSameAccount() {
+ bank.registerAccount(1, new BigDecimal("1000"));
+ assertThrows(IllegalArgumentException.class, () -> bank.transfer(1, 1, new BigDecimal("500")));
+ }
+
+ @Test
+ void testTransferNonExistentAccounts() {
+ assertThrows(IllegalArgumentException.class, () -> bank.transfer(1, 2, new BigDecimal("500")));
+ }
+
+ @Test
+ void testTransferInsufficientFunds() {
+ bank.registerAccount(1, new BigDecimal("500"));
+ bank.registerAccount(2, new BigDecimal("500"));
+ assertThrows(IllegalStateException.class, () -> bank.transfer(1, 2, new BigDecimal("1000")));
+ }
+
+ @Test
+ void testTransferLocking() throws InterruptedException {
+ Bank bank = new Bank();
+ Account account = bank.registerAccount(1, new BigDecimal("1000.00"));
+
+ Thread thread1 = new Thread(() -> {
+ account.withdraw(new BigDecimal("500.00"));
+ });
+
+ Thread thread2 = new Thread(() -> {
+ assertThrows(IllegalStateException.class, () -> account.withdraw(new BigDecimal("800.00")));
+ });
+
+ thread1.start();
+ thread2.start();
+ thread1.join();
+ thread2.join();
+
+ assertEquals(new BigDecimal("500.00"), account.getBalance());
+ }
+}