Feature/#94 사용자 제출 관련 api 구현 #97
Conversation
| return ResponseEntity.ok(clientGetService.getCommentary(publishId, problemId)); | ||
| } | ||
|
|
||
| @GetMapping("allProblem/{year}/{month}") |
There was a problem hiding this comment.
problems/all/ 이런 방식이 괜찮을 것 같아요
URL을 디렉토리 구조라고 생각했을때 문제 도메인에서 전체문제를 찾는 위치를 들어가는 느낌이 괜찮지않나 싶네요
There was a problem hiding this comment.
좋은것 같습니다! 도메인을 타고 들어간다면 problem/all 은 어떨까요?
| @RestController | ||
| @RequestMapping("/api/v1/client") | ||
| @RequiredArgsConstructor | ||
| public class ClientGetController { |
There was a problem hiding this comment.
Client 이름이 너무 방대한 것 같아요
Client 안에서도 쪼개는게 좋을 것 같아요.
해설, 문제 이런것들도 client안에서 각 controller로 나누는게 좋은 것 같습니다
나중에 멀티모듈로 쪼개면서 각 모듈에서는 이름이 겹쳐도 되니까 쪼개면서 나눠봐요
There was a problem hiding this comment.
좋은 것 같아요.
일단 문제랑 해설 2개로 분리해놓겠습니다!
| return isCorrect ? RETRY_CORRECT : INCORRECT; | ||
| } else if (currentStatus == IN_PROGRESS) { | ||
| return isCorrect ? CORRECT : INCORRECT; | ||
| } else { |
There was a problem hiding this comment.
간단한 리뷰인데요! Reurn으로 내보낼거라면 else if 안쓰고
if (currentStatus == CORRECT) {
return isCorrect ? CORRECT : INCORRECT;
}
if (currentStatus == INCORRECT) {
return isCorrect ? RETRY_CORRECT : INCORRECT;
}
if (currentStatus == IN_PROGRESS) {
return isCorrect ? CORRECT : INCORRECT;
}
return Comment;처럼 하고 틀렸는지 맞았는지 로직도 메서드 추출하면 가독서잉 좀더 좋아질것 같아요
아니면 switch문도 괜찮은것 같습니다
There was a problem hiding this comment.
그렇네요! switch문으로 변경하겠습니다.
| problem.getMainProblemImageUrl(), | ||
| problem.getPrescriptionImageUrls(), | ||
| problemSubmit.getStatus() | ||
| ); |
There was a problem hiding this comment.
파라미터 개수가 많아져서 도메인을 넘겨서 Dto로 바꿔주는 mapper로직을 DTO안에 구성하면 좋을 것 같아요
| cp.getImageUrl(), | ||
| cp.getPrescriptionImageUrls(), | ||
| childProblemSubmitRepository.findByMemberIdAndPublishIdAndChildProblemIdElseThrow(memberId, publishId, | ||
| cp.getId()).getStatus() |
There was a problem hiding this comment.
이 친구도 mapper 로직이 DTO로 들어가면 좋겠네용
| problem.getMainHandwritingExplanationImageUrl(), | ||
| problem.getReadingTipImageUrl(), | ||
| problem.getSeniorTipImageUrl(), | ||
| prescription |
| public List<AllProblemGetResponse> getAllProblem(int year, int month) { | ||
| Long memberId = 1L; | ||
|
|
||
| if (month < 1 || month > 12) { |
There was a problem hiding this comment.
상수로 처리하는 것도 괜찮지만 달 계산하는 로직은 많이 쓴다면 util처럼 감싸두 되구용
| List<ProblemSubmit> submissions = problemSubmitRepository.findByMemberIdAndPublishId(memberId, publishId); | ||
| List<ProblemSubmitStatus> problemStatuses = submissions.stream() | ||
| .map(ProblemSubmit::getStatus) | ||
| .toList(); |
There was a problem hiding this comment.
요새 저의 코드도 그렇고 특정 조건으로 도메인을 조회하는 repository가 여러 service레이어에서 사용되면서 의존성이 점점 퍼지고 얽혀서 나중에 분리하기가 어려워질 것 같다는 생각이 드는데요.
특정 도메인 조회하는 서비스레이어를 거쳐서 가져오는것도 괜찮다는 생각이 드는 요즘입니다.
There was a problem hiding this comment.
좋습니다. 특히 조회 로직 작성할 때 의존성이 점점 불어나더라고요. 리팩토링 단계에서 같이 이야기해봐요!
💡 Issue
📄 Description
💬 To Reviewers