Fix and refactor Bad Bank assignment

.gitignore

@@ -0,0 +1,3 @@
+.idea/*
+target/*
+

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.  
+

pom.xml

@@ -7,6 +7,32 @@
     <groupId>ch.engenius</groupId>
     <artifactId>accounts</artifactId>
     <version>1.0-SNAPSHOT</version>
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-compiler-plugin</artifactId>
+                <configuration>
+                    <source>8</source>
+                    <target>8</target>
+                </configuration>
+            </plugin>
+        </plugins>
+    </build>
+    <dependencies>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-engine</artifactId>
+            <version>5.8.1</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter</artifactId>
+            <version>5.8.1</version>
+            <scope>test</scope>
+        </dependency>
+    </dependencies>
 
 
 </project>

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;
     }
 }

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<Integer, Account> accounts = new HashMap<>();
+    private final ConcurrentHashMap<Integer, Account> accounts = new ConcurrentHashMap<>();
+    private final ConcurrentHashMap<Integer, ReentrantLock> 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);
     }
+
 }

src/main/java/ch/engenius/bank/BankRunner.java

@@ -1,71 +1,97 @@
 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();
 
 
     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<Callable<Void>> 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.");
     }
-
-
 }