-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: 응원톡 삭제, 갯수 카운팅 및 랜덤으로 뽑아오기 API 작성 #217
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.
페이지 랜덤 뽑기 아이디어 굿입니다 LGTM🌱
|
||
@Operation(summary = "응원글 개수를 카운팅합니다.") | ||
@GetMapping(value = "/counts") | ||
public RetrieveCommentCountResponse getCommentCounts(@PathVariable Long eventId) { |
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.
retrieve는 검색보다 회수의 의미가 더 강한거 같아용! 전에 개발사항들도 나중에 헷갈릴수도 있으니 한번 변경해놓으면 좋을거같아요
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.
해당 컨벤션에 대해 오늘 회의 때 말해봅시당
@Override | ||
public Comment queryRandomComment(Long eventId, Long countComment) { | ||
SecureRandom secureRandom = new SecureRandom(); | ||
int idx = (int) (secureRandom.nextFloat(1) * countComment); | ||
|
||
PageRequest pageRequest = PageRequest.of(idx, 1); | ||
List<Comment> comments = | ||
queryFactory | ||
.selectFrom(comment) | ||
.where(eventIdEq(eventId), comment.commentStatus.eq(CommentStatus.ACTIVE)) | ||
.offset(pageRequest.getOffset()) | ||
.limit(1) | ||
.fetch(); | ||
|
||
JPAQuery<Long> countQuery = | ||
queryFactory | ||
.select(comment.count()) | ||
.from(comment) | ||
.where(eventIdEq(eventId), comment.commentStatus.eq(CommentStatus.ACTIVE)); | ||
|
||
Page<Comment> commentOfPage = | ||
PageableExecutionUtils.getPage(comments, pageRequest, countQuery::fetchOne); | ||
|
||
if (commentOfPage.hasContent()) { | ||
return commentOfPage.getContent().get(0); | ||
} | ||
throw RetrieveRandomCommentNotFoundException.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.
페이지 랜덤 쿼리 참신한 아이디어 인것 같아요!🤭
마지막 exception 네이밍 관련해서는
특정 comment를 찾고자 한게 아니여서 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.
굿굿
랜덤으로 하나 뽑아오는것 같은데
우리 보통막 5개정도 뽑지 않았었남?
Long countComment = commentAdaptor.queryCommentCount(event.getId()); | ||
Comment comment = commentAdaptor.queryRandomComment(event.getId(), countComment); |
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.
commentAdaptor 로 묶어도?
좋을것같아요
commentAdaptor. queryRandomComment 에 countComment 인자를 없애도 될것같네유
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.
좋은것 같습니다
COMMENT_NOT_FOUND(NOT_FOUND, "Comment_404_1", "응원글을 찾을 수 없습니다."), | ||
@ExplainError(value = "eventId 경로변수와 commentId가 맞지 않을 때 발생하는 에러입니다.") | ||
COMMENT_NOT_MATCH_EVENT(BAD_REQUEST, "Comment_400_1", "응원글과 이벤트가 맞지 않습니다."), | ||
COMMENT_ALREADY_DELETE(BAD_REQUEST, "Comment_400_2", "이미 삭제된 응원글입니다."), | ||
RETRIEVE_RANDOM_COMMENT_NOT_FOUND(NOT_FOUND, "Comment_404_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.
예시컨트롤러에 CommentErrorCode 노출 부탁드릴게유! 에러코드!
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.
아맞다
PageRequest pageRequest = PageRequest.of(idx, 1); | ||
List<Comment> comments = | ||
queryFactory | ||
.selectFrom(comment) | ||
.where(eventIdEq(eventId), comment.commentStatus.eq(CommentStatus.ACTIVE)) | ||
.offset(pageRequest.getOffset()) | ||
.limit(1) | ||
.fetch(); | ||
|
||
JPAQuery<Long> countQuery = | ||
queryFactory | ||
.select(comment.count()) | ||
.from(comment) | ||
.where(eventIdEq(eventId), comment.commentStatus.eq(CommentStatus.ACTIVE)); | ||
|
||
Page<Comment> commentOfPage = | ||
PageableExecutionUtils.getPage(comments, pageRequest, countQuery::fetchOne); | ||
|
||
if (commentOfPage.hasContent()) { | ||
return commentOfPage.getContent().get(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.
랜덤하게 뽑는건 페이지네이션 쿼리가 필요할라나융
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.
난수로 접근할 때 리스트로 접근하면 id와 같은 인덱스로 접근해야하는데 삭제된(INACTIVE)경우에는 없는 경우가 발생할 수 있음 / 리스트로 orderBy 랜덤때려서 상위 접근하면 쿼리 성능 떨어짐 => 페이지로 쪼갠 뒤 접근하면 위와 같은 경우가 다 해소된다고 하더라구요
|
||
@DomainService | ||
@RequiredArgsConstructor | ||
public class CommentDomainService { | ||
|
||
public void deleteComment(Comment comment, Long eventId) { | ||
comment.checkEvent(eventId); | ||
comment.delete(); | ||
} |
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.
그래도 여긴 있으면 usecase에서 실수할일은 없을것가탕유
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.
추가하겠습니다
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
개요
작업사항
변경로직