-
Notifications
You must be signed in to change notification settings - Fork 1
[FEAT] 중분류 폴더 전체 목록 조회 API #27
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
Conversation
unanchoi
left a comment
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.
Approve 했습니다.
Comment 한 부분만 확인 부탁드령ㅅ!
| @Getter | ||
| @RequiredArgsConstructor(access = AccessLevel.PRIVATE) | ||
| @AllArgsConstructor(access = AccessLevel.PRIVATE) | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) |
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.
모르고 있던 Annotation을 알아가네요! 혹시 중분류 폴더 전체 목록 조회에 조회가 안될 경우에 대해서 data column이 없이 조회 되는 것으로 결정이 된 걸 까요?
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.
조회가 안될때라기 보다는 현재 명세서를 보면 성공 케이스에서 data를 response body에 담아서 내려줄 필요가 없을 때 모두 statusCode, success, message만 담아서 내려주도록 되어있어서요 ! data를 담지 않는 경우에 ApiResponseDto 객체를 생성할 때
public static ApiResponseDto success(SuccessStatus successStatus) {
return new ApiResponseDto<>(successStatus.getHttpStatus().value(), true, successStatus.getMessage());
}위의 메소드를 사용해서 객체를 만들 때 data를 안넣어주니까 json 형식으로 바뀌면 data:null 이라는 불필요한 프로퍼티 값이 들어가는 것을 깔끔하게 없애는게 좋을 것 같아서 사용했습니다 !
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 FolderService folderService; | ||
|
|
||
| @GetMapping() | ||
| @ResponseStatus(HttpStatus.OK) |
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.
p4;
@ResponseStatus의 경우 성공 case에 대한 응답 code를 결정해주는 것으로 알고 있는데 , 이미 SuccessStatus에서 요청에 대한 응답 code를 명시하고 있으니 사용하지 않는 방향은 어떨까요?
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.
제가 알기로는 SuccessStatus에서 응답 코드를 명시하는 것은 HttpBody를 저희가 response body를 만들어서 json으로 내려드릴 때 드리는 code 필드를 명시하는 것인데, HttpBody말고 HttpStatus에도 응답 코드가 들어있어서 둘을 통일시켜드리는게 클라분들에게 혼돈을 덜 드린다고 생각해서 사용했습니다 !
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.
저희가 HttpStatus에 대해서, 생성되는 경우 201 CREATED를 제외하고는 Status OK로 했기 때문에 201인 경우를 제외하고는 불필요하다고 생각했는데, 그러면 그 방향으로 모든 API 작성하겠습니다
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.
@ResponseStatus는 이 HttpStatus의 응답 코드를 변경해주는 것이니 함께 사용하면 일관성을 지킬 수 있지 않을까 하구요 !
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.
좋습니다
| public interface FolderService { | ||
|
|
||
| //* 중분류 폴더 전체 목록 조회 | ||
| List<FolderResponseDto> getAllFolder(Long memberId); | ||
| } |
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.
p5;
FolderService interface를 만들고, 구현체에서 로직을 작성하시는 이유가 있을까요?
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.
역할과 구현을 분리하는 것이 좋을 것 같아서 인터페이스로 역할의 틀을 잡아주고, 이후 구현은 해당 인터페이스를 구현하는 구현체로 나눴습니다. 추후 해당 서비스의 로직이 변경되었을 때 구현체만 수정하도록 하여 유연하고 변경에 용이하도록 구현하고자 했습니다 ! 객체지향의 SOLID 원칙 중 OCP를 생각했다고 정리할 수 있을 것 같아요 ! 😀
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 List<FolderResponseDto> getAllFolder(Long memberId) { | ||
| return categoryRepository.findByMemberId(memberId).stream() | ||
| .flatMap(category -> folderRepository.findByCategoryId(category.getId()).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.
p5;
map을 사용안하시고 flatMap을 사용하신 이유가 궁금합니다!
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.
이건 똑순이 인텔리제이가 간결하게 정리 해주긴 했습니다..ㅎㅎㅎ
Seokyeong237
left a comment
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 Long id; | ||
| private String name; | ||
|
|
||
| public static FolderResponseDto of(Long id, String name) { | ||
| return new FolderResponseDto(id, name); | ||
| } | ||
| } |
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.
p4;
대분류 카테고리 전체 목록 조회에서는 categoryId로 해두셨던데 두 API 모두 id 나 folderId로 통일시켜주는 방향은 어떨까요?
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.
넵 ! folderId로 변수명 수정해서 통일 시키겠습니다 !
📝 Summary
👩💻 Contents
API 개발 : 해당 유저의 모든 대분류에 있는 중분류들을 전체 조회합니다.
ApiResponseDto 수정 : json으로 내려주는 프로퍼티에서 명세서와 다른 부분 수정했습니다.
📝 Review Note
📣 Related Issue
📬 Reference