-
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] feat: 하이라이트 추가 및 수정 API 구현 #813
Conversation
Test Results126 tests 126 ✅ 4s ⏱️ Results for commit 24b706b. ♻️ 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.
커비 고생많았습니다
우선 1차 코멘트 남겨요 확인 부탁해요~
그리고 우리가 명령에 해당하는 로직(CUD)는 객체 지향적으로 하기로 했던가 했으면 좋겠나 얘기했던 것 같은데, 절차지향적으로 되어 있는 것 같아요.
때문에 검증 로직이 상당히 길어지고 복잡해지는 것 같아요. 객체를 생성하면서 생성자에서 책임을 넘기면 간단하게 검증할 수 있지 않을까요?
우선 코멘트 확인 후 다시 리뷰 할게요~
@@ -19,7 +19,7 @@ public class HighlightController { | |||
@PostMapping("/v2/highlight") | |||
public ResponseEntity<Void> highlight(@Valid @RequestBody HighlightsRequest request, | |||
@SessionAttribute("reviewRequestCode") String reviewRequestCode) { | |||
highlightService.highlight(request); | |||
highlightService.highlight(request, reviewRequestCode); |
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.
리뷰 컨트롤러처럼 개행을 맞추면 어떨까요?
@GetMapping("/v2/reviews/{id}")
public ResponseEntity<ReviewDetailResponse> findReceivedReviewDetail(
@PathVariable long id,
@SessionAttribute("reviewRequestCode") String reviewRequestCode
) {
ReviewDetailResponse response = reviewDetailLookupService.getReviewDetail(id, reviewRequestCode);
return ResponseEntity.ok(response);
}
where q.id = :questionId | ||
""", nativeQuery = true) | ||
Set<Long> findIdsByQuestionId(long questionId); | ||
} |
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.
지금 아루의 PR이 올라와 있으니 미리 JPQL로 바꾸죠!
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 void validate(HighlightsRequest request, long reviewGroupId) { | ||
validateAnswerByReviewGroup(request, reviewGroupId); | ||
validateQuestionByReviewGroup(request, reviewGroupId); |
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.
요청의 뎁스 순서로 하면 validateQuestionByReviewGroup이 먼저 되어야 할 것 같아요
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.
오 맞네요 질문 먼저 검증!
validateLineIndex(request); | ||
validateRange(request); | ||
validateDuplicate(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.
validateAnswerByReviewGroup, validateQuestionByReviewGroup, validateAnswerByQuestion
이 부분은 highlight가 연관된 것들을 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.
리팩토링 pr에서 반영하겠습니다!
|
||
@Column(name = "review_group_id", nullable = false) | ||
private long reviewGroupId; | ||
|
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.
굳이 questionId와 reviewId를 가지고 있지 않아도 되지 않을까요?
answerId로 소속은 충분히 알 수 있다고 생각해요,
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.
questionId와 reviewGroupId를 말하는 게 맞겠죠?
새로운 하이라이트가 들어왔을 때 question id = ? and review_group_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.
JOIN
해서 DELETE
하는 게 좋지 않을까요? 데이터 삭제에만 필요한 것이 DB에 속성으로 등록될 필요가 있을지 고민해보아야겠네용,,
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.
맞네요 answerId로 처리하고 review group id와 question id는 뺐어요!
} | ||
} | ||
|
||
private void validateRange(HighlightsRequest 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.
이 검증은 객체 책임으로 가지는 것이 좋을 것 같다는 의견입니다~ (산초 말투)
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.
서비스적인 속성이 아니네요 반영완!
} | ||
} | ||
|
||
private void validateLineIndex(HighlightsRequest 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.
이 부분도 객체의 책임으로 할 수 있는 방법이 없을까요?
(개인적으로 답변을 함께 생성자에 넘겨서 의존하지 않고 answer를 사용하게 하거나 답변에 대한 index 길이 검증 책임을 가지는 클래스가 있어도 될 것 같아요)
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.
리팩토링 pr에서 반영하겠습니다!
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.
앞쪽 커밋 보고 일단 RC 올립니다~! 💪🏻
questionId BIGINT NOT NULl, | ||
review_group_id BIGINT NOT NULl, | ||
answer_id BIGINT NOT NULl, | ||
line_index BIGINT NOT NULl, | ||
start_index BIGINT NOT NULl, | ||
end_index BIGINT NOT NULl, |
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.
NULl
???- index는 몇억 단위로 커질 일이 없는데
BIGINT
로 하지 않고 그냥INT
로 해도 되지 않을까요?
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.
BIGINT
, INT
는 DB에서는 반영하되, 어플리케이션에서 long
을 int
로 바꾸지는 않아도 될 듯해요.
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.
db에 차지하는 크기를 줄이는 목적이니 ddl만 바꾸면 된다는 말이죠? 반영완!
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.
(어플리케이션이 DB에 종속적일 필요가 없다는 맥락에서 바꾸지 않아도 된다는 이야기를 했슴다)
highlightService.highlight(request, reviewRequestCode); | ||
return ResponseEntity.ok().build(); |
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.
아차차😂
@@ -4,4 +4,6 @@ | |||
import reviewme.highlight.domain.HighLight; | |||
|
|||
public interface HighlightRepository extends JpaRepository<HighLight, Long> { | |||
|
|||
void deleteByReviewGroupIdAndQuestionId(long reviewGroupId, long questionId); |
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건당 DELETE 쿼리가 날아가는 것으로 알고 있어요.
@Modifying
, @Query
로 한 번에 다 날아가게 하면 어떨까요?
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() | ||
.flatMap(highlightRequest -> highlightRequest.lines().stream() | ||
.flatMap(line -> line.ranges().stream() | ||
.map(range -> new HighLight( | ||
highlightRequest.answerId(), highlightsRequest.questionId(), reviewGroupId, | ||
line.index(), range.startIndex(), range.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.
for문으로 일단 돌려놓고 어떻게 해볼게요........!!!!!
@@ -10,5 +10,7 @@ public interface ReviewGroupRepository extends JpaRepository<ReviewGroup, Long> | |||
|
|||
Optional<ReviewGroup> findByReviewRequestCode(String reviewRequestCode); | |||
|
|||
Optional<Long> findIdByReviewRequestCode(String reviewRequestCode); |
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.
위의 findByReviewRequestCode
의 결과에 .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.
지웠어요 반영완!
|
||
public InvalidHighlightRangeException(long startIndex, long endIndex) { | ||
super("하이라이트 시작 글자 인덱스는 종료 글자 인덱스보다 작아야해요."); | ||
log.info("Highlight index is out of bound - startIndex: {}, endIndex: {}", 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.
out of bound
는 주어진 배열보다 더 큰/작은 값에 대한 예외 사항이예요. Invalid highlight range: {} {}
로 해도 충분하지 않을까요?
private final Long answerId; | ||
private final Long lineIndex; | ||
private final Long startIndex; | ||
private final Long 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.
long
?
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) { | ||
return true; | ||
} | ||
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
HighlightKey that = (HighlightKey) o; | ||
return answerId.equals(that.answerId) && lineIndex.equals(that.lineIndex) && | ||
startIndex.equals(that.startIndex) && endIndex.equals(that.endIndex); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(answerId, lineIndex, 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.
@EqualsAndHashCode(of)
로 해결할 수 없었나용?
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.
@EqualsAndHashCode
만 붙이면 모든 필드에 데헤서 equals 처리해주는군요!
import lombok.Getter; | ||
|
||
@Getter | ||
public class HighlightKey { |
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.
객체 도출 좋네요! 이거 @Embedded
로 하지 않은 이유가 있나요?
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.
highlightPosition 어떤가요? (사실 아루가 제안함)
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 HighlightKey(Long answerId, Long lineIndex, Long startIndex, Long endIndex) { | ||
this.answerId = answerId; | ||
this.lineIndex = lineIndex; | ||
this.startIndex = startIndex; | ||
this.endIndex = 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.
애초에 begin <= end
는 불변식이니 여기에서 검증해도 되지 않을까요?
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.
커비 수고했어요
원래 누군가 당연하게 하는 일은, 그렇게 되기까지 많은 시간이 걸린다고 해요!
조금씩 다듬어가면 훨씬 더 좋은 코드가 될 것 같아요~
private void saveNewHighlight(HighlightsRequest highlightsRequest, long reviewGroupId) { | ||
List<HighLight> highLights = highlightsRequest.highlights() | ||
.stream() | ||
.flatMap(highlightRequest -> highlightRequest.lines().stream() | ||
.flatMap(line -> line.ranges().stream() | ||
.map(range -> new HighLight( | ||
highlightRequest.answerId(), highlightsRequest.questionId(), reviewGroupId, | ||
line.index(), range.startIndex(), range.endIndex() | ||
)) | ||
)) | ||
.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.
스트림으로 하나의 라인에 처리하려고 하는 커비의 마음은 알겠지만 읽는 사람 입장에서 잘 읽히지 않네요🥲 적당한 함수로 분리한다면, 더 작은 단위로 쪼갤 수 있고, 함수 이름으로도 의미를 전달할 수 있을 것이라 생각해요~
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 (HighlightRequest highlight : request.highlights()) { | ||
TextAnswer textAnswer = textAnswerRepository.findById(highlight.answerId()) | ||
.orElseThrow(() -> new AnswerNotFoundByIdException(highlight.answerId())); | ||
long maxLineIndex = textAnswer.getContent().lines().count(); |
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 validateQuestionByReviewGroup(HighlightsRequest request, long reviewGroupId) { | ||
Set<Long> providedQuestionIds = questionRepository.findIdsByReviewGroupId(reviewGroupId); | ||
long submittedQuestionId = request.questionId(); | ||
|
||
if (!providedQuestionIds.contains(submittedQuestionId)) { | ||
throw new SubmittedQuestionAndProvidedQuestionMismatchException(submittedQuestionId, providedQuestionIds); | ||
} | ||
} |
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.
questionRepository.existByIdAndReviewGroupId() 어떠신가요? 성능도 더 좋을 것 같아요!
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.
직접적으로 연관이 없는 review group을 통해 question을 가져오도록 하는 것이 너무 특정적이고 비즈니스 로직적이지 않나해서
review group의 속성인 template을 가져오고, template을 통해 question을 가져오도록 변경하였습니다!
validateAnswerByReviewGroup(request, reviewGroupId); | ||
validateQuestionByReviewGroup(request, reviewGroupId); | ||
validateAnswerByQuestion(request); | ||
validateLineIndex(request); | ||
validateRange(request); | ||
validateDuplicate(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.
(개인 취향일 수도 있음)
무엇을 validate___By___ 함수들을 무엇을 검증하는지 더 명확하게 이름을 바꿀 수 있을 것 같아요!
예를 들면
validateAnswerByReviewGroup
-> validateHighlitedAnswerBelongToReviewGroup
validateQuestionByReviewGroup
-> validateHighlitedQuestionBelongToReviewGroup
validateAnswerByQuestion
-> validateHighlightedAnswerBelongToQuestion
(이건 정말정말 개인 취향일 수 있음)
혹은 이 세가지를 묶어서 이런 validation 함수를 만들수도 있겠네요!
validateHighlightedResourceHiarachyRelationship()
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.
(아루 코멘트 차용) validateReviewGroupContainsQuestion처럼 validate + 긍정문의 검증 행위 형식으로 바꿔보았어요!
} | ||
} | ||
|
||
private void validateLineIndex(HighlightsRequest 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.
저도 동의합니다~
1ab6d99
to
834d900
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.
일단 어프룹합니다~ 코멘트는 확인해주세요~!
@Embedded | ||
private HighlightPosition highlightPosition; | ||
|
||
public Highlight(long answerId, HighlightPosition highlightPosition) { |
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.
(캡슐화 관점) 이 클래스 밖에서 HighlightPosition
을 몰라도 되지 않을까요? 내부에서 만들어주는 게 낫지 않을까 싶습니다.
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.
외부에서 예외를 터뜨려 early fail을 의도했다면, 예외를 터뜨리는 책임이 누구인지도 고민해보면 좋습니다. 내부 클래스를 세부적으로 알고 있는 자는 누구인가? 잘 모르는 클래스가 예외를 발생할 책임이 있는가? 등등...
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.
예외를 터뜨리는 책임이 누구인지도 고민해보면 좋습니다.
내부 클래스를 세부적으로 알고있는자는 (이전 기준) HighlightService였네요.
하지만 HighlightService보다는 안그래도 필드로 갖고있어서 관련이 높은 Highlight 도메인이 HighlightPosition의 세부사항을 아는 게 맞겠네요.
잘 모르는 클래스가 예외를 발생할 책임이 있는가? 등등...
예외를 터트리는 책임이라...
예외가 발생하는 곳은 HighlightPosition 자체이지만, 타고타고 들어오는 곳인 HighlightService도 예외를 발생한 책임�을 가지게 되었다고 보는 걸까요? 그렇다면 역시 같은 맥락으로 HighlightService가 그 책임을 가지지 않는 것이 적절하겠어요!
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 validateRange(long startIndex, long endIndex) { | ||
if (startIndex > endIndex) { | ||
throw new InvalidHighlightRangeException(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.
0 넘어가는 건 이곳에서 검증하지 않나요 ?
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 (HighlightedLineRequest line : highlight.lines()) { | ||
long submittedLineIndex = line.index(); | ||
if (maxLineIndex < submittedLineIndex) { | ||
throw new InvalidHighlightLineIndexException(submittedLineIndex, maxLineIndex); | ||
} | ||
} | ||
} | ||
} |
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, 2에서 어떻게 리팩터링했는지 돌아보면 실마리가 보일거예요~
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.
해볼게요!
@@ -25,14 +25,20 @@ public class HighlightPosition { | |||
private long endIndex; | |||
|
|||
public HighlightPosition(long lineIndex, long startIndex, long endIndex) { | |||
validateRange(startIndex, endIndex); | |||
validatePositiveIndexNumber(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.
Positive
은 0보다 큰 값을 의미합니다. NonNegative
로 음이 아닌을 의미하면 어떨까요?
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.
오호 헷갈렸네요 반완!
long maxLineIndex = textAnswer.getContent().lines().count(); | ||
|
||
for (HighlightedLineRequest line : highlight.lines()) { | ||
long submittedLineIndex = line.index(); | ||
if (maxLineIndex < submittedLineIndex) { | ||
throw new InvalidHighlightLineIndexException(submittedLineIndex, maxLineIndex); | ||
} | ||
} |
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.
이 if 문 수정이 필요해보여요.
지금 maxLineIndex 는 서술형 답변의 "라인 수(마지막 idx + 1)"을 나타내기 때문에 <=
여야 합니다.
그리고, maxLineIndex 라는 변수명을 answerLineCount
혹은 existingLineNumbers
같은 걸로 바꾸는 것도 의미 전달이 더 잘 될 것 같아요.
그리고 테스트 코드에서도 아래 부분 수정해주세요!
long answerLineCount = textAnswer.getContent().lines().count();
HighlightedLineRequest highlightedLineRequest = new HighlightedLineRequest(answerLineCount + 1, List.of()); // ❗️이 부분을 answerLineCount로 바꿔야 경계값이 됩니다.
HighlightRequest highlightRequest = new HighlightRequest(textAnswer.getId(), List.of(highlightedLineRequest));
HighlightsRequest highlightsRequest = new HighlightsRequest(questionId, List.of(highlightRequest));
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 개념이어야 쉽게 이해갈 것 같아서 providedMaxLineIndex로 해봤는데 어떤가요?
저 경계값은 고쳐야겠네요! 반완!
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 코드 autoWired 필드에 private 붙여주세요! |
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.
코멘트는 확인 후 다음 PR에서 반영 부탁!
private long answerId; | ||
|
||
@Embedded | ||
private HighlightPosition highlightPosition; |
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 HighlightValidator highlightValidator; | ||
|
||
@Transactional | ||
public void highlight(HighlightsRequest request, String reviewRequestCode) { |
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.
리팩토링 pr에서 editHighlight로 변경하였습니다~
} | ||
|
||
private void validateReviewGroupContainsQuestion(HighlightsRequest request, long reviewGroupId) { | ||
long templateId = reviewGroupRepository.findById(reviewGroupId) |
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.
HighlightService에서 찾은 리뷰 그룹의 아이디를 통해서 보낸 id를 또 리뷰 그룹 repo에서 찾는 것은 비효율적인 것 같아요.
saveNewHighlight(request); | ||
} | ||
|
||
private void deleteOldHighlight(long questionId, long reviewGroupId) { |
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.
beforeHighlight로 수정!
validateReviewGroupContainsAnswer(request, reviewGroupId); | ||
validateQuestionContainsAnswer(request); | ||
validateLineIndex(request); | ||
// TODO: 중복 요청 검증 추가 예정 |
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.
리팩토링 pr에서 제거했어요!
TextAnswer textAnswer1 = new TextAnswer(questionId, "text answer1"); | ||
TextAnswer textAnswer2 = new TextAnswer(questionId, "text answer2"); | ||
Review review = reviewRepository.save(new Review(templateId, reviewGroupId, List.of(textAnswer1, textAnswer2))); | ||
|
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.
반완!
void 하이라이트의_인덱스들이_0보다_작은_경우_예외를_발생한다() { | ||
assertThatCode(() -> new HighlightPosition(1, -2, -1)) | ||
.isInstanceOf(NegativeHighlightIndexException.class); | ||
|
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.
반완!
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말