Conversation
Commit_1 Commit_2
|
|
|
|
|
|
| @Configuration | ||
| public class RestTemplateConfig { | ||
| @Bean | ||
| public RestTemplate restTemplate() { |
There was a problem hiding this comment.
Почему решил через RestTemplate делать, а не через feign или web client?
| @ApiResponse(responseCode = "200", description = "URL нового аватара по умолчанию", | ||
| content = @Content(mediaType = MediaType.TEXT_PLAIN_VALUE, | ||
| schema = @Schema(implementation = String.class))) | ||
| @ApiResponse(responseCode = "200", description = "Список новых аватаров пользователя", |
There was a problem hiding this comment.
"Список новых аватаров" не совсем ясно. Аватар же по сути один только? Просто разные его форматы
| * @return {@code String}, содержащий URL нового аватара по умолчанию. | ||
| */ | ||
| String deleteAvatar(long userId); | ||
| UserProfilePic deleteAvatar(long userId); |
There was a problem hiding this comment.
javadoc обновить нужно
| @Override | ||
| @Transactional | ||
| public String deleteAvatar(long userId) { | ||
| public UserProfilePic deleteAvatar(long userId) { |
There was a problem hiding this comment.
Вообще интересная логика: пользователь удаляет аватар, а ему генерируется новый. Т.е. получается пользователь вообще не может быть без аватара? По идее генерировать должны только при регистрации пользователя, а дальше нужно дать возможность пользователю удалить аватар и остаться совсем без картинки. На что обращаю внимание: на работе нельзя придумывать логику самостоятельно. У нас уже пусть так остается, раз сделано, но всегда лучше уточнить.
| * Сервис для работы с аватарками пользователей. | ||
| * <p> | ||
| * Позволяет генерировать случайные аватарки через DiceBear API и получать ссылки на разные размеры, | ||
| * а также удалять аватарки из S3. |
There was a problem hiding this comment.
Среди методов интерфейса нет ничего про удаление
| * используется fallback-аватарка из статических ресурсов проекта. | ||
| * | ||
| * @param username имя пользователя, используется для формирования имени файла в S3 и логирования. | ||
| * @return карта с ключами "small" и "medium" и значениями — URL соответствующих аватарок. |
There was a problem hiding this comment.
Сейчас не карта возвращается, а UserProfilePic
| private static final int SMALL_SIZE = 64; | ||
| private static final int BIG_SIZE = 128; | ||
| private static final int MAX_AVATAR_SIZE_BYTES = 2_000_000; |
There was a problem hiding this comment.
Эти значения нужно брать из application.yaml
| String bigKey = "random-user-avatars/" + username + "/big-" + randomSeed + ".png"; | ||
| String smallKey = "random-user-avatars/" + username + "/small-" + randomSeed + ".png"; |
There was a problem hiding this comment.
Лучше через String.format и вынести это в константы
| maxAttempts = 4, | ||
| backoff = @Backoff(delay = 1000, multiplier = 2)) |
There was a problem hiding this comment.
Очень хорошо, что Retryable использовал. Но значения (4, 1000, 2) должны браться из application.yaml. И ещё аннотация в таком варианте работать не будет, т.к. внутри класса метод вызывается (особенность спринга, читай про proxy. Можем на встрече поговорить, если напомнишь).
| @Slf4j | ||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class RandomAvatarServiceImpl implements RandomAvatarService { |
There was a problem hiding this comment.
На мой взгляд, этот сервис не удовлетворяет принципу единственной ответственности. Он и к стороннему сервису обращается и ошибки от него обрабатывает, и изображение сжимает, и в s3 ходит.
- Для обращения к стороннему api должен быть отдельный класс-клиент. RandomAvatarClient или даже DicebearClient, который через restTemplate обращается к dicebear и обрабатывает ошибки от него
- Я бы предложила вынести в отдельный утилитный класс методы по работе с изображением: imageToPngBytes, resizeImage
- У метода generateRandomAvatarForUser логика должна быть один-в-один как у метода uploadAvatar с разницей лишь в том, откуда аватар пришел (от пользователя или из dicebear). Поэтому выглядит так, что всё это могло спокойно остаться внутри AvatarServiceImpl
Итого: RandomAvatarServiceImpl выглядит лишним классом. Можем также обсудить на встрече, если нужно
2 participants
Реализовал генерацию рандомных аватарок для юзеров.