search

dorofeev22 / caregiving

CRM of caregiving
The Unlicense
0 stars 1 forks source link

Code review issues #2

Open ForNeVeR opened 6 years ago

ForNeVeR commented 6 years ago

Общие замечания по проекту:

  1. Смутило отсутствие миграций БД. Работать со средой вручную — нехорошо и противоречит принципам позитивного developer experience: https://habrahabr.ru/company/jugru/blog/351144/#pervyy-den-na-rabote-kompaniya-y
  2. Явно не хватает модульных тестов. Их нужно сразу писать, пока проект молодой!

По коду и архитектуре бэкенда:

  1. Мне кажется, что стоит сразу выделять отдельный слой сервисов. Контроллеры отвечают за работу с HTTP-протоколом, а вся бизнес-логика делается в сервисах; контроллер напрямую не лазит в репозиторий. Сервисы же сразу можно и подвергнуть модульным тестам (а вот контроллеры уже необязательно, потому что в них не должно быть особо интересной логики).
  2. Мне кажется, или не настроена безопасность?
  3. UserController::users — мне кажется, не хватает пажинации. Сложно представить, что мы хоть в какой-то системе захотим отдавать список объектов из БД целиком. (Для этого посмотри на стандартный спринговый PagingAndSortingRepository.)
  4. Немножко неаккуратно, что в некоторых местах не хватает транзакций. Например, в методе UserController::delete: ты сначала запрашиваешь пользователя, а потом его удаляешь. Если между этими действиями с ним что-то случится — будет плохо (одновременные запросы на удаление, скорее всего, упадут с ошибкой). Желательно защищаться от этого хотя бы repeatable read-транзакцией. (И, понятное дело, @Transactional тоже напрашивается в метод сервиса, а не контроллера.)

  5. Вот тут плохой код: https://github.com/dorofeev22/caregiving/blob/1d5b5f014e4eae61ec837574191e8967e4b2a9b4/src/main/java/ru/dorofeev22/caregiving/controllers/UserController.java#L62

    Ты объявляешь переменную непонятного типа Optional, хотя стоило бы сделать Optional<User>. На следующей строке у меня IDE показывает ворнинг.

    К тому же, этот метод мне вообще не нравится — получаешь безопасный тип Optional<User>, а потом разворачиваешь его в null. Будто прям напрашиваешься на NullPointerException! Предлагаю его убрать, и везде юзать Optional<User>, который получен тобой из репозитория (конкретные примеры работы могу подсказать, только спроси).

По фронтенду:

  1. Его нет. Надо написать! Раз уж на бэке API (что я, конечно же, люто одобряю), на фронте можно заюзать тот же Angular.

  2. Помимо этого, можно озадачиться и настроить Swagger. Это автоматически генерируемая страничка для твоего API, которая выглядит примерно вот так: http://petstore.swagger.io/

    Получается, что для тестирования API не нужно никаких дополнительных инструментов и вообще ничего, кроме, собственно, браузера. Наши тестировщики очень любят эту штуку и не стесняются вызывать API-методы при тестировании напрямую (без UI наших мобильных и браузерных приложений). Я считаю, что разработчику при отладке эта штука тоже не помешает.

По базе данных:

  1. Меня смущает, что у тебя указана кодировка UTF8, но при этом LC_COLLATE = 'Russian_Russia.1251'. Выглядит это очень стрёмно, нет ли там стандартной русской юникодовой collate?
ForNeVeR commented 6 years ago

Раунд второй.

Инфраструктура:

  1. app-run.bat: gradlew build && java -jar build/libs/my-app-0.1.0.jar — разве gradlew run делает не то же самое?
  2. form-info-data-example.sql: название поля "fileds" — опечатка?

Бэкенд:

  1. IDEA (которой я теперь доверяю 😉) говорит, что инъекции через @Autowired устарели, и нужно инжектить вещи через конструктор — мол, Spring team recommends. Предлагаю это исправить.
  2. MainController::index до сих пор возвращает Hello World — наверное, можно уже там сделать что-то вменяемое :)
  3. UsersController::users: (type != null ? Arrays.asList(type) : Arrays.asList(User.Type.values())) — я считаю, что даже этот кусочек логики было бы лучше унести в сервис, а не выполнять его в контроллере.
  4. Точно ли нам нужны раздельные сущности UserRole и UserType? Нет ли тут дублирования функциональности?
  5. Я сильно разочарован в вещах наподобие BaseEntity. Сейчас я не рекомендую наследовать все сущности от одного класса.
  6. Кажется, Spring Security так и не прикручена, а это сверхважный момент!

Frontend:

  1. user.ts, userRole.ts: мне кажется, должны быть интерфейсами, а не классами.

Фронтенд я особо эффективно поревьюить не смог, потому что у меня не очень много опыта в работе с новым Angular.

dorofeev22 commented 6 years ago

IDEA (которой я теперь доверяю 😉) говорит, что инъекции через @Autowired устарели, и нужно инжектить вещи через конструктор — мол, Spring team recommends. Предлагаю это исправить.

Т.е. вместо 

@RestController
public class EmailController {

    @Autowired
    private EmailService emailService;
...

надо 

@RestController
public class EmailController {

    private EmailService emailService;

    public EmailController(EmailService emailService) {
        this.emailService = emailService;
    }

?

ForNeVeR commented 6 years ago

Да. Только в определение поля ещё можно добавить final: private EmailService emailService;

Если что, в IDE есть быстрофикс для того, чтобы автоматически заполнить поле из конструктора :)

Инженерные детали по этому поводу: DI-фреймворк (в нашем случае Spring) умеет вызывать конструкторы сервисов, подбирая для них зависимости из своего контекста. Это считается более правильным, чем использование аннотаций на полях, потому что так сложнее сделать ошибку, а в стороннем коде (в тестах, например) можно будет создавать сервисы без использования Spring.