Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Nov 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import java.util.Arrays;
import java.util.List;
import java.util.function.Function;
import java.util.stream.IntStream;
import lombok.Getter;
import reviewme.highlight.domain.exception.InvalidHighlightLineIndexException;
import reviewme.highlight.domain.exception.NegativeHighlightLineIndexException;
Expand Down Expand Up @@ -37,4 +39,12 @@ private void validateLineIndexRange(int lineIndex) {
throw new InvalidHighlightLineIndexException(lineIndex, lines.size());
}
}

public List<Highlight> toHighlights(long answerId) {
return IntStream.range(0, lines.size())
.mapToObj(lineIndex -> lines.get(lineIndex).getRanges().stream()
.map(range -> new Highlight(answerId, lineIndex, range)))
.flatMap(Function.identity())
.toList();
nayonsoso marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +43 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ public void editHighlight(HighlightsRequest highlightsRequest, ReviewGroup revie

Set<Long> answerIds = answerRepository.findIdsByQuestionId(highlightsRequest.questionId());
highlightRepository.deleteAllByAnswerIds(answerIds);

highlightRepository.saveAll(highlights);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import jakarta.validation.constraints.NotEmpty;
import jakarta.validation.constraints.NotNull;
import java.util.List;
import java.util.stream.Stream;
import reviewme.highlight.service.mapper.HighlightFragment;

public record HighlightRequest(

Expand All @@ -13,4 +15,15 @@ public record HighlightRequest(
@Valid @NotEmpty(message = "하이라이트 된 라인을 입력해주세요.")
List<HighlightedLineRequest> lines
) {
public List<HighlightFragment> toFragments() {
return lines.stream()
.flatMap(this::mapRangesToFragment)
.toList();
}

private Stream<HighlightFragment> mapRangesToFragment(HighlightedLineRequest line) {
return line.ranges()
.stream()
.map(range -> new HighlightFragment(answerId, line.index(), range.startIndex(), range.endIndex()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import jakarta.validation.Valid;
import jakarta.validation.constraints.NotNull;
import java.util.List;
import reviewme.highlight.service.mapper.HighlightFragment;

public record HighlightsRequest(

Expand All @@ -20,4 +21,10 @@ public List<Long> getUniqueAnswerIds() {
.distinct()
.toList();
}

public List<HighlightFragment> toFragments() {
return highlights.stream()
.flatMap(request -> request.toFragments().stream())
.toList();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
package reviewme.highlight.service.mapper;

public record HighlightFragment(long answerId, int lineIndex, int startIndex, int endIndex) {
}
Original file line number Diff line number Diff line change
@@ -1,21 +1,14 @@
package reviewme.highlight.service.mapper;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.stream.Collectors;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
import reviewme.highlight.domain.HighlightedLines;
import reviewme.highlight.domain.HighlightedLine;
import reviewme.highlight.domain.Highlight;
import reviewme.highlight.domain.HighlightRange;
import reviewme.highlight.service.dto.HighlightIndexRangeRequest;
import reviewme.highlight.service.dto.HighlightRequest;
import reviewme.highlight.service.dto.HighlightedLineRequest;
import reviewme.highlight.domain.HighlightedLines;
import reviewme.highlight.service.dto.HighlightsRequest;
import reviewme.review.domain.Answer;
import reviewme.review.domain.TextAnswer;
import reviewme.review.repository.TextAnswerRepository;

@Component
Expand All @@ -25,53 +18,19 @@ public class HighlightMapper {
private final TextAnswerRepository textAnswerRepository;

public List<Highlight> mapToHighlights(HighlightsRequest highlightsRequest) {
Map<Long, HighlightedLines> answerHighlightLines = textAnswerRepository
Map<Long, HighlightedLines> answerIdHighlightedLines = textAnswerRepository
.findAllById(highlightsRequest.getUniqueAnswerIds())
.stream()
.collect(Collectors.toMap(Answer::getId, answer -> new HighlightedLines(answer.getContent())));
addIndexRanges(highlightsRequest, answerHighlightLines);
return mapLinesToHighlights(answerHighlightLines);
}

private void addIndexRanges(HighlightsRequest highlightsRequest, Map<Long, HighlightedLines> answerHighlightLines) {
for (HighlightRequest highlightRequest : highlightsRequest.highlights()) {
HighlightedLines highlightedLines = answerHighlightLines.get(highlightRequest.answerId());
addIndexRangesForAnswer(highlightRequest, highlightedLines);
}
}

private void addIndexRangesForAnswer(HighlightRequest highlightRequest, HighlightedLines highlightedLines) {
for (HighlightedLineRequest lineRequest : highlightRequest.lines()) {
int lineIndex = lineRequest.index();
for (HighlightIndexRangeRequest rangeRequest : lineRequest.ranges()) {
highlightedLines.addRange(lineIndex, rangeRequest.startIndex(), rangeRequest.endIndex());
}
}
}

private List<Highlight> mapLinesToHighlights(Map<Long, HighlightedLines> answerHighlightLines) {
List<Highlight> highlights = new ArrayList<>();
for (Entry<Long, HighlightedLines> answerHighlightLine : answerHighlightLines.entrySet()) {
createHighlightsForAnswer(answerHighlightLine, highlights);
}
return highlights;
}
.collect(Collectors.toMap(TextAnswer::getId, answer -> new HighlightedLines(answer.getContent())));

private void createHighlightsForAnswer(Entry<Long, HighlightedLines> answerHighlightLine,
List<Highlight> highlights) {
long answerId = answerHighlightLine.getKey();
List<HighlightedLine> highlightedLines = answerHighlightLine.getValue().getLines();

for (int lineIndex = 0; lineIndex < highlightedLines.size(); lineIndex++) {
createHighlightForLine(highlightedLines, lineIndex, answerId, highlights);
for (HighlightFragment fragment : highlightsRequest.toFragments()) {
HighlightedLines highlightedLines = answerIdHighlightedLines.get(fragment.answerId());
highlightedLines.addRange(fragment.lineIndex(), fragment.startIndex(), fragment.endIndex());
}
}

private void createHighlightForLine(List<HighlightedLine> highlightedLines, int lineIndex, long answerId,
List<Highlight> highlights) {
for (HighlightRange range : highlightedLines.get(lineIndex).getRanges()) {
Highlight highlight = new Highlight(answerId, lineIndex, range);
highlights.add(highlight);
}
return answerIdHighlightedLines.entrySet()
.stream()
.flatMap(entry -> entry.getValue().toHighlights(entry.getKey()).stream())
.toList();
Comment on lines +31 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,21 @@ class HighlightedLinesTest {
assertThatCode(() -> highlightedLines.addRange(invalidLineIndex, 0, 1))
.isInstanceOf(InvalidHighlightLineIndexException.class);
}

@Test
void 하이라이트가_존재하는_부분만_엔티티로_변환한다() {
// given
HighlightedLines lines = new HighlightedLines("0\n11\n222");
lines.addRange(0, 0, 0);
lines.addRange(2, 2, 2);

// when
List<Highlight> highlights = lines.toHighlights(1L);

// then
assertThat(highlights).containsExactly(
new Highlight(1L, 0, new HighlightRange(0, 0)),
new Highlight(1L, 2, new HighlightRange(2, 2))
);
}
}