Fix all bugs, improve code quality, add tests

.gitignore

@@ -0,0 +1,3 @@
+.idea
+accounts.iml
+target

pom.xml

@@ -7,6 +7,32 @@
     <groupId>ch.engenius</groupId>
     <artifactId>accounts</artifactId>
     <version>1.0-SNAPSHOT</version>
+    <dependencies>
+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <version>4.12</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-junit-jupiter</artifactId>
+            <version>3.6.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

@@ -3,29 +3,31 @@
 import java.math.BigDecimal;
 
 public class Account {
-    private double money;
+    private BigDecimal money;
+
+    public Account(BigDecimal money) {
+        this.money = money;
+    }
 
     public void withdraw(double amount) {
-        if ((money - amount) < 0) {
+        if (transactionAllowed(BigDecimal.valueOf(amount))) {
             throw new IllegalStateException("not enough credits on account");
         }
-        setMoney(money - amount);
-
+        this.money = money.subtract(BigDecimal.valueOf(amount));
     }
 
     public void deposit(double amount) {
-        setMoney(money + amount);
+        this.money = money.add(BigDecimal.valueOf(amount));
     }
 
-    public double getMoney() {
-        return money;
+    public BigDecimal getMoney() {
+        return new BigDecimal(String.valueOf(this.money));
     }
 
-    public void setMoney(double money) {
-        this.money = money;
+    private boolean transactionAllowed(BigDecimal amount) {
+        return this.money
+                .subtract(amount)
+                .compareTo(BigDecimal.ZERO) < 0;
     }
 
-    public BigDecimal getMoneyAsBigDecimal() {
-        return BigDecimal.valueOf(money);
-    }
 }

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

@@ -1,18 +1,25 @@
 package ch.engenius.bank;
 
+import java.math.BigDecimal;
 import java.util.HashMap;
+import java.util.Map;
+import java.util.NoSuchElementException;
 
 public class Bank {
-    private HashMap<Integer, Account> accounts = new HashMap<>();
 
-    public Account registerAccount(int accountNumber, int amount) {
-        Account account = new Account();
-        account.setMoney(amount);
+    private final Map<Integer, Account> accounts = new HashMap<>();
+
+    public void registerAccount(int accountNumber, int amount) {
+        Account account = new Account(BigDecimal.valueOf(amount));
         accounts.put(accountNumber, account);
-        return account;
     }
 
-    public Account getAccount( int number) {
-        return accounts.get(number);
+    public Account getAccount(int number) {
+        Account account = accounts.get(number);
+        if (account == null) {
+            throw new NoSuchElementException();
+        }
+        return account;
     }
+
 }

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

@@ -14,58 +14,59 @@ public class BankRunner {
     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;
+        int defaultDeposit = 1000;
+        int iterations = 10000;
         runner.registerAccounts(accounts, defaultDeposit);
-        runner.sanityCheck(accounts, accounts*defaultDeposit);
+        runner.sanityCheck(accounts, accounts * defaultDeposit);
         runner.runBank(iterations, accounts);
-        runner.sanityCheck(accounts, accounts*defaultDeposit);
+        runner.sanityCheck(accounts, accounts * 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;
+    private synchronized 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);
+
+        Account accIn = bank.getAccount(accountInNumber);
+        Account accOut = bank.getAccount(accountOutNumber);
+
         accOut.withdraw(transfer);
+        accIn.deposit(transfer);
     }
 
-    private void  registerAccounts(int number, int defaultMoney) {
-        for ( int i = 0; i < number; i++) {
+    private void registerAccounts(int number, int defaultMoney) {
+        for (int i = 0; i < number; i++) {
             bank.registerAccount(i, defaultMoney);
         }
     }
 
-    private void sanityCheck( int accountMaxNumber, int totalExpectedMoney) {
+    private void sanityCheck(int accountMaxNumber, int totalExpectedMoney) {
         BigDecimal sum = IntStream.range(0, accountMaxNumber)
-                .mapToObj( bank::getAccount)
-                .map ( Account::getMoneyAsBigDecimal)
-                .reduce( BigDecimal.ZERO, BigDecimal::add);
+                .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.intValue() != totalExpectedMoney) {
+            String message = String.format("We got %f != %d (expected)", sum.doubleValue(), totalExpectedMoney);
+            throw new IllegalStateException(message);
         }
-        System.out.println("sanity check OK");
+        System.out.println("Sanity check OK");
     }
 
-
 }

src/notes

@@ -0,0 +1,35 @@
+All changes and bugs are described here.
+
+Account:
+- refactored all to BigDecimal, because floating point primitives are not exactly precise, in banking systems
+    this can cause big problems
+- added helper method for checking possibility to withdraw money
+- added constructor with BigDecimal parameter, removed setter, in order to limit possibility of manipulation regarding
+    money amount
+- passing clone of money for getter, because of need to prevent changing anything by reference from outside
+
+Bank:
+- it is always better to set interface as type of attribute because of code flexibility, so then HashMap is replaced
+    with Map as type of accounts
+- adapted to work with BigDecimals
+- removed returning type from registering Account method, since it can be considered as some type of setter
+- calling Account constructor with parameter instead of removed setter (in Account class)
+- wrapped getting account method with check for null, throwing NoSuchElementException if not found account with given
+    key
+
+BankRunner:
+- there are couple of bugs:
+    method runRandomOperation(int maxAccount):
+    - BUG 1: first it was doing deposit, and then withdrawing, in that case money can be paid in, but then if
+        amount to reduce is bigger than there are money on account, it will break flow - making bank state inconsistent
+    - BUG 2: method should be synchronized because multiple threads can execute this method in state of manipulating
+        the same account, and by that we have concurrency and some dirty read/writes. By setting this method as
+        synchronized we claiming the highest level of isolation - SERIALIZE (if we make parallel with database
+        transaction)
+- calling getter directly for getting amount of money from accounts
+
+Overall:
+- String.format() is always nicer that string concatenation
+- at classes Bank and Account left double as parameters type because I wanted to change as less as possible BankRunner
+    class
+- added some test cases, just to verify methods after completion of task

src/test/java/AccountTest.java

@@ -0,0 +1,35 @@
+import ch.engenius.bank.Account;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.math.BigDecimal;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class AccountTest {
+
+    private Account testAccount;
+
+    @Before
+    public void setUp() {
+        testAccount = new Account(BigDecimal.valueOf(TestConstants.INITIAL_ACCOUNT_MONEY));
+    }
+
+    @Test
+    public void deposit_ShouldIncreaseAmount() {
+        testAccount.deposit(TestConstants.DEPOSIT_ACCOUNT_MONEY);
+        assertEquals(TestConstants.AFTER_DEPOSIT_ACCOUNT_MONEY, testAccount.getMoney().intValue());
+    }
+
+    @Test
+    public void withdraw_ShouldDecreaseAmount() {
+        testAccount.withdraw(TestConstants.WITHDRAW_ACCOUNT_MONEY);
+        assertEquals(TestConstants.AFTER_WITHDRAW_ACCOUNT_MONEY, testAccount.getMoney().intValue());
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void withdraw_ShouldThrowException_IfMoneyInsufficient() {
+        testAccount.withdraw(TestConstants.WITHDRAW_INSUFFICIENT_ACCOUNT_MONEY);
+    }
+
+}

src/test/java/BankTest.java

@@ -0,0 +1,35 @@
+import ch.engenius.bank.Account;
+import ch.engenius.bank.Bank;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.NoSuchElementException;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class BankTest {
+
+    private Bank testBank;
+
+    @Before
+    public void setUp() {
+        testBank = new Bank();
+        testBank.registerAccount(TestConstants.ACCOUNT_KEY_1, TestConstants.ACCOUNT_MONEY_1);
+        testBank.registerAccount(TestConstants.ACCOUNT_KEY_2, TestConstants.ACCOUNT_MONEY_2);
+        testBank.registerAccount(TestConstants.ACCOUNT_KEY_3, TestConstants.ACCOUNT_MONEY_3);
+    }
+
+    @Test
+    public void getAccount_ShouldReturnExistingOne() {
+        Account firstQueriedAccount = testBank.getAccount(TestConstants.ACCOUNT_KEY_1);
+        assertEquals(TestConstants.ACCOUNT_MONEY_1, firstQueriedAccount.getMoney().intValue());
+        Account secondQueriedAccount = testBank.getAccount(TestConstants.ACCOUNT_KEY_2);
+        assertEquals(TestConstants.ACCOUNT_MONEY_2, secondQueriedAccount.getMoney().intValue());
+    }
+
+    @Test(expected = NoSuchElementException.class)
+    public void getAccount_ShouldThrowException_IfAccountKeyDoesNotExist() {
+        testBank.getAccount(TestConstants.NOT_EXISTING_ACCOUNT_KEY);
+    }
+
+}

src/test/java/BankTestSuite.java

@@ -0,0 +1,10 @@
+import org.junit.runner.RunWith;
+import org.junit.runners.Suite;
+
+@RunWith(Suite.class)
+@Suite.SuiteClasses({
+        AccountTest.class,
+        BankTest.class,
+})
+public class BankTestSuite {
+}

src/test/java/TestConstants.java

@@ -0,0 +1,21 @@
+public class TestConstants {
+
+    // -------------------- ACCOUNT TEST DATA ---------------------------//
+    public static final int INITIAL_ACCOUNT_MONEY = 10000;
+    public static final int DEPOSIT_ACCOUNT_MONEY = 100;
+    public static final int WITHDRAW_ACCOUNT_MONEY = 200;
+    public static final int AFTER_DEPOSIT_ACCOUNT_MONEY = 10100;
+    public static final int AFTER_WITHDRAW_ACCOUNT_MONEY = 9800;
+    public static final int WITHDRAW_INSUFFICIENT_ACCOUNT_MONEY = 11000;
+
+    // --------------------- BANK TEST DATA --------------------- //
+    public static final int ACCOUNT_KEY_1 = 1;
+    public static final int ACCOUNT_KEY_2 = 2;
+    public static final int ACCOUNT_KEY_3 = 3;
+    public static final int NOT_EXISTING_ACCOUNT_KEY = 5;
+    public static final int ACCOUNT_MONEY_1 = 1000;
+    public static final int ACCOUNT_MONEY_2 = 1500;
+    public static final int ACCOUNT_MONEY_3 = 2000;
+
+
+}