[Spring Core] 김태우 8, 9, 10단계 미션 제출합니다. #535
Conversation
tae-wooo
force-pushed
the
roomescape-taewoo3
branch
from
2e1aa32 to
397a863
Compare
70825
left a comment
70825
left a comment
There was a problem hiding this comment.
안녕하세요 태우님~ 저도 반갑습니다 🙇♂️
태우님의 요구사항대로 ㅋㅋ 어떤 내용을 리뷰해야 기초적인 부분에도 도움이 되고, 앞으로도 도움이될만한 내용이 없을까 고민해봤는데, 코드 보다가 조건에 부합하는게 하나 있더라구요
아래 코멘트들처럼 코드를 지정해 리뷰할 수도 있겠지만, 전반적으로 크게 패키지 구조 살펴보면서 수정하시는게 좋아보여서 여기에 리뷰 남길게요
Layered Architecture
next-step 미션에 있는 Layered Architecture 사진인데요
- Web Layer와 Service Layer는 DTO로 데이터를 주고 받는다
- Service Layer와 Repository Layer는 Domain Model로 데이터를 주고 받는다
라는 규칙이 있는걸 보실 수 있습니다.
그런데 현재 태우님의 코드는...
- Web Layer 혼자 DTO를 사용한다
- Web Layer와 Service Layer는 Domain Model로 데이터를 주고 받는다
- Service Layer와 Repository Layer는 Domain Model로 데이터를 주고 받는다
위 규칙으로 되어 있어서 그림대로 만드려면 어떻게 코드를 수정해야할까 고민해보고 적용해보시면 될 것 같습니다 😁
이런건 어떻게 쉽게 찾을 수 있을까?에 대한 고민도 드실 수 있는데요
토스 SLASH 22 - 지속 성장 가능한 코드를 만들어가는 방법 요거 영상 추천드립니다 (11분 32초까지 보시면 됩니다)
저는 우테코에서 미션하는동안 이 영상을 보기 전까지는 import문을 쳐다도 안봤었는데, 이거 본 이후로는 한 번씩 살펴보게 되더라구요
import문만으로도 우리가 코딩할 때 만든 패키지가 올바르게 만들어졌는지, 응집도는 높은 설계인지 큰 시간을 들이지 않고 파악할 수 있어서 좋은 방법이라고 생각해요
참고로 위 영상에서 패키지 구조는 현재 태우님이 미션 진행하는 패키지 구조와 조금 다른데요. 계층형 vs 도메인형 글 읽어보시면 도움이 될 것 같아요
| compileOnly 'org.projectlombok:lombok' | ||
|
|
There was a problem hiding this comment.
스터디 자료에서 처음 보는 어노테이션을 발견해 찾아보니 롬복이라는 라이브러리였습니다.
롬복을 사용하면 Getter나 생성자처럼 반복적으로 작성해야 하는 코드를 줄일 수 있어서,
전체 코드의 가독성과 유지보수성이 높아진다고 판단해 사용하게 되었습니다.
There was a problem hiding this comment.
처음 사용해보는 개념이라 궁금한 점이 있습니다!
실제 프로젝트에서도 롬복을 많이 사용하는지 궁금합니다.
만약 사용한다면, 어떤 애노테이션을 주로 사용하는지도 알고 싶습니다.
There was a problem hiding this comment.
음.. 저도 자바를 썼을 때 사용하긴 했는데, 우테코 팀프로젝트에서는 사용하지 않기로 팀원들이랑 합의 했었어요
이유는 어차피 인텔리제이 단축키로도 빠르게 코드 생성 할 수 있고, 이미 다들 써봤기 때문에 안써도 문제 없어가지구요
- Mac 기준 단축키: Cmd + N > Constructor, getter
- 윈도우 기준 단축키: Alt + Insert > Constructor, getter (윈도우를 안써서 이 단축키가 아닐 수도 있습니다)
자주 쓰는 어노테이션은 @getter, @RequiredArgsConstructor, @Builder, @Value 이정도? 사용했던 것 같네요
지금 회사에서는 코틀린을 사용하는데, 생성자, getter와 같은 코드를 따로 작성 안해도 자동으로 만들어주는 언어이기도 하고,, 롬복처럼 따로 편의성을 위해 추가하는 의존성은 로깅쪽 말고는 추가 안하는 것 같아요
= 코틀린은 자바 코드 읽으면 굳이 안읽어도 or 굳이 없어도 되는 코드들을 제거한게 특징중에 하나인 자바랑 유사한 언어라고 보시면 됩니다
| return "new-reservation"; | ||
| } |
There was a problem hiding this comment.
현재 controller 패키지에 view 경로를 가르키는 컨트롤러와 api 응답을 제공하는 컨트롤러가 섞여져 있는데요
목적에 따라 패키지를 두 개로 분리해서 구분 지으면 어떨까요??
패키지는 controller 안에서 2개의 패키지로 나누어주셔도 되고, controller 패키지와 또 다른 패키지를 만들어주셔도 됩니다
There was a problem hiding this comment.
controller 안에 view와 api 패키지를 생성하여 역할에 따라 컨트롤러를 분리했습니다!
| private final TimeService timeService; | ||
|
|
||
|
|
||
| @PostMapping("/times") |
There was a problem hiding this comment.
궁금한 점
Controller 코드를 보니 @PostMapping, @GetMapping, @DeleteMapping 순서가 컨벤션인 것 같은데 이런 순서로 하게된 이유가 있나요?
There was a problem hiding this comment.
따로 컨벤션을 정하지는 않았지만, 우선 조회 기능을 먼저 구현했고
커서가 조회 메서드에 위치해 있어서 추가 메서드도 그 위쪽에 작성하게 되었습니다.
메서드 작성 순서는 꼭 정해진 규칙이 없어도 된다고 생각했습니다.
다만 다빈님이 컨벤션 이야기를 해주셔서,
접근 지정자나 역할에 따라 메서드 순서를 정하는 컨벤션도 필요한지 궁금합니다.
| @GetMapping("/times") | ||
| public ResponseEntity<List<TimeResponse>> readTimes() { | ||
| List<Time> times = timeService.getTime(); | ||
|
|
||
| List<TimeResponse> responses = times.stream(). | ||
| map(TimeResponse::from).toList(); | ||
|
|
||
| return ResponseEntity | ||
| .ok() | ||
| .body(responses); | ||
| } |
There was a problem hiding this comment.
| @GetMapping("/times") | |
| public ResponseEntity<List<TimeResponse>> readTimes() { | |
| List<Time> times = timeService.getTime(); | |
| List<TimeResponse> responses = times.stream(). | |
| map(TimeResponse::from).toList(); | |
| return ResponseEntity | |
| .ok() | |
| .body(responses); | |
| } | |
| @GetMapping("/times") | |
| public List<TimeResponse> readTimes() { | |
| List<Time> times = timeService.getTime(); | |
| List<TimeResponse> responses = times.stream(). | |
| map(TimeResponse::from).toList(); | |
| return responses; | |
| } |
200 OK면서 단순 리턴하는 경우에는 수정된 코드랑 똑같은 결과가 나오는데요. 요거는 시간 나실 때 이유도 찾아보시면 좋아보입니다
There was a problem hiding this comment.
@RestController는 HttpMessageConverter를 통해 객체를 자동으로 JSON으로 직렬화해준다는 점 확인했습니다.
제가 작성한 ResponseEntity 방식은 상태 코드(200 OK)와 응답 body를 명시적으로 설정한 것이고,
리뷰어님께서 주신 방식은 객체를 그대로 반환하면 Spring이 직렬화를 자동으로 처리하는 형태라고 이해했습니다.
두 방식 모두 결과적으로 동일한 JSON 응답을 반환하는 구조라는 점도 확인했습니다!
There was a problem hiding this comment.
코드 가독성을 높이기 위해 제안해주신 방식대로 리팩토링해보았습니다!
| public void deleteTime(Long id) { | ||
| int result = timeDao.delete(id); | ||
| if (result == 0) { | ||
| throw new NotFoundTimeException(FailMessage.BAD_REQUEST); | ||
| } | ||
| } |
There was a problem hiding this comment.
삭제에 대한 생각이 두가지 관점으로 나눌 수 있을 것 같아요
- 데이터가 존재하지 않는게 목적이니 삭제 요청이 계속 들어오면 이미 없는 데이터여도 성공적으로 삭제 했다고 응답한다
- 삭제 요청이 계속 들어오면 두번째 요청부터는 삭제할 리소스가 없다고 판단해 에러를 반환한다
두가지 관점중에 어떤게 더 나은 것 같다고 생각하시나요?? (정답 없음)
There was a problem hiding this comment.
일단 제 개인적인 생각으로는 첫 번째 관점(항상 성공을 반환하는 방식)은
실제로 어떤 상황인지 구분하기 어렵다는 문제가 있다고 생각합니다.
예를 들어, 첫 번째 요청이 정말 삭제에 성공한 것인지,
아니면 이미 데이터가 없는 상태에서 단순히 “성공”을 반환한 것인지
응답만으로는 구별하기 어렵습니다.
이런 경우 프론트엔드 개발자가 현재 상태를 정확히 파악하기 힘들 수 있습니다.
이번 미션을 진행하면서도 비슷한 경험이 있었는데,
예약자 추가가 되지 않는 문제가 발생했지만 관리자 페이지에서는 계속 400만 반환되고 있었습니다.
미션 테스트 요구사항이 모든 예외를 400으로 통일하다 보니
무슨 원인으로 실패했는지 판단하기가 어려웠습니다.
이처럼 실패 원인을 구분할 수 없는 응답은 협업 과정에서 혼란을 줄 수 있다고 느꼈습니다.
반면 두 번째 관점(존재하지 않으면 에러를 반환하는 방식)은
첫 번째 요청과 이후 요청의 응답이 구분되기 때문에
상황을 더 명확하게 이해할 수 있어 프론트엔드 입장에서도 직관적이라고 생각합니다.
그래서 저는 협업 측면에서 두 번째 방식이 조금 더 유리하다고 생각합니다.
정해진 정답이 없는 주제라고 하셨는데, 리뷰어님의 관점도 궁금합니다!
| @Getter | ||
| public enum FailMessage { |
There was a problem hiding this comment.
FailMessage로 에러 메시지들을 한 곳에 관리하면 어떤 장단점이 있다고 생각하시나요??
There was a problem hiding this comment.
enum으로 에러 메시지를 한 곳에서 관리하면 다음과 같은 장점이 있다고 생각합니다.
우선, 모든 에러 메시지가 하나의 enum에 모여 있기 때문에 유지보수성이 높아지고 책임이 명확해진다는 점이 있습니다.
또한 에러 메시지를 수정하거나 추가할 때 enum만 변경하면 되기 때문에 다른 클래스에 영향을 주지 않아, 결합도가 낮고 응집도가 높은 구조를 만들 수 있습니다.
다만, 모든 에러 메시지가 한 enum에 집중되다 보니 파일이 점점 비대해질 수 있다는 trade-off도 있다고 생각합니다.
혹시 다른 의견도 있으시면 편하게 말씀 부탁드립니다!
| } | ||
|
|
||
| public static Reservation newReservationFromDb(Long id, String name, String date, String time) { | ||
| public static Reservation newReservationFromDb(Long id, String name, String date, Time time) { |
There was a problem hiding this comment.
Reservation, Time 모두 정적 팩토리 메서드 네이밍 안에 객체 이름이 들어가 있는데요
우리가 정팩메를 사용하면 Reservation.정팩메를 사용하기 때문에 정팩메 이름에 도메인 이름이 들어가지 않아도 의미가 충분히 표현될 수 있을 것 같은데 어떻게 생각하시나요??
There was a problem hiding this comment.
저도 같은 의견입니다.
정적 팩토리 메서드는 Reservation.from(...)처럼 클래스명과 함께 사용되기 때문에
메서드 이름에 도메인 이름이 다시 포함되면(Reservation.newReservationFromDb)
제가 보기에도 다소 반복적으로 느껴질 수 있을 것 같습니다.
해당 부분은 도메인 이름이 중복되지 않도록 더 간결하게 리팩토링해보겠습니다.
좋은 피드백 감사합니다!
| @ExceptionHandler(NotFoundTimeException.class) | ||
| public ResponseEntity<ErrorResponse> handleNotFoundTimeException(NotFoundTimeException e) { | ||
| return buildErrorResponse(e.getFailMessage()); | ||
| } |
There was a problem hiding this comment.
로컬에서 서버 실행하고 에러를 발생하게 만들면 응답값에만 에러메시지가 나오고 터미널에 아무런 로그가 남지 않더라구요 😭
만약 에러가 발생해서 어디가 문제인지 찾으려고 할 때, 위 사진처럼 어느쪽에서 에러가 터졌는지 로그가 보이면 수월하게 원인을 찾을 수 있는데 한 번 추가해볼까요?? 방식은 자유롭게 하시면 됩니다
There was a problem hiding this comment.
GlobalExceptionHandler에 log.error를 추가해 예외가 콘솔에 기록되도록 수정했습니다.
말씀해주신 내용이 공감되어 바로 반영해보았습니다.
좋은 피드백 감사합니다!
|
다빈님이 추천해주신 영상과 ‘계층형 VS 도메인형’ 글을 읽어보았습니다. 특히 영상에서 강조된 “import 문의 방향성”이 큰 도움이 되었습니다. 리팩터링을 진행하면서 궁금한 점이 두 가지 생겨 질문드립니다.
읽어주셔서 감사합니다. |
2 participants
안녕하세요 다빈님. 다시 만나 뵙게 되어 반갑습니다!
이번 단계에서 구현한 프로젝트 구조는 아래와 같습니다.
controller
domain
dto
exception
dao
service
이전 단계에서는 단일 테이블만 사용했지만, 이번 미션에서는 두 개의 테이블을 함께 다루다 보니 구현하면서 헷갈리는 부분이 많았습니다 😢
리뷰 과정에서 개선할 점이나 부족한 부분이 보인다면 편하게 피드백 부탁드립니다.
잘 부탁드립니다!