timmawv - Cloud-File-Storage.md

https://github.com/timmawv/Cloud-File-Storage
Ревью от Ильи

1. Переменная user не нужна, лучше в одну строку. https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/config/security/PersonDetailsService.java#L19-L22

2. Лучше заавторварить через конструктор, и повесить @Component https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/config/security/PersonDetailsService.java#L12C1-L15C43

Вот здесь можно будет убрать https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/config/security/SecurityConfig.java#L21C3-L24C6

В 27 строке просто в аргументах напишешь UserDetails и спринг сам твой компонент завтоварит. Не понял прикол с Person...Это же по сути UserDetailsServiceImpl и UserDetailsImpl. Лучше оставлять оригинальные названия, person мне не ясно что это может быть, не открывая класс.

3. Уже permitAll в SecurityConfig. Если хочешь показать что это открытая ручка/контроллер, то в названии приписку public можно сделать. Аннотации из security всё таки логику несут, лучше в одном месте её определить (в config) https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/controller/auth/LoginController.java#L10

4. Инжекти Validator, нарушение D из solid. И не сюда а в userService, в service делай всякие проверки. https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/controller/auth/RegisterController.java#L26

5. Прокидывай просто в сервис, не надо через exception-ы управлять приложением. https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/controller/auth/RegisterController.java#L34-L48

    @PostMapping
    public String registerUser(@ModelAttribute("user") @Valid UserDto userDto, BindingResult bindingResult, Model model) {
        return service.createUser(UserDto userDto, BindingResult bindingResult, Model model);
    }

    @Transactional
    public void saveUser(UserDto userDto, BindingResult bindingResult, Model model) {
        loginAndPasswordValidator.validate(userDto, bindingResult);
        if (bindingResult.hasErrors()) {
            return "auth/registration";
        }

        User user = userMapper.mapUserDtoToUser(userDto);
        user.setPassword(passwordEncoder.encode(user.getPassword()));
        try {
            userRepository.save(userDto.map());
            model.addAttribute("success_registration", true);
        } catch (DataIntegrityViolationException e) {
            log.error(...)
            bindingResult.rejectValue("login", "", e.getMessage());
            return "auth/registration";
        }
        
        return "auth/registration";
    }

6. "@Transactional(readOnly = true)" в userService не понял зачем, сами транзакции у тебя не включены и аннотации не работают, нажна аннотация EnableTransactionManager на config или на класс с "@SpringBootApplication".

7. Не хватает static, и скорее это не утил пакет, а security. В util обычно статик. https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/util/validator/LoginAndPasswordValidator.java#L13C4-L17C69

8. Надо сделать матчер из регулярки в поле класса, и у этого матчера вызывать метод matched. И желательно NotNull использовать, там всё таки npe может быть, раз идея светит. Используй NotNull из jetbrains зависимости. https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/util/validator/LoginAndPasswordValidator.java#L19-L23

9. Лучше было затащить в статик ресурсы, названия через большие (констатнта) https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/util/csv_parser/CsvFileParser.java#L18

Ниже map-а, она в многопоточной среде - используй ConncurentHashMap и final на поле.

10. Используй один try catch, is тоже передавай в try with resources. У тебя же если assert в 31 вылетит, то is не закрывается вообще (не надо думать что reader по чепочке должен закрыть is, если ты на это пологался). https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/util/csv_parser/CsvFileParser.java#L27C5-L42C6

11. Используй named parametr стиль, через 1, 2 и плохая практика. Я бы в проде не делал такой update, при том что уровень транзации базовый. Не мог сейчас сходу предложить вариант лучше, но я бы не делал так. Ощушение что мейб делал, если бы транзакция была repeateble read. В данный момент они не включены вообще или я не увидел как и где) https://github.com/timmawv/Cloud-File-Storage/blob/4d320bfa80e70112f20853876778d57417b7bf48/src/main/java/avlyakulov/timur/CloudFileStorage/user/UserRepository.java#L17-L28

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