-
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 구현 #806
Conversation
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차 RC 드립니다! 구현하느라 고생했어요~
@@ -7,7 +7,7 @@ public record ReviewsGatheredByQuestionResponse( | |||
SimpleQuestionResponse question, | |||
|
|||
@Nullable | |||
List<AnswerContentResponse> answers, | |||
List<TextResponse> answers, |
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.
Response
접미사를 빼면 남는 건 Text
인데, 맥락 전달이 부족하지 않을까요? 기존 AnswerContent
에서 바뀌게 된 이유가 무엇일까용?
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 라는 이름의 도메인이 있었기 때문이에요.
서비스 코드에서 Answer 도메인과 AnswerContentResponse 를 함께 사용하다보니AnswerContentResponse 이 기존의 의미인 "서술형 답변" 이라기보다
Answer 자체에 더 가깝다고 느껴졌어요!
Assertions.assertAll( | ||
() -> assertThat(answerForQuestion1).containsOnly(answer1, answer2), | ||
() -> assertThat(answerForQuestion2).containsOnly(answer3), | ||
() -> assertThat(answerForAllQuestion).containsOnly(answer1, answer2, answer3) | ||
); |
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, 3)에 대한 답변 각각 작성
- 질문 (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.
반영했습니다! 🫡
@Query(value = """ | ||
SELECT o.* FROM option_item o | ||
JOIN option_group og | ||
ON o.option_group_id = og.id | ||
JOIN question q | ||
ON og.question_id = q.id | ||
WHERE q.id = :questionId | ||
""", nativeQuery = true) | ||
List<OptionItem> findAllByQuestionId(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.
Question
하위에 OptionItem
이 있을 텐데, 상위 레포지토리(Question
)에서 findAllOptionItemByQuestionId
로 가져오면 어떨까요?
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.
"반환하는 값이 기준이 아니라 상위 클래스가 기준" 이 되어야 한다는 말씀이시군요!
이렇게 했을 때의 장점은 "의존의 방향이 일정해진다"이고요.
(이 의도가 맞으신가요?🥹)
무튼 반영했습니다! (이 과정에서 특이사항 발생 : #807)
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.
✽다시 보면서 깨달은 것✽
지금 Question 과 OptionItem 사이에는
- 직접 참조가 없고
- 패키지 위치도 question 하위로 같으며
- 개념상으로만 Question 이 마치 '상위'인것처럼 느껴졌던 것 같아서
오히려 OptionItem 객체가 반환되기 때문에 OptiomItemRepository 에 있는게 좋았겠다는 생각이 듭니다.
여기에 대해서는 어떻게 생각하시나요?
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.
- 직접 참조를 지양한 것는 생명주기가 다르기 때문이며, 이는 곧 서비스가 커질 때 패키지 분리에 용이하게 적용될 수도 있습니다.
- 현재는 패키지 위치가 같지만, 관심사 자체는 다를 수도 있습니다 (이건 추후 정책이 어떻게 될 지에 따라 달라지겠죠? 패키지가 분리될 수도 있습니다)
- 개념과 코드가 일치하는 게 인지부하 측면에서 좋지 않은가요?
assertAll( | ||
() -> assertThat(optionItemsForQuestion1).containsOnly(optionItem1, optionItem2), | ||
() -> assertThat(optionItemsForQuestion2).containsOnly(optionItem3) | ||
); |
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) 생성
- 옵션그룹 (1, 2) 생성
- 옵션아이템 (1, 2) 생성
- 1로 찾았을 때 옵션아이템 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.
이건 "질문에 해당하는 옵션아이템이 하나인 경우 / 여러개인 경우" 검증이니, 의미있는 검증이라 생각해요🧐
if (question.isSelectable()) { | ||
return 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.
Answer
추상화해뒀는데 이렇게 하기보다는 클래스 뽑는 게 낫지 않을까유
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.
산초~~ 분량이 커서 고생이네요
우선 다른 부분 더 봐야겠지만 서비스 로직 초입부분 코멘트 진행 후에 다음을 봐야할 것 같아서 우선 RC하고 다음 리뷰 진행할게요
개인적으로 서비스 로직 초입 부분에서 자원을 가져오는 부분이 이해가 잘 되지 않아요.
- requestCode로 리뷰 그룹을 확인하고
- 섹션에 대한 모든 질문을 가져오고
- 질문과 리뷰 그룹에 해당하는 답변만 가져오기
의 순서가 맞을 것 같은데 산초의 로직 순서 설명이 좀 필요할 것 같아요! 제가 틀린 부분이 있다면 말씀해주십쇼
import reviewme.review.service.dto.response.gathered.ReviewsGatheredBySectionResponse; | ||
import reviewme.review.service.dto.response.gathered.SimpleQuestionResponse; | ||
import reviewme.review.service.dto.response.gathered.TextResponse; | ||
import reviewme.review.service.dto.response.gathered.VoteResponse; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
public class GatheredReviewLookupService { |
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.
Review 로 시작하고 LookupService 로 끝나도록 말씀이시죠?
GatheredReviewLookupService
-> ReviewGatheredLookupService
로 변경하겠습니다🫡
public ReviewsGatheredBySectionResponse getReceivedReviewsBySectionId(String reviewRequestCode, long sectionId) { | ||
return null; | ||
List<Question> questions = questionRepository | ||
.findAllByReviewRequestCodeAndSectionId(reviewRequestCode, sectionId); |
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.
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.
reviewGroup이 template이랑 붙어있어서 그런데, 한 가지 제안:
- Controller에서 reviewRequestCode로 templateId 찾기
- Controller에서 templateId, sectionId로 서비스 불러 Response 만들기
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.
templateId 으로만 Response 만들 수 없으므로 ReviewGroup을 통하도록 변경했습니다!
(리리코 유효 검증도 할 겸)
.findAllByReviewRequestCodeAndSectionId(reviewRequestCode, sectionId); | ||
Map<Long, Question> questionMap = questions.stream() | ||
.collect(Collectors.toMap(Question::getId, Function.identity())); | ||
List<Answer> answers = answerRepository.findAllByQuestions(new ArrayList<>(questionMap.keySet())); |
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.
메서드명을 ~QestionIds
로 하는 것이 좋을 것 같아요. 다른 곳과 통일성을 주기 위해~
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.
앗 그렇네요🫨 반영했어요!
SELECT a FROM Answer a | ||
WHERE a.questionId IN :questionIds | ||
""") | ||
List<Answer> findAllByQuestions(List<Long> questionIds); |
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.
jpql을 쓴 이유가 있나요? (다른 곳은 native 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.
아주아주 중요한 질문입니다!! 네이티브 쿼리를 사용했더니, 아래의 Answer 로 매핑하지 못한다는 식의 예외가 뜨더라고요😢
Unable to find column position by name: clazz_ [Column "clazz_" not found [42122-224]] [n/a]; SQL [n/a]
org.springframework.dao.InvalidDataAccessResourceUsageException: Unable to find column position by name: clazz_ [Column "clazz_" not found [42122-224]] [n/a]; SQL [n/a]
그리고 찾아보니, Hibernate 의 상속을 네이티브 쿼리가 이해하지 못해 발생한 것이었습니다.. Hibernate는 @Inheritance(strategy = InheritanceType.JOINED)
전략을 사용할 때, 부모 클래스와 자식 클래스 간의 상속 관계를 유지하기 위해 특정 컬럼 (clazz_ 등)을 자동으로 생성하고 관리합니다. 하지만, 네이티브 쿼리를 사용하면서 이러한 컬럼에 접근하려고 할 때 매핑 문제가 발생할 수 있다고 하네요. https://stackoverflow.com/questions/49804053/psqlexception-the-column-name-clazz-was-not-found-in-this-resultset
위 링크처럼 clazz 컬럼을 사용해서 native 쿼리로 쓸 수도 있지만, JPQL 로 작성하니 예외가 뜨지 않아 JPQL 을 사용하도록 바꿔주었습니다. 아마 JPQL 은 Hibernate의 상속 전략을 알잘딱으로 이해하나봅니다!
.collect(Collectors.toMap(Question::getId, Function.identity())); | ||
List<Answer> answers = answerRepository.findAllByQuestions(new ArrayList<>(questionMap.keySet())); | ||
|
||
Map<Question, List<Answer>> questionIdAnswers = questions.stream() |
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.
변수명에 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.
반영했습니다~ 🙆🏻♀️
} | ||
|
||
return answers.stream() | ||
.map(answer -> (TextAnswer) answer) |
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.
제안 .map(answer -> (TextAnswer) answer)
-> .map(TextAnswer.class::cast)
(mapToVoteResponse()
의 동일한 부분도)
} | ||
|
||
return answers.stream() | ||
.map(answer -> (TextAnswer) answer) |
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.
ClassCastException이 발생할 수 있지 않나요??
(mapToVoteResponse()
의 동일한 부분도)
public ReviewsGatheredBySectionResponse getReceivedReviewsBySectionId(String reviewRequestCode, long sectionId) { | ||
return null; | ||
List<Question> questions = questionRepository | ||
.findAllByReviewRequestCodeAndSectionId(reviewRequestCode, sectionId); |
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.
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.
추가하겠습니다😅
.findAllByReviewRequestCodeAndSectionId(reviewRequestCode, sectionId); | ||
Map<Long, Question> questionMap = questions.stream() | ||
.collect(Collectors.toMap(Question::getId, Function.identity())); | ||
List<Answer> answers = answerRepository.findAllByQuestions(new ArrayList<>(questionMap.keySet())); |
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.
그렇겠네요...😭 이 부분 수정하겠습니다!!
Map<Question, List<Answer>> questionAnswers = answerRepository.findAllByQuestionIds(questionIds) | ||
.stream() | ||
.collect(Collectors.groupingBy(answer -> questionIdQuestion.get(answer.getQuestionId()))); |
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
에서 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.
아 위에 테드가 이야기했네요
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 ReviewsGatheredBySectionResponse getReceivedReviewsBySectionId(String reviewRequestCode, long sectionId) { | ||
ReviewGroup reviewGroup = reviewGroupRepository.findByReviewRequestCode(reviewRequestCode) | ||
.orElseThrow(() -> new ReviewGroupNotFoundByReviewRequestCodeException(reviewRequestCode)); | ||
validateSectionId(sectionId, reviewGroup); |
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.
인자로 전달 받은 sectionId를 검증만 하고 그대로 사용하는 것 보단, 자원을 찾아서 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.
확실히 코드가 더 깔끔해졌네요 ㅎㅎ👍
@Query(value = """ | ||
SELECT q FROM Question q | ||
JOIN SectionQuestion sq ON q.id = sq.questionId | ||
JOIN TemplateSection ts ON sq.sectionId = ts.sectionId |
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.
TemplateSection join 필요 없을 것 같아요~
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.
와우 이걸 어떻게 봤어요!!! 감사합니다 🙇🏻♀️
// then | ||
assertThat(actual.reviews()).hasSize(1); | ||
assertThat(actual.reviews().get(0).question().name()).isEqualTo(question1.getContent()); | ||
assertThat(actual.reviews().get(0).answers()).extracting("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.
.extracting(TextResponse::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.
앗 하드코딩보다 그게 더 좋겠네요 ㅎㅎㅎ
} | ||
|
||
@Test | ||
void 질문에_응답이_없는_경우_질문_내용은_반환되_응답은_빈_배열로_반환한다() { |
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.
꼼장어 테드 고마워용🎣
import reviewme.template.repository.TemplateRepository; | ||
|
||
@ServiceTest | ||
class ReviewGatheredLookupServiceTest { |
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.
예외 테스트도 있으면 좋을 듯 합니다!
.findAllBySectionId(sectionId).stream() | ||
.collect(Collectors.toMap(Question::getId, Function.identity())); | ||
List<Answer> receivedAnswers = answerRepository.findReceivedAnswersByQuestionIds( | ||
reviewGroup.getId(), new ArrayList<>(questionIdQuestion.keySet())); |
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.
new ArrayList<>() 안하고 그냥 써도 되지 않나요?
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.
그럼 List<> 에는 Set<> 이 올 수 없다면서 오류 나요!
f322981
to
9ee6044
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.
산초~~~ 많이 그리고 꼼꼼히 못봐서 미안해요😂
question type별 추상화는 일단 rc하고 저도 고민해볼게요!!
|
||
public SectionNotFoundInTemplateException(long sectionId, long templateId) { | ||
super("섹션 정보를 찾을 수 없습니다."); | ||
log.info("Section is not found in template - sectionId: {}, templateId: {}", sectionId, templateId, this); |
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.
Section not found
가 문법상 맞을 것 같아요!
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.
be + p.p 가 맞지 않을까요? 🫨
예외 이름을 간단히 하기 위해서 예외에서는 NotFound 라고 붙였어요!
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.
우리 프로젝트에서 그렇게 하고 있었네요 ㅎㅎ
그럼 통일해주겠습니다!
Map<Long, Question> questionIdQuestion = questionRepository | ||
.findAllBySectionIdOrderByPosition(section.getId()).stream() | ||
.collect(Collectors.toMap(Question::getId, Function.identity())); | ||
List<Answer> receivedAnswers = answerRepository.findReceivedAnswersByQuestionIds( | ||
reviewGroup.getId(), new ArrayList<>(questionIdQuestion.keySet())); | ||
Map<Long, List<Answer>> questionIdAnswers = receivedAnswers.stream() | ||
.collect(Collectors.groupingBy(Answer::getQuestionId)); | ||
|
||
Map<Question, List<Answer>> questionAnswers = questionIdQuestion.entrySet() | ||
.stream() | ||
.collect(Collectors.toMap(Map.Entry::getValue, entry -> questionIdAnswers.getOrDefault(entry.getKey(), List.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.
테드가 말한 흐름과 비슷하게 이렇게 로직만 있어도 될 것 같아요!
- 섹션에 대한 모든 질문을 가져오기
- 리뷰 그룹에 해당하는 답변만 가져오기
- 두개 겹치는 걸로 매핑하기
(questionRepository 찌르는 건 캐싱할 거니까 괜찮다고 생각해서요~)
Map<Long, Question> questionIdQuestion = questionRepository | |
.findAllBySectionIdOrderByPosition(section.getId()).stream() | |
.collect(Collectors.toMap(Question::getId, Function.identity())); | |
List<Answer> receivedAnswers = answerRepository.findReceivedAnswersByQuestionIds( | |
reviewGroup.getId(), new ArrayList<>(questionIdQuestion.keySet())); | |
Map<Long, List<Answer>> questionIdAnswers = receivedAnswers.stream() | |
.collect(Collectors.groupingBy(Answer::getQuestionId)); | |
Map<Question, List<Answer>> questionAnswers = questionIdQuestion.entrySet() | |
.stream() | |
.collect(Collectors.toMap(Map.Entry::getValue, entry -> questionIdAnswers.getOrDefault(entry.getKey(), List.of()))); | |
List<Long> questionIdsBySection = questionRepository.findIdsBySectionIdOrderByPosition(section.getId()); | |
Map<Long, List<Answer>> answersByQuestionId = answerRepository.findByReviewGroupId(reviewGroup.getId()) | |
.stream() | |
.collect(Collectors.groupingBy(Answer::getQuestionId)); | |
Map<Question, List<Answer>> questionAnswers = questionIdsBySection.stream() | |
.collect(Collectors.toMap( | |
questionId -> questionRepository.findById(questionId).orElseThrow(), | |
questionId -> answersByQuestionId.getOrDefault(questionId, new ArrayList<>()) | |
)); |
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 ReviewGatherMapper { | ||
|
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 생기는 건 점점 무섭긴 하네요..
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.
진짜진짜어프로브
AnswerContentResponse -> TextResponse
- 인자가 List<Long> 이므로, ids 로 끝나게 변경함
AS-IS: 특정 질문에 대해서 답변된 것 다 받아오기 TO-BE: 내 리뷰 그룹에 해당한다는 조건 추가
AS-IS: boolean 으로 검증, sectionId 인자 그대로 사용 TO-BE: Optional<Section>으로 검증, section.getId() 사용
AS-IS: attribute 값을 문자열로 하드코딩 TO-BE: dtp 의 필드명으로 가져오기 e.g. VoteResponse::content
함수 분리, 변수명 변경, 함수 인자 변경
- 컨벤션 유지를 위함
- Long -> long
d8b511a
to
8a8ce13
Compare
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
시행착오가 조금 있습니다 ㅎㅎ
처음에는 한방쿼리로 데이터 가져오기 & 매핑까지 해보려 했습니다.
하지만 JOIN 이 너무 많다는 생각이 들었고, 더 효율적이라는 생각도 들지 않았습니다.
시행착오는 이 커밋에서 볼 수 있습니다.
아마 제가 한방쿼리를 작성하는 실력이 좋지 않아서 그랬던 걸 수도 있습니다😓
한방쿼리를 정말로 쿼리 하나만 하는게 아니라 몇가지로 나누어서 더 효율적으로 작성하는 방법도 있더라고요.
그런데 우선은 제가 익숙한 방법으로 해야 구현을 끝마칠 수 있을거란 생각이 들어서, 서비스 코드에서 매핑하는 방법으로 바꿨습니다.