Как сделать рефакторинг данный код?

Question

[student_1]

Как можно сократить и сделать правильнее данный код c помощью stream api?

public void updateUserByInn(List<Users> usersList) {

        for (Users dto : usersList) {
            try {

                if (dto.getUserType().equals(UserTypes.PARTNER)) {
                    Users user = usersRepository.findByInnAndExternalPartnerId(dto.getInn(), dto.getExternalPartnerId());
                    if (user != null) {
                        user.setName(dto.getName());
                        user.setInn(dto.getInn());
                        user.setPhone(dto.getPhone());
                        user.setBrand(dto.getBrand());
                        user.setUserType(dto.getUserType());
                        usersRepository.save(user);
                    }
                    usersRepository.save(dto);

                } else if (dto.getUserType().equals(UserTypes.EMPLOYEE)) {
                    Users user = usersRepository.findByInn(dto.getInn());
                    if (user != null) {
                        user.setName(dto.getName());
                        user.setInn(dto.getInn());
                        user.setPhone(dto.getPhone());
                        user.setBrand(dto.getBrand());
                        user.setUserType(dto.getUserType());
                        usersRepository.save(user);
                    }
                    usersRepository.save(dto);
                }

            } catch (NullPointerException e) {
                log.error("Get null value: {}", e.getMessage());
            }

        }
    }

Answer 1

[Орхан Гасанлы]

Добрый день.
Поделюсь своими мыслями. А вы уже сами решайте стоит ли вносить правки в ваш код.
1) Судя по этому сниппету:

public void updateUserByInn(List<Users> usersList) {
        for (Users dto : usersList) {

Метод принимает Dto.
Ну тут во-первых, класс должен быть в ед. числе Users -> User
Во-вторых, если это Dto, то это должно быть как-то отображено в названии. Например, UserDto, UserUpdateRequest и т.д.
А раз это DTO, то маппинг можно вынести в отдельный метод или даже класс. Можно воспользоваться интерфейсом Converter <UserDto, User>, можно подключить либы (ModelMapper, MapStruct).
Если решите использовать ModelMapper, то обратиться внимание на этот вопрос, дабы исключить null параметры:
https://stackoverflow.com/questions/45451025/how-t...

2) Вместо того, чтобы выбрасывать NPЕ можно использовать Optional класс и выбросить кастомное исключение:

Users user = usersRepository.findByInnAndExternalPartnerId(dto.getInn(), dto.getExternalPartnerId()).orElseThrow(UserNotFoundException::new);

Для этого из репозитория ваши методы должны возвращать Optional<User>

3) Сам метод должен быть назван корректно.
updateUserByInn обновить юзера по ИНН. А в методе вы обновляете пользователей, а не одного юзера еще и притом обновляете не только по Inn, а по ExternalPartnerId тоже. Значит, метод назван некорректно

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

Answer 2

[Antony]

Первое и самое главное - у вас дублируется относительно большой кусок кода ибо вне зависимости от того, кто ваш юзер, после получения объекта из репозитория, вы делаете с ним одно и тоже. Нахрена тогда это писать два раза? (это можно и без stream api переписать)

spoiler

Вот зачем это писать два раза?

if (user != null) {
                        user.setName(dto.getName());
                        user.setInn(dto.getInn());
                        user.setPhone(dto.getPhone());
                        user.setBrand(dto.getBrand());
                        user.setUserType(dto.getUserType());
                        usersRepository.save(user);
                    }
                    usersRepository.save(dto);

По первому пункту дополнительный совет - всегда пользуйтесь статическими анализаторами, банально той же графаной - дублирование кода она находит вообще отлично, другие проблемы тоже довольно не плохо (но наличие собственного мозга она вам не заменит)

Второе - да чуть-чуть можно сократить, украсить и (опционально) распаралелить исполнение кода, переписать его с использованием stream api. Но тут, возможно, кто-нибудь аргументированно опротестует данный совет. В любом случае прежде чем тупо использовать сей api, почитайте что это, как работает и какие он имеет недостатки и достоинства.

Писать код за вас никто не будет, спрашивайте что конкретно не понятно по части srtream api, как пробовали и что не получается.