RaunoReview.md

gistfile1.md

1. README.md: no information on how to run and test the app.
2. README.md: no information on minimum runtime requirements (Java 11+).
3. Unused code: getSurname(), getLoans(), setLoans(...) etc. Best practice: never write/generate code "just in case".
4. Packaging: according to Common Closure Principle, classes that change together must be packaged together. So, instead of putting closely related classes in different packages (exceptions, models, enums, repositories, requests, services), put them together or split by domain (lending, client). More info here.
5. API: the API tries to be, but is not RESTful. E.g. plurals should be used: /loans, /loans/{clientUUID}
6. Architecture: domain model leaks to the API. Instead, we should decouple the domain model from our API/screens, because they change for different reasons. In practice, our APIs should always return DTOs, not entities.
7. Exception handling: in Spring, you don't need to catch exceptions and manually convert them into appropriate http responses. You can do this declaratively. See @ResponseStatus annotation.
8. Instead of using reflection (ReflectionTestUtils.setField), make certain fields package-private and, optionally, mark them with annotation @VisibleForTesting. Or better – use constructor injection.
9. Tests are difficult to reason about, because they are not written as behaviour specifications. See book Growing Object-Oriented Software Guided by Tests.
10. Not all critical functionality is covered by tests (e.g. catch blocks in applyForLoan).
11. Repository must not be responsible for object creation: findByPersonalIdOrNew.
12. Echo chambers: client.assignLoan(loan) vs. client.assign(loan), ApplyForLoan.loanAmount vs. ApplyForLoan.amount
13. Getters could have been replaced with simple nouns (e.g. client.status())
14. Getters are setters are not needed in data structures, because public fields are enough. (e.g. ApplyForLoan)
15. Code smell: null arguments (e.g. loanController.applyForLoan(loanApplication, null))
16. Architecture smell: test/dev code leaks in production code. (@Value("${localDev.mockIp:}") String ipOverwrite; or @Value("${api.notASecret}")).
17. Anemic domain model: Client and Loans are just data bags, not objects.
18. Misused Optionals: return country.isPresent() ? country.get() : new Country(); Use country.orElseGet(Country::new); instead.
19. Variable type inference is not used when appropriate (e.g. HttpRequest request = HttpRequest.newBuilder() could have been just var request = HttpRequest.newBuilder())
20. CountryResolver over-engineered. Simpler version is here. Could have been better with Retrofit library.
21. Unnecessary serialVersionUIDs.
22. if (ClientStatus.BLACKLISTED.equals(loan.getClient().getStatus())) { ... } -> if (client.isBlacklisted()) { ... }
23. Tests don't catch critical bugs (changing TIMEFRAME_IN_DAYS from 2 to 1);
24. Where fast unit tests are enough, slow Spring component tests are used.
25. Deprecated Date/Calendar API usage: use Java 8 date (LocalDate, LocalDateTime) instead of Date/Calendar.
26. There is no need for CountryRepository. LoanRepository would suffice.
27. Very generic names: if (count >= MAX_REQUESTS) { ... } ("count", "MAX_REQUESTS")
28. assertTrue(loanRepo.count() == 1); should be assertEquals(loanRepo.count(), 1);
29. Long train wrecks: assertEquals("EE", loanRepo.findAll().iterator().next().getCountry().getCode()); Could have been solved by introducing a loan variable. Moreover, it's better to extend from JpaRepository than CrudRepository, because its methods return Lists, not Iterators.
30. List<Borrower> findByPersonalId(String personalId); could return Optional<Borrower>
31. LoanApplication entity is missing; Fields appliedAt and country do not belong to Loan. Also Borrower, not Client.
32. IValidationRule could have been replaced with Specification pattern.