MrShoffen - currency-exchange.md

https://github.com/MrShoffen/currency-exchange/
Ревью от Ильи @coderilya

Ревью проекта “Обмен валют” для @MrShoffen Ссылка на его проект: https://github.com/MrShoffen/currency-exchange/

Проект получился интересный так Андрей уже начал использовать библиотеку (guice) для DI, mapstruct, hibernate-validator, что на опережение плана немного.

1. Момент хотел подсветить с DI. Инжектить в проекте желательно было через конструктор (или поправь меня Андрей), а не через поле, если по аналогии со спрингом (в гайде по guice видел примерный функционал библиотеки и есть инъекция через конструктор). О плюсах можно почитать в поисковике, запрос “способы инъекции зависимостей spring”. https://github.com/MrShoffen/currency-exchange/blob/178c89daf6d906ae594aed95e83485c2276e098c/src/main/java/org/mrshoffen/exchange/service/CurrencyService.java#L23-L30

2. Следующий момент тоже с DI связан. В Guice есть аннотация синглтон и описано дефолтное поведение (оно нас не совсем устраивает) поэтому надо было вероятнее использовать аннотацию, которая обеспечит что будет один объект инжектится в классы. (может в DependencyManager так и работает, я бы убедился просто) https://habr.com/ru/articles/358278/

3. Есть неиспользуемые импорты. Их можно удалить сочетанием ctr + alt + o, а ещё в меню коммита снизу есть коллесико и там есть настройка связанная с импортами. Сюда же можно докинуть работу с чек стайлером, потому что вижу что код не чекается -но в целом код пишется близко к чистому (забываешь комменты только удалять, может их // TODO делать тогда их идея чекнет и перед пушем прямо о них напомнит). https://github.com/MrShoffen/currency-exchange/blob/178c89daf6d906ae594aed95e83485c2276e098c/src/main/java/org/mrshoffen/exchange/dto/request/CurrencyRequestDto.java#L3-L5

4. Код не видишь как писать более емко местами, например здесь просто !equals можно оставлять же только. Проверить что поля не нул ты валидатором можешь (или почему у тебя и аннотации, и ручной подход…) https://github.com/MrShoffen/currency-exchange/blob/178c89daf6d906ae594aed95e83485c2276e098c/src/main/java/org/mrshoffen/exchange/validator/DifferentCurrenciesValidator.java#L12-L26

5. Можно сокращать до одной строки в подобных местах. Переменная используется один - значит может её не объявлять тогда. https://github.com/MrShoffen/currency-exchange/blob/178c89daf6d906ae594aed95e83485c2276e098c/src/main/java/org/mrshoffen/exchange/servlet/CurrenciesServlet.java#L39-L41

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
   writeJsonValueToResponse(resp, currencyService.findAll());
}

6. Magic number. Старайся описывать подобные значения в константы, проблема не просто так называется магическими числа - если человек только погружается в твой код для него любые значения могут быть магией (на пет проекте так и не скажешь, но в целом людям нравится думать что их код понятный… это не так) 3, 6 и подобное в константы желательно, а if с чеком длины кода - это дублирование, выноси в метод раз уже два раза повторяется + название метода тоже избавляет от деталей и меньше засоряется метод. Ещё туда что у тебя отступы чуть по разному ведут себя везде, тоже лучше писать в оджном стиле, но это придёт со временем. https://github.com/MrShoffen/currency-exchange/blob/178c89daf6d906ae594aed95e83485c2276e098c/src/main/java/org/mrshoffen/exchange/servlet/ExchangeRateServlet.java#L42-L50

7. Используй подходящий тип, не строку. Аннотация и валидатор тоже не нужна на этот случай. При приведение строки к нужному типу можно либо паттерном регулрки чекать (объект паттерн переиспользовать лучше), либо обернуть в try-catch (если там не число) и уже чтобы в дто лежал нужный тип после обработки HttpRequest из сервлета https://github.com/MrShoffen/currency-exchange/blob/178c89daf6d906ae594aed95e83485c2276e098c/src/main/java/org/mrshoffen/exchange/dto/request/ExchangeRequestDto.java#L26-L27

8. Поля в дто объявляй приватными, это не рекорды) (некретично)

9. Холиварная тема, кто-то считает что можно на это 404 возвращать, кому не нравится (у меня было что надо было 400 возвращать). Не говорю здесь как надо, просто можно почитать и для себя решить. А по методу в целом, то можно было бы логировать ошибки здесь - фильтры в целом удобное место для этого - есть респонс, реквест. https://github.com/MrShoffen/currency-exchange/blob/178c89daf6d906ae594aed95e83485c2276e098c/src/main/java/org/mrshoffen/exchange/filter/ExceptionHandlerFilter.java#L29

10. Не лучший подход если ты уже начинаешь в DI. Мы открываем пул коннектов, не не закрываем. В спринге это делает метод дестрой бина, как воспроизвести на гугловой библиотечке я не знаю. В принципе частая проблема 3-го проекта наверно (если использовать пулл коннектов), но подсветил так как у тебя и в целом заход в сторону DI. https://github.com/MrShoffen/currency-exchange/blob/178c89daf6d906ae594aed95e83485c2276e098c/src/main/java/org/mrshoffen/exchange/utils/ConnectionManager.java#L18-L31

11. Сервис слой ок, но чуть переносы смотрятся не в java стиле а kotlin/scala, но вкусовщина (в любом коллективе она тоже будет).

12. В дао не мьють своими эксепшенами те что ты перехватываешь (в свой добавь и оригинальный), потом не поймёшь что не так когда у тебя заместо нормального ответа “Error! Failed to find currencies in database

Проект понравился, потенциал Андрей у тебя хороший, желаю 4-ой проект написать на ура)