20 типовых ошибок Java-разработчиков в пул реквестах

Введение
Монолитные “God-классы” в контроллерах
Отсутствие четкой архитектуры и структуры пакетов
Длинные методы и нарушение принципа SRP
Инъекция полей вместо конструкторов в Spring
Хардкод конфигураций и секретов в коде
“Магические” строки и числа
Игнорирование Spring Boot Starter-зависимостей
“Изобретение велосипеда” вместо готовых библиотек
Неиспользование профилей для разных сред
Отключение и невключение мониторинга (Actuator)
Неинформативное логирование или его отсутствие
Отсутствие глобального обработчика ошибок
Проглатывание исключений в блоках catch
Неправильное использование транзакций
Утечки ресурсов (незакрытые потоки, соединения)
Отсутствие тестов и слабое покрытие
Смешивание синхронного и асинхронного кода
Непотокобезопасный код (проблемы многопоточности)
Неэффективные запросы к базе данных (N+1)
Игнорирование безопасности (SQL-инъекции, отсутствие валидации)
Заключение

Введение

Code review (ревью кода) – ключевой этап в корпоративной разработке, позволяющий повысить качество программного обеспечения. Опытные инженеры при проверке pull request’ов обращают внимание на типичные ошибки, которые повторяются из проекта в проект. Эти системные проблемы затрагивают разные аспекты: от архитектуры и читабельности кода до производительности, устойчивости и безопасности приложения. Игнорирование этих аспектов приводит не только к замечаниям на код-ревью, но и к появлению багов, технического долга и нестабильности в продакшене. В этой статье на основе реальных наблюдений рассмотрены 20 самых распространенных ошибок Java-разработчиков (с акцентом на Spring Boot и Quarkus), которые часто становятся причинами отказа в слиянии пул реквеста. Для каждой ошибки мы разберем ее проявление в коде, объясним, почему она считается ошибкой и к каким последствиям приводит, а также покажем, как ее избежать или исправить – с примером улучшенного кода.

Монолитные “God-классы” в контроллерах

Как выглядит проблема: Новичкам свойственно реализовывать всю логику сразу в контроллере Spring/Quarkus, превращая его в громоздкий god object. Например, метод контроллера может последовательно выполнять в десятки строк и в одном месте: проверку входных данных, бизнес-расчеты, обращения к базе данных, вызовы внешних сервисов, отправку сообщений и формирование HTTP-ответа. Ниже упрощен фрагмент реального контроллера:

@PostMapping("/users")
public ResponseEntity<?> createUser(@RequestBody Map<String, Object> request) {
    // 47 lines of validation logic
    // 23 lines of database queries
    // 15 lines of email sending
    // 8 lines of file processing
    // 1 line of forming HTTP response
    return ResponseEntity.ok("User created... maybe?");
}

В примере выше весь код намешан в одном методе контроллера, который растянулся на ~85 строк (условно). Фактически это “не метод контроллера, а небольшой роман, который случайно отвечает на HTTP-запрос”.

Почему это ошибка: Подобный монолитный контроллер затрудняет поддержку кода. Нарушается разделение ответственности: контроллер перегружен задачами, которые ему не предназначены. Это усложняет чтение и тестирование – изменения в бизнес-логике требуют правок прямо в контроллере, повышая риск регрессий. Объемный метод сложно понять и почти невозможно переиспользовать частично. Ревьюверы обычно просят разбить такой код на слой сервисов, репозиториев и т.д., иначе команда получит техдолг.

Как исправить: Вынесите бизнес-логику из контроллера. Контроллер должен лишь принимать запрос и возвращать ответ, делегируя сложные операции соответствующим сервисам и компонентам. Разделите код на слои: валидатор/сервис для бизнес-правил, репозиторий для работы с БД, утилиты для файловой обработки и т.д. Стройте архитектуру по принципу single responsibility для каждого компонента. В Spring Boot обычно применяют многоуровневую структуру проекта:

com.yourcompany.yourapp
|-- controller/    # только REST-контроллеры (API)
|-- service/       # бизнес-логика приложения
|-- repository/    # слой доступа к базе данных
|-- model/         # модели данных (DTO, сущности)
|-- config/        # конфигурационные классы
|-- util/          # вспомогательные утилиты

Такой подход упорядочивает код. Контроллеры становятся “скучными” – и это хорошо: их код минимален и понятен. Например, перепишем вышеуказанный контроллер, распределив задачи по слоям:

@RestController
@RequestMapping("/api/v1/users")
public class UserController {
    private final UserService userService;
    public UserController(UserService userService) {
        this.userService = userService;
    }

    @PostMapping
    public ResponseEntity<UserResponse> createUser(@Valid @RequestBody CreateUserRequest request) {
        User user = userService.createUser(request);           // бизнес-логика в сервисе
        UserResponse response = UserResponse.from(user);       // формируем DTO ответа
        return ResponseEntity.status(HttpStatus.CREATED).body(response);
    }
}

В методе контроллера теперь только вызов сервиса и формирование ответа – никакого SQL, отправки писем или файловых операций напрямую. Внутри UserService.createUser реализованы все необходимые шаги (валидация, работа с репозиториями, интеграции и т.п.), но их тестирование и поддержка значительно проще благодаря разбиению. Такой код проходит ревью, поскольку соответствует принципам чистой архитектуры и облегчает дальнейшую разработку.

Отсутствие четкой архитектуры и структуры пакетов

Как выглядит проблема: Еще одна схожая проблема организации кода – хаотичная структура проекта. В Spring/Quarkus-приложениях иногда все классы сваливают в один пакет без разбивки по слоям. Например, можно встретить пакет com.company.app с десятками классов вперемешку: контроллеры рядом с сущностями, сервисами и утилитами. Это все равно что зайти на кухню, а там лежит белье, инструменты, еда – полная неструктурированность. Такой проект трудно понять новым членам команды: непонятно, где искать конкретный функционал.

Почему это ошибка: Без четкой модульной структуры страдает читабельность и поддерживаемость. Разработчику сложно ориентироваться в кодовой базе, возрастает вероятность дублирования кода и нарушения инкапсуляции. Если классы не разделены логически по пакетам (контроллеры, сервисы, репозитории и т.д.), то исследование кода превращается в “поиск сокровищ”, где на реализацию одной функции уходит много времени. Code review обычно требует навести порядок – иногда даже останавливают PR до реструктуризации.

Как исправить: Ввести ясную пакетную структуру и слои. Следуйте общепринятым соглашениям, особенно в Spring Boot-проектах: выделите отдельные пакеты для контроллеров (web), сервисов (service), доступа к данным (repository/dao), моделей (model/domain), конфигурации (config) и т.д. Такой стандарт показан выше в примере древовидной структуры. Каждый класс следует хранить в соответствующем пакете по роли. Например, контроллер UserController лежит в ...controller, а UserRepository – в ...repository. Разделяйте ответственность по пакетам – это упростит навигацию и модификацию системы.

В результате новый разработчик сможет “как по книжке” читать проект: контроллеры отвечают за API, сервисы – за бизнес-логику, репозитории – за связь с БД и т.д. Такой код легче поддерживать, и ревьюверы ценят когда структура проекта выглядит логично, “словно ее не затронул торнадо”.

Длинные методы и нарушение принципа SRP

Как выглядит проблема: Другая частая проблема – методы или классы, делающие слишком многое сразу, нарушая Single Responsibility Principle (SRP, принцип единственной ответственности). Например, могут встречаться методы на 200+ строк, последовательно выполняющие серию разнородных действий, или классы, имеющие массу несвязанных полей и методов. Такие “сущности” трудно читать и тестировать. Признаки: глубоко вложенные условные операторы (if/else в несколько уровней), большие switch-case, обработка множества кейсов подряд, большое число параметров у метода и пр.

Почему это ошибка: Длинный метод – почти наверняка признак нарушения SRP. У него появляется множество причин для изменения, что противоречит определению: “у модуля должна быть только одна причина для изменения”. Поддержка подобного кода трудоемка: чтобы внести изменение, приходится вникать во все 200 строк, высок риск затронуть лишнее. Многоуровневая вложенность ухудшает читаемость кода и повышает вероятность ошибок. Ревьюеры обычно требуют декомпозиции – разбить метод на меньшие или выделить часть логики в отдельный класс.

Как исправить: Рефакторинг на более мелкие методы или классы. Если метод делает несколько вещей – разделите его на последовательность вызовов приватных методов, каждый из которых решает свою подзадачу. Например, вместо одного метода processOrder(order) на 100 строк, можно сделать: validate(order), calculatePrice(order), saveOrder(order), notifyCustomer(order) и вызывать их поочередно. Каждый из новых методов будет коротким и сфокусированным.

Сокращайте уровень вложенности. Один из приемов – инвертировать условия и ранний выход из метода, чтобы избежать “лесенки” if/else. Например, вместо:

if (condition1) {
    if (condition2) {
        doWork();
    }
}

лучше написать:

if (!condition1) return;
if (!condition2) return;
doWork();

Так мы устраняем глубокую вложенность. Также стоит объединять логически связанные условия и выносить их в метод с говорящим названием. Вместо:

if (account.status != SUSPENDED && account.status != TERMINATED) {
    // ...
}

введем метод accountIsActive() и используем:

if (accountIsActive()) {
    // ...
}
boolean accountIsActive() {
    return account.status != SUSPENDED && account.status != TERMINATED;
}

Это повышает понятность кода.

В целом, придерживайтесь правила: метод должен умещаться в экран (условно 20-30 строк) и иметь четкую цель. Класс – реализовывать один функциональный компонент. Если трудно описать назначение метода/класса одним предложением – это сигнал к декомпозиции. Выполнив такие правки, вы значительно облегчите жизнь ревьюерам и самим себе при дальнейшем сопровождении кода.

Инъекция полей вместо конструкторов в Spring

Как выглядит проблема: В Spring-приложениях распространен способ внедрения зависимостей через аннотацию @Autowired прямо над полями класса. Пример:

@Service
public class UserService {
    @Autowired 
    private UserRepository userRepository;   // неправильно
    @Autowired 
    private EmailService emailService;       // неправильно
    // ...
}

Зависимости UserRepository, EmailService и др. объявлены как private поля с @Autowired. На первый взгляд, это работает, ведь Spring проставит туда нужные бины. Однако данный шаблон считается плохой практикой.

Почему это ошибка: Инъекция поля затрудняет тестирование и ведет к неявным зависимостям. При таком подходе невозможно создать объект UserService вручную без запуска контейнера Spring, что осложняет модульное тестирование (надо поднимать контекст или использовать reflection для внедрения моков). Кроме того, поля остаются изменяемыми (не final), что позволяет случайно перезаписать зависимость. Код становится менее явным – с первого взгляда непонятно, какие именно зависимости требуются классу для работы. В больших командах уже давно договорились использовать constructor injection, и отклонение от этого соглашения почти наверняка вызовет замечание на ревью.

Как исправить: Вместо автосвязывания полей, используйте инъекцию через конструктор. Это позволяет сделать поля final и гарантирует их наличие при создании объекта. Пример исправления:

@Service
public class UserService {
    private final UserRepository userRepository;
    private final EmailService emailService;
    // зависимости объявлены final

    // Конструктор с параметрами - Spring сам вызовет его и передаст бины
    public UserService(UserRepository userRepository, EmailService emailService) {
        this.userRepository = userRepository;
        this.emailService = emailService;
    }
    // ...
}

В Spring (начиная с 4.3) даже не требуется аннотация @Autowired на конструкторе, если он единственный – контейнер сам обнаружит и использует его. Теперь UserService легко тестировать: можно вручную подставить моки UserRepository и EmailService при создании экземпляра, без магии Reflection. Класс стал иммутабельным в части зависимостей, а его контракт явно виден – по сигнатуре конструктора сразу понятно, какие компоненты нужны для работы сервиса.

Резюмируя, constructor injection повышает надежность и удобство поддержки кода. Это лучший стиль, которому следуют в современных Spring/Quarkus-проектах, поэтому ревьюеры требуют придерживаться его и переписывать участки с полевой инъекцией.

Хардкод конфигураций и секретов в коде

Как выглядит проблема: Разработчики часто ошибочно захардкоживают конфигурационные значения прямо в коде или push-конфиги, содержащие секреты, в репозиторий. Типичные примеры: URL сервисов, учетные данные БД, API-ключи задаются как строковые литералы в классах. Например:

@Service
public class MySampleService {
    private final String url = "http://localhost:8080";  // хардкод адреса
    // ...
}

Или в коде встречаются пароли/токены: String dbPassword = "P@ssw0rd";. Такие значения со временем меняются для разных окружений (dev, stage, prod) или должны скрываться, но хардкод затрудняет управление ими.

Почему это ошибка: Жестко прописанные настройки делают приложение негибким и небезопасным. Для смены порта или адреса сервиса приходится перекомпилировать код. При деплое на разные среды нельзя легко поменять параметры без правки кода. Главная опасность – случайный коммит чувствительных данных (паролей, ключей) в репозиторий, что представляет угрозу безопасности. Эксперты категорически предупреждают: “пожалуйста, ради всего святого, не помещайте пароли в файлы application.yml”. В корпоративном code review обнаружение секретов в коде немедленно ведет к отказу в слиянии и инциденту безопасности. Кроме того, хардкод затрудняет поддержку: неизвестно, какие параметры использует система без изучения кода, нельзя централизованно управлять конфигами.

Как исправить: Выносите конфигурации во внешние настройки – application.properties/application.yml или системы управления секретами. Spring Boot поощряет использовать файлы конфигурации и профили, откуда значения легко менять. Пример: вынесем URL из кода в application.yml и подтянем через аннотацию:

# application.yml
myapp:
  url: http://localhost:8080

@Service
public class MySampleService {
    @Value("${myapp.url}")
    private String url;
    // ...
}

Теперь адрес определяется извне и может отличаться между средами. Для секретных данных (пароли, ключи) – никогда не храним их в репозитории. Вместо этого: либо задавать через переменные среды и ссылаться на них (например, ${DB_PASSWORD} в yaml), либо использовать менеджеры секретов (Vault, AWS Secrets Manager и т.п.). Spring Boot поддерживает привязку к env-переменным из коробки. Например:

spring:
  datasource:
    username: ${DB_USERNAME}
    password: ${DB_PASSWORD}

Таким образом, реальные значения подтягиваются из окружения при запуске приложения.

Если конфигураций много, стоит воспользоваться аннотацией @ConfigurationProperties для привязки целого класса к префиксу настроек – это упростит работу с ними в коде. Главное – никаких паролей и URL-ов в коде. Соблюдение этого правила не только порадует ревьюера, но и облегчает DevOps-процессы, позволяя менять параметры без перекомпиляции приложения.

“Магические” строки и числа

Как выглядит проблема: В коде часто встречаются “magic values” – литералы строк или чисел, используемые напрямую без объяснения. Примеры: проверка роли через строку "ADMIN", использование числа 1024 для лимита, ключи словарей написаны вручную в нескольких местах. Например:

if (user.getRole().equals("ADMIN")) {
    // ...
}
...
log.info("User with status=ACTIVE processed");
...
if (retryCount > 3) { ... }

Здесь "ADMIN", "ACTIVE" и число 3 – магические значения. Почему именно 3 попытки? Какие еще бывают роли пользователей? Неясно из контекста, а эти же литералы могут повторяться по коду.

Почему это ошибка: Магические строки/числа ухудшают понимание и поддержку. При их использовании возникает дублирование – одна и та же строка может быть в разных классах, и поправить ее (например, переименовать роль) придется во многих местах, что чревато ошибками. Человек, читающий код, не видит явного смысла литерала: "ADMIN" – это роль пользователя, а "ACTIVE" – статус, но лучше бы эти понятия были представлены константами или Enum с говорящими именами. К тому же, такие значения часто должны конфигурироваться, а не быть забиты в код. Ревьюеры расценивают это как нарушение принципа DRY (Don’t Repeat Yourself) и плохой стиль, поскольку код становится менее самодокументированным.

Как исправить: Вынести магические значения в константы, Enum или настройки. В Java объявите public static final константы с осмысленными именами:

public static final String ROLE_ADMIN = "ADMIN";
public static final int MAX_RETRY = 3;

И используйте их: if (user.getRole().equals(ROLE_ADMIN)) { ... }. Еще лучше – завести enum Role { ADMIN, USER, ... } и оперировать типами, а не строками. Например: if (user.getRole() == Role.ADMIN) { ... } – так и читается проще, и IDE поможет отследить все использования, исключая опечатки.

Для настраиваемых величин (лимитов, флагов) – выносите их в application.properties. Например, порог повторных попыток:

app.max-retry=3

и привязать через @Value("${app.max-retry}") либо через @ConfigurationProperties. Тогда сменить значение можно без правки кода, и оно явно задокументировано как часть конфигурации.

Строки, которые повторяются (например, ключи сообщений, статусы) – тоже выносите в константы или используйте Enum. Это упростит изменение и уменьшит риск несогласованности (когда в одном месте "ACTIVE", а в другом случайно "Actvie" – и баг).

Исключение можно сделать для буквальных констант, общеизвестных в контексте (например, SECONDS_PER_MINUTE = 60), но и их лучше оформить константой с именем, поясняющим смысл. Следуя этому правилу, вы сделаете код более чистым и понятным – ревьюер точно оценит положительно отсутствие “магии” в коде.

Игнорирование Spring Boot Starter-зависимостей

Как выглядит проблема: Spring Boot предлагает “стартеры” – готовые наборы зависимостей для типичных функций (веб, JPA, безопасность и т.д.). Однако некоторые разработчики по неопытности вручную перечисляют библиотеки Spring, вместо использования стартеров. Пример неправильного pom.xml:

<dependencies>
    <dependency>
        <groupId>org.springframework</groupId>
        <artifactId>spring-core</artifactId>
        <version>5.3.8</version>
    </dependency>
    <dependency>
        <groupId>org.springframework</groupId>
        <artifactId>spring-context</artifactId>
        <version>5.3.8</version>
    </dependency>
    <!-- еще куча зависимостей Spring -->
</dependencies>

Здесь явно подключены отдельные модули Spring (core, context и т.д.) с версией. Это не только избыточно, но и может привести к несогласованным версиям.

Почему это ошибка: Неиспользование стартеров лишает вас преимуществ Spring Boot по управлению зависимостями. Spring Boot стартер (например, spring-boot-starter-web) сам подтянет все нужные артефакты совместимых версий. Когда вы перечисляете вручную, есть риск конфликта версий или забыть какую-то транситивную зависимость. Кроме того, такой pom труднее поддерживать – при обновлении Spring надо менять каждую версию. Ревьюер, заметив “ванильные” spring-core, spring-context и прочие, скорее всего укажет: “заменить на spring-boot-starter”. Это упрощение и стандарт для Boot-проектов.

Как исправить: Использовать стартеры Spring Boot для соответствующей функциональности. В pom-файле достаточно одного <dependency> на нужный стартер. Например, эквивалент вышеуказанного вручную перечисленного:

<dependency>
    <groupId>org.springframework.boot</groupId>
    <artifactId>spring-boot-starter</artifactId>
</dependency>

Этот стартер уже включает core, context и другие базовые модули. Для веб-приложения берем spring-boot-starter-web, для JPA – spring-boot-starter-data-jpa и т.д. Стартеры автоматически подтягивают совместимые версии Spring и популярных библиотек (Jackson, Tomcat/Jetty, Hibernate etc.). В результате: меньше кода в pom.xml, меньше потенциальных ошибок с версиями и зависимостями, проще обновление Spring Boot версии.

К примеру, если нужен веб + Jackson, достаточно иметь:

<dependency>
    <groupId>org.springframework.boot</groupId>
    <artifactId>spring-boot-starter-web</artifactId>
</dependency>

и Boot принесет с собой Spring MVC, Jackson, валидатор и прочее. Это облегчает жизнь всем. Поэтому при ревью всегда проверяют pom/gradle на соответствие best practices, и отсутствие стартеров будет отмечено как недочет с предложением переделать под стандартный подход.

“Изобретение велосипеда” вместо готовых библиотек

Как выглядит проблема: Иногда разработчики реализуют вручную то, что уже существует в виде отлаженной библиотеки. Классические случаи: собственный парсер JSON вместо Jackson/Gson, самодельный пул потоков вместо ExecutorService, ручная реализация алгоритмов, которые есть в Guava/Apache Commons, и т.д. Например, был случай, когда программист написал свою функцию экранирования HTML-спецсимволов, вместо использования готовой библиотеки.

Почему это ошибка: Reinventing the wheel тратит время и зачастую приводит к менее надежному решению. Готовые библиотеки отточены сообществом, покрыты тестами и проверены временем. Собственная реализация может содержать скрытые баги, которые всплывут в продакшене. Например, упомянутая самодельная функция экранирования HTML работала годами, пока не встретила специфичный ввод, вызвавший бесконечный цикл и повесивший сервис. Пользователь повторял запрос, и все CPU сервера оказались заняты этим циклом. Такая авария не случилась бы, воспользуйся автор стандартной библиотекой для экранирования (например, HtmlEscapers из Google Guava). Внутри предприятий часто есть утвержденные библиотеки для разных задач, и отклонение от них потребует особых причин. Ревьюеры обычно знают эти инструменты и не примут PR с “велосипедом” без крайней необходимости.

Как исправить: Исследовать существующие решения прежде чем писать код. Если требуется парсинг JSON – почти всегда берут Jackson (или Gson). Нужна работа с датами – Java Time API или библиотека (JavaTime, Joda-Time в старых проектах). Генерация PDF – есть iText/Apache PDFBox. Любая распространенная задача скорее всего решается готовым компонентом. В корпоративном коде часто действуют правила: сперва библиотека, и только если ее нет или она не подходит, писать свое.

Конкретные шаги:

По функциональности, которую собираетесь реализовать, проведите поиск (внутри компании или в открытых источниках).
Ознакомьтесь с популярными библиотеками в этой области и их интеграцией с вашим стеком.
Обсудите с командой: возможно, у них уже есть опыт или внутренние утилиты для этого.

Пример: вместо самописного HTML-экранирования использовать Guava: HtmlEscapers.htmlEscaper().escape(inputString) – готовый метод для безопасного экранирования. Вместо собственного пула потоков – использовать Executors.newFixedThreadPool. Вместо велосипедного HTTP-клиента – взять Apache HttpClient или Spring RestTemplate/WebClient.

Исключения бывают (например, очень специфичная логика, под которую нет библиотеки), но это редкость. Вывод: используйте богатую экосистему Java. Это ускорит разработку и снизит число ошибок. При code review такое решение покажет вашу компетентность и знание инструментов, а неведение об общедоступных библиотеках будет минусом.

Неиспользование профилей для разных сред

Как выглядит проблема: Spring Boot и Quarkus позволяют иметь разные конфигурации для различных сред (development, testing, production) через механизм профилей. Тем не менее, некоторые разработчики игнорируют эту возможность и используют один набор настроек для всех случаев, либо переключают параметры вручную. Например, один application.properties, в котором прописаны dev-настройки (h2 база, локальные сервисы на localhost). В лучшем случае – комментируют/раскомментируют строки перед деплоем на прод, в худшем – вообще запускают прод с dev-конфигом.

Почему это ошибка: Отсутствие профилей ведет к ошибкам конфигурации и дополнительным усилиям при релизах. Параметры, оптимальные для разработки (например, логирование SQL, debug-режимы, встроенная БД) не должны попадать в production. Без профилей легко забыть поменять что-то при выкладке – были реальные случаи, когда приложение в продакшене подключалось не к той базе или с отключенным кэшированием, потому что конфиг остался девелоперским. Кроме того, единый конфиг раздувается множеством условных настроек, его трудно читать. Code review обычно требует разделить настройки на профили и явно указать, какой профиль активен по умолчанию.

Как исправить: Внедрить Spring Profiles (или их аналог в Quarkus) для разделения окружений. В Spring Boot делается так:

В основном application.yml указать активный профиль по умолчанию, например: spring.profiles.active: dev (для локальной разработки).
Создать отдельные файлы конфигурации: application-dev.yml, application-test.yml, application-prod.yml – с настройками, специфичными для среды. Например, в application-dev.yml использовать H2 и порт 8080, а в application-prod.yml – боевую БД и порт 80.
Либо использовать application.properties + application-dev.properties и т.д. – эффект тот же.

При запуске на конкретной среде активировать нужный профиль (-Dspring.profiles.active=prod или через переменную окружения). В результате разработчики не рискуют залить dev-конфиг на прод, потому что есть отдельный продовый профиль.

Пример: Пусть в dev-режиме приложение работает на встроенной БД H2, а в prod должно идти на PostgreSQL. В application-dev.yml пишем:

spring:
  datasource:
    url: jdbc:h2:mem:devdb

А в application-prod.yml:

spring:
  datasource:
    url: jdbc:postgresql://prod-db:5432/mydb

При сборке артефакт включает оба, и только нужный профиль применяется при запуске. Итог – конфиги для разных окружений изолированы, что снижает риск ошибок. Ревьюеры из DevOps-инженеров и опытных разработчиков настоятельно рекомендуют такой подход, и pull request с хардкодом окружения без профилей, скорее всего, завернут на доработку.

Отключение и невключение мониторинга (Actuator)

Как выглядит проблема: Spring Boot Actuator предоставляет готовые эндпоинты мониторинга (здоровье, метрики, инфо и пр.), которые крайне полезны в продакшене. Но некоторые разработчики либо вовсе не подключают Actuator, либо отключают все его endpoints из соображений “они не нужны” или “лишняя информация”. Например, в application.yml можно встретить:

management:
  endpoints:
    web:
      exposure:
        include: none    # выключаем все actuator-эндпоинты

Или просто отсутствие зависимости spring-boot-starter-actuator – тогда мониторинг не активирован.

Почему это ошибка: Игнорирование Actuator лишает команду ценных инструментов для поддержки приложения. Endpoints /actuator/health, /metrics, /env, /threaddump и др. позволяют быстро оценить состояние сервиса и собрать информацию для отладки в проде. Без них сложнее писать системы мониторинга (например, Kubernetes liveness/readiness пробы часто бьют по /health). Отключение этих функций – шаг назад в наблюдаемости (observability). В корпоративной среде часто есть требование иметь health-check и базовые метрики; если их нет, PR могут не принять до добавления. Кроме того, Actuator безопасен: по умолчанию чувствительные данные не экспонируются без включения, можно открыть только нужные endpoints и защитить их. Поэтому полное выключение выглядит неоправданным.

Как исправить: Включить Spring Boot Actuator и настроить разумный набор эндпоинтов. Во-первых, подключить зависимость:

<dependency>
    <groupId>org.springframework.boot</groupId>
    <artifactId>spring-boot-starter-actuator</artifactId>
</dependency>

Во-вторых, сконфигурировать, что открыть наружу. Например, обычно достаточно открыть хотя бы /health и /info:

management:
  endpoints:
    web:
      exposure:
        include: health, info

Тем самым, /actuator/health покажет статус приложения, а /actuator/info – полезную информацию (например, версию сборки). Остальные эндпоинты (метрики, трассировка) можно по ситуации тоже включить или закрыть за аутентификацией.

Итого: С Actuator вы получаете встроенный мониторинг, что крайне ценно для эксплуатации. После добавления этой фичи ревьюеры обычно довольны: приложение соответствует корпоративным стандартам по наблюдаемости. Если же оставить Actuator полностью отключенным без причины – это повод завернуть pull request, ведь вы лишаете команду инструмента, “который может предоставить важные сведения о состоянии приложения”.

Неинформативное логирование или его отсутствие

Как выглядит проблема: Логирование – еще одна область, где допускают ошибки. Две крайности: либо логов почти нет (разработчик полагается на отладку и не пишет логи), либо логи есть, но бесполезные. Например, встречается код:

log.info("User created");

И больше никакого контекста – кто создал, какой пользователь, сколько времени заняло – неизвестно. В другом случае могут использовать неправильные уровни: выводят чрезмерно много на INFO, что тонет в шуме. Или пишут стектрейс в ответ клиенту вместо лога на сервере.

Почему это ошибка: Плохое логирование усложняет сопровождение. В продакшене без хороших логов сложно разбираться в инцидентах. Сообщение "User created" каждый раз не скажет администратору ничего полезного – кто создан? успешно ли? сколько осталось до лимита? – и т.д. Darren Tan метко отметил: “лог “User created” бесполезен… Когда ваше приложение упадет в 3 ночи, вы поблагодарите меня за второй вариант”. Он предлагает вместо этого содержательный лог: log.info("Created user {} with email {} in {}ms", user.getId(), user.getEmail(), duration);. Также важно использовать правильные уровни логов: ERROR для критичных сбоев, WARN для потенциальных проблем, INFO для ключевых бизнес-событий, DEBUG для деталей отладки. Если все подряд пишется на INFO – логи превращаются в мусор, в котором трудно найти проблему.

Как исправить: Сделать логирование информативным и сбалансированным. Рекомендации:

Каждое лог-сообщение должно нести ценность. Добавляйте контекст: идентификаторы объектов, параметры, времени выполнения. Например, вместо просто "Start process" можно: log.info("Starting process {} for user {}", processId, userId).
Используйте placeholder’ы ({}), а не конкатенацию. Это лучше для производительности и избегает логирования, если уровень отключен.
Разделяйте уровни: ошибки (Exceptions) логируйте на ERROR вместе с стектрейсом (через log.error("Message", ex)), контролируемые проблемы – на WARN (без спама), нормальный бизнес-флоу – INFO, отладочные детали – DEBUG. Тогда при фильтрации по уровню можно получить нужный срез.
Не записывайте чувствительные данные в логи. Пароли, персональные данные – ни в коем случае, это требование безопасности.
Не перебарщивайте с объемом INFO-логов. Например, не нужно логировать вход каждого метода на INFO в продакшене – для этого есть DEBUG. Иначе гигабайты логов затруднят поиск реальной информации.

Перепишем приведенный пример:

// Плохо:
log.info("User created");
// Хорошо:
log.info("New user created: id={}, email={}, elapsed={}ms", user.getId(), user.getEmail(), elapsedMs);

Первый вариант – “лишний шум”, второй – даст понимание происходящего. Также убедитесь, что ключевые события покрыты логами. Отсутствие логирования там, где происходит важное действие (например, перевод денег, изменение данных) – упущение. Добавьте хотя бы INFO с результатом операции.

В итоге, грамотное логирование существенно помогает поддержке и отладке. В код-ревью на это тоже обращают внимание. Хороший ревьюер спросит: “А как мы узнаем, что тут произошло, если случится проблема в проде?”. Поэтому старайтесь сами задавать себе этот вопрос, пишет ли ваш код достаточно информации для будущего расследования проблем.

Отсутствие глобального обработчика ошибок

Как выглядит проблема: Если приложение не определяет единого механизма обработки исключений, то неожиданные ошибки могут “протекать” наружу. В REST-приложении это проявляется как трассировки стека Java в HTTP-ответах или необработанные 500 ошибки. Например, без контроллера советов (exception handler) падение метода может привести к тому, что пользователь API получит HTML-страницу с исключением или пустой ответ. Новички часто полагаются на поведение по умолчанию или обрабатывают ошибки прямо в контроллерах точечно, что не покрывает всех случаев.

Почему это ошибка: Необработанные исключения на уровне API – признак сырого, “любительского” решения. Пользователям не нужны стектрейсы NullPointerException в ответе, им нужен понятный код ошибки и сообщение. Без единого обработчика код получается засоренным повторяющимися try-catch в каждом контроллере или вовсе неучтенными кейсами. Это ухудшает пользовательский опыт и безопасность (ведь стек может раскрывать детали реализации). Ревьюер, увидев отсутствие @ControllerAdvice или аналогичного механизма, скорее всего порекомендует его добавить, чтобы централизовать обработку ошибок.

Как исправить: Внедрить глобальный exception handler. В Spring Boot достаточно создать класс с @RestControllerAdvice (или @ControllerAdvice + @ResponseBody) и объявить методы с @ExceptionHandler на нужные типы исключений. Пример:

@RestControllerAdvice
public class GlobalExceptionHandler {
    private static final Logger log = LoggerFactory.getLogger(GlobalExceptionHandler.class);

    @ExceptionHandler(UserNotFoundException.class)
    public ResponseEntity<ErrorResponse> handleUserNotFound(UserNotFoundException ex) {
        log.info("User not found: {}", ex.getMessage());
        return ResponseEntity.status(HttpStatus.NOT_FOUND)
                .body(new ErrorResponse("USER_NOT_FOUND", "User not found"));
    }

    @ExceptionHandler(ValidationException.class)
    public ResponseEntity<ErrorResponse> handleValidation(ValidationException ex) {
        log.warn("Validation failed: {}", ex.getMessage());
        return ResponseEntity.badRequest()
                .body(new ErrorResponse("VALIDATION_ERROR", ex.getMessage()));
    }

    @ExceptionHandler(Exception.class)
    public ResponseEntity<ErrorResponse> handleGeneral(Exception ex) {
        log.error("Unexpected error", ex);
        return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
                .body(new ErrorResponse("INTERNAL_ERROR", "Something went wrong"));
    }
}

Здесь ErrorResponse – ваш класс DTO для ошибок. Мы обработали конкретные бизнес-исключения (например, отсутствие пользователя, ошибка валидации) и общее исключение на последний случай. Логи пишутся с разными уровнями в зависимости от критичности.

Теперь все неперехваченные исключения в приложении преобразуются в структурированный ответ с понятным сообщением, а внутри все детали записаны в лог. Пользователи API получают консистентные ошибки, разработчики – централизованный код обработки.

Такой подход демонстрирует зрелость приложения. В code review его наличие – жирный плюс, отсутствие – минус. В Quarkus тоже есть аналогичные возможности (например, @Provider и ExceptionMapper для JAX-RS), их также стоит использовать.

Код-ревью обычно требует: “В приложении должен быть глобальный обработчик ошибок, возвращающий понятные клиенту сообщения”. Реализовав его, вы избежите кучи проблем и получите одобрение от ревьюеров.

Проглатывание исключений в блоках catch

Как выглядит проблема: Антипод предыдущего пункта – когда исключения ловят, но ничего с ними не делают. Например:

try {
    // ... какая-то опасная операция
} catch (Exception e) {
    // тихо проигнорируем
}

Или оставляют комментарий вроде // TODO: handle later и продолжают выполнение. Иногда в пустом catch могут хотя бы напечатать e.printStackTrace() или лог на debug, но с точки зрения надежности это мало что меняет – ошибка фактически замалчивается.

Почему это ошибка: Игнорирование исключений не устраняет проблему, а скрывает ее. Приложение может перейти в некорректное состояние, ведь блок catch перехватил исключение, но не исправил ситуацию и не сообщил вызывающему коду. Это ведет к непредсказуемым ошибкам дальше по исполнению или просто к тихому неверному результату. К тому же, если исключение подавлено, отладить потом такой случай крайне сложно: ни стектрейса, ни сообщения – ничего нет. Toptal отмечает: исключения бросаются не просто так, обычно их надо либо обработать, либо хотя бы залогировать. Проглатывание – плохая практика как для новичков, так и для опытных. Ревьюеры рассматривают пустой catch как баг, требуя либо обработать, либо убрать вовсе.

Как исправить: Обрабатывать или прокидывать исключения, но не молчать. Конкретные варианты:

Если знаем, как исправить ситуацию – сделать это в catch. Например, при временной недоступности сервиса – повторить запрос через некоторое время.
Если не можем обработать – по крайней мере залогировать на ERROR/WARN с контекстом, чтобы проблема не прошла незамеченной.
Если исключение не критично – можно подавить, но явно указать причину. Например, если пустой catch оправдан (очень редкий случай), хотя бы оставьте комментарий, почему это безопасно: // Нормальная ситуация, если записи не существует - просто пропускаем.
Часто правильнее пробросить исключение выше – либо тем же типом (делегируя ответственность), либо обернуть в свое (добавив контекст) и бросить. Тогда глобальный обработчик (см. предыдущий пункт) или вышележащий код разберется.

Пример плохого кода и исправления:

try {
    processOrder(order);
} catch (IOException e) {
    // ничего
}

Лучше так:

try {
    processOrder(order);
} catch (IOException e) {
    log.error("Failed to process order {}: {}", order.getId(), e.getMessage(), e);
    throw new OrderProcessingException("Error processing order " + order.getId(), e);
}

Здесь мы не оставили сбой незамеченным: залогировали и пробросили свое исключение (можно обработать на уровне API и вернуть понятную ошибку пользователю). Либо, если решаем продолжить без падения:

} catch (IOException e) {
    log.warn("Order {} will be skipped due to error: {}", order.getId(), e.getMessage());
    // пропустить этот заказ и продолжить цикл, например
}

Важно, что мы явно отразили это решение – и в коде (комментарием/логом), и для читателя кода очевидна логика.

В итоге: никогда не оставляйте catch пустым. Пустой catch – “заметание грязи под ковер”, приводящее к будущим сюрпризам. Хороший код либо обрабатывает, либо сообщает об исключениях, и ревьюеры настаивают на этом.

Неправильное использование транзакций

Как выглядит проблема: Работа с транзакциями в Spring часто пренебрегается или применяется неверно. Типичные ошибки:

Забывают поставить @Transactional над методом сервиса, выполняющим несколько операций с базой (insert/update), которые должны быть атомарными.
Ставят @Transactional не туда – например, на private-метод (который Spring не проксифирует, и транзакция не стартует), или на метод контроллера (нежелательно, лучше на уровень сервисов).
Выполняют операции вне транзакции, в результате изменения сохраняются частично при ошибках.

Пример: метод обновления заказа сохраняет Order и затем Payment, но без транзакции. Если сохранение Payment упадет, Order уже останется в базе – данные в непоследовательном состоянии.

Почему это ошибка: Отсутствие корректной транзакционности ведет к неконсистентности данных. Если несколько связанных операций не завернуть в транзакцию, то при сбое посередине часть изменений останется в БД, а часть откатится, нарушая целостность. Это очень опасно для бизнес-данных (например, спишутся деньги, но заказ не создастся и т.п.). С другой стороны, неправильное использование аннотации – например, на методе, который Spring не обрабатывает – создает иллюзию транзакции, хотя на деле ее нет. Ревьюер, замечая вызовы нескольких репозиториев подряд без @Transactional, обязательно спросит, не нужна ли транзакция. Или укажет, что @Transactional на private методе не сработает, потому что Spring AOP по умолчанию не проксирует приватные вызовы. Игнорирование этих нюансов – частая причина багов в продакшене.

Как исправить: Применять @Transactional осознанно там, где это требуется. Рекомендации:

Аннотацию @Transactional вешать на публичные методы сервисного слоя, которые выполняют несколько операций, требующих атомарности (несколько сохранений, или чтение + запись, и т.п.).
Не использовать @Transactional на приватных или внутренних вызовах – она попросту не будет действовать. Если нужно, сделайте такой метод публичным в том же бине или объедините логику.
Понимать границы транзакции: транзакция откатывается при RuntimeException по умолчанию. Если ловите исключение внутри, при необходимости вызывайте TransactionAspectSupport.currentTransactionStatus().setRollbackOnly() для отката.
В Quarkus/Panache аналог – аннотация @Transactional на методе, работает схожим образом.
Не открывайте чрезмерно длинные транзакции (например, охватывающие внешние вызовы или длительные операции) – это уже вопрос оптимизации.

Пример правильного использования:

@Service
public class OrderService {
    @Transactional
    public void placeOrder(Order order, Payment payment) {
        orderRepository.save(order);
        paymentRepository.save(payment);
        // при любой RuntimeException Spring откатит обе операции
    }
}

Если забыть @Transactional, то возможна ситуация частичного сохранения. Поэтому всегда проверяйте: логически связанная последовательность операций с БД должна быть в транзакции.

Ревьюеры в корпоративных проектах очень внимательны к этому: данные – сердце бизнеса. Они скорее потребуют добавить @Transactional, чем позволят потенциальную рассинхронизацию. И наоборот, лишняя транзакция там, где не нужна, тоже может вызвать вопросы (но это реже вредит). В итоге придерживайтесь правил транзакционности Spring – и ваш PR получит одобрение, а данные – защиту от несогласованных изменений.

Утечки ресурсов (незакрытые потоки, соединения)

Как выглядит проблема: В Java по-прежнему актуальны ошибки, связанные с неосвобождением ресурсов. Файлы, сетевые сокеты, JDBC ResultSet/Statement – все это нужно закрывать после использования. Если разработчик забыл вызвать close(), ресурс останется висеть до сборщика мусора. Пример проблемы:

FileInputStream fis = new FileInputStream("data.txt");
int data = fis.read();
// ... читаем файл
// fis.close(); // ОПУЩЕНО!

Или открыли Connection к базе вручную и не закрыли. В Web-приложениях также встречается утечка HTTP-коннекшенов, если не закрывать response объекты.

Почему это ошибка: Утечка ресурсов может вызвать исчерпание файловых дескрипторов, портов или соединений. Это проявляется не сразу, но со временем сервис начнет странно работать: отказывать в новых соединениях, тормозить или падать. Например, если не закрывать файлы, OS рано или поздно не даст открыть новые (ошибка “Too many open files”). В случае БД-пулов – соединения могут утекать и пул исчерпается, блокируя приложение. Java имеет сборщик мусора, но полагаться на финализаторы – плохая идея: мы не знаем, когда GC сработает. В итоге ресурс может быть занят очень долго. Ревьюеры, замечая явное использование ресурсов (файлов, потоков) без try-with-resources или явного закрытия, обязательно указывают на это, потому что это распространенная проблема в реальных баг-репортах.

Как исправить: Всегда освобождать ресурсы гарантированно. С введением try-with-resources (Java 7+) это стало проще:

try (FileInputStream input = new FileInputStream("file.txt")) {
    // использовать input
    int data;
    while ((data = input.read()) != -1) {
        // ... обработка
    }
}  // input.close() вызовется автоматически тут

Конструкция try(FileInputStream input = ...){ } сама закроет input по выходу из блока, независимо от исключений. Это лучший способ для всех AutoCloseable ресурсов (файлы, сокеты, JDBC и т.д.). Если по каким-то причинам нельзя использовать try-with-resources (допустим, в старом коде на Java6), то хотя бы в finally блоке вызывать close():

FileInputStream input = new FileInputStream(...);
try {
    // ... работа
} finally {
    if (input != null) input.close();
}

Но manual try-finally легко забыть, поэтому предпочтительно обновить до try-with-resources.

В Spring приложениях многие ресурсы управляются фреймворком (например, DataSource с пулом сам возвращает соединение в пул), но при ручной работе все равно внимательно следите. Например, получая InputStream от MultipartFile, заверните чтение в try-с-ресурсами.

Дополнительно: утечки могут быть не только явных ресурсов, но и памяти через коллекции (см. далее) или потоков. Всегда, где берете ресурс – планируйте его освобождение.

Как отметил один эксперт, Java-младшим разработчикам важно помнить: “каждый раз, когда программа открывает файл или соединение, нужно освобождать ресурс, когда он больше не нужен”. Соблюдайте это правило, и ваши код-ревью пройдут гладко, а приложения – будут стабильнее.

Отсутствие тестов и слабое покрытие

Как выглядит проблема: В условиях жестких сроков некоторые разработчики пишут код без достаточных автотестов. Pull Request может содержать новую функциональность, но не содержать сопутствующих unit/integration тестов. Либо тесты есть, но написаны абы как – один гигантский метод, проверяющий сразу все, или исключительно “счастливый путь” без проверок ошибок. Пример анти-паттерна теста:

@SpringBootTest
class UserServiceTest {
    @Test
    public void testEverything() {
        // 200 lines of setup and assertions testing сразу все сценарии
        // Трудно понять, что именно проверяется и где сбой
    }
}

Почему это ошибка: Отсутствие/плохое качество тестов снижает доверие к коду и увеличивает риск дефектов. Code review в крупных компаниях обычно предполагает: если добавляется новая логика, должны быть тесты, покрывающие основные случаи (и особенно краевые). Без тестов велика вероятность, что код содержит скрытые баги, которые всплывут уже на проде. Кроме того, тесты выступают в роли спецификации – по ним другие разработчики понимают, как должен работать код. Их отсутствие осложняет рефакторинг: никто не уверен, что изменение не сломает функциональность. Многие команды имеют минимальные требования по покрытию (%) или хотя бы по наличию тестов в PR. Неудивительно, что PR без тестов могут развернуть с комментарием “добавить, пожалуйста, тесты”.

Как исправить: Писать тесты параллельно с кодом. Лучше всего применять TDD или хотя бы добавлять тесты сразу, не откладывая “на потом”. Рекомендации:

Покрыть unit-тестами ключевые компоненты бизнес-логики (сервис-слой, утилиты). Использовать Mock для внешних зависимостей (БД, интеграции) либо подход Arrange-Act-Assert.
Для веб-слоя использовать Spring’s MockMvc или аналог в Quarkus для имитации HTTP-запросов, проверять ответы (код, тело).
Если логика зависит от контекста Spring, можно писать тесты с @SpringBootTest, но умеренно. Интеграционные тесты важны, но они медленнее – основные проверки лучше вынести в unit-tests с подменой зависимостей.
Писать мелкие, изолированные тесты вместо одного громадного. Каждый тест проверяет одну вещь, тогда при падении сразу ясно, что сломалось.
Не забывать про негативные сценарии: тест на то, что бросается исключение при неверных параметрах, и т.п.

Пример улучшения структуры теста из указанного плохого варианта:

@ExtendWith(MockitoExtension.class)
class UserServiceTest {
    @Mock private UserRepository userRepository;
    @Mock private EmailService emailService;
    @InjectMocks private UserService userService;

    @Test
    void shouldCreateUserWithValidRequest() {
        // Given
        CreateUserRequest req = new CreateUserRequest("John", "john@example.com");
        when(userRepository.save(any(User.class))).thenReturn(new User(1L, "John", "john@example.com"));

        // When
        User result = userService.createUser(req);

        // Then
        assertNotNull(result.getId());
        assertEquals("John", result.getName());
        verify(emailService).sendWelcomeEmail(result);
    }

    // ... другие тесты на разные сценарии ...
}

Здесь с помощью Mockito мы изолировали UserService от зависимостей, проверяем конкретный исход с понятными Given-When-Then блоками. Такой тест быстрый, простой и локализованный.

Конечно, нужны и интеграционные тесты для проверок сквозных историй, но даже минимальный набор unit-тестов уже сильно повышает качество.

Суть в том, что тесты – не опция, а часть задачи. Культуры разработки, ориентированные на качество, не приемлют PR без тестов, покрывающих новые изменения. Как метко сказано: “Нет времени писать тесты” – обычно говорит тот, у кого потом найдется три недели на отладку багов, которые мог бы поймать простой тест”. Так что не экономьте на тестировании – и ваши ревьюеры, и ночной сон вашей команды будут вам благодарны.

Смешивание синхронного и асинхронного кода

Как выглядит проблема: При использовании асинхронных операций (CompletableFuture, Reactive Streams, Quarkus Mutiny, или просто @Async в Spring) иногда неправильно сочетают их с синхронным кодом. Например:

Вызывают .get() или .join() на Future/CompletableFuture внутри асинхронного метода, блокируя поток.
В Quarkus Reactive (Mutiny) или Spring WebFlux – вызывают императивные блокирующие операции внутри реактивных цепочек (например, Thread.sleep, или JDBC-запрос без адаптера), что тормозит event loop.
В целом, пишут async метод, но сразу же ожидают результат синхронно.

Пример в псевдо-Java:

@Async
public CompletableFuture<Data> getDataAsync() {
    Data data = syncClient.getData(); // синхронный вызов внутри async метода
    return CompletableFuture.completedFuture(data);
}

// И где-то при вызове:
CompletableFuture<Data> future = service.getDataAsync();
Data result = future.get(); // блокирующее ожидание

Здесь получается бессмысленная асинхронность: метод помечен @Async, но сразу блокируется на syncClient, а потом вызывающий код блокируется на .get().

Почему это ошибка: Неправильное смешение моделей лишает преимуществ асинхронности и может привести к deadlock’ам или истощению ресурсов. Например, в C# (аналогичная концепция) известно, что вызов .Result на Task внутри async-контекста может повесить поток (deadlock). В Java тоже, если вызвать Future.get() в event loop потоке, вы блокируете его. Смысл асинхронного подхода – не блокировать поток, а дать выполнить другие задачи. Если же внутри все равно идет блокирующий вызов, вы получаете худшее из двух миров: сложность async-кода + блокировки потоков. Rевьюеры, знакомые с асинхронным программированием, сразу обращают внимание на такие места. Это одна из более сложных для отладки ошибок – приложение может зависать под нагрузкой или не масштабироваться из-за заблокированных потоков.

Как исправить: Выбирать единый подход и не блокировать без необходимости. Если вы пишете реактивный/асинхронный код, старайтесь:

По возможности использовать неблокирующие аналоги операций. Например, для доступа к БД – реактивные драйверы (R2DBC) если вы на WebFlux, либо выносить блокирующие операции в отдельный Scheduler (для Reactor) или Uni.runAsync (для Mutiny) и пр.
Не ждать Future внутри потоков обработки. Вместо future.get(), верните сам Future вверх по вызовам или используйте механизм колбэков/thenAccept. Пусть верхний уровень (например, контроллер) либо возвращает DeferredResult/CompletableFuture (Spring MVC умеет его ждать), либо подписывается на Mono/Flux.
Если API не позволяет и надо смешать, например, вызвать синхронный код внутри асинхронного: выделите этот вызов в отдельный thread-pool, чтобы не блокировать основной рабочий поток. В Spring можно аннотировать метод @Async или использовать CompletableFuture.supplyAsync с Executor.

Например, проблема блокировки:

CompletableFuture<String> dataFuture = service.getDataAsync();
String data = dataFuture.get(); // ПЛОХО: блокирует текущий поток

Решение – сделать метод, вызывающий service, тоже асинхронным, возвращающим CompletableFuture или использовать callback:

service.getDataAsync().thenAccept(data -> {
    // обработка результата без блокирования
});

В реактивном stream вместо прямого блока через .block() – использовать операторы Reactor (map, flatMap) или Mutiny chain.

Или если mixing unavoidable:

CompletableFuture<Data> future = CompletableFuture.supplyAsync(() -> syncClient.getData(), myExecutor);

по крайней мере выносим блокирующую вызов в другой пул.

Главное правило: либо все делаем последовательно (синхронно), либо от начала до конца реактивно/асинхронно. Не делайте “полу-async, полу-sync” – это подводит. Как заметил один автор, нельзя смешивать sync и async без последствий: это чревато дедлоками. В контексте Quarkus reactive, подобная ошибка особенно критична – event loop поток должен оставаться свободным.

На ревью код с .get() на Future, .block() на Mono или await().indefinitely() на Uni почти всегда вызовет вопросы: “Почему здесь блокирующий вызов? Можно ли переписать, чтобы обойтись без блокировки?”. Поэтому заранее проектируйте асинхронное взаимодействие правильно.

Непотокобезопасный код (проблемы многопоточности)

Как выглядит проблема: Enterprise-приложения часто многопоточные (веб-сервер обрабатывает запросы параллельно, фоновые задачи и т.д.). Типичные concurrency-ошибки:

Использование непотокобезопасных объектов из нескольких потоков без синхронизации. Например, классическое: SimpleDateFormat в static-поле – приводящий к непредсказуемым результатам, потому что SimpleDateFormat не thread-safe.
Модификация коллекции во время итерации из другого потока -> ConcurrentModificationException.
Общая изменяемая переменная без ключевого слова volatile или без синхронизации, что приводит к гонкам данных.
Неправильная реализация double-checked locking или ленивая инициализация без synchronization, что нарушает happens-before и может вернуть неинициализированный объект.

Например, подобный код – проблема:

public class UserCache {
    private static final List<User> USERS = new ArrayList<>();

    public static void addUser(User u) {
        USERS.add(u);
    }
    public static User getUser(int index) {
        return USERS.get(index);
    }
}

Если UserCache.addUser вызывается параллельно с getUser из разных потоков, то отсутствие синхронизации может привести к неправильным чтениям или ConcurrentModificationException.

Почему это ошибка: Несоблюдение потокобезопасности приводит к трудноуловимым багам: некорректные данные, случайные падения, зависания. В высоконагруженных системах такие ошибки проявляются на продакшене под нагрузкой, хотя могли не проявиться в тестах. ConcurrentModificationException – частый сигнал, что коллекцию модифицируют из параллельного потока во время перебора. Другой пример: без volatile флаг остановки потока может не сработать вовремя из-за кеширования. Ревьюеры внимательно смотрят на shared-объекты. Если видят new Thread(...) или ExecutorService, или static-синглтоны – проверят, обеспечена ли правильная синхронизация. Не отмеченные как thread-safe коллекции (ArrayList, HashMap) – тоже маркер. В крупных компаниях могут быть статические анализаторы, ловеющие такие вещи, но и люди тоже.

Как исправить: Делать код потокобезопасным там, где это необходимо:

Избегайте мутируемого глобального состояния. Если нужно – защищайте его. Например, вместо ArrayList используйте CopyOnWriteArrayList или синхронизируйте доступ (Collections.synchronizedList либо явно через synchronized блок).
Если коллекцию изменяют из нескольких потоков – либо используйте коллекции из java.util.concurrent (например, ConcurrentHashMap, ConcurrentLinkedQueue), либо синхронизируйте на внешнем объекте.
При итерации и модификации – или копируйте коллекцию, или используйте итераторы с remove (Iterator.remove() безопасен, если итератор от Concurrent коллекции).
Для shared флагов или кэшей – изучите атомарные типы (AtomicBoolean, AtomicInteger) или volatile, или Lock-и для сложных случаев.
Особо остерегайтесь одиночек с ленивой инициализацией – правильно реализовывайте с помощью enum Singleton (потокобезопасно by design) или static holder. Double-checked locking в Java требует volatile для instance поля.

Например, решим проблему UserCache выше так:

public class UserCache {
    private static final List<User> USERS = Collections.synchronizedList(new ArrayList<>());
    // методы addUser, getUser можно уже оставлять - список сам синхронизирован на операции
}

Или использовать CopyOnWriteArrayList если читается часто, а пишется редко (для списков слушателей, например).

Если нужна более высокая производительность, то ConcurrentHashMap/ConcurrentLinkedQueue для соответствующих задач.

Другой пример: класс DateTimeUtil со static SimpleDateFormat – решить заменой на thread-safe аналог (например, DateTimeFormatter из java.time, он иммутабельный и безопасен).

По сути, правильно используйте примитивы синхронизации и concurrent-библиотеки Java. Они существуют, чтобы облегчить жизнь – не нужно “изобретать” свою логику без очень хорошей причины.

При code review, когда дело касается многопоточности, лучше перестраховаться: спросите себя, будет ли этот код выполняться параллельно? Если да, убедитесь, что безопасно. Ревьюер, заметив потенциальную гонку, обязательно напишет об этом – такие баги потом дорого обходятся. Лучше самому их устранить заранее.

Неэффективные запросы к базе данных (N+1)

Как выглядит проблема: Одна из самых известных проблем производительности – N+1 запрос. Это происходит, когда код вместо одного хорошо спроектированного запроса выполняет сначала запрос за списком, а затем внутри цикла по этому списку делает еще N запросов по одному на элемент. Например, pseudo-code:

List<Order> orders = orderRepository.findAll();         // 1 запрос: SELECT * FROM orders
for (Order o : orders) {
    Customer c = customerRepository.findById(o.customerId);  // N запросов: SELECT * FROM customers WHERE id=...
    o.setCustomer(c);
}

В итоге, если 1000 заказов – будет 1001 запрос к БД. Это крайне неэффективно. Такая проблема часто возникает при неоптимизированных ORM-связях (ленивая загрузка без join).

Почему это ошибка: N+1 ведет к серьезным тормозам на больших данных или высоких нагрузках. База данных испытывает гораздо больше нагрузки из-за множества мелких запросов, сетевые задержки суммируются. В реальных кейсах такие места обнаруживались, когда страница, загружающая данные, генерировала сотни запросов и работала медленно. Как отмечено в одном блоге, некоторые приложения “делали 100 запросов к БД, чтобы отрендерить одну страницу”. Это ухудшает масштабируемость. На код-ревью производительность – важный аспект, особенно в корпоративных приложениях. Опытные ревьюеры знают, где может прятаться N+1: часто при выборке сущностей с связями. Они могут попросить привести SQL-лог или обратить внимание, что в коде внутри цикла вызывается метод репозитория – явный запах N+1.

Как исправить: Оптимизировать выборку данных и использовать подходящие механизмы. Способы решения:

Жадная загрузка/Join fetch: Если используете ORM (Hibernate), настроить отношения с fetch=JOIN при необходимости, или использовать EntityGraph/join fetch в JPQL, чтобы нужные связанные данные получались сразу одним запросом.
Явно писать SQL с JOIN: Например, перейти к написанию запроса, который объединяет таблицы (SELECT … FROM orders o JOIN customer c ON o.customer_id = c.id), чтобы получить все за раз.
Кэширование: В некоторых случаях можно кешировать справочные данные (например, если часто вытягивается один и тот же Customer).
Batch операции: Вместо индивидуальных запросов в цикле, использовать WHERE id IN (...) с набором ID из списка. Многие ORM умеют это через методы типа findAllById(list).
Stream/Chunking: Если нужно обработать очень много записей, можно итерировать по стриму из БД (не загружать все в память сразу).
ПрофILING SQL: Всегда полезно включить вывод запросов при тестировании (spring.jpa.show-sql=true) или использовать профайлер базы, чтобы увидеть, сколько запросов идет и оптимизировать.

Конкретно для N+1 с примером выше: можно решить с помощью JOIN-запроса или ORM-relationship. Если у Order есть поле Customer с @ManyToOne(fetch = LAZY), то при access order.getCustomer() без сессии – будет отдельный запрос. Решение: написать метод репозитория findAllWithCustomers() с JPQL: SELECT o FROM Order o JOIN FETCH o.customer. Тогда один запрос вернет заказы вместе с клиентами.

Пример JDBC-решения: вместо цикла с findById, выполнить один запрос:

SELECT o.*, c.* 
FROM orders o 
JOIN customers c ON o.customer_id = c.id;

и мапить результат на объекты.

Вывод: следите за загрузкой связанных данных. 100 запросов там, где можно 1 – это плохо. Ревьюеры, увидев цикл, делающий репозиторий-вызов, сразу укажут на N+1. Лучше самому это предотвратить, особенно при работе со Spring Data JPA/Hibernate: изучите логи выполнения запросов, убедитесь, что все оптимально. В противном случае, даже если ревью пропустит, продакшн под нагрузкой может выдать сюрпризы – а крайних искать начнут с того, кто написал этот код.

Игнорирование безопасности (SQL-инъекции, отсутствие валидации)

Как выглядит проблема: Безопасность кода – критически важна, но иногда упускается из виду при разработке. Распространенные ошибки:

SQL-инъекция: Формирование SQL-запросов через конкатенацию строк с пользовательским вводом. Например: String query = "SELECT * FROM users WHERE name = '" + name + "'"; stmt.executeQuery(query); Если name содержит что-то вроде a' OR '1'='1, запрос изменится на непредвиденный, что позволяет извлечь лишние данные или хуже.
Отсутствие валидации входных данных: Приложение принимает данные от пользователя и напрямую использует их, не проверяя формат, длину, диапазоны. Это может привести к XSS, SQL-инъекциям, XML-инъекциям и проч.
Хранение паролей в открытом виде: В коде или базе данных хранить пароли/ключи неhashed или в коде – огромная проблема.
Недостаточные проверки прав доступа: Например, API-метод пропускает запросы без авторизации там, где она нужна (отсутствует @PreAuthorize или эквивалент).

Почему это ошибка: Уязвимости безопасности могут привести к компрометации данных, взлому системы, штрафам за несоблюдение стандартов (типа GDPR) и репутационным потерям. SQL-инъекция – один из самых опасных и до сих пор часто встречающихся багов, особенно если напрямую работать с JDBC или формировать HQL вручную. OWASP приводит пример: если приложение конкатенирует строку в SQL, атакующий может выполнить произвольный SQL-код. Последствия – утечка данных, удаление таблиц и т.д. Отсутствие проверок вводимых данных может привести к XSS (когда вредоносный скрипт отобразится другим пользователям) или другим атакам. Code review обязательно включает проверку на очевидные проблемы безопасности. Найти SQL-конкатенацию – легко, и такой PR не пройдет.

Как исправить: Следовать принципу “Безопасность по умолчанию”:

Использовать параметризированные запросы вместо конкатенации. В JDBC – PreparedStatement с ? параметрами. В примере выше: String query = "SELECT * FROM users WHERE name = ?"; PreparedStatement pstmt = conn.prepareStatement(query); pstmt.setString(1, name); ResultSet rs = pstmt.executeQuery(); Теперь даже если name содержит кавычки или SQL, оно не изменит структуру запроса – БД воспримет это как значение, а не код. В ORM-фреймворках – используйте Named Parameters или метод с @Param в репозитории, чтобы данные подставлялись безопасно, либо QueryDSL, где запрос строится безопасно. Никакой конкатенации SQL строк с входом пользователя.
Валидация и санитизация входных данных: Проверять длину строк, формат (через регулярки, через аннотации в Spring @Size, @Pattern, @Email и т.д.), убирать или экранировать опасные символы. Для web-приложений – использовать встроенные механизмы (например, Spring автоматически экранирует Model атрибуты при выводе в JSP, если включен ESCaping, но для JSON API нужно самому думать, что отдаешь).
Пароли и секреты: никогда не хранить в коде (см. пункт 5), а при хранении в БД – только в виде хэш + соль (например, BCrypt). Проверить, что нигде в логах или исключениях не утекут секреты.
Проверка прав: убедиться, что все чувствительные эндпоинты защищены. В Spring Security – использовать @PreAuthorize на методах или настройку HttpSecurity, чтобы никто не вызвал admin-функцию без прав. Не полагаться на “скрытие” URL – проверяйте на сервере.
Минимальные привилегии: аккаунт БД, под которым работает приложение, не должен иметь прав на те операции, что не нужны (например, DROP TABLE и т.п.). Тогда даже при SQL-инъекции ущерб будет ограничен.

Применив эти меры, вы закроете основные дыры. Ревьюеры, увидев, что вы используете PreparedStatement или JPA Repository с параметрами, сразу спокойны – SQL-инъекции не будет. А вот строчка "WHERE id=" + userInput мгновенно вызовет реакцию с просьбой переделать на безопасный вариант. Аналогично, отсутствие явной валидации, особенно в местах работы с внешним вводом (REST-контроллеры, сервисы, обрабатывающие пользовательские файлы) – скорее всего, будет комментировано.

Безопасность – не та область, где стоит упрощать. В корпоративной разработке могут быть даже отдельные security-рецензии кода. Но если в вашем PR ревьюер сам заметил очевидную уязвимость – велика вероятность, что доверие к вашему коду снизится. Поэтому лучше самому подойти с параноей: “Что если злоумышленник сюда что-то подсунет?” – и убедиться, что ничего страшного не произойдет.

Заключение

Код-ревью – это не придирки ради придирок, а важный фильтр качества. Часто замечания повторяются от проекта к проекту, формируя системный список “анти-паттернов” корпоративной разработки. В этой статье мы рассмотрели 20 наиболее частых ошибок Java-разработчиков: от архитектурных просчетов (как “божественные” контроллеры и хаотичная структура) до низкоуровневых багов (утечки ресурсов, гонки потоков) и промахов в использовании фреймворков (Spring, Quarkus). Каждая из них способна стать причиной отклонения pull request’а – и это к лучшему, ведь исправление на этапе обзора кода предотвращает будущие сбои в работе приложения.

Чтобы резюмировать, приведем несколько рекомендаций:

Думайте о поддержке и читателе кода. Соблюдайте чистую архитектуру, пишите мелкие методы с понятными именами. Поставьте себя на место коллеги, который будет ревьюить или сопровождать код – сделайте так, чтобы ему не хотелось “выйти в окно” при чтении вашего PR.
Используйте возможности экосистемы. Не бойтесь полагаться на проверенные библиотеки и инструменты Spring Boot. Правильно настраивайте конфигурацию, профили, мониторинг – это избавит от множества проблем и ускорит разработку.
Уделяйте внимание деталям надежности. Глобальный обработчик ошибок, корректная работа с транзакциями, закрытие ресурсов – это основа зрелого приложения. Такие детали отличают джуниора от синьора.
Пишите тесты. Они не только ловят ошибки, но и демонстрируют ревьюеру (и вам самим), что код работает как задумано. Отсутствие тестов – красный флаг в корпоративной среде.
Не забывайте о безопасности и производительности. Следите за вводом данных (никаких SQL-инъекций, XSS), контролируйте количество запросов к базе, оптимизируйте критичные участки. Лучше сразу сделать правильно, чем потом получать инциденты в продакшене.

В конечном счете, цель code review – не придраться, а сделать код лучше. Прислушиваясь к типовым замечаниям и заранее избегая рассмотренных ошибок, вы экономите время всей команды и повышаете качество продукта.

PROSELYTE

Software Engineering

20 типовых ошибок Java-разработчиков в пул реквестах

Оглавление

Введение

Монолитные “God-классы” в контроллерах

Отсутствие четкой архитектуры и структуры пакетов

Длинные методы и нарушение принципа SRP

Инъекция полей вместо конструкторов в Spring

Хардкод конфигураций и секретов в коде

“Магические” строки и числа

Игнорирование Spring Boot Starter-зависимостей

“Изобретение велосипеда” вместо готовых библиотек

Неиспользование профилей для разных сред

Отключение и невключение мониторинга (Actuator)

Неинформативное логирование или его отсутствие

Отсутствие глобального обработчика ошибок

Проглатывание исключений в блоках catch

Неправильное использование транзакций

Утечки ресурсов (незакрытые потоки, соединения)

Отсутствие тестов и слабое покрытие

Смешивание синхронного и асинхронного кода

Непотокобезопасный код (проблемы многопоточности)

Неэффективные запросы к базе данных (N+1)

Игнорирование безопасности (SQL-инъекции, отсутствие валидации)

Заключение

Related

Оглавление

Введение

Монолитные “God-классы” в контроллерах

Отсутствие четкой архитектуры и структуры пакетов

Длинные методы и нарушение принципа SRP

Инъекция полей вместо конструкторов в Spring

Хардкод конфигураций и секретов в коде

“Магические” строки и числа

Игнорирование Spring Boot Starter-зависимостей

“Изобретение велосипеда” вместо готовых библиотек

Неиспользование профилей для разных сред

Отключение и невключение мониторинга (Actuator)

Неинформативное логирование или его отсутствие

Отсутствие глобального обработчика ошибок

Проглатывание исключений в блоках catch

Неправильное использование транзакций

Утечки ресурсов (незакрытые потоки, соединения)

Отсутствие тестов и слабое покрытие

Смешивание синхронного и асинхронного кода

Непотокобезопасный код (проблемы многопоточности)

Неэффективные запросы к базе данных (N+1)

Игнорирование безопасности (SQL-инъекции, отсутствие валидации)

Заключение

Поделиться

Related