diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..7160789 --- /dev/null +++ b/.gitignore @@ -0,0 +1,3 @@ +.idea +accounts.iml +target \ No newline at end of file diff --git a/pom.xml b/pom.xml index 2ca15ec..48a388a 100644 --- a/pom.xml +++ b/pom.xml @@ -7,6 +7,32 @@ ch.engenius accounts 1.0-SNAPSHOT + + + junit + junit + 4.12 + test + + + org.mockito + mockito-junit-jupiter + 3.6.0 + test + + + + + + org.apache.maven.plugins + maven-compiler-plugin + + 8 + 8 + + + + \ 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..2263eb0 100644 --- a/src/main/java/ch/engenius/bank/Account.java +++ b/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); - } } diff --git a/src/main/java/ch/engenius/bank/Bank.java b/src/main/java/ch/engenius/bank/Bank.java index 571ebc7..55bf83a 100644 --- a/src/main/java/ch/engenius/bank/Bank.java +++ b/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 accounts = new HashMap<>(); - public Account registerAccount(int accountNumber, int amount) { - Account account = new Account(); - account.setMoney(amount); + private final Map 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; } + } diff --git a/src/main/java/ch/engenius/bank/BankRunner.java b/src/main/java/ch/engenius/bank/BankRunner.java index 10b30fd..55d2b06 100644 --- a/src/main/java/ch/engenius/bank/BankRunner.java +++ b/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"); } - } diff --git a/src/notes b/src/notes new file mode 100644 index 0000000..e695ff8 --- /dev/null +++ b/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 diff --git a/src/test/java/AccountTest.java b/src/test/java/AccountTest.java new file mode 100644 index 0000000..ad69575 --- /dev/null +++ b/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); + } + +} diff --git a/src/test/java/BankTest.java b/src/test/java/BankTest.java new file mode 100644 index 0000000..ce905f5 --- /dev/null +++ b/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); + } + +} diff --git a/src/test/java/BankTestSuite.java b/src/test/java/BankTestSuite.java new file mode 100644 index 0000000..c188be7 --- /dev/null +++ b/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 { +} diff --git a/src/test/java/TestConstants.java b/src/test/java/TestConstants.java new file mode 100644 index 0000000..b872164 --- /dev/null +++ b/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; + + +}