Skip to content

Step 3: add feature#6

Merged
MaxiBit1 merged 4 commits into
mainfrom
feature_subscriptions
Sep 25, 2023
Merged

Step 3: add feature#6
MaxiBit1 merged 4 commits into
mainfrom
feature_subscriptions

Conversation

@MaxiBit1

Copy link
Copy Markdown
Owner

No description provided.

@Sla-als Sla-als left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здравствуйте! Ваш код в целом выглядит отлично, но есть несколько замечаний и предложений по улучшению, в основном по одному излишне сложному методу.

}

@Override
public List<EventDtoOutFull> getFollowEventsById(Long userId, Long followerId, String sort, Integer from, Integer size) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лучше использовать аннотацию @NotNull для обязательных параметров, чтобы явно показать, что параметры не должны быть null.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

При обработке исключений используется одна и та же строка "Пользователь с id = %d не найден", хотя второй раз идентификатор должен относиться к followerId.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование if-else для сортировки событий может быть улучшено. Предлагаю определить компаратор один раз и затем использовать его для сортировки, избегая дублирования кода.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В обеих ветках if-else вызывается один и тот же метод репозитория. Можно упростить, вызывая метод один раз и затем применяя необходимую сортировку к результату.

@MaxiBit1 MaxiBit1 merged commit 6e6ddb2 into main Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants