-
Notifications
You must be signed in to change notification settings - Fork 0
add-controllers. Добавить апи для пользователей и вещей. #1
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
Conversation
|
|
||
| Item updateItem(Item item); | ||
|
|
||
| Item getItemById(Long itemId); |
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.
оберни результат метода в Optional, а проверку существования проводи в вызывающих его сервисах через метод elseThrow()
| Item findItem = itemRepository.getItemById(itemId); | ||
| checkUser(findItem, userId); | ||
| userRepository.getUserById(userId); | ||
| findItem.setName(itemDto.getName() != null ? itemDto.getName() : findItem.getName()); |
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.
не нужно обновлять не только null, но и пустую строку
| checkUser(findItem, userId); | ||
| userRepository.getUserById(userId); | ||
| findItem.setName(itemDto.getName() != null ? itemDto.getName() : findItem.getName()); | ||
| findItem.setDescription(itemDto.getDescription() != null ? itemDto.getDescription() : findItem.getDescription()); |
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.
и тут тоже
|
|
||
| List<User> getUsers(); | ||
|
|
||
| User getUserById(Long userId); |
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.
оберни результат метода в Optional, а проверку существования проводи в вызывающих его сервисах через метод elseThrow()
| public UserDto createUser(UserDto userDto) { | ||
| log.info("Создание пользователя"); | ||
| checkEmail(userDto.getEmail()); | ||
| return UserMapper.toUserDto(userRepository.createUser(UserMapper.toUser(userDto, getNewUserId()))); |
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.
такие конструкции лучше оформлять вот так, читается гораздо лучше
UserMapper.toUserDto(
userRepository.createUser(
UserMapper.toUser(userDto, getNewUserId())));| public UserDto updateUser(UserDto userDto, Long userId) { | ||
| log.info("Обновление пользователя с ID = {}", userId); | ||
| User findUser = userRepository.getUserById(userId); | ||
| if (userDto.getEmail() != null) checkEmail(userDto.getEmail()); |
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.
а если пользователь в качестве новой почты указал старую, то наверно можно и не прерывать обновление
| log.info("Обновление пользователя с ID = {}", userId); | ||
| User findUser = userRepository.getUserById(userId); | ||
| if (userDto.getEmail() != null) checkEmail(userDto.getEmail()); | ||
| findUser.setEmail(userDto.getEmail() != null ? userDto.getEmail() : findUser.getEmail()); |
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.
не нужно обновлять не только null, но и пустую строку
| User findUser = userRepository.getUserById(userId); | ||
| if (userDto.getEmail() != null) checkEmail(userDto.getEmail()); | ||
| findUser.setEmail(userDto.getEmail() != null ? userDto.getEmail() : findUser.getEmail()); | ||
| findUser.setName(userDto.getName() != null ? userDto.getName() : findUser.getName()); |
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.
и тут тоже
| } | ||
|
|
||
| @Override | ||
| public void deleteUserById(Long userId) { |
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.
сначала проверь, есть ли такой пользователь
| } | ||
|
|
||
| private void checkEmail(String email) { | ||
| if (userRepository.getUsers().stream() |
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.
не вытягивай в сервис все хранилище, перенеси логику в userRepository, а из сервиса вызывай
…та. Дополнить условия обновления полей. Формирование нового ID вынести в репозиторий.
a6dc9e2 to
b11c48b
Compare
| @Repository | ||
| public class ItemRepositoryImpl implements ItemRepository { | ||
|
|
||
| private Map<Long, Item> items = new HashMap<>(); |
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.
добавь final
| } | ||
|
|
||
| @Override | ||
| public Long getNewItemId() { |
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.
давай этот метод сделаем внутренним и использовать его будем при создании вещи в текущем классе. Как следствие, нужно будет поправить ItemMapper.toItem()
| @Override | ||
| public List<ItemDto> getItemsByUserId(Long userId) { | ||
| log.info("Поиск вещей пользователя с ID = {}", userId); | ||
| userRepository.getUserById(userId) |
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.
вынеси получение пользователя/выброс исключения в отдельный метод класса
| @Override | ||
| public ItemDto getItemById(Long itemId) { | ||
| log.info("Запрос вещи с ID = {}", itemId); | ||
| Item item = itemRepository.getItemById(itemId) |
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.
и получение вещи тоже
| @Repository | ||
| public class UserRepositoryImpl implements UserRepository { | ||
|
|
||
| private Map<Long, User> users = new HashMap<>(); |
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.
добавь final
| } | ||
|
|
||
| @Override | ||
| public Long getNewUserId() { |
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.
аналогично с методом getNewItemId()
| @Override | ||
| public UserDto getUserById(Long userId) { | ||
| log.info("Запрос пользователя с ID = {}", userId); | ||
| User userById = userRepository.getUserById(userId) |
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.