Post

[리팩토링] 레거시 코드와 SRP

Posted Updated
By 9osari
11 min read
[리팩토링] 레거시 코드와 SRP

점점 비대해지는 savePlan

공급사 점검계획 등록/수정 기능을 만들다가 요구사항이 추가되면서 하나의 메서드에 모든 로직이 쌓이기 시작했다

1
2
3
4
5
6
7
8
9
10
11

// 최초저장
if (vo.getPlanId() == null || vo.getPlanId().isEmpty()) {
    planRepository.save(vo);
    processAddedSchedules(vo, true);
} else {
    // 수정
    planRepository.update(vo);
    processAddedSchedules(vo, false);
    processEditedSchedules(vo);
    processRemovedSchedules(vo);
}
  • 키값이 없으면 최초저장, 있으면 수정 정도만 있었는데 공통승인 로직까지 생각하게 되니 점점 복잡해졌다.

그 결과

메서드 하나에 모든 로직이 들어갔다.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122

@Service
public class AnnualPlanServiceImpl implements AnnualPlanService {

    private final PlanRepository planRepository;
    private final ApprovalService approvalService;

    @Autowired
    private ScheduleRepository scheduleRepository;

    public AnnualPlanServiceImpl(
            PlanRepository planRepository,
            ApprovalService approvalService) {
        this.planRepository = planRepository;
        this.approvalService = approvalService;
    }

    // 저장
    @Override
    public PlanVO savePlan(PlanVO vo) {
        // 승인 처리 (승인 요청 시에만)
        if ("approval".equals(vo.getSaveType())) {
            processApproval(vo);
        } else {
            // 일반 저장 시 진행상황 코드 설정
            if (vo.getProgressStatus() == null || vo.getProgressStatus().isEmpty()) {
                vo.setProgressStatus(PlanStatus.DRAFT.getCode());
            }
        }

        // 최초저장
        if (vo.getPlanId() == null || vo.getPlanId().isEmpty()) {
            planRepository.save(vo);
            processAddedSchedules(vo, true);
        } else {
            // 수정
            planRepository.update(vo);
            processAddedSchedules(vo, false);
            processEditedSchedules(vo);
            processRemovedSchedules(vo);
        }

        // 승인요청정보가 있을 때만 승인정보 생성 및 업데이트
        if (vo.getApprovalInfoList() != null && !vo.getApprovalInfoList().isEmpty()) {
            // 신규 승인정보 저장 → approvalId 생성
            approvalService.save(vo);
            // approvalId를 세팅
            vo.setApprovalId(vo.getApprovalId());
            // 승인 ID를 메인 테이블에 업데이트
            planRepository.updateApprovalId(vo);
        }

        return vo;
    }

    private void processApproval(PlanVO vo) {
        if (vo.getApprovalInfoList() != null && !vo.getApprovalInfoList().isEmpty()) {
            // 신규 승인정보 저장 → approvalId 생성
            approvalService.save(vo);

            // approvalId를 세팅
            vo.setApprovalId(vo.getApprovalId());

            // 진행상황을 "계획 승인 요청"으로 변경
            vo.setProgressStatus(PlanStatus.PENDING_APPROVAL.getCode());

            // 승인 상신 (진행상황 코드 업데이트)
            approvalService.request(vo);

            // 승인 ID를 메인 테이블에 업데이트
            planRepository.updateApprovalId(vo);
        }
    }

    private void processAddedSchedules(PlanVO vo, boolean isNew) {
        if (vo.getAddedScheduleList() != null && !vo.getAddedScheduleList().isEmpty()) {
            for (ScheduleVO schedule : vo.getAddedScheduleList()) {
                schedule.setPlanId(vo.getPlanId());
                if (isNew) {
                    scheduleRepository.save(schedule);
                } else {
                    scheduleRepository.insert(schedule);
                }
            }
        }
    }

    private void processEditedSchedules(PlanVO vo) {
        if (vo.getEditedScheduleList() != null && !vo.getEditedScheduleList().isEmpty()) {
            for (ScheduleVO schedule : vo.getEditedScheduleList()) {
                schedule.setPlanId(vo.getPlanId());
                scheduleRepository.update(schedule);
            }
        }
    }

    private void processRemovedSchedules(PlanVO vo) {
        if (vo.getRemovedScheduleList() != null && !vo.getRemovedScheduleList().isEmpty()) {
            for (ScheduleVO schedule : vo.getRemovedScheduleList()) {
                schedule.setPlanId(vo.getPlanId());
                schedule.setDeleted(true);
                scheduleRepository.update(schedule);
            }
        }
    }

    // 계획 삭제
    @Override
    public PlanVO deletePlan(PlanVO vo) {
        scheduleRepository.deletePlan(vo);
        return vo;
    }

    // 계획 승인요청
    @Override
    public PlanVO requestApproval(PlanVO vo) {
        // 진행상황 코드 업데이트
        vo.setProgressStatus(PlanStatus.PENDING_APPROVAL.getCode());
        planRepository.update(vo);
        return vo;
    }

}

이건 아닌거 같아서 도움을 요청했다.

OOP.. 냄새 정도는 맡기는 했으나, 실무레벨에서는 어떻게 적용할지 감이 안왔다.

회사선배는 전형적인 절차지향 코드라 했다.

문제점

SRP 위반

  • 하나의 메서드(saveFyerSplyBzentyChckPlanM)가 너무 많은 책임을 가지고 있다 승인 분기 처리, 상태코드 초기화, 저장/수정 분기, DB저장 등… 많다! 각자 한 가지 책임만 가지게 한다.
  • 읽기가 너무 힘들다

추상화 레벨 불일치

  • 고수준, 저수준 로직이 뒤석여 있어 코드가 눈에 잘 안들어온다. 같은 추상화 레벨로 통일한다 위에서 아래로 점점 구체화 한다.

문자열 비교로 분기

  • “aprv”, “saveGbn”, isNew 같은 플래그로 동작이 결정된다.

절차지향적 사고

  • 할 일을 순서대로 나열 했을 뿐, 책임분리를 지키지 못했다.

그 외 의미를 알 수 없는 이름들..

리팩토링 후

1
2
3
4
5
6
7
8

// 저장
@Override
public PlanVO savePlan(PlanVO vo) {
    initializeStatusIfNeeded(vo);        // 진행상황 코드가 없으면 초기화
    saveOrUpdatePlan(vo.getPlanId(), vo); // 저장 또는 수정
    registerApprovalInfo(vo);            // 승인정보 등록
    return vo;
}
  • 각자 한 가지 책임만 가지게 한다.
  • 같은 추상화 레벨로 통일 한다.
  • 위에서 아래로 점점 구체화 한다.

전체 코드

이제 좀 봐줄만하다 (아직 개선할 점은 많지만..)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118

@Service
public class AnnualPlanServiceImpl implements AnnualPlanService {

    private final PlanRepository planRepository;
    private final ApprovalService approvalService;
    private final ScheduleRepository scheduleRepository;

    public AnnualPlanServiceImpl(
            PlanRepository planRepository, 
            ApprovalService approvalService,
            ScheduleRepository scheduleRepository) {
        this.planRepository = planRepository;
        this.approvalService = approvalService;
        this.scheduleRepository = scheduleRepository;
    }

    // 저장
    @Override
    public PlanVO savePlan(PlanVO vo) {
        initializeStatusIfNeeded(vo);
        saveOrUpdatePlan(vo.getPlanId(), vo);
        registerApprovalInfo(vo);
        return vo;
    }

    private void initializeStatusIfNeeded(PlanVO vo) {
        if (StringUtils.isEmpty(vo.getProgressStatus())) {
            vo.setProgressStatus(PlanStatus.DRAFT.getCode());
        }
    }

    private void saveOrUpdatePlan(String planId, PlanVO vo) {
        if (planId == null || planId.isEmpty()) {
            createNewPlan(vo);
            return;
        }
        updateExistingPlan(vo);
    }

    private void createNewPlan(PlanVO vo) {
        planRepository.save(vo);
        processAddedSchedules(vo, true);
    }

    private void updateExistingPlan(PlanVO vo) {
        planRepository.update(vo);
        processAddedSchedules(vo, false);
        processEditedSchedules(vo);
        processRemovedSchedules(vo);
    }

    private void registerApprovalInfo(PlanVO vo) {
        if (vo.getApprovalInfoList() == null || vo.getApprovalInfoList().isEmpty()) {
            return;
        }
        
        approvalService.save(vo);
        vo.setApprovalId(vo.getApprovalId());
        planRepository.updateApprovalId(vo);
    }

    private void processAddedSchedules(PlanVO vo, boolean isNew) {
        if (vo.getAddedScheduleList() == null || vo.getAddedScheduleList().isEmpty()) {
            return;
        }

        for (ScheduleVO schedule : vo.getAddedScheduleList()) {
            schedule.setPlanId(vo.getPlanId());
            if (isNew) {
                scheduleRepository.save(schedule);
            }
        }
    }

    private void processEditedSchedules(PlanVO vo) {
        if (vo.getEditedScheduleList() == null || vo.getEditedScheduleList().isEmpty()) {
            return;
        }

        for (ScheduleVO schedule : vo.getEditedScheduleList()) {
            schedule.setPlanId(vo.getPlanId());
            scheduleRepository.update(schedule);
        }
    }

    private void processRemovedSchedules(PlanVO vo) {
        if (vo.getRemovedScheduleList() == null || vo.getRemovedScheduleList().isEmpty()) {
            return;
        }

        for (ScheduleVO schedule : vo.getRemovedScheduleList()) {
            schedule.setPlanId(vo.getPlanId());
            schedule.setDeleted(true);
            scheduleRepository.update(schedule);
        }
    }

    // 승인요청
    @Override
    public PlanVO requestApproval(PlanVO vo) {
        escalatePlanForApproval(vo);
        return vo;
    }

    private void escalatePlanForApproval(PlanVO vo) {
        saveOrUpdatePlan(vo.getPlanId(), vo);
        vo.setProgressStatus(PlanStatus.PENDING_APPROVAL.getCode());
        planRepository.updateProgressStatus(vo);
        
        approvalService.request(vo);
    }

    @Override
    public PlanVO deletePlan(PlanVO vo) {
        scheduleRepository.deletePlan(vo);
        return vo;
    }
}

좀 더 개선할 점

  • 단일 vo 사용
    • 현재 단일 VO를 여러 메서드에서 돌려 쓰고있다. (회사 컨벤션)
    • 의존성 불명확 → 메서드가 VO의 어떤 필드를 사용하는지 모른다.
    • 예상 못한 참조 → 다른 메서드가 수정한 값을 모르고 참조할 수 있다 → 디버깅 지옥
    • 테스트 어려움
    • 승인 로직이 복잡해 단일 VO를 사용했지만 DTO로 분리해 사용하는 습관을 들이자.
  • 메서드 명 개선
    • 메서드명만 개선해도 주석을 달 필요가없다!

어떤점이 더 좋아졌나

1. 가독성 향상

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

// Before: 뭘 하는 코드인지 파악하려면 전체를 다 읽어야 함
if ("approval".equals(vo.getSaveType())) {
    processApproval(vo);
} else {
    if (vo.getProgressStatus() == null || vo.getProgressStatus().isEmpty()) {
        vo.setProgressStatus(PlanStatus.DRAFT.getCode());
    }
}
// ... 40줄 후 ...
if (vo.getApprovalInfoList() != null && !vo.getApprovalInfoList().isEmpty()) {

// After: 메서드명만 봐도 무슨 일이 일어나는지 알 수 있음
initializeStatusIfNeeded(vo);
saveOrUpdatePlan(vo.getPlanId(), vo);
registerApprovalInfo(vo);

2. 유지보수성 개선

  • “승인 로직만 수정해야 해요” → registerApprovalInfo() 메서드만 보면 됨
  • 이전엔 전체 코드를 읽으며 승인 관련 부분을 찾아야 했음

3. 테스트 용이성

1
2
3
4
5
6
7
8
9

// 개별 메서드 단위 테스트 가능
@Test
void 진행상황이_없으면_DRAFT로_초기화() {
    PlanVO vo = new PlanVO();
    service.initializeStatusIfNeeded(vo);
    assertEquals(PlanStatus.DRAFT.getCode(), vo.getProgressStatus());
}

// 이전엔 savePlan() 전체를 테스트해야 했음 (DB, 승인, 일정 등 다 Mock 필요)

4. 책임 분리로 변경 영향도 감소

  • 일정 처리 로직 변경 시 processAddedSchedules() 내부만 수정
  • 승인 로직과 저장 로직이 서로 영향 없음
  • 이전엔 한 줄 수정해도 전체 로직 영향도 파악 필요했음

5. 추상화 레벨 통일로 흐름 파악 쉬워짐

1
2
3
4
5
6
7

// 고수준에서 전체 흐름 파악
savePlan() 
  → 상태 초기화
  → 저장/수정
  → 승인정보 등록

// 필요하면 각 메서드 내부로 들어가서 상세 로직 확인

배우고 느낀점

  • 설계가 매우매우매우x100 중요하다.
  • 코드가 길어진다고 어려워지는건 아니다. 짧은 코드를 만드려 애쓰지 말자.
  • 내가 만드는 로직 만큼은 SRP 만큼은 준수하자.
  • 적당한 중복은 괜찮다.
  • 극한까지 구체화하면 메서드 하나에 동작 하나 → ? 뭐든 적당히..


코드는 회사 보안 정책을 준수하기 위해 실제 도메인 용어와 비즈니스 로직을 일반적인 예시로 수정하였습니다.

java
oop spring refactor
This post is licensed under CC BY 4.0 by the author.