вариант 1 #5

alisilakg · 2023-10-02T05:33:29Z

No description provided.

KirPyt

Приветствую!

Работа выполнена на хорошем уровне! Далее опишу общие слова:

Некоторые замечания и советы актуальны и для остальных частей кода, поэтому стоит обратить внимание и на другие аналогичные места в коде, чтобы всё поправить. В целом я добавил несколько объемных рекомендаций, их можно попробовать учесть при наличии желания и времени, главное сильно не утонуть)

Если возникнут вопросы по ревью, то пишите мне в Пачку, обсудим

KirPyt · 2023-10-03T17:19:26Z

ewm-service/src/main/java/ru/practicum/explore/comment/controller/CommentPrivateController.java

+    private final CommentService commentService;
+
+    @Autowired
+    public CommentPrivateController(CommentService commentService) {


Если хотите, можно заменить данный конструктор аннотацией @RequiredArgsConstructor от lombok, тогда можно будет просто навесить данную аннотацию над классом и убрать данный код для конструктора

KirPyt · 2023-10-03T17:19:55Z

ewm-service/src/main/java/ru/practicum/explore/comment/controller/CommentPrivateController.java

+    }
+
+    @GetMapping
+    @ResponseStatus(HttpStatus.OK)


Данный статус возвращается по умолчанию, можно не писать явно
Далее аналогично

KirPyt · 2023-10-03T17:20:47Z

ewm-service/src/main/java/ru/practicum/explore/comment/controller/CommentPrivateController.java

+    @GetMapping
+    @ResponseStatus(HttpStatus.OK)
+    public List<CommentDto> getAllCommentsByUserId(@PathVariable Long userId,
+                                                   @RequestParam(required = false, defaultValue = "0") Integer from,


При наличии defaultValue в required = false нет смысла
Далее аналогично

KirPyt · 2023-10-03T17:21:44Z

ewm-service/src/main/java/ru/practicum/explore/comment/controller/CommentPrivateController.java

+    @ResponseStatus(HttpStatus.OK)
+    public List<CommentDto> getAllCommentsByUserId(@PathVariable Long userId,
+                                                   @RequestParam(required = false, defaultValue = "0") Integer from,
+                                                   @RequestParam(required = false, defaultValue = "10") Integer size) {


Рекомендую перейти к проверке данных параметров с использованием штатных инструментов валидации в виде, например, аннотации @Positive или @PositiveOrZero. Разметите данную аннотацию в методе перед проверяемым параметров и она выполнит всю работу за Вас)

KirPyt · 2023-10-03T17:22:35Z

ewm-service/src/main/java/ru/practicum/explore/comment/controller/CommentPublicController.java

+                                           @RequestParam List<Long> users,
+                                           @RequestParam(required = false) String text,
+                                           @RequestParam(required = false)
+                                               @DateTimeFormat(pattern = "yyyy-MM-dd HH:mm:ss")


Рекомендую разместить паттерн в отдельной переменной, так можно будет удобно изменить её при необходимости

KirPyt · 2023-10-03T17:23:23Z

ewm-service/src/main/java/ru/practicum/explore/comment/dto/NewCommentDto.java

+@Getter
+@Setter
+public class NewCommentDto {
+    @NotBlank(message = "Текст комментария не может быть пустым")


Данная аннотация уже включает в себя @NotEmpty, можно убрать @NotEmpty

KirPyt · 2023-10-03T17:25:27Z

ewm-service/src/main/java/ru/practicum/explore/comment/mapper/CommentMapper.java

+import java.util.List;
+
+@Component
+public class CommentMapper {


Рекомендую подумать над тем, чтобы отказаться от @Component и перейти на аннотацию @UtilityClass, она подчеркнет, что данный класс служебный и сделает все методы статическими, а так же добавит приватный конструктор. Так же данная аннотация позволит не писать ключевое слово static и сделает это за Вас
Более подробно https://projectlombok.org/features/experimental/UtilityClass

К сожалению, от использования сервисов придется отказаться, но это лишний раз подстрахует Вас от случайных запросов к сервису в цикле

KirPyt · 2023-10-03T17:26:13Z

ewm-service/src/main/java/ru/practicum/explore/comment/service/CommentServiceImpl.java

+
+@Service
+@Slf4j
+public class CommentServiceImpl implements CommentService {


Рекомендую подумать над тем, чтобы навесить аннотации для транзакций, чтобы Ваши методы стали транказционными:
Для этого Вы можете добавить аннотацию @Transactional над каждым методом, который необходимо выполнять в транзакции, либо можно просто разместить данную аннотацию над классом) Таким образом она будет использована для каждого метода. Так же рекомендую подумать над тем, чтобы для тех методов, где Вы используете только чтение данных, можно использовать @Transactional(readOnly = true)
Более подробно Вы можете почитать:
https://www.baeldung.com/transaction-configuration-with-jpa-and-spring

KirPyt · 2023-10-03T17:27:00Z

ewm-service/src/main/java/ru/practicum/explore/comment/service/CommentServiceImpl.java

+                text,
+                rangeStart,
+                rangeEnd,
+                PageRequest.of(from, size));


Рекомендую сделать Ваши настройки пагинации немного проще:
Сформируйте класс, наследуемый от PageRequest и определите для него конструктор, принимающий параметры from и size. Данный класс будет реализовывать логику расчёта пагинации. Таким образом алгоритм расчёта пагинации будет инкапсулирован в данный класс.
Это позволит расширить стандартный spring класс пагинации под Вашу бизнес логику

KirPyt

Приветствую!

В целом всё хорошо, но указал ещё некоторые моменты по коду, рекомендую попробовать их учесть. Рекомендую пробовать объемные исправления по коду в новой ветке, чтобы в случае чего иметь возможность отказаться от изменений. Рекомендацию по отказу циклических запросов к БД пробуйте только если позволяет время, в ином случае просто рекомендую иметь в виду на будущее

Если возникнут вопросы по ревью, то пишите в Пачку

KirPyt · 2023-10-04T18:52:03Z

ewm-service/src/main/java/ru/practicum/explore/comment/service/CommentServiceImpl.java

-        checkUser(userId);
-        List<Comment> comments = commentRepository.findByAuthorId(userId, PageRequest.of(0, 10));
+        userService.findUserByIdForMapping(userId);
+        PageRequest pageRequest = CustomPageRequest.of(0, 10);


Не стоит явно подставлять числа, это плохая практика

KirPyt · 2023-10-04T18:53:24Z

ewm-service/src/main/java/ru/practicum/explore/comment/service/CommentServiceImpl.java

    }
+
+    private List<Comment> getCommentsWithEventWithViewsAndRequests(List<Comment> comments) {
+        for (Comment comment : comments) {


Запросы в цикле - это плохая практика, так как каждый такой запрос требуется целого ряда действий для обращения к БД, стоит попробовать реализовывать всё за 1 запрос к БД, чтобы сильно не нагружать базу

KirPyt · 2023-10-04T19:07:21Z

...e/src/main/java/ru/practicum/explore/compilation/controller/CompilationPublicController.java

    public List<CompilationDto> getAllCompilations(@RequestParam(required = false) Boolean pinned,
-                                                   @RequestParam(required = false, defaultValue = "0") Integer from,
-                                                   @RequestParam(required = false, defaultValue = "10") Integer size) {
+                                                   @PositiveOrZero


Чтобы аннотации на подобие @PositiveOrZero над классом надо разместить аннотацию @Validated

KirPyt · 2023-10-04T19:07:56Z

ewm-service/src/main/java/ru/practicum/explore/compilation/service/CompilationServiceImpl.java

        } else {
            compilations = compilationRepository.findByPinned(pinned, PageRequest.of(from / size, size));
        }
+        for (Compilation compilation : compilations) {


Здесь так же рекомендую попробовать отказаться от циклических запросов

KirPyt

Всё хорошо)

Это был Ваш заключительный проект на курсе, по этой причине хочу пожелать Вам удачи в дальнейшей карьере уже как разработчика, хорошего дня и быстрого профессионального роста! 🚀

KirPyt · 2023-10-06T05:20:29Z

ewm-service/src/main/java/ru/practicum/explore/event/mapper/EventMapper.java

                .annotation(event.getAnnotation())
                .category(CategoryMapper.toCategoryDto(event.getCategory()))
-                .confirmedRequests(event.getConfirmedRequests())
+                //.confirmedRequests(event.getConfirmedRequests())


Не рекомендую оставлять закомментированные участки кода

KirPyt · 2023-10-06T05:22:05Z

ewm-service/src/main/java/ru/practicum/explore/request/model/ConfirmedRequest.java

+    private Long count;
+    private Long eventId;
+
+    public ConfirmedRequest(Long eventId, Long count) {


Если хотите, данный конструктор можно заменить при помощи аннотации @AllArgsConstructor

3

d6233a5

alisilakg force-pushed the feature_comments branch from 0972946 to d6233a5 Compare October 2, 2023 07:38

alisilakg added 2 commits October 2, 2023 12:43

вариант 1

90e8ce6

add a link to PullRequest

c628287

KirPyt reviewed Oct 3, 2023

View reviewed changes

alisilakg added 11 commits October 4, 2023 08:47

add @requiredargsconstructor

95274d6

delete @ResponseStatus(HttpStatus.OK)

80a87bd

add @positive and @PositiveOrZero

51d96f5

add TIME_FORMAT

ffc2a5a

delete @notempty

3ed29dd

add @Transident

28caefa

all mappers are UtilityClass

e7a79f5

add customPageRequest

b3ff7d7

checkstyle

222bc5d

add Sort.unsorted() in CustomPageRequest

a5e2f32

fix: change getCommentWithEventWithViewsAndRequests(Long commentId)

28ec332

KirPyt reviewed Oct 4, 2023

View reviewed changes

убрала запросы к БД в цикле

add35ca

KirPyt reviewed Oct 6, 2023

View reviewed changes

alisilakg added 2 commits October 9, 2023 11:13

codestyle

353939c

codestyle

5dfb8e7

alisilakg merged commit 4f97a1e into main Oct 9, 2023

вариант 1 #5

вариант 1 #5

Uh oh!

Conversation

alisilakg commented Oct 2, 2023

Uh oh!

KirPyt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirPyt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirPyt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KirPyt left a comment •

edited

Loading