-
Notifications
You must be signed in to change notification settings - Fork 1
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
[BSVR-118] 블록별 리뷰 조회 API 구현 #23
Conversation
VALUES (1, '잠실 야구 경기장', 'main_image_a.jpg', 'seating_chart_a.jpg', 'labeled_seating_chart_a.jpg', true), | ||
(2, '김포 야구 경기장', 'main_image_b.jpg', 'seating_chart_b.jpg', 'labeled_seating_chart_b.jpg', true), | ||
(3, '부산 야구 경기장', 'main_image_c.jpg', 'seating_chart_c.jpg', 'labeled_seating_chart_c.jpg', false); | ||
VALUES (1, '잠실 야구 경기장', 'main_image_a.jpg', 'seating_chart_a.jpg', 'labeled_seating_chart_a.jpg', 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.
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.
오잉 뭐지 나는 로컬에서 stadiums 잘 Insert 되던데..?! 다음 티켓 작업할때 다시 확인해볼게!
record KeywordCountResponse(String content, Long count) {} | ||
|
||
record FilterInfo(Long rowId, Integer seatNumber) {} | ||
} |
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<ReviewImage> images; | ||
private final List<ReviewKeyword> keywords; |
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.
도메인 상에서 image와 keyword는 배열로 관리해줄 수 있도록 필드를 추가했어
if (reviews.isEmpty()) { | ||
throw new ReviewException.ReviewNotFoundException( | ||
"No review found for blockId:" + blockId); | ||
} |
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.
조회가 하나도 안되는 경우: 404 예외처리
if (review == null) { | ||
throw new InvalidReviewDataException( | ||
"Failed to convert entity to domain for reviewId: " + reviewEntity.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.
변환이 안될 때 -> 데이터가 잘못들어왔을 시 400 bad 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.
LGTM! 고생했어~ 테스트...빡셌겠는걸.....? 보기만해도 어지럽구만 😂
몇 가지 코멘트 남겨두었어! 확인해주어~~
(++ 지금 브랜치 main이랑 conflict나는데 이것도 확인해줘!)
@RequestParam(defaultValue = "0") | ||
@PositiveOrZero | ||
@Parameter(description = "시작 위치 (기본값: 0)") | ||
int offset, |
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.
요건 단순 질문인데, offset = null로 들어오면
- defaultValue가 0으로 세팅되는거야
- @PositiveOrZero에 걸려서 exception이 발생하는거야 ?.?
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로 들어오면 0으로 세팅돼!
- positiveOrZero로 0이상의 값만 받도록 -> 음수면 exception발생!
- 요청에 null로 들어오면 디폴트 0으로 세팅
@PositiveOrZero @Parameter(description = "시작 위치 (기본값: 0)") int offset, | ||
@Max(50) @Parameter(description = "조회할 리뷰 수 (기본값: 10, 최대: 50)") int limit) { |
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.
단순 질문! Pageable을 안쓰고 offset, limit parameter를 이용한건 validation 때문이야?
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.
진짜.. 바보인가봐 제대로 알아보지도 않고 하드코딩 했네 ㅠㅠㅠㅠ 수정해야겠다
@PositiveOrZero @Parameter(description = "시작 위치 (기본값: 0)") int offset, | ||
@Max(50) @Parameter(description = "조회할 리뷰 수 (기본값: 10, 최대: 50)") int limit) { | ||
public ReviewBlockRequest { | ||
if (offset == 0) offset = 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.
요 라인은 불필요한 것 같다!
.collect(Collectors.toList()); | ||
|
||
boolean hasMore = (result.offset() + result.reviews().size()) < result.totalCount(); | ||
FilterInfo filter = new FilterInfo(null, null); // Assuming no filter info for now |
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 값이 들어가는 것 같은데, 이번 티켓에 포함된 이유가 있을까?!
어떤 역할을 하는 친구인지 궁금해요
.map(kc -> new KeywordCountResponse(kc.content(), kc.count())) | ||
.collect(Collectors.toList()); | ||
|
||
boolean hasMore = (result.offset() + result.reviews().size()) < result.totalCount(); |
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.
JPA에서 제공하는 Page에 hasNext()가 있는데, 혹시 hasMore을 직접 계산한 이유가 있을까?
List<ReviewImage> images = | ||
reviewCustomRepository.findImagesByReviewId(reviewEntity.getId()).stream() | ||
.map(ReviewImageEntity::toDomain) | ||
.collect(Collectors.toList()); | ||
|
||
List<ReviewKeyword> keywords = | ||
reviewCustomRepository.findKeywordsByReviewId(reviewEntity.getId()).stream() | ||
.map(ReviewKeywordEntity::toDomain) | ||
.collect(Collectors.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.
사~~~소한 의견인데 메서드를 분리하면 어떨까 싶당! 이번 리뷰에선 반영 안해도 괜찮아!
Review review = reviewEntity.toDomain(); | ||
if (review == null) { | ||
throw new InvalidReviewDataException( | ||
"Failed to convert entity to domain for reviewId: " + reviewEntity.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.
ReviewEntity.toDomain()에서 null이 반환될 가능성이 있을까?!
만약 ReviewEntity가 null인 상황에 대한 방어코드라면, null.toDomain()이라 NPE가 발생할 수 있을 것 같아!
toDomain()이 호출되기 전에, 또는 toDomain() 내부에서 ReviewEntity가 null인지 체크하는게 더 좋지 않을까?
throw new InvalidReviewDataException( | ||
"Failed to convert entity to domain for reviewId: " + reviewEntity.getId()); | ||
} | ||
return Review.builder() |
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.
ReviewEntity 내부에 새로운 정적 팩토리 메서드를 추가하는건 어때?
ReviewEntity.toDomain(List<ReviewImage> images, List<ReviewKeyword> keywords)
위에서(61번째 라인) Review를 만들었는데 아래에서(66번째 라인) 또 새롭게 만들어줄 필요는 없을 것 같아!
List<KeywordCount> topKeywords = | ||
reviewRepository.findTopKeywordsByBlockId(stadiumId, blockId, 5); |
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.
5 << 는 상위 5개 키워드를 의도한거지?
상수로 뽑아주면 가독성 & 유지관리가 더 쉬울 것 같아!
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.
요거 유스케이스 반환형으로 사용되는 친구인거지?
도메인은 정말 우리 서비스의 비즈니스 영역을 추상적으로 나타낸 것이고, ReviewListResult는 단순히 API response로 매핑될 값이라 조금 역할이 다른 것 같아!
새로운 도메인을 만들지 않고 구현하는 방법으로는 당장은 요렇게 두 가지 정도가 생각나네 👀
- controller - usecase 사이에 facade 와 같은 계층을 추가하기
- ReviewFacade 등을 만들어서, ReviewUsecase, ReviewKeywordUsecase 등등을 호출하고 조합해서 controller로 반환
- usecase 반환용의 dto를 만들기
나는 일단 작업량이 너무 늘어나는 것 같고 + 비즈니스 로직이 분산되는 것 같아서 후자로 작업했어!
📌 개요 (필수)
🔨 작업 사항 (필수)
0.리뷰쪽 domain에 빌더 적용, entity에는 base entity 적용
⚡️ 관심 리뷰 (선택)
🌱 연관 내용 (선택)
💻 실행 화면 (필수)
404
200
테스트 결과