Сделать рефакторинг сервиса пользователей

13. Сделать рефакторинг сервиса пользователей

Условие задачи:
Дан сервис UserService, который создаёт пользователей и связывает их с регионом.

Необходимо провести code review и исправить проблемы, связанные с:

созданием и внедрением зависимостей;
использованием ApplicationContext;
транзакциями;
ошибками компиляции;
стилем и именованием;
обработкой списка пользователей;
выбором корректной границы транзакции.

Код:

class UserService {

    private UserRepository repo = new UserRepository();
    private RegionService regionService;

    public UserService(final ApplicationContext appCtx) {
        regionService = appCtx.getBean(
                "regionService",
                RegionService.class
        );
    }

    public void processNewUsers(
            final List<User> users,
            String regionName
    ) {
        // ...

        users = createUsers(users);

        // ...

        users.stream()
                .foreach(u ->
                        regionService.updateRegionLink(
                                u.getId(),
                                regionName
                        )
                );
    }

    @Transactional
    public List<User> createUsers(
            final List<User> users
    ) {
        return users.stream()
                .map(u -> repo.saveUser(u))
                .collect(Collectors.toList());
    }

    private User getUser(final int userId) {
        return repo.getUserById(userId);
    }
}

Спойлеры к решению

Подсказки

💡 Сервис не должен самостоятельно создавать UserRepository или доставать зависимости из ApplicationContext.
💡 Параметр users объявлен final, поэтому присвоить ему новый список нельзя.
💡 В Stream API метод называется forEach(), а не foreach().
💡 @Transactional на createUsers() не сработает при обычном внутреннем вызове из processNewUsers() через this.
💡 Нужно определить настоящую транзакционную границу: только создание пользователей или весь сценарий целиком.
💡 Если обновление региона — удалённый вызов, одна JDBC-транзакция всё равно не сделает весь процесс атомарным.

Решение

В исходном коде есть как минимум две ошибки компиляции.

Параметр:

final List<User> users

нельзя переназначать:

users = createUsers(users);

Нужно сохранить результат в отдельную переменную:

List<User> savedUsers = createUsers(users);

Также в Stream API нет метода:

foreach(...)

Правильно:

forEach(...)

Главная архитектурная проблема — работа с зависимостями.

Так делать не следует:

private UserRepository repo = new UserRepository();

Spring-сервис не должен самостоятельно создавать repository. Кроме того, если UserRepository является интерфейсом Spring Data, такой код вообще не скомпилируется.

Также нежелательно использовать Service Locator:

appCtx.getBean("regionService", RegionService.class);

Зависимости лучше явно передавать через конструктор:

@Service
public class UserService {

    private final UserRepository userRepository;
    private final RegionService regionService;

    public UserService(
            UserRepository userRepository,
            RegionService regionService
    ) {
        this.userRepository = userRepository;
        this.regionService = regionService;
    }

    // ...
}

Теперь зависимости:

явно видны;
обязательны;
могут быть final;
легко подменяются в тестах;
создаются и управляются Spring.

Есть важная проблема с транзакцией.

Сейчас:

public void processNewUsers(...) {
    createUsers(users);
}

@Transactional
public List<User> createUsers(...) {
    ...
}

createUsers() вызывается из того же объекта.

При стандартном proxy-based AOP вызов выглядит так:

Spring proxy
    ↓
processNewUsers()
    ↓
this.createUsers()

Внутренний вызов не проходит через proxy ещё раз, поэтому @Transactional на createUsers() не создаёт ожидаемую транзакцию.

Если создание пользователей и обновление региональных связей являются одной локальной бизнес-операцией в одной БД, транзакционной точкой входа логичнее сделать processNewUsers():

@Service
public class UserService {

    private final UserRepository userRepository;
    private final RegionService regionService;

    public UserService(
            UserRepository userRepository,
            RegionService regionService
    ) {
        this.userRepository = userRepository;
        this.regionService = regionService;
    }

    @Transactional
    public void processNewUsers(
            List<User> users,
            String regionName
    ) {
        Objects.requireNonNull(users, "users");
        Objects.requireNonNull(regionName, "regionName");

        List<User> savedUsers = createUsers(users);

        savedUsers.forEach(user ->
                regionService.updateRegionLink(
                        user.getId(),
                        regionName
                )
        );
    }

    private List<User> createUsers(
            List<User> users
    ) {
        return users.stream()
                .map(userRepository::saveUser)
                .collect(Collectors.toList());
    }

    private User getUser(int userId) {
        return userRepository.getUserById(userId);
    }
}

Теперь вызов:

processNewUsers(...)

приходит извне через Spring proxy, поэтому транзакция действительно начинается перед выполнением метода.

createUsers() становится обычным приватным вспомогательным методом и отдельная @Transactional ему не нужна.

Однако транзакционную границу нужно выбирать по реальной природе RegionService.

Если:

regionService.updateRegionLink(...)

изменяет данные в той же БД и участвует в том же PlatformTransactionManager, общая транзакция может быть оправдана:

создать User
+
обновить Region
+
COMMIT

При исключении изменения можно откатить вместе.

Если же RegionService выполняет HTTP-вызов, отправляет сообщение или работает с независимым хранилищем, JDBC-транзакция не делает весь процесс атомарным.

Например:

User сохранён
↓
внешний RegionService успешно обновлён
↓
следующая операция падает
↓
локальная БД делает ROLLBACK

Внешнее изменение уже невозможно откатить обычной @Transactional.

В таком сценарии нужно отдельно проектировать согласованность: например, через событие/outbox, retry и идемпотентность.

Ещё один момент — вызов:

savedUsers.forEach(user ->
        regionService.updateRegionLink(
                user.getId(),
                regionName
        )
);

делает одну операцию на каждого пользователя.

Если users может быть большим списком и RegionService поддерживает batch-операцию, эффективнее использовать что-то вроде:

regionService.updateRegionLinks(
        savedUsers,
        regionName
);

Это особенно важно, если каждый вызов приводит к отдельному SQL или сетевому запросу.

Метод:

private User getUser(int userId)

в представленном фрагменте нигде не используется.

Если он действительно не нужен, его следует удалить.

Если это операция публичного API сервиса, тогда private некорректен и нужно явно определить контракт, включая поведение при отсутствии пользователя.

Например, если repository возвращает Optional:

public User getUser(int userId) {
    return userRepository.findById(userId)
            .orElseThrow(() ->
                    new EntityNotFoundException(
                            "User not found: " + userId
                    )
            );
}

Но изменять видимость только ради самого рефакторинга не нужно — это зависит от назначения метода.

Итоговый минимальный вариант, если весь сценарий должен выполняться в одной локальной транзакции:

@Service
public class UserService {

    private final UserRepository userRepository;
    private final RegionService regionService;

    public UserService(
            UserRepository userRepository,
            RegionService regionService
    ) {
        this.userRepository = userRepository;
        this.regionService = regionService;
    }

    @Transactional
    public void processNewUsers(
            List<User> users,
            String regionName
    ) {
        Objects.requireNonNull(users, "users");
        Objects.requireNonNull(regionName, "regionName");

        List<User> savedUsers = users.stream()
                .map(userRepository::saveUser)
                .collect(Collectors.toList());

        savedUsers.forEach(user ->
                regionService.updateRegionLink(
                        user.getId(),
                        regionName
                )
        );
    }
}

Основные проблемы исходного кода:

repository создаётся вручную вместо DI;
используется ApplicationContext как Service Locator;
зависимости не final;
отсутствует нормальный constructor injection;
final-параметр users пытаются переназначить;
написано foreach вместо forEach;
@Transactional на self-invoked createUsers() не работает ожидаемым образом;
транзакционная граница выбрана неочевидно;
возможны многочисленные отдельные вызовы RegionService;
getUser() в представленном коде не используется;
имя repo менее информативно, чем userRepository.

Ключевое исправление — не просто перенести @Transactional, а сначала определить, какие операции действительно должны составлять одну атомарную транзакцию.