Bad bank refactor

.gitignore

@@ -0,0 +1,2 @@
+.idea
+target

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.

pom.xml

@@ -7,6 +7,36 @@
     <groupId>ch.engenius</groupId>
     <artifactId>accounts</artifactId>
     <version>1.0-SNAPSHOT</version>
+    <dependencies>
+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-api</artifactId>
+            <version>2.17.1</version>
+        </dependency>
+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-core</artifactId>
+            <version>2.17.1</version>
+        </dependency>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-api</artifactId>
+            <version>5.2.0</version>
+            <scope>test</scope>
+        </dependency>
+    </dependencies>
+    <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>
 
 
 </project>

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

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

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<Callable<Void>> 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");
     }
 
-
 }

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

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<Boolean> operation) throws InterruptedException {
+        if (lock.tryLock(1, TimeUnit.SECONDS)) {
+            try {
+                return operation.get();
+            } finally {
+                lock.unlock();
+            }
+        } else {
+            return false;
+        }
+    }
+}

src/main/resources/log4j2.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<Configuration status="WARN">
+    <Appenders>
+        <Console name="Console" target="SYSTEM_OUT">
+            <PatternLayout pattern="%d{HH:mm:ss.SSS} [%t] %-5level %logger{36} - %msg%n"/>
+        </Console>
+    </Appenders>
+    <Loggers>
+        <Root level="debug">
+            <AppenderRef ref="Console"/>
+        </Root>
+    </Loggers>
+</Configuration>