-
Notifications
You must be signed in to change notification settings - Fork 8
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] 약속 입장 조회 로직 추가 #390
[BE] 약속 입장 조회 로직 추가 #390
Conversation
Test Results156 tests 156 ✅ 20s ⏱️ Results for commit 8e21ef9. ♻️ 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.
PR 본문 처럼 실제 성능 차이가 크지 않을 것 같고 유지보수가 더 쉽고 단순한 uuid 기반 조회라면 QueryMethod로도 충분해보여요. 나중에 성능 상 문제가 생겼을 때 @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.
다온, 빠르게 처리하셨네요👍
제 생각도 마크와 동일하게, queryMethod만으로도 현재 충분하다고 생각합니다!
이후에 성능의 문제가 생긴다면 수정하는 것도 좋을 것 같아요~
@@ -73,6 +74,12 @@ public MomoApiResponse<ConfirmedMeetingResponse> findConfirmedMeeting(@PathVaria | |||
return new MomoApiResponse<>(response); | |||
} | |||
|
|||
@GetMapping("/api/v1/meetings/{uuid}/home") | |||
public MomoApiResponse<MeetingHomeResponse> findMeetingEntry(@PathVariable String uuid) { |
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.
findMeetingEntry
라는 메서드명보다 findMeetingHome
이나, 가져오는 필드 값을 명시해주는 건 어떨까요?
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.
다온 고생하셨습니다!!
저는 1번 근거가 가장 와닿는 것 같습니다. 이런 메서드가 점점 많아지면 코드를 처음 보는 개발자는 일일히 메서드를 다 확인해야할 것 같아요.
); | ||
} | ||
|
||
@DisplayName("약속 입장 정보를 조회한다.") |
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.
DisplayName
만 봐서는 정상적인 상황같이 느껴지는 것 같아요 😅
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.
안녕하세요 다온~ 코드 잘 읽어 봤습니다!
잘 구현해 주셨는데 API 문서화를 위해 사용하기로 했던 Controller 인터페이스에는 새로 만든 API에 대한 문서화가 진행되어 있지 않은 것 같아요. 추가해주시면 감사하겠습니다~
본문에 남겨주신 질문의 경우에는 현재 성능 문제가 발생하지 않은 상태이므로 굳이 커스텀 쿼리를 추가해 가며 최적화를 진행할 이유가 없다고 생각해요. 커버링 인덱스를 사용할 수 있는 상황이라면 모르겠지만, meetingName
을 가져와야 하므로 커버링 인덱스도 사용하지 못하는 쿼리이구요. JPA를 도입한 이상 기본 메서드들을 최대한 활용해야 영속성 컨텍스트의 이점을 누릴 수 있을 것 같아요(비록 지금 구현에서는 활용하지 않는다고 하더라도요🙂)
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.
고생하셨습니다🚀
5fb3b1f
to
8e21ef9
Compare
관련 이슈
작업 내용
이슈에서 언급한 약속 입장 페이지에 대한 조회 API를 구현하였습니다.
반환 값들은 아래와 같습니다.
응답 결과
리뷰 요구사항
논의가 필요한 부분: 코드 리뷰 중 논의가 필요해 보이는 부분은 무엇인가요?
두 개의 컬럼만 조회하는 로직에 대해 persistence 계층에
@Query
를 추가할 필요가 있을까요? 현재 JPA에서 제공해주는 queryMethod를 재사용할 목적으로findByUuid()
메서드를 사용하는데요. 아래 근거들이 타당할지 궁금하네요 :)