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

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

Условие задачи:
📌 Дан сервис UserService. Нужно провести рефакторинг кода: исправить проблемы с зависимостями, транзакциями, стилем и потенциальными ошибками в обработке пользователей.

Код:

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);
    }
}

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

Подсказки

💡 new UserRepository() и appCtx.getBean(...) в сервисе — плохая практика, лучше обычный DI.
💡 users = createUsers(users) не скомпилируется, если параметр объявлен как final.
💡 foreach нужно писать как forEach.
💡 @Transactional на методе, который вызывается внутри того же класса, может не сработать через Spring proxy.
💡 Лучше сделать зависимости final и передавать их через конструктор.
💡 Стоит подумать, какой метод должен быть транзакционной точкой входа: createUsers() или processNewUsers().

Решение

@Service
@RequiredArgsConstructor
class UserService {

    private final UserRepository repo;
    private final RegionService regionService;

    @Transactional
    public void processNewUsers(final List<User> users, final String regionName) {
        List<User> savedUsers = createUsers(users);

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

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

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

В исходном коде основные проблемы: ручное создание зависимостей вместо DI, использование ApplicationContext в сервисе, ошибка с final users, опечатка foreach и риск, что @Transactional не сработает при внутреннем вызове.
Лучше сделать processNewUsers() транзакционной точкой входа, а зависимости внедрять через конструктор.