[Code Review] 누찬 code review #16
Conversation
…ervice, DTO, Converter 작성
There was a problem hiding this comment.
고생 많으셨습니다. 질문, 리뷰받고 싶은 부분들에 대해 내용 남겨달라고 요청했는데 없다보니
리뷰 포인트에 대해 알맞지 않을 수 있어도 이해바랍니다.
간단히 총평을 남기면,
레이어드 아키텍처, 객체지향 5원칙, 패키징, 네이밍, 설계에 대해 더 학습하시면 좋을 것 같습니다.
학습하면서 도움이 될만한 질문을 드리자면
_ 이 구조는 변경에 유연한 구조인가?
- 레이어드 아키텍처는 어떠한 걸까? 내 아키는 본래의 레이어드의 의미에 상응할까?
- 내가 작성한 클래스, 인스턴스는 객체지향에 알맞는가?
- 적절한 책임을 부여하고 분리가 잘 이뤄졌는가?
- 내가 작성한 네이밍은 다른사람들이 보기에도 이해가 쉬운 네이밍인가
- 이 패키지는 적절한 책임과 분리를 가졌는지, 적절한 위치에 계층을 가졌는지 파일들의 위치가 알맞을까?
이런 고민들 해보면 좋을 것 같습니다.
수고하셨습니다!
| mavenCentral() | ||
| } | ||
|
|
||
| dependencies { |
There was a problem hiding this comment.
개인적인 선호이지만, 의존성에 대해 어떤 의존성인지 작성해서 관리하는 것을 선호합니다.
| @Getter | ||
| @Builder | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @AllArgsConstructor |
There was a problem hiding this comment.
AllArgsConstructor의 액세스 레벨은 설정하지 이유가 있을까요?
| @JoinColumn(name = "user_id") | ||
| private Users user; | ||
|
|
||
| private Integer Rating; |
There was a problem hiding this comment.
변수명이 대문자로 시작하게 작성한게 의도일까요? 아니면 수정바랍니다
| @Builder | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @AllArgsConstructor | ||
| public class ReviewAnswers { |
There was a problem hiding this comment.
개인적인 궁금증을 더 적어보자면, ReviewAnswer라는 변수명이 무슨 의미를 뜻하는지 상상이 안가네요. 적절한 네이밍이었을지도 고민해보면 좋을 것 같습니다
| } | ||
|
|
||
| // 이미 도전 중인지 확인 | ||
| boolean exists = progressingMissionRepository.findByUserIdAndMissionId(dto.getUserId(), dto.getMissionId()).isPresent(); |
There was a problem hiding this comment.
boolean 타입에 대한 변수명인데, 괜찮은 변수명인지 고민해보면 좋을 것 같아요
| )) | ||
| .toList(); | ||
|
|
||
| return MarketResponseDto.MissionListResponseDto.builder() |
There was a problem hiding this comment.
어떤 거는 converter를 활용하고 어떤거는 사용 안하는지 기준이 있을까요�?
| private final UsersRepository usersRepository; | ||
| private final MissionRepository missionRepository; | ||
| private final ProgressingMissionRepository progressingMissionRepository; | ||
| private final ProgressingMissionConverter progressingMissionConverter; | ||
|
|
There was a problem hiding this comment.
presentation layer가 persistence layer에 접근하는 행위는 지름길 패턴? 같은 것을 활용하려는 걸까요?
기존의 보이던 코딩 패턴과는 다른 모습이네요
| return ResponseEntity.ok(responseDto); | ||
|
|
||
| @Transactional | ||
| public ProgressingMissionResponseDto.MissionPreviewDto addProgressingMission(ProgressingMissionRegisterDto dto) { |
There was a problem hiding this comment.
controller 메소드에 @transactional이 붙은 이유가 있을까요?
| @@ -46,4 +47,11 @@ public ProgressingMissionResponseDto.MissionPreviewDto addProgressingMission(Pro | |||
| .build(); | |||
| } | |||
|
|
|||
| @PatchMapping("/{progressingMissionId}/complete") | |||
There was a problem hiding this comment.
URL에 카멜케이스는 처음 보네요. URL 네이밍 가이드에 대해 알아보면 좋을 것 같아요. SEO와 관련이 많답니다.
No description provided.