Abdulkhanov mod4 typical project#2
Conversation
demologin
left a comment
There was a problem hiding this comment.
Общий вывод по проекту
Проект представляет собой хорошую рабочую заготовку для системы миграции данных. Видно понимание объектно-реляционного отображения (ORM) и работы с NoSQL хранилищами. Основные замечания касаются архитектурного слоя: жесткая связанность компонентов (Tight Coupling) и отсутствие автоматизированного управления зависимостями. Также стоит уделить больше внимания безопасности типов при работе с результатами Hibernate-запросов.
|
|
||
| @Getter | ||
| @Setter | ||
| @RequiredArgsConstructor |
There was a problem hiding this comment.
Аннотация @requiredargsconstructor не имеет смысла, так как в классе отсутствуют final поля. Рекомендуется использовать @NoArgsConstructor или добавить модификатор final к полям. [WARNING]
| @RequiredArgsConstructor | ||
| public class Language { | ||
| private String language; | ||
| private Boolean isOfficial; |
There was a problem hiding this comment.
Использование префикса 'is' для поля типа Boolean может привести к некорректной генерации геттеров библиотекой Lombok (например, isOfficial() вместо getIsOfficial()), что иногда нарушает стандарты JavaBeans. [INFO]
| @Getter | ||
| @Setter | ||
| @RequiredArgsConstructor | ||
| public class Language { |
There was a problem hiding this comment.
В классе отсутствуют методы equals() и hashCode(). Для объектов, которые могут использоваться в коллекциях (Set), их реализация обязательна для корректного сравнения данных. [WARNING]
|
|
||
| @Getter | ||
| @Setter | ||
| @RequiredArgsConstructor |
There was a problem hiding this comment.
Аннотация @requiredargsconstructor здесь избыточна, так как нет final полей. Для чистоты кода следует использовать соответствующую аннотацию или определить конструктор явно. [WARNING]
|
|
||
| private Integer countryPopulation; | ||
|
|
||
| private Set<Language> languages; |
There was a problem hiding this comment.
Использование Set предполагает наличие реализации equals и hashCode в классе Language. Без них Set будет работать как List, допуская дубликаты по ссылке. [ERROR]
| public class CityDAO { | ||
| private final SessionFactory sessionFactory; | ||
|
|
||
| public CityDAO(SessionFactory sessionFactory) { |
There was a problem hiding this comment.
Передача SessionFactory в каждый DAO вручную усложняет архитектуру. В современных Java-приложениях этот процесс автоматизируется через DI-контейнеры. [WARNING]
| @@ -0,0 +1,16 @@ | |||
| package com.javarush.redis; | |||
There was a problem hiding this comment.
Классы в пакете 'redis' фактически являются DTO (Data Transfer Objects) для кэша. Рекомендуется добавить суффикс Dto к именам классов для ясности намерений. [INFO]
| } | ||
|
|
||
|
|
||
| public static void main(String[] args) { |
There was a problem hiding this comment.
Метод main содержит логику создания приложения. Рекомендуется разделять запуск приложения и выполнение бизнес-задач (например, через метод run()). [WARNING]
|
|
||
| public class Main { | ||
|
|
||
| private final SessionFactory sessionFactory; |
There was a problem hiding this comment.
Поля sessionFactory и redisClient должны закрываться (close()) при завершении работы приложения. В коде не предусмотрен механизм закрытия ресурсов (Graceful Shutdown). [ERROR]
|
|
||
| private final ObjectMapper mapper; | ||
|
|
||
| private final CityDAO cityDAO; |
There was a problem hiding this comment.
Для улучшения читаемости в Java 21 при инициализации локальных переменных внутри методов (например, в main или методах подготовки) следует чаще использовать ключевое слово var. [INFO]
Общий вывод по проектуПроект представляет собой хорошую рабочую заготовку для системы миграции данных. Видно понимание объектно-реляционного отображения (ORM) и работы с NoSQL хранилищами. Основные замечания касаются архитектурного слоя: жесткая связанность компонентов (Tight Coupling) и отсутствие автоматизированного управления зависимостями. Также стоит уделить больше внимания безопасности типов при работе с результатами Hibernate-запросов. |
|
|
||
| @Getter | ||
| @Setter | ||
| @RequiredArgsConstructor |
There was a problem hiding this comment.
Аннотация @requiredargsconstructor не имеет смысла, так как в классе отсутствуют final поля. Рекомендуется использовать @NoArgsConstructor или добавить модификатор final к полям.
| @RequiredArgsConstructor | ||
| public class Language { | ||
| private String language; | ||
| private Boolean isOfficial; |
There was a problem hiding this comment.
Использование префикса 'is' для поля типа Boolean может привести к некорректной генерации геттеров библиотекой Lombok (например, isOfficial() вместо getIsOfficial()), что иногда нарушает стандарты JavaBeans.
| @Getter | ||
| @Setter | ||
| @RequiredArgsConstructor | ||
| public class Language { |
There was a problem hiding this comment.
В классе отсутствуют методы equals() и hashCode(). Для объектов, которые могут использоваться в коллекциях (Set), их реализация обязательна для корректного сравнения данных.
|
|
||
| @Getter | ||
| @Setter | ||
| @RequiredArgsConstructor |
There was a problem hiding this comment.
Аннотация @requiredargsconstructor здесь избыточна, так как нет final полей. Для чистоты кода следует использовать соответствующую аннотацию или определить конструктор явно.
|
|
||
| private Integer countryPopulation; | ||
|
|
||
| private Set<Language> languages; |
There was a problem hiding this comment.
Использование Set предполагает наличие реализации equals и hashCode в классе Language. Без них Set будет работать как List, допуская дубликаты по ссылке.
| } | ||
|
|
||
| public List<City> getItems(int offset, int limit) { | ||
| Query<City> query = sessionFactory.getCurrentSession().createQuery("select c from City c", City.class); |
There was a problem hiding this comment.
HQL-запрос жестко прописан в коде метода. Рекомендуется выносить такие строки в константы или использовать NamedQueries для улучшения читаемости и поддержки.
| } | ||
|
|
||
| public List<City> getItems(int offset, int limit) { | ||
| Query<City> query = sessionFactory.getCurrentSession().createQuery("select c from City c", City.class); |
There was a problem hiding this comment.
Вызов getCurrentSession() предполагает наличие активной транзакции. В коде DAO не видно управления границами транзакций, что может привести к HibernateException.
|
|
||
| public int getTotalCount() { | ||
| Query<Long> query = sessionFactory.getCurrentSession().createQuery("select count(c) from City c", Long.class); | ||
| return Math.toIntExact(query.uniqueResult()); |
There was a problem hiding this comment.
Метод uniqueResult() может вернуть null, если записи отсутствуют. Передача null в Math.toIntExact приведет к NullPointerException. Требуется предварительная проверка.
| public City getById(Integer id) { | ||
| Query<City> query = sessionFactory.getCurrentSession().createQuery("select c from City c join fetch c.country where c.id = :ID", City.class); | ||
| query.setParameter("ID", id); | ||
| return query.getSingleResult(); |
There was a problem hiding this comment.
Метод getSingleResult() выбрасывает NoResultException, если сущность не найдена. В современных приложениях на Java 21 предпочтительнее использовать uniqueResultOptional() для обработки отсутствия данных.
| } | ||
|
|
||
| public City getById(Integer id) { | ||
| Query<City> query = sessionFactory.getCurrentSession().createQuery("select c from City c join fetch c.country where c.id = :ID", City.class); |
There was a problem hiding this comment.
Использование 'join fetch c.country' является отличной практикой для предотвращения проблемы N+1. Это свидетельствует о глубоком понимании работы Hibernate.
| public List<Country> getAll() { | ||
| Query<Country> query = sessionFactory | ||
| .getCurrentSession() | ||
| .createQuery("select c from Country c join fetch c.languages", Country.class); |
There was a problem hiding this comment.
Текст запроса следует вынести в статическую константу класса. Это упростит тестирование и сделает код более чистым согласно принципам Clean Code.
| this.sessionFactory = sessionFactory; | ||
| } | ||
|
|
||
| public List<Country> getAll() { |
There was a problem hiding this comment.
Метод getAll() загружает все записи из таблицы стран. При росте объема данных это может вызвать переполнение памяти (OutOfMemoryError). Рекомендуется реализовать пагинацию.
| private final SessionFactory sessionFactory; | ||
| private final RedisClient redisClient; | ||
|
|
||
| private final ObjectMapper mapper; |
There was a problem hiding this comment.
Поле mapper следует пометить модификатором final, так как оно инициализируется один раз в конструкторе и не изменяется в дальнейшем.
| private final CityDAO cityDAO; | ||
| private final CountryDAO countryDAO; | ||
|
|
||
| public Main() { |
There was a problem hiding this comment.
Конструктор содержит сложную логику инициализации ресурсов (БД, Redis). Согласно принципам SOLID, конструкторы должны заниматься только присваиванием зависимостей.
|
|
||
| public Main() { | ||
| sessionFactory = prepareRelationalDb(); | ||
| cityDAO = new CityDAO(sessionFactory); |
There was a problem hiding this comment.
Ручное создание экземпляров DAO (new CityDAO) создает жесткую связанность. Рекомендуется внедрять зависимости через конструктор, что упростит модульное тестирование.
| countryDAO = new CountryDAO(sessionFactory); | ||
|
|
||
| redisClient = prepareRedisClient(); | ||
| mapper = new ObjectMapper(); |
There was a problem hiding this comment.
Создание нового экземпляра ObjectMapper в каждом экземпляре Main неэффективно. Этот объект потокобезопасен и должен использоваться как синглтон.
| import io.lettuce.core.api.sync.RedisStringCommands; | ||
| import org.hibernate.Session; | ||
| import org.hibernate.SessionFactory; | ||
| import org.hibernate.cfg.Configuration; |
There was a problem hiding this comment.
Класс Configuration для настройки Hibernate считается устаревшим. В новых проектах рекомендуется использовать ServiceRegistry и MetadataSources.
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static java.util.Objects.nonNull; |
There was a problem hiding this comment.
Статический импорт Objects.nonNull не используется в представленном фрагменте кода. Неиспользуемые импорты следует удалять для поддержания чистоты проекта.
| import org.hibernate.cfg.Configuration; | ||
| import org.hibernate.cfg.Environment; | ||
|
|
||
| import java.util.ArrayList; |
There was a problem hiding this comment.
Импорт java.util.ArrayList не используется. Это захламляет секцию импортов и затрудняет чтение кода.
| public class Main { | ||
|
|
||
| private final SessionFactory sessionFactory; | ||
| private final RedisClient redisClient; |
There was a problem hiding this comment.
Инициализация тяжеловесного RedisClient в конструкторе затрудняет создание объекта Main. Такую логику лучше вынести в отдельный метод инициализации или жизненного цикла.
| public class CityDAO { | ||
| private final SessionFactory sessionFactory; | ||
|
|
||
| public CityDAO(SessionFactory sessionFactory) { |
There was a problem hiding this comment.
Передача SessionFactory в каждый DAO вручную усложняет архитектуру. В современных Java-приложениях этот процесс автоматизируется через DI-контейнеры.
| @@ -0,0 +1,16 @@ | |||
| package com.javarush.redis; | |||
There was a problem hiding this comment.
Классы в пакете 'redis' фактически являются DTO (Data Transfer Objects) для кэша. Рекомендуется добавить суффикс Dto к именам классов для ясности намерений.
| } | ||
|
|
||
|
|
||
| public static void main(String[] args) { |
There was a problem hiding this comment.
Метод main содержит логику создания приложения. Рекомендуется разделять запуск приложения и выполнение бизнес-задач (например, через метод run()).
|
|
||
| public class Main { | ||
|
|
||
| private final SessionFactory sessionFactory; |
There was a problem hiding this comment.
Поля sessionFactory и redisClient должны закрываться (close()) при завершении работы приложения. В коде не предусмотрен механизм закрытия ресурсов (Graceful Shutdown).
|
|
||
| private final ObjectMapper mapper; | ||
|
|
||
| private final CityDAO cityDAO; |
There was a problem hiding this comment.
Для улучшения читаемости в Java 21 при инициализации локальных переменных внутри методов (например, в main или методах подготовки) следует чаще использовать ключевое слово var.
Typical version of the final project from module 4.