bad bank refactored and fixed potential problems

pom.xml

@@ -7,6 +7,43 @@
     <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.projectlombok</groupId>
+            <artifactId>lombok</artifactId>
+            <version>1.18.24</version>
+            <scope>provided</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-engine</artifactId>
+            <version>5.9.0-RC1</version>
+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-junit-jupiter</artifactId>
+            <version>4.6.1</version>
+            <scope>compile</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <version>4.6.1</version>
+            <scope>compile</scope>
+        </dependency>
+    </dependencies>
 
 
 </project>

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

@@ -1,5 +1,13 @@
 package ch.engenius.bank;
 
+import ch.engenius.bank.exception.AccountExistsException;
+import ch.engenius.bank.exception.AccountNotFoundException;
+import ch.engenius.bank.exception.IllegalAccountAmountException;
+import ch.engenius.bank.model.Account;
+import ch.engenius.bank.model.Bank;
+import ch.engenius.bank.service.AccountService;
+import ch.engenius.bank.service.BankService;
+
 import java.math.BigDecimal;
 import java.util.Random;
 import java.util.concurrent.ExecutorService;
@@ -12,60 +20,70 @@ public class BankRunner {
     private static final ExecutorService executor = Executors.newFixedThreadPool(8);
 
     private final Random random = new Random(43);
-    private final Bank bank = new Bank();
+    private Bank bank = new Bank();
+    private AccountService accountService = new AccountService();
+    private final BankService bankService = new BankService(bank, accountService);
 
 
     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);
-
+        int accountsNumber = 100;
+        BigDecimal defaultDeposit = BigDecimal.valueOf(1000);
+        int iterations = 100000;
+        runner.registerAccounts(accountsNumber, defaultDeposit);
+        runner.sanityCheck(accountsNumber, BigDecimal.valueOf(accountsNumber).multiply(defaultDeposit));
+        runner.runBank(iterations, accountsNumber);
+        runner.sanityCheck(accountsNumber, BigDecimal.valueOf(accountsNumber).multiply(defaultDeposit));
     }
 
     private void runBank(int iterations, int maxAccount) {
-        for (int i =0; i< iterations; i++ ) {
-            executor.submit( ()-> runRandomOperation(maxAccount));
+        for (int i = 0; i < iterations; i++) {
+            executor.submit(() -> runRandomOperation(maxAccount));
         }
         try {
             executor.shutdown();
-            executor.awaitTermination(100,TimeUnit.SECONDS);
+            executor.awaitTermination(100, TimeUnit.SECONDS);
         } catch (InterruptedException e) {
             e.printStackTrace();
         }
     }
 
     private void runRandomOperation(int maxAccount) {
-        double transfer = random.nextDouble()*100.0;
+        BigDecimal transferAmount = BigDecimal.valueOf(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);
+        try {
+            bankService.transferMoney(accountInNumber, accountOutNumber, transferAmount);
+        } catch (AccountNotFoundException accountNotFoundException) {
+            System.out.println("Account with account number:" + accountNotFoundException.getAccountNumber());
+        } catch (IllegalAccountAmountException illegalAccountAmountException) {
+            if (illegalAccountAmountException.getIllegalAmount().intValue() < 0) {
+                System.out.println("Amount for transfer can't be negative value");
+            } else {
+                System.out.println("insufficient funds on account with account number:" + accountOutNumber);
+            }
+        }
     }
 
-    private void  registerAccounts(int number, int defaultMoney) {
-        for ( int i = 0; i < number; i++) {
-            bank.registerAccount(i, defaultMoney);
+    private void registerAccounts(int number, BigDecimal defaultMoney) {
+        for (int i = 0; i < number; i++) {
+            try {
+                bankService.registerAccount(i, defaultMoney);
+            } catch (AccountExistsException accountExistsException) {
+                System.out.println("Account with account number:" + accountExistsException.getAccountNumber() + " already exist");
+            }
         }
     }
 
-    private void sanityCheck( int accountMaxNumber, int totalExpectedMoney) {
+    private void sanityCheck(int accountMaxNumber, BigDecimal totalExpectedMoney) {
         BigDecimal sum = IntStream.range(0, accountMaxNumber)
-                .mapToObj( bank::getAccount)
-                .map ( Account::getMoneyAsBigDecimal)
-                .reduce( BigDecimal.ZERO, BigDecimal::add);
+                .mapToObj(bankService::getAccount)
+                .map(Account::getMoney)
+                .reduce(BigDecimal.ZERO, BigDecimal::add);
 
-        if ( sum.intValue() != totalExpectedMoney) {
-            throw new IllegalStateException("we got "+ sum + " != " + totalExpectedMoney +" (expected)");
+        if (sum.compareTo(totalExpectedMoney) != 0) {
+            throw new IllegalStateException("we got " + sum + " != " + totalExpectedMoney + " (expected)");
         }
         System.out.println("sanity check OK");
     }
-
-
 }

src/main/java/ch/engenius/bank/exception/AccountExistsException.java

@@ -0,0 +1,10 @@
+package ch.engenius.bank.exception;
+
+import lombok.AllArgsConstructor;
+import lombok.Getter;
+
+@Getter
+@AllArgsConstructor
+public class AccountExistsException extends RuntimeException{
+    private int accountNumber;
+}

src/main/java/ch/engenius/bank/exception/AccountNotFoundException.java

@@ -0,0 +1,10 @@
+package ch.engenius.bank.exception;
+
+import lombok.AllArgsConstructor;
+import lombok.Getter;
+
+@Getter
+@AllArgsConstructor
+public class AccountNotFoundException extends RuntimeException{
+    private int accountNumber;
+}

src/main/java/ch/engenius/bank/exception/IllegalAccountAmountException.java

@@ -0,0 +1,12 @@
+package ch.engenius.bank.exception;
+
+import lombok.AllArgsConstructor;
+import lombok.Getter;
+
+import java.math.BigDecimal;
+
+@AllArgsConstructor
+@Getter
+public class IllegalAccountAmountException extends RuntimeException {
+    private BigDecimal illegalAmount;
+}

src/main/java/ch/engenius/bank/model/Account.java

@@ -0,0 +1,41 @@
+package ch.engenius.bank.model;
+
+import ch.engenius.bank.exception.IllegalAccountAmountException;
+import lombok.*;
+
+import java.math.BigDecimal;
+import java.util.concurrent.locks.ReentrantLock;
+
+@Getter
+@Setter
+@Builder
+@NoArgsConstructor
+@AllArgsConstructor
+public class Account {
+    private BigDecimal money;
+    private int accountNumber;
+    private ReentrantLock lock;
+
+    public void withdraw(BigDecimal amount) throws IllegalAccountAmountException {
+        validateWithdrawAmount(amount);
+        setMoney(money.subtract(amount));
+    }
+
+    public void lockAccount() {
+        this.lock.lock();
+    }
+
+    public void unlockAccount() {
+        this.lock.unlock();
+    }
+
+    public void deposit(BigDecimal amount) {
+        setMoney(money.add(amount));
+    }
+
+    private void validateWithdrawAmount(BigDecimal amount) {
+        if ((money.subtract(amount).compareTo(BigDecimal.ZERO)) < 0 || amount.compareTo(BigDecimal.ZERO) < 0) {
+            throw new IllegalAccountAmountException(amount);
+        }
+    }
+}

src/main/java/ch/engenius/bank/model/Bank.java

@@ -0,0 +1,20 @@
+package ch.engenius.bank.model;
+
+import lombok.Getter;
+
+import java.util.ArrayList;
+import java.util.List;
+
+@Getter
+public class Bank {
+    private List<Account> accounts;
+
+    public Bank(){
+        this.accounts = new ArrayList<>();
+    }
+
+    public boolean addAccount(Account newAccount) {
+        return accounts.add(newAccount);
+    }
+
+}

src/main/java/ch/engenius/bank/note

@@ -0,0 +1,5 @@
+Another improvements that could be done is adding validation service layer, so for example we would have
+BankServiceImplementaion and BankServiceValidation. Main benefit of this would be that our implementaion
+service would we responsibility to only do business logic and not bothering with validation
+
+Another good improvement would be using one of the loggers(log4j probably) for logging certain events like creating accounts, transferring money etc.

src/main/java/ch/engenius/bank/service/AccountService.java

@@ -0,0 +1,32 @@
+package ch.engenius.bank.service;
+
+import ch.engenius.bank.exception.IllegalAccountAmountException;
+import ch.engenius.bank.model.Account;
+
+import java.math.BigDecimal;
+
+public class AccountService {
+    //Idea of adding AccountService was not to expose directly Account model to BankService. This approach is also good for unit testing, because
+    //AccountService will just be mocked and verified that is called. it's not a good idea to put synchronization in model, so AccountService is also
+    //Proxy to Account with responsibility of synchronization
+
+    public void withdraw(Account account, BigDecimal amount) throws IllegalAccountAmountException {
+        try {
+            account.lockAccount();
+            account.withdraw(amount);
+        }
+        finally {
+            account.unlockAccount();
+        }
+    }
+
+    public void deposit(Account account, BigDecimal amount) {
+        try {
+            account.lockAccount();
+            account.deposit(amount);
+        }
+        finally {
+            account.unlockAccount();
+        }
+    }
+}