[BE] refactor: 하이라이트 매핑 로직 간소화

[donghoony]

• related 하이라이트 생성 로직 이게 최선인가 #861

🚀 어떤 기능을 구현했나요 ?

• 하이라이트의 매핑 로직을 현명(?)하게 풀어보려고 노력했습니다.
• 2중 for문이나 메서드 따라가느라 힘들었던 시간은 안녕(아마도)

🔥 어떻게 해결했나요 ?

• 매핑을 돕는 객체를 하나 만들었습니다. 이는 계층적인 DTO 구조를 약간 반정규화해 보다 연산하기 쉽도록 하기 위함입니다.

public record HighlightFragment(long answerId, int lineIndex, int startIndex, int endIndex) {
}

HighlightRequest에서 Fragment들을 생성하도록 합니다. HighlightRequest는 answerId, LineRequest (lineIndex, range)를 가지므로 한 번에 생성할 수 있습니다.
Service에서는 만들어 둔 HighlightFragment를 토대로 도메인 객체와 소통 / 매핑을 진행합니다. 4-50줄 가까이 되던 게 10줄 정도로 줄었습니다. private 메서드도 없습니다. 테스트를 곳곳에 넣을 수 있었네요.

public List<Highlight> mapToHighlights(HighlightsRequest highlightsRequest) {
    // ID -> HighlightedLines 매핑
    Map<Long, HighlightedLines> answerIdHighlightedLines = textAnswerRepository
            .findAllById(highlightsRequest.getUniqueAnswerIds())
            .stream()
            .collect(Collectors.toMap(TextAnswer::getId, answer -> new HighlightedLines(answer.getContent())));

    // 반정규화된 것을 바탕으로 Highlight 도메인 관련 객체에 값 추가
    for (HighlightFragment fragment : highlightsRequest.toFragments()) {
        HighlightedLines highlightedLines = answerIdHighlightedLines.get(fragment.answerId());
        highlightedLines.addRange(fragment.lineIndex(), fragment.startIndex(), fragment.endIndex());
    }

    // HighlightedLines를 List<Highlight>로 변환
    return answerIdHighlightedLines.entrySet()
            .stream()
            .flatMap(entry -> entry.getValue().toHighlights(entry.getKey()).stream())
            .toList();
}

📝 어떤 부분에 집중해서 리뷰해야 할까요?

• 이해되지 않는 부분에 대해서 물어봐주세요. flatMap이 자주 사용되는 건 계층 구조가 깊어서 어쩔 수 없었습니다.

🐴 할 말

• 계층이 생기는 이유는 보통 중복을 제거하기 위함이었습니다. 이는 테이블을 설계할 때에도, API 구조를 설계할 때에도 마찬가지였어요.
• 계층이 깊어질수록 매핑 로직이 어려워지는 걸 겪었습니다. Map<?, ?>와 같이 Id, Object를 매핑하는 게 필요했고 이는 생각보다 깔끔해보이지 않았어요.
• 완충 역할을 해 줄 반정규 객체가 등장하고 나니 생각보다 쉽게 문제가 해결되었어요. API 설계 / 테이블 설계할 때에도 무조건 정규화하는 게 좋은 것만은 아니라는 걸 배웠네요.

[github-actions]

Test Results

155 tests   152 ✅  5s ⏱️
 59 suites    3 💤
 59 files      0 ❌

Results for commit b151f1e.

♻️ This comment has been updated with latest results.

[nayonsoso]

하이라이트를 추가하는 과정을 정리해봤어요.

1. textRepository 에 있는 각 답변의 content 를 받아온 다음, HighlightLines 라는 중간 객체로 생성
2. 요청에 있는 range 를 중간객체에 반영
3. 중간 객체인 HighlightLines 를 엔티티로 변경

이 PR 에서는 2번 과정의 깊이를 줄이기 위해서 HighlightFragment라는 중간객체를 사용했고,
3번을 HighlightLines 내부로 옮겼군요!
mapper 가 하고 있던 역할을 다른 객체들에게 나누어줘서 더 읽기 편한 코드가 된 것 같아요😊

그리고 HighlightFragment 가 record 클래스라는 것도, mapper 의 과정중에서만 사용되는 것임을 드러내는듯 하여 적절하다 생각합니다.
바~로 approve 합니다🥳

PS.
이 PR을 리뷰하기 위해서 '기존은 어땠나 / 어떻게 바뀌었나 / 이렇게 바뀜으로써 뭐가 좋아졌나'를 봤는데,
첫번째 단계인 '기존은 어땠나'를 다시 이해하는데에 가장 오래 걸렸습니다 ㅋㅋ
그만큼 리팩터링이 반드시 필요한 부분이었는데, 잘 처리해주셨습니다👏👏

[donghoony]

return answerIdHighlightedLines.entrySet()
        .stream()
        .flatMap(entry -> entry.getValue().toHighlights(entry.getKey()).stream())
        .toList();

List<Highlight> highlights = new ArrayList<>();
for (Map.Entry<Long, HighlightedLines> entry : answerIdHighlightedLines.entrySet()) {
    long answerId = entry.getKey();
    HighlightedLines highlightedLines = entry.getValue();
    highlights.addAll(highlightedLines.toHighlights(answerId));
}
return highlights;

마지막 리턴 구문 stream으로 충분히 이해할 수 있나요? 위와 같이 for문으로 작성하는 게 더 좋은가요?

[Kimprodp]

어..미쳤다 기가막히네요 💡
펼쳐진걸 접어서 입 안에 넣는 기분..

[skylar1220]

이렇게 fragment 같이 dto와 도메인 사이의 뭔가 객체를 만들 수 있겠군요..! 제가 고민했던 포인트를 이렇게 풀어갈 수 있다는 게 재밌었어요😱😱😱 몇가지의 고민점과 제안 남겨요!

• 들었던 생각(푸념입니다)
  내가 질러놓은 코드를 아루가 분리할 수 있는 포인트를 잘 잡아서 리팩토링해줬네. 어떻게 하면 저런 생각을 할 수 있을까? 코드를 더 많이 짜봐야하는 걸까? 그런 것 같다.

[skylar1220]

1. 이해가 자연스러운 흐름은
   higlightLines가 하이라이트가 쳐져있는 line만 갖고 있고, 그걸 엔티티로 변환한다.인데

2. 현재는
   HighlightedLines의 필드 lines는 하이라이트가 안쳐져있는 line까지 포함하고 있지만, 엔티티로 변환할 때는 하이라이트가 쳐져있는 부분만 걸러서 변환이 된다.의 로직이라 조금더 이해하는데 시간이 걸리네요.
   (네 맞습니다. 제가 생성해놓은 구조..ㅎㅎ)

3. 그렇다면 왜 하이라이트가 안쳐져있는 라인까지 포함해야했나 보면 이런 이유때문이었네요.

   1. answer을 라인별로 split해서 갖고있어야, addRange가 호출될 때 라인을 갖고 그 라인을 대상으로 addRange를 할 수 있으니까
   2. 전체 라인보다 넘어서는 라인 입력이 들어오는지 검증하기 위해서

1번의 흐름으로 코드를 구성하는 방법이 있을지 고민입니다...!

[skylar1220]

(지금 변경하자는 논의는 아니고, 함께 고민해보자입니다~)
위의 코멘트와 별개로 이곳이 연산이 많이 일어나는 포인트겠구나 싶어요. 하이라이트의 대상인 라인만큼 스트림 연산이 일어날테니까요. 새삼 json으로 저장했다면 이런 연산 비용은 적었겠다는 생각이 들었습니다!

[skylar1220]

사실 이것도 엄청 이해가 어려운 건 아니에요~ 오잉 싶었던 부분 짚고 넘어가고 싶어서!
제일 땡겼던 건: 1번으로 구조 자체가 바뀐다면 이해하기도 좋고 네이밍 문제도 해결되겠다
근데: 지금 당장 바꿀정도는 아니다

[skylar1220]

마지막 리턴 구문 stream으로 충분히 이해할 수 있나요? 위와 같이 for문으로 작성하는 게 더 좋은가요?

저는 스트림의 키가 뭐더라... value가.. 여기서 flatMap을 하면...처럼 생각이 어려워져서, for문이 더 길더라도 이해가 쉽네요😂

[donghoony]

stream과 entry에 익숙해지는 것이 좋을까요, 풀어두는 게 좋을까요? 🤔

[donghoony]

answerIdHighlightedLines가 이미 맵의 형태 네이밍이 되어 있으니 entrySet을 했을 때의 결과가 예측되려나요?

[skylar1220]

네이밍을 통해 key-value가 뭐일지 예측이 되는 건 맞네요!
뭔가 계속 읽다보니 스트림도 충분히 이해 가는 것 같기도하고...ㅋㅋㅋㅋ
흠.. 다들 스트림 방식을 더 선호한다면 크게 이해가 어려운 것은 아니니 스트림으로 가도 좋습니다!

[skylar1220]

higlightLines가 하이라이트가 쳐져있는 line만 갖고 있고, 그걸 엔티티로 변환한다

요 흐름이 가능할지 다음에 생각해볼게요! 수고 많았슴당😀😀