DarkRubin - CurrencyExchange.md

https://github.com/DarkRubin/CurrencyExchange

Автор ревью Stepan Primshic

1. Слишком много действий в одной строчке https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/CurrenciesServlet.java#L41C11-L41C81

2. Работай на уровне интерфейсов. List P.S. Здесь стоит просто переменные использовать. https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/CurrenciesServlet.java#L33C9-L33C26

3. Больше похоже на util метод. Gson тоже можно обернуть https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/StartServlet.java#L33C1-L36C6

https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/StartServlet.java#L18C4-L18C80

4. Тоже можно обернуть в свой метод принимающий реквест и возвращающий стринг https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/CurrencyServlet.java#L21C8-L21C62

5. Обрабатываешь ошибку чтобы изъять из этого какую-то бизнес логику и пробрасываешь дальше?

• Метод лучше назвать что-то типа saveOrReturnExisting
• Вроде бы по тз тебе не нужно возвращать currency при ошибке
• Возможная альтернатива твоему методу:

  Optional<Currency> savedCurrency = table.save(currency);
    
    if (savedCurrency.isPresent()) {
        return savedCurrency.get();
    } else {
        return findInTable(currency.getCode())
    }
или
return table.save(currency).orElse(table.find(currency));

https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/service/CurrenciesService.java#L15C1-L21C6 https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/CurrenciesServlet.java#L45C11-L48C10

6. Почему ты пробрасываешь ошибку по всему коду если уже её обработал? upd. Понял в чем дело. Лучше наследуйся от рантайм эксепшена когда создаешь свои ошибки. Так ты привязался к конкретной бд, не всегда есть этот sqlexception. + RuntimException можно не пробрасывать по всему приложению. https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/exceptions/DB/DbException.java#L5

7. Не могу не посоветовать ломбок https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/model/Currency.java#L3

8. Не могу не посоветовать мапстракт Это методы мапперы, приватные методы - не лучшее место для них. https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/service/ExchangeRatesService.java#L66C4-L80C6

9. Почему inTable? Первый раз вижу. Вроде бы и так все понятно. https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/service/ExchangeRatesService.java#L56C41-L56C48

10. Ты не по назначению try catch используешь. У тебя метод по хорошему должен возвращать Optional, и ты смотришь вернул он тебе что-то или нет. https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/service/ExchangeService.java#L15C1-L33C6

11. Это не интерфейс, это утил метод. А CurrencyPair - dto https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/AddressReader.java#L6C1-L35C2

12. Тоже утил или типа того https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/CurrencyServlet.java#L32C2-L36C6

13. ты Currency в дто не обернул. (У тебя по тз поле FullName должно быть в таблице, а следовательно и в модели. А в дтошке это же поле должно называться name) https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/CurrencyServlet.java#L24

14. У тебя по 2 коннекшена на вызов открывается, это лишнее. https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/DAO/CurrencyTable.java#L23C1-L58C6

15. Где-то есть трай с ресурсами, а где-то нет. И везде коннекшн закрывается в ручную, в случае с try-with-resources это можно не делать. https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/DAO/CurrencyTable.java#L60C1-L91C6

16. Это зачем? Напиши свой месседж и все. https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/DAO/CurrencyTable.java#L112C11-L112C69

17. За что отвечает аннотация в этом месте? https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/servlets/StartServlet.java#L14

18. Здесь ты снова дто придумал (стоит сделать класс дто для ошибки) https://github.com/DarkRubin/CurrencyExchange/blob/29d7a85644a30083e0c1f7bd96622b475a93fd99/src/main/java/exceptions/Service/ServiceException.java#L4C1-L16C2

19. Стоит сделать фильтр чтобы в хедере был contentType

@WebFilter(urlPatterns = "/*")
public class HeaderFilter implements Filter {
  @Override
  public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
          throws IOException, ServletException {
      response.setContentType("application/json;charset=UTF-8");
      request.setCharacterEncoding("UTF-8");
      response.setCharacterEncoding("UTF-8");
      chain.doFilter(request, response);
  }
}

• Дальше тебе понадобится понимание что такое WebListener и WebFilter