feat(core): QnA CRUD 기능 개발 #18
Conversation
|
수고하셨습니다!! 코드 컨벤션 처음으로 맞춘다고 정말 고생 많으셨어요! 리뷰 시작하기 전에 결국 해낸 웅빈님을 칭찬합니다 ㅋㅋㅋㅋ |
| public void createQnA(CreateQnARequest request) { | ||
| qnACreator.save(request.toEntity()); | ||
| } | ||
|
|
||
| public void updateQnA(Long qnAId, CreateQnARequest request) { |
There was a problem hiding this comment.
저는 DTO는 presentation 레이어에서 사용하는 객체로 외부와 소통은 언제나 변할 수 있기 때문에 사용하는 객체라고 생각해요. 그래서 저는 DTO가 비즈니스 레이어에는 내려가면 안된다고 생각해요. 비즈니스 레이어에서는 도메인 객체를 사용하는 것이 맞다고 생각하는데 웅빈님의 생각은 어떠하신가요?
비즈니스 레이어와 DTO
There was a problem hiding this comment.
저는 DTO가 비지니스 레이어에 내려가는 것 보다 도메인을 프레젠테이션 레이어에 노출시키지 않는 것이 더 안정적이라 생각해 외부와 소통만 하는 DTO를 비지니스 레이어에 사용하고 presentation 영역에서는 도메인 객체를 호출하지 않도록 하는 게 좋은 것 같아서 저렇게 코드를 작성했어요.
There was a problem hiding this comment.
dto.toEntity()이렇게 작성하면 노출하지 않고, 비즈니스 레이어에도 내려가지 않게 할 수 있습니다!
| public List<QnAResponse> findAll() { | ||
| return qnAReader.readAll().stream() | ||
| .map(QnAResponse::from) | ||
| .toList(); | ||
| } | ||
|
|
||
| public List<QnAResponse> findAllByCategory(String category) { | ||
| return qnAReader.readAllByCategory(category).stream() | ||
| .map(QnAResponse::from) | ||
| .toList(); |
There was a problem hiding this comment.
여기도 ResponseDto를 참조하고 있네요. 그리고 같은 그리고 stream과 map이 여기 있으니 비즈니스의 흐름을 알기가 어려운 것 같아요. 비즈니스의 흐름이 잘 보이게 생각하고 개선한다면 Controller에서 List<도메인 클래스> 받은 것을 QnAResponse.of()로 변환하는 것이 좋을 것 같아요.
|
|
||
| public QnAResponse findOne(Long qnAId) { | ||
| QnA qna = qnAReader.read(qnAId); | ||
| return QnAResponse.from(qna); |
There was a problem hiding this comment.
매개 변수가 한 개일 때는 of보다는 from을 주로 사용하는 걸로 알고 있어요. 하지만 확장성을 고려해 of를 사용하는 것도 좋을 것 같아요 ~
정적 팩토리 메서드
There was a problem hiding this comment.
명명규칙에 대해서 자세히 몰랐는데 from이 더 적합한 것 같네요!
| public class QnACreator { | ||
|
|
||
| private final QnARepository qnARepository; | ||
|
|
||
| public void save(QnA qnA) { | ||
| qnARepository.save(qnA); |
There was a problem hiding this comment.
클래스 이름에 creator니 save 대신 create이 함수 명으로 더 적합해보여요!
| return queryQnAService.findOne(qnAId); | ||
| } | ||
|
|
||
| @ResponseStatus(HttpStatus.NO_CONTENT) |
| @RequestParam(name = "category", required = false, defaultValue = "ALL") String category) { | ||
| if (category.equals("ALL")) { | ||
| return queryQnAService.findAll(); | ||
| } | ||
| return queryQnAService.findAllByCategory(category); |
There was a problem hiding this comment.
이 부분에서 저는 카테고리 판단하는 것도 엄연한 비즈니스 흐름으로 생각하는데, Presentation 레이어에서 처리 되서 비즈니스 레이어에서는 흐름을 볼 수 없다는게 아쉽게 느껴져요. 웅빈님은 어떻게 생각하세요?
There was a problem hiding this comment.
전 하나의 비지니스 로직에서 한 종류의 쿼리만 실행되는 것이 책임 분할이 잘 된다고 생각했는데 창보님 말대로 비지니스 레이어에서 흐름을 볼 수 없다는 점이 있는 것 같네요!
| public QnA(String title, String content, Category category) { | ||
| this.title = title; | ||
| this.content = content; | ||
| this.category = category; | ||
| this.createTime = LocalDateTime.now(); |
| @PutMapping("/{qna-id}") | ||
| public void updateQnA( | ||
| @PathVariable(name = "qna-id") Long qnAId, | ||
| @RequestBody CreateQnARequest request | ||
| ) { | ||
| commandQnAService.updateQnA(qnAId, request); | ||
| } | ||
|
|
||
| @ResponseStatus(HttpStatus.NO_CONTENT) | ||
| @DeleteMapping("/{qna-id}") |
There was a problem hiding this comment.
이부분도 http 통신이라서 케밥 케이스를 사용하셨군요! 나중에 spring docs쓰거나 swagger 쓸 때 더 표준에 적합하게 보여서 좋을 것 같습니다~
| public void createQnA(QnA qnA) { | ||
| qnACreator.create(qnA); | ||
| } | ||
|
|
||
| public void updateQnA(Long qnAId, CreateQnARequest request) { | ||
| QnA qnA = qnAReader.read(qnAId); | ||
| qnAUpdater.update(qnA, request); | ||
| public void updateQnA(Long qnAId, QnA qnA) { | ||
| QnA updatableQnA = qnAReader.read(qnAId); | ||
| qnAUpdater.update(updatableQnA, qnA); |
| public List<QnA> findAllByCategory(String category) { | ||
| if (category.equals("ALL")) { | ||
| return qnAReader.readAll(); | ||
| } | ||
| return qnAReader.readAllByCategory(category); |
🎫 관련 이슈
close #15
📄 개요
🔨 작업 내용
🏁 확인 사항
🙋🏻 덧붙일 말