Feature comments by SugarFoxy · Pull Request #3 · SugarFoxy/java-explore-with-me

SugarFoxy · 2023-03-11T22:32:11Z

No description provided.

LexLippi

Привет!

Неплохая работа, но есть несколько замечаний, которые необходимо исправить, и вопросы на которые необходимо ответить.

Желаю удачи!!!

LexLippi · 2023-03-12T08:35:51Z

@@ -1,10 +1,10 @@
-DROP TABLE IF EXISTS compilation_events,compilations,requests,events,categories,users;
+DROP TABLE IF EXISTS rating, comments, compilation_events,compilations,requests,events,categories,users;


Может можно как-то не удалять таблицы?

Таким образом при каждом разворачивании наших сервисов данные будут теряться, а польо=зователи негодовать, не хотелось бы этого :)

LexLippi · 2023-03-12T08:39:55Z

+CREATE TABLE IF NOT EXISTS comments
+(
+    id           BIGINT GENERATED BY DEFAULT AS IDENTITY NOT NULL,
+    text         VARCHAR(2000)                           NOT NULL,


Если мы пишем PostgreSQL-специфичный скрипт, то лучше не называть колонку text, так как в PostgreSQL есть тип данных с таким же названием, а это считается bad practice в любом ЯП.

Поэтому стоит подобрать какое-нибудь синонимичное название или на крайний случай comment_text

LexLippi · 2023-03-12T08:40:12Z

+    text         VARCHAR(2000)                           NOT NULL,
+    user_id      BIGINT                                  NOT NULL,
+    event_id     BIGINT                                  NOT NULL,
+    comment_time TIMESTAMP WITHOUT TIME ZONE,


Кажется, что оно не может быть null

LexLippi · 2023-03-12T08:41:28Z

    state              VARCHAR,
    title              VARCHAR(255)                            NOT NULL,
    views              BIGINT,
+    comment_switch     BOOLEAN,


Что означает это поле?

Может ли оно быть null?

Это по сути своей рубильник который выключает и включает комментарии.
Была невнимательна null быть не может

LexLippi · 2023-03-12T08:47:15Z

+        if (!event.getCommentSwitch()) {
+            return new ArrayList<>();
+        }
+        Sort sort1 = Sort.by(DESC, "commentTime");


А почему не просто sort?

Изначально был аргумент с таким же название, потом оно мне не понравилось и я изменила, а эта переменная осталась

LexLippi · 2023-03-12T08:50:07Z

+    }
+
+    @Override
+    public CommentDto updateComment(Long userId, Long commId, NewAndUpdateCommentDto dto) {


commentId можно польностью написать

а точно нужно такое сложное название для типа данных объекта dto? Может просто CommentDto?
Или на крайний случай просто UpdateCommentDto?

LexLippi · 2023-03-12T08:50:55Z

+        User user = objectCreator.getUserById(userId);
+        Event event = objectCreator.getEventById(dto.getEventId());
+
+        if (!comment.getCommentator().equals(user)) {


можно сравнить на уровне идентификаторов, и если возникает ошибка, то не делать лишний запрос по получению пользователя

LexLippi · 2023-03-12T08:51:36Z

+            throw new BadRequestException(String
+                    .format("Только комментатор id = %d может изменить коментарий id = %d ", userId, commId));
+        }
+        if (!comment.getEvent().equals(event)) {


Аналогичное замечание про сравнение идентификаторов

LexLippi · 2023-03-12T08:51:50Z

+        User user = objectCreator.getUserById(userId);
+
+        if (!comment.getCommentator().equals(user)) {
+            throw new BadRequestException(String
+                    .format("Только комментатор может удалить коментарий id = %d ", commId));
+        }


Тоже самое, что и выше

…omments

LexLippi

Привет!

Еще пару замечаний накинул!

Желаю удачи!!!

LexLippi · 2023-03-13T07:22:24Z

+    public List<OutCommentDto> getCommentByEvent(@NotNull @RequestParam Long eventId,
+                                                 @RequestParam(defaultValue = "false") Boolean rating,
+                                                 @PositiveOrZero @RequestParam(defaultValue = "0") int from,
+                                                 @Positive @RequestParam(defaultValue = "10") int size) {


Почему какие-то поля объектные, а какие-то примитивные, тем более если eventId не может быть null. Во всех контроллерах это вижу, давай к какому-то одному стилю придем

LexLippi · 2023-03-13T07:22:47Z

+        return service.updateComment(userId, commId, dto);
+    }
+
+    @DeleteMapping("/{commId}")


Может на commentId всё-таки переделаем?

SugarFoxy · 2023-03-13T18:16:31Z

Sign in to view

+
+    @PostMapping
+    @ResponseStatus(HttpStatus.CREATED)
+    public OutCommentDto createComment(@PathVariable Long userId,


Спасибо что подсказал. Не дружу с английским. Поэтому и названия кривые

LexLippi · 2023-03-13T07:24:09Z

+
+@Setter
+@Getter
+@NoArgsConstructor


Вот это ты где-то используешь? Если нет, то лучше удалить это из всех классов

Когда дто приходи и десериализуется требует конструкторы. Неужели это так нагружает систему? Могу удалить билдер он тут не нужен.

На самом деле систему это не нагружает, тут проблема в том, что такой доступный зоопарк возможностей по конструированию объекта приводит к тому, что каждый разработчик создает объекты по своему и это больно потом одному человеку сводить, когда надо довести горящий релиз до логического завершения.

А так в целом билдер это отличный шаблон, очень удобно с ним работать, и вообще это довольно интересная концепция.

LexLippi · 2023-03-13T07:25:05Z

+@AllArgsConstructor
+@Builder


И Setter, и Builder, и AllArgsConstructor, может от чего-то можно отказаться?

только от билдера. Хотя для написания тестов он был бы удобен. (очень он мне полюбился)

LexLippi · 2023-03-13T07:27:14Z

+
+public class CommentMapper {


Если все методы статические, то лучше сделать пустой конструктор этого класса приватным, чтобы везде гарантированно использовать его, как синглтон:

https://en.wikipedia.org/wiki/Singleton_pattern

Либо можно вместо класса использовать enum, как синглтон. Кстати, чтобы никто не испортил поведение нашего класса, то лучше пометить его как final, чтобы у него не могло быть наследников.

https://www.geeksforgeeks.org/advantages-and-disadvantages-of-using-enum-as-singleton-in-java/

LexLippi · 2023-03-13T07:28:36Z

+    @Override
+    public OutCommentDto createComment(Long userId, InputCommentDto dto) {
+        Event event = objectCreator.getEventById(dto.getEventId());
+        if (!event.getCommentSwitch()) {


Наверное лучше commentAvailable, чтобы было понятнее, а то switch немного пугает

Это не я, это гугл переводчик тебя испугал))))).

Я буду скучать по твоим ревью :`(
Спасибо большое за твой труд и прости за кровавые слезы при прочтении моего кода =D

LexLippi

Привет!

Отличная работа!

Дипломный проект сдан!

Желаю удачи в свободном плавании!

AshKeeper and others added 7 commits March 10, 2023 13:01

feat: Добавила возможность коментировать события

d551e6e

feat: Добавила возможность включать и отключать коментарии

c7ceb2e

fix: Настройка

49e4f02

feat: Добавила Postman тесты

6b79c5e

Style: поправила код

a550ce9

Style: поправила код

a31c02d

ссылка на пул

b49f3c3

LexLippi suggested changes Mar 12, 2023

View reviewed changes

AshKeeper added 4 commits March 12, 2023 12:25

fix: исправления после ревью

0a07c55

Merge remote-tracking branch 'origin/feature_comments' into feature_c…

5bf2732

…omments

fix: настройка

d04c6ae

fix: настройка

58a1e15

LexLippi suggested changes Mar 13, 2023

View reviewed changes

AshKeeper added 2 commits March 13, 2023 22:05

fix: исправления после ревью

7440203

fix: настройка

3208b6e

LexLippi approved these changes Mar 14, 2023

View reviewed changes

		@@ -1,10 +1,10 @@
		DROP TABLE IF EXISTS compilation_events,compilations,requests,events,categories,users;
		DROP TABLE IF EXISTS rating, comments, compilation_events,compilations,requests,events,categories,users;

		@AllArgsConstructor
		@Builder

Conversation

SugarFoxy commented Mar 11, 2023

Uh oh!

LexLippi 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LexLippi 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!

This comment was marked as resolved.

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!

LexLippi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants