-
Notifications
You must be signed in to change notification settings - Fork 2
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: 도메인을 활용한 하이라이트 추가 및 수정 로직 리팩토링 #859
Conversation
Test Results153 tests +8 150 ✅ +8 5s ⏱️ -1s Results for commit 80479d5. ± Comparison against base commit 8c3c5ae. This pull request removes 6 and adds 14 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
거의 다 왔네요 😁 많이 나아졌다고 생각해요. 리뷰하면서 어? 모먼트도 많이 줄었습니다 👍🏻 웬만하면 반영하기를 원하지만 의도가 어땠는지 함께 이야기해주면 좋겠습니다~
private void validateNonNegativeIndexNumber(int startIndex, int endIndex) { | ||
if (startIndex < 0 || endIndex < 0) { | ||
throw new NegativeHighlightIndexException(startIndex, endIndex); | ||
throw new NegativeHighlightIndexRangeException(startIndex, endIndex); | ||
} | ||
} | ||
|
||
private void validateEndIndexOverStartIndex(long startIndex, long endIndex) { | ||
private void validateEndIndexOverStartIndex(int startIndex, int endIndex) { | ||
if (startIndex > endIndex) { | ||
throw new HighlightStartIndexExceedEndIndexException(startIndex, endIndex); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
둘 다 같은 예외로 처리되면 별로일까요? 어쨌든 같은 인덱스와 관련된 예외인데 너무 구체적으로 나뉘지 않았나 해서요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서버 입장에서는 사실 크게 상관없을 것 같아요. 예외명이나 메시지가 하나로 통일되어도 로그에 start, endIndex가 포함되어있으니 문제 파악은 가능하니까요.
그렇다면 두 예외를 나눴을 때 좋은점은 클라이언트가 뭘 잘못했길래 예외인지 정보를 전달할 수 있다는 점이었는데요.
그렇다면 이것도 예외를 추상적으로 하나로 합치고, start, endIndex를 예외 메시지에 담아 클라이언트에게 전달하는 건 어떨까 싶네요. 시작, 종료 위치는 과한 정보가 아니라고 생각해서요!
|
||
@Getter | ||
@EqualsAndHashCode | ||
public class HighlightLine { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하이라이트된 라인이니 HighlightedLine
은 어떤가요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋네요 반완!
private final int lineIndex; | ||
private final String lineContent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
highlightLine.lineIndex
? 앞에 line
을 안 쓰면 가독성이 떨어질까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그냥 index가 낫겠네요 반완!
} | ||
|
||
private void validateRangeByContentLength(int startIndex, int endIndex) { | ||
int providedEndIndex = lineContent.length() - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1
하지 말고, 등호로 표현하는 건 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반완!
private final List<HighlightLine> lines; | ||
|
||
public HighlightLines(String content) { | ||
this.lines = mapLines(content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한줄로 표현하는 것보다 여기에서 풀어 쓰면 안될까요..? 😁 👍🏻👍🏻👍🏻👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 생성자에 매핑 로직이 길게 들어가면 매핑 로직 때문에 초기화 작업 자체를 파악하기 어렵다고 생각했어요.
혹시 어떤 이유때문에 풀어쓰는 게 낫다고 생각했나요? 읽다가 아래로 내렸다 올라와야하는 불편함 때문일 거라고 추측하는데 이정도의 메서드양과 왔다갔다할 범위(?)에서는 괜찮다고 생각했어요!
public HighlightLines(String content) {
List<HighlightedLine> mappedLines = new ArrayList<>();
String[] lineContents = content.split(LINE_SEPARATOR);
for (int i = 0; i < lineContents.length; i++) {
mappedLines.add(new HighlightedLine(i , lineContents[i]));
}
this.lines = mappedLines;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HighlightedLine
에index
가 필요없다고 생각해요. 그렇다면 아래와 같이 하나의 stream 처리가 가능하지 않은가요?
this.lines = Arrays.stream(content.split(LINE_SEPARATOR))
.map(HighlightedLine::new)
.toList()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HighlightedLine을 나중에 Highlight 엔티티를 생성할 때 사용하는데 이 때 index를 바로 꺼내서 쓸 수 있게하기 위해 필요했어요!
for (HighlightRange range : line.getRanges()) {
Highlight highlight = new Highlight(answerId, line.getIndex(), range);
highlights.add(highlight);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인 그 자체에서는 필요하지 않은 값 아닌가요 ?? 밖에서 필요한 값을 명목상 가지고 있는 게 필요할 지도 생각해봐야 할 일이겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인 자체에서 필요하지 않은값22군요..! 뺄 수 있을 것 같아요 더불어 매핑이 간단해졌으니 생성자 안으로 넣는 것도 되겠습니당 반완!!
public Set<Long> getUniqueAnswerIds() { | ||
return highlights() | ||
.stream() | ||
.map(HighlightRequest::answerId) | ||
.collect(Collectors.toSet()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.stream().distinct()
, toList()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오 같은 중복제거를 하면서도 set으로 제한두지 않을 수 있겠군요 반완!
@@ -37,4 +37,10 @@ public Highlight(long answerId, int lineIndex, int startIndex, int endIndex) { | |||
this.lineIndex = lineIndex; | |||
this.highlightRange = new HighlightRange(startIndex, endIndex); | |||
} | |||
|
|||
public Highlight(long answerId, int lineIndex, HighlightRange range) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성자 추가했으니 기존 생성자를 체이닝하면 어떨까요? 개인적으로는 지금 이 생성자가 주 생성자가 되었으면 하는 바람입니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성자 체이닝을 놓쳤네요 반완!
for (HighlightLine line : highlightLines.getLines()) { | ||
for (HighlightRange range : line.getRanges()) { | ||
Highlight highlight = new Highlight(answerId, line.getLineIndex(), range); | ||
highlights.add(highlight); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
장풍 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아아 이 장풍 어떡하죠.... 메서드 분리를 하자니 너무 타고타고 들어가는 메서드인게 걸리는데...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 메서드 분리 해봤습니당
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class HighlightMapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모든 엔티티, 응답에 대해 Mapper
가 생성되는 현재 구조.. 고민해야겠습니다
@@ -5,6 +5,7 @@ | |||
import lombok.EqualsAndHashCode; | |||
import lombok.Getter; | |||
import reviewme.highlight.domain.exception.HighlightIndexExceedLineLengthException; | |||
import reviewme.highlight.entity.HighlightRange; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로는 패키지 나누는 게 더 신경쓰이긴 해요, 도메인이면 엔티티를 몰라야 하는데 나뉘어 있으니 import
로 참조하겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인이 엔티티를 몰라야하는 이유가 무엇인가요..?😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인이 엔티티보다 상위의 개념이군요. 그런데 그런 도메인이 엔티티를 의존한다는 측면에서 걸리는 것이었네요.
현재 도메인과 엔티티를 크게 구분하여 사용하고 있지 않기때문에 패키지 통합했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
얘는 안 바뀌었어요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lineIndex 말하는 거 맞을까요? 여기는 start, end 등 다른 인덱스도 함께 있어서 구분을 위해 lineIndex라고 명시하는 게 낫겠다 생각했어요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HighlightedLines
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반완!
@@ -11,12 +11,12 @@ | |||
@EqualsAndHashCode | |||
public class HighlightedLine { | |||
|
|||
private final int lineIndex; | |||
private final int index; | |||
private final String lineContent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것도 같이 한 말이었습니다 ㅎㅎ... 👍🏻👍🏻👍🏻👍🏻👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content 말하는 것 맞죠?ㅎㅎ 반완!
List<HighlightedLine> mappedLines = new ArrayList<>(); | ||
String[] lineContents = content.split(LINE_SEPARATOR); | ||
for (int i = 0; i < lineContents.length; i++) { | ||
mappedLines.add(new HighlightedLine(i , lineContents[i])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
???? 😁 😮
mappedLines.add(new HighlightedLine(i , lineContents[i])); | |
mappedLines.add(new HighlightedLine(i, lineContents[i])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반완..ㅎㅎ
a415c30
to
e060289
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 어프룹
public Highlight(long answerId, int lineIndex, int startIndex, int endIndex) { | ||
this(answerId, lineIndex, new HighlightRange(startIndex, endIndex)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안 쓰는 생성자입니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반완!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HighlightedLines
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
하이라이트를 전체 삭제해야 하는 경우 (빈 값으로 오는 경우)에 대한 테스트만 부탁해요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초간단🍰 리뷰들을 남기게 되었네요~
전체적으로 코드도 훨씬 깔끔해졌고, 객체를 분리하니 test 의 의도도 알기 편했어요
그런데 반복적으로 리뷰하게 되는 부분들이 있는데, 제출 전에 한번 검토하고 PR 올려도 좋을 것 같아요!
mapper 로직이 이번 구현에서의 핵심이었던 것 같은데, 저 역시 다른 분들과 동일하게
"변경되기 쉬운 조회 로직인데 코드가 그 중요성 이상으로 길고 복잡한 것 같다"는 생각은 드네요.
그래도 기가막힌 대안이 떠오르지 않는 것 같습니다😅
커비의 코드가 아니라, 구조의 문제인 것 같은데 그냥 적어봅니당 ㅎㅎ
Set<Long> allAnswerIds = answerRepository.findIdsByQuestionId(highlightsRequest.questionId()); | ||
highlightRepository.deleteAllByAnswerIds(allAnswerIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[왕사소 & 취향이라 생각하면 반영 안해도 됨]
변수명 allAnswerIds 보다 answerIds 어떤가요?
allAnswerIds 는 "DB 에 존재하는 모든 답변의 ID"라고 느껴져요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고민했었는데 산초도 그렇게 생각했다니 반영 갑니다~~
validateReviewGroupContainsAnswer(request, reviewGroupId); | ||
public void validate(HighlightsRequest request, ReviewGroup reviewGroup) { | ||
validateReviewGroupContainsAnswer(request, reviewGroup); | ||
validateQuestionContainsAnswer(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[초간단🍰]
validateReviewGroupContainsAnswer 와 validateQuestionContainsAnswer 의 호출 순서와
private 메서드 선언 순서가 일치되면 좋을 것 같아요,
) { | ||
public List<Long> getUniqueAnswerIds() { | ||
return highlights() | ||
.stream() | ||
.map(HighlightRequest::answerId) | ||
.distinct() | ||
.toList(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[초간단🍰]
클래스 시작 개행
class HighlightedLineTest { | ||
|
||
@Test | ||
void 하이라이트_대상_라인의_글자수보다_큰_시작_종료_인덱스_범위를_추가하려고_하면_예외를_발생한다() { | ||
// given | ||
String content = "12345"; | ||
HighlightedLine highlightedLine = new HighlightedLine(content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확실히 클래스를 분리하니 테스트 코드 작성이 보기 좋아지네요😊
@Autowired | ||
ReviewRepository reviewRepository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[🍰]
private 자꾸 놓치는 것 같아요~
} | ||
|
||
@Test | ||
void 하이라이트_반영을_요청하면_새로운_하이라이트가_저장된다() { | ||
// given | ||
long questionId = questionRepository.save(서술형_필수_질문()).getId(); | ||
long questionId = questionRepository.save(QuestionFixture.서술형_필수_질문()).getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[🍰]
static?
|
||
@Autowired | ||
HighlightMapper highlightMapper; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[🍰]
private
@Test | ||
void 하이라이트_요청과_기존_서술형_답변으로_하이라이트를_매핑한다() { | ||
// given | ||
long questionId = questionRepository.save(QuestionFixture.서술형_필수_질문()).getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[🍰]
static import
long questionId1 = questionRepository.save(QuestionFixture.서술형_필수_질문()).getId(); | ||
long questionId2 = questionRepository.save(QuestionFixture.서술형_필수_질문()).getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
드디어 어프루부! 고생했습니다 커비😊
코드가 정말정말 좋아진 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 어제 확인했는데 어프룹 까먹었음..
리리팩터링은 다음에 고민해봅시다 👍👍
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말