[리팩토링] 같은 차수 계산인데 결과가 달랐던 이유

같은 보고서인데 목록 화면에서는 17차, 공식 출력물에서는 23차로 표시되는 문제가 있었다.

처음에는 두 군데에 흩어진 계산 로직이 서로 달라진 문제라고 생각했다. 하나로 통일하면 될 것 같았는데, 과거 CS 내용을 확인해보니 두 계산 방식 모두 이유가 있었다.

결론부터 말하면 화면과 출력물이 묻는 질문이 달랐다.

화면: 이 보고서는 몇 번째 추적 보고서인가?
출력물: 이 보고서는 해당 사례의 몇 번째 전체 보고서인가?

문제는 규칙이 두 개인 게 아니라, 왜 두 개인지 코드에 전혀 드러나지 않았다는 점이었다.

차수 계산 로직이 두 군데 있었다

먼저 목록 화면과 엑셀 다운로드에서 사용하는 코드는 삭제된 보고서를 제외하고 FOLLOW_UP만 계산하고 있었다.

int sequence = 0;
for (ReportSummary summary : chain) {
    if (summary.isDeleted()) continue;
    if (summary.getClassification() == FOLLOW_UP) {
        numberByReportId.put(summary.getId(), ++sequence);
    }
}

공식 출력물에서는 체인 전체에서 현재 보고서의 위치를 찾았다.

private int getSequenceNumber(List<ChainItem> chain, Long currentId) {
    int number = chain.size();
    for (int i = 0; i < chain.size(); i++) {
        if (chain.get(i).getId().equals(currentId)) {
            number = i + 1;
            break;
        }
    }
    return number;
}

같은 사례번호로 데이터를 가져오고 정렬하는 부분은 동일했다. 차이는 번호를 세는 기준이었다.

	화면	공식 출력물
Initial 보고서	제외	포함
삭제된 보고서	제외	포함
현재 보고서를 못 찾은 경우	null	전체 건수

화면과 출력물의 차이가 6이었던 이유도 확인됐다. 앞에 삭제된 보고서 5건과 Initial 보고서 1건이 있었고, 출력물 로직은 이 데이터를 모두 세고 있었다.

처음에는 하나로 통일하려고 했다

두 로직을 처음 봤을 때는 복사된 코드가 시간이 지나면서 서로 달라진 것으로 판단했다.

화면에서 사용하는 로직은 여러 곳에서 공통으로 사용하고 있었고, 출력물만 private 메서드로 따로 계산하고 있었다. 그래서 출력물도 화면의 계산 방식을 사용하도록 바꾸려고 했다.

다만 공식 출력물은 고객에게 전달되는 값이라 바로 수정하지 않고 과거 CS 티켓을 먼저 확인했다. 여기서 예상과 다른 내용을 찾았다.

추적 1차 보고서는 #2, 추적 2차 보고서는 #3으로 출력되어야 하지 않을지 문의드립니다.

고객은 Initial 보고서를 #1로 보고 있었다. 첫 번째 추적 보고서가 #2로 출력되는 게 맞다는 뜻이다.

즉 두 로직 중 하나가 무조건 틀린 게 아니었다.

화면에서는 Initial을 제외하고 추적 보고서끼리의 순서를 보여준다.
공식 출력물에서는 Initial을 포함한 전체 보고서의 순서를 보여준다.

코드만 봤을 때는 이 차이를 알 수 없었다. 메서드 이름도 비슷했고, 주석이나 테스트에도 계산 기준이 적혀 있지 않았다. 요구사항은 CS 티켓에만 남아 있었다.

실제 버그는 따로 있었다

계산 기준을 확인하고 보니 출력물 로직에는 별개의 문제가 두 가지 있었다.

현재 보고서를 못 찾으면 전체 건수를 반환

기존 코드는 기본값을 chain.size()로 두고 있었다.

int number = chain.size();

현재 보고서 id를 체인에서 찾지 못하면 오류를 내거나 빈 값을 반환하지 않고 전체 건수를 반환한다. 과거에 모든 보고서의 차수가 최대값으로 출력된 CS가 있었는데 원인이 이 부분이었다.

값을 계산하지 못했는데도 정상적인 숫자처럼 반환하니 호출부에서는 문제를 알 수 없었다.

삭제된 보고서까지 계산

삭제된 보고서는 화면과 후속 보고서 생성 로직에서 모두 제외하고 있었다. 그런데 출력물에서만 삭제된 보고서를 포함했다.

출력물에서 사용하는 DTO에도 deleted 값이 있었지만 실제 계산에서는 사용하지 않았다. 이 때문에 중간에 삭제된 보고서가 있는 사례는 출력 차수가 계속 커졌다.

정리하면 Initial 보고서를 포함하는 것은 요구사항이 맞았고, 찾지 못한 경우 전체 건수를 반환하는 것과 삭제 데이터를 세는 것은 버그였다.

계산 규칙을 값 객체로 분리

두 계산을 각 서비스에 그대로 둘 수도 있었지만, 그러면 나중에 다시 한쪽만 수정될 가능성이 있었다. 차수 계산에 필요한 데이터와 규칙을 FollowUpChains로 모았다.

/**
 * 같은 사례번호로 묶인 보고서의 차수를 계산한다.
 *
 * followUpNumberOf: Initial을 제외한 추적 보고 차수
 * reportNumberOf: Initial을 포함한 전체 보고 차수
 *
 * 삭제된 보고서는 두 계산에서 모두 제외한다.
 * 체인에 없는 id는 null을 반환한다.
 */
public class FollowUpChains {

    private static final Comparator<ReportSummary> CHAIN_ORDER = ...;

    private final Map<Long, Integer> followUpNumberById = new HashMap<>();
    private final Map<Long, Integer> reportNumberById = new HashMap<>();

    private FollowUpChains() {}

    public static FollowUpChains of(List<ReportSummary> summaries) {
        FollowUpChains chains = new FollowUpChains();
        // 사례번호별 그룹핑, 정렬, 삭제 제외 후 두 차수를 계산
        ...
        return chains;
    }

    public Integer followUpNumberOf(Long reportId) { ... }
    public Integer reportNumberOf(Long reportId) { ... }
}

메서드 이름에서 두 기준의 차이가 보이도록 했다.

followUpNumberOf: Initial을 제외한 추적 보고서 차수
reportNumberOf: Initial을 포함한 전체 보고서 차수

서비스에서는 직접 계산하지 않고 필요한 값을 물어보기만 한다.

FollowUpChains chains =
        FollowUpChains.of(repository.findChainsBy(companyId, caseNumbers));

Integer number = chains.followUpNumberOf(reportId);

공식 출력물은 reportNumberOf를 사용한다. 계산 기준이 바뀌더라도 수정할 곳은 이 객체 하나로 줄었다.

생성할 때 계산을 끝내도록 했다

FollowUpChains는 생성자를 private으로 막고 of()로만 만들 수 있게 했다. 빈 객체를 먼저 만들고 나중에 데이터를 채울 이유가 없기 때문이다.

of()에서 그룹핑, 정렬, 삭제 데이터 제외, 차수 계산까지 모두 끝낸다. 생성 후에는 setter 없이 조회만 가능하다.

처음에는 리포지토리에서 이미 정렬된 데이터를 넘겨주니 객체 안에서는 정렬하지 않아도 된다고 생각했다. 그런데 이렇게 두면 다른 곳에서 이 객체를 사용할 때 정렬 여부를 매번 확인해야 한다.

차수는 순서에 따라 결정되므로 정렬 기준도 차수 계산 규칙의 일부라고 보는 게 맞았다. 그래서 정렬도 of() 안에서 처리하도록 바꿨다.

현재 보고서를 찾지 못한 경우에는 null을 반환한다. 기존처럼 전체 건수로 대신하지 않는다. 출력물에서는 이 경우 잘못된 숫자보다 공란이 나오도록 했다.

테스트

이 객체는 Spring에 의존하지 않아서 단위 테스트로 계산 규칙을 바로 확인할 수 있었다.

@Test
@DisplayName("공식 출력물 차수는 Initial이 1을 차지해 첫 추적 보고가 2다")
void reportNumberCountsInitialAsOne() { ... }

@Test
@DisplayName("체인에 없는 보고서는 null을 반환한다")
void returnsNullForUnknownId() { ... }

추가로 아래 경우를 각각 테스트했다.

화면용 차수는 Initial을 제외하는지
출력물 차수는 Initial을 포함하는지
두 계산 모두 삭제된 보고서를 제외하는지
입력 순서가 달라도 같은 결과가 나오는지
체인에 없는 id는 null을 반환하는지

정리

이번에는 비슷한 로직 두 개를 보고 바로 중복이라고 판단했다. 하지만 확인해보니 계산 기준의 차이 자체는 요구사항이었다.

코드에서 이상한 차이를 발견했을 때 바로 하나로 합치기 전에, 각 코드가 어떤 값을 만들고 어디에서 사용되는지 먼저 확인해야 한다. 특히 고객에게 전달되는 값이라면 코드 밖에 남아 있는 요구사항도 같이 봐야 한다.

그리고 규칙이 여러 개라면 각각 이름을 붙여 코드에 드러내는 게 중요했다. 이번 리팩토링에서는 코드 양을 줄인 것보다, 화면용 차수와 출력물용 차수가 왜 다른지 한 곳에서 확인할 수 있게 된 점이 더 컸다.

···