-
Notifications
You must be signed in to change notification settings - Fork 0
Add bookings #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add bookings #3
Conversation
avfyodorov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Добрый день!
Хорошая работа, от меня всего пара уточнений.
| import java.time.LocalDate; | ||
| import java.time.LocalDateTime; | ||
|
|
||
| @Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data
Лучше не использовать эту аннотацию для сущностей. Дело в том, что ломбок генерирует EqualsAndHashCode, И могут быть конфликты с аналогичными механизмами hibernate’a
В данной работе ничего страшного не произойдет, но лучше не привыкать).
Если бы в проект добавили кэширование, то возникли бы проблемы.
https://stackoverflow.com/questions/75181366/why-jpa-buddy-complains-about-data-annotation-over-jpa-entity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Спасибо за замечание, исправила
| import lombok.NoArgsConstructor; | ||
| import ru.practicum.shareit.user.User; | ||
|
|
||
| @Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
и здесь Data лучше заменить на Getter/Setter.
Посмотрите, пожалуйста, аналогичные ситуации и в других сущностях.
| private final ItemRepository itemRepository; | ||
|
|
||
| @Override | ||
| public BookingResponseDto createBooking(Long userId, BookingDto bookingDto) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transactional
Над методами сервиса, которые меняют данные в БД (раз используется data-jpa) нужна аннотация Transactional
Два варианта.
Если аннотация объявлена над классом, то для методов, меняющих данные в базе, её повторно объявлять не нужно, А а вот для методов выборки лучше добавить параметр readonly=true
Если аннотация на классом не объявлена, то её нужно объявить для всех методов, которые изменяют данные в базе, а для методов Выборки данных она вообще не нужна.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Исправила - выбрала первый способ (аннотация над классом и в методах get поставила readOnly)
| } | ||
|
|
||
| @Override | ||
| public BookingResponseDto approveBooking(Long userId, Long bookingId, Boolean approved) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transactional
И сюда тоже добавить аннотацию.)
Посмотрите, пожалуйста, аналогичные ситуации и в других методах и в других сервисах.
avfyodorov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Добрый день, Дарья!
Замечаний нет.
Работа принята.
No description provided.