-
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
[FEAT] 질문 새로고침 구현 #144
[FEAT] 질문 새로고침 구현 #144
Conversation
@@ -198,11 +198,8 @@ protected ResponseEntity<ApiResponse> handleCustomException(CustomException e, f | |||
|
|||
if (e.getHttpStatus() == 501) { | |||
notificationService.sendExceptionToSlack(e, request); | |||
return ResponseEntity.status(e.getHttpStatus()) |
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.
501 코드를 반환하는 경우가 추가되어서, 인위적으로 ErrorType을 리턴하지 않도록 변경했습니다.
- 질문 DB가 고갈됐을때
- 가까워지기 질문 DB가 고갈됐을때
- 기록하기에서 일정 수 이상으로 사진을 업로드했을 때
|
||
public static TodayCloserAnswerRequestDto of (int answer) { | ||
return new TodayCloserAnswerRequestDto(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.
Request DTO에 불필요한 생성자가 존재하는 것을 정리했습니다.
@@ -33,8 +34,8 @@ public ApiResponse<TodayQnAResponseDto> getTodayQna(Principal principal) { | |||
@PostMapping("/qna/answer") | |||
@ResponseStatus(HttpStatus.CREATED) | |||
public ApiResponse answerTodayQuestion( | |||
Principal principal, |
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.
엇 저도 똑같은데,,, 뭔가 그 전에 잘못 설정되어 있던것 같아요!
제가 최근에 8로 되어있던걸 4로 바꿨어가지구, 지금 바뀌고 있는게 제대로 되고 있는 것 같습니다!
@GetMapping("/reroll/check") | ||
@ResponseStatus(HttpStatus.OK) | ||
public ApiResponse<RerollCheckResponseDto> rerollCheck(Principal principal) { | ||
return ApiResponse.success(SuccessType.GET_REROLL_CHECK_SUCCESS, qnAService.rerollCheck(getUserFromPrincial(principal))); | ||
} | ||
|
||
@PatchMapping("/reroll/change") | ||
@ResponseStatus(HttpStatus.OK) | ||
public ApiResponse rerollChange(Principal principal, @RequestBody RerollChangeRequestDto request) { | ||
qnAService.rerollChange(getUserFromPrincial(principal), request.getQuestionId()); | ||
return ApiResponse.success(SuccessType.REROLL_CHANGE_SUCCESS); | ||
} |
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.
해당 2가지 API가 추가되었습니다.
명세서 참고하셔서 봐주시면 좋을 것 같아요!
@@ -71,7 +71,7 @@ public TodayCloserQnAResponseDto getTodayCloserQnA(Long userId) { | |||
@Transactional | |||
public void addFirstCloserQnA(Parentchild parentchild) { | |||
CloserQuestion firstCloserQuestion = closerQuestionRepository.findRandomExceptIds(new ArrayList<>()) | |||
.orElseThrow(() -> new CustomException(ErrorType.NOT_FOUND_CLOSER_QUESTION)); | |||
.orElseThrow(() -> new CustomException(ErrorType.NO_MORE_CLOSER_QUESTION)); |
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.
가까워지기 질문이 고갈되었을 때도 501 에러를 리턴하도록 변경했습니다.
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.
꽤나 복잡하게 구현이었을텐데 깔끔하게 잘 구현해주신 덕에 이해가 쏙쏙 갔습니다 !! 수고하셨습니다 .. 👍
@@ -33,8 +34,8 @@ public ApiResponse<TodayQnAResponseDto> getTodayQna(Principal principal) { | |||
@PostMapping("/qna/answer") | |||
@ResponseStatus(HttpStatus.CREATED) | |||
public ApiResponse answerTodayQuestion( | |||
Principal principal, |
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.
@Column(name = "question_id", nullable = false) | ||
@ElementCollection | ||
private List<Long> questionBlackList; | ||
|
||
public void addQuestionBlackList(Long questionId) { | ||
questionBlackList.add(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.
ID 리스트로 관리하는 게 Question 객체 리스트로 유지하는 방식보다 훨씬 좋은 것 같네요 !!
private LocalDateTime lastRerollChange = LocalDateTime.of(2024, 3, 1, 12, 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.
해당 필드는 24시간에 한번씩만 새로고침을 할 수 있도록 추가한 "가장 마지막 새로고침 시간" 필드인데요,
기존에 해당 필드가 존재하지 않았었고 추가된 필드이기 때문에, 첫번째 새로고침은 무조건 가능하도록 값이 설정되어야 합니다!
따라서 임의로 2024/03/01 날짜로 초기화해줌으로써, 첫번째 새로고침이 무조건 가능하도록 해놓았습니다...!
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.
비즈니스 로직을 확인해보니, 초기화 없는 방식이 더 깔끔할 것 같아 수정완료했습니다!
.filter(question -> question.getSection().equals(section)) | ||
.collect(Collectors.toList()); | ||
|
||
Random random = new Random(); |
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.
NativeQuery로 rand()를 사용하면 쿼리로 한번에 가져올 수 있어 비즈니스 로직의 복잡성이 줄 것 같습니다!
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를 가져옴 -> 비즈니스 로직 상에서 stream으로 돌며 필터링 진행 -> 랜덤하게 추출
순으로 진행을 하고 있다보니, 역시 랜덤 호출을 비즈니스 로직 상에서 진행을 하게 되었는데요..!
아래 리뷰와 관련한 리팩토링을 진행할때 NativeQuery로 rand()까지 사용해보면 좋을 것 같습니다!!
if (!equalSectionQuestions.isEmpty()) { | ||
// 3. 최근에 주고받은 질문의 section과 같은 질문들 중에서 랜덤하게 추출 | ||
randomQuestion = equalSectionQuestions.get(random.nextInt(equalSectionQuestions.size())); | ||
} else { | ||
// 4. 없다면 다른 section의 질문 중에서라도 랜덤하게 추출 | ||
List<Question> differentSectionQuestions = targetQuestions.stream() | ||
.filter(question -> !question.getSection().equals(section)) | ||
.collect(Collectors.toList()); | ||
randomQuestion = differentSectionQuestions.get(random.nextInt(differentSectionQuestions.size())); | ||
} | ||
|
||
return RerollCheckResponseDto.of(randomQuestion); | ||
} |
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.
전반적으로 Java stream api를 사용하신 것 같은데 추후 리팩토링 시 쿼리로 최대한 필터링해서 가져오는 방식으로 바꿔보면 좋을 것 같아요 !! (저희 프로젝트 상에는 대부분 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.
와아,, 아주 좋은 레퍼런스네요!
그러게요 현재는 질문 DB가 크지 않아서 성능상 큰 문제는 없겠지만,
쿼리적으로 더 잘 해결한다면 추후 질문 DB가 늘어났을 때 크게 체감될 것 같습니다!
지금 코드들이 전체적으로 정돈이 안된 부분도 엄청 많아서 대대적인 리팩토링 꼭 해야할 것 같아요...!
📌 관련 이슈
closed #123
✨ 어떤 이유로 변경된 내용인지
🙏 검토 혹은 리뷰어에게 남기고 싶은 말