CurrencyExchange - Solo83.md

https://github.com/Solo83/CurrencyExchange
Ревью от Ивана

1. Это можно скипнуть Зачем сеттеры, когда можно сделать норм апи (Exchange::create, Exchange::convert(amount), e.g) для модельки и оставить геттеры для маппинга в дто (которых нету) https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/repository/exchangerates/ExchangeRate.java#L9

2. Дженерик нужно бы убрать, он тут не нужен. Тем более если класс называется CurrencyRepository https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/repository/currencies/CurrencyRepository.java#L9

3. Исключение можно было и не ловить, оно и так наверх пробросится. Другой вопрос, что не нужно пробрасывать наверх SQLException, т.к слой датасурса протекает, можно сделать кастомный эксепшн и пробросить уже его https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/repository/currencies/CurrencyRepositoryImpl.java#L28

4. Это можно скипнуть Не вижу смысла писать человекочитаемый мессадж в исключении (кроме как информацию для логов), для этого есть слой контроллеров (т.е вьюха) https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/repository/currencies/CurrencyRepositoryImpl.java#L56

5. Возможно джавка это и оптимизирует, поскольку все эти конкатенации компайл тайм, но вообще так лучше не делать. Каждый + создает новый объект, аллоцирует память по преколу, тут можно поюзать StringBuilder (или jdk повыше с мультилайн строками) https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/repository/exchangerates/ExchangeRatesRepositoryImpl.java#L24

6. Выносим в env коннекшн стринг и подобные переменные конфига https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/repository/DBConnector.java#L10

7. А вот и не ignored. С чего быть уверенным, что это именно из-за того, что в базе отсутствует обмен, а не потому что базка упала. К тому же не понятно зачем бросать исключение, если не найден обмен (так еще и с Option в сигнатуре). Возвращаешь пустой Option, это вполне корректный результат. В итоге у тебя везде проверки на ::isPresent, но при этом пустого не может быть https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/service/ExchangeService.java#L32

8. Название не согласуется со слоем и звучит слишком обобщенно, вродеб уже в сервисе и никаких строк из таблиц уже нету. Можно просто ExchangeDoesNotExistsException https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/service/ExchangeService.java#L74

9. В целом слишком много try catch и из-за этого тяжело читать. В идеале писать не более одного try catch блока на метод, (причем весь код метода должен быть внутри try). Это отсылка для книжного клуба кста https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/service/ExchangeService.java#L27

10. Чтоб таких мемов не было, используют дтохи для передачи сообщений между слоями. В твоем случае моделька используется всеми слоями, да и ее валидность тяжело отследить. В общем очень сложно и не понятно https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/service/ExchangeService.java#L37

11. Не стоит выводить на клиент внутренности (InternalServerError) через эксепшн мессадж, а вот залогировать хотелось бы https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/controllers/CurrenciesServlet.java#L36

12. То что валидация есть - это хорошо, но то, что это сделано через статику - уже не оч. Методы валидатора названы слишком обобщенно и плюс передаются захардкоженные регэкспы, из-за чего нужно вникать в написанное. Можно вынести в константы эти регэкспы, как минимум, а можно сделать отдельные классы для валидации каждого типа параметра (ValidateNumber e.g). https://github.com/Solo83/CurrencyExchange/blob/2da8a071b0a3f364b9f2c695f23adde941f19efc/src/main/java/com/solo83/currencyexchange/controllers/ExchangeServlet.java#L38

13. Хотелось бы посмотреть на констреинты в sql схеме, но ты залил свою бд целиком)