-
Notifications
You must be signed in to change notification settings - Fork 170
[Spring MVC] 김하늘 3,4단게 미션제출합니다 #521
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
base: kimsky247-coder
Are you sure you want to change the base?
[Spring MVC] 김하늘 3,4단게 미션제출합니다 #521
Conversation
🌱<인사>안녕하세요 하늘님 :) 또 만나뵙게 되어 영광입니다~ 제가 던지는 질문의 의도는 ~~이렇게 반영해주세요. 가 아닌 그럼 이번 미션도 화이팅입니다!! ❓<질문에 대한 답>🐤(하늘) 제 코드의 Service가 너무 많은 책임을 지고 있나요?-> 다만 입력값 검증(null 체크 등)과 에러 메시지까지 Service에서 모두 처리하고 있다 보니 이 부분을 분리해주면 Service는 예약 생성/삭제 같은 핵심 흐름에만 집중할 수 있어서 구조가 더 깔끔해질 것 같습니다 :0 참고로 에러 메시지는 DTO의 validation 메시지나 전용 enum 등으로 따로 관리하면 Service 내부가 훨씬 가벼워져요. 🐤(하늘) 유효성 검증 로직을 Service말고 DTO로 두는 것이 더 적합할까요?-> 다만 **"예약을 찾을 수 없습니다"**처럼 단순 값 검증이 아닌 도메인 규칙이나 비즈니스 흐름에 따른 검증은 |
| @GetMapping("/reservations") | ||
| public ResponseEntity<List<ReservationResponse>> getReservations() { | ||
| List<ReservationResponse> responses = reservationService.getAllReservations(); | ||
| return ResponseEntity.ok().body(responses); | ||
| } | ||
|
|
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.
| @GetMapping("/") | ||
| public String home() { | ||
| return "home"; | ||
| } | ||
| } |
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.
| @RestController | ||
| public class ReservationController { |
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.
HomeController에서는 Controller 어노테이션을 붙이셨는 데 여기에는 왜 RestController를 쓰셨는 지 여쭤봐도 될까요?
둘의 차이점은 무엇일까요?
| package roomescape.dto; | ||
|
|
||
| public record ReservationRequest(String name, String date, String time) { | ||
| } |
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.
name, date, time 중 하나라도 빠지면 Reservation은 생성되면 안 됩니다.
그럼 @NotBlank 또는@NotNull 또는 @NotEmpty을 붙여 DTO단계에서 필드검증을 해보는 건 어떨까요?
그리고 이 어노테이션들의 차이점은 뭘까요?
- 추가적인 중요 키워드 :
@Valid
| @@ -0,0 +1,36 @@ | |||
| package roomescape.domain; | |||
|
|
|||
| import com.fasterxml.jackson.annotation.JsonFormat; | |||
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.
Reservation 도메인에 JsonFormat import가 들어가 있는데, 현재 클래스 내부에서는 해당 어노테이션을 사용하지 않는 것 같아요.
필요없는 import는 지워볼까요?
| LocalDate date, | ||
| @JsonFormat(pattern = "HH:mm") | ||
| LocalTime time |
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.
json 포맷을 잘 활용해주었어요 :)
제가 보기엔 date 필드도 포맷을 지정해주면 응답 형식이 더 일관되게 보일 것 같아요.!
| private void validateReservationRequest(ReservationRequest request) { | ||
| if (request.name() == null || request.name().isEmpty()) { | ||
| throw new BadRequestReservationException("이름은 필수 항목입니다."); | ||
| } | ||
| if (request.date() == null || request.date().isEmpty()) { | ||
| throw new BadRequestReservationException("날짜는 필수 항목입니다."); | ||
| } | ||
| if (request.time() == null || request.time().isEmpty()) { | ||
| throw new BadRequestReservationException("시간은 필수 항목입니다."); | ||
| } | ||
| } |
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.
request 값에 대한 검증은 Controller + DTO 단에서 처리해도 좋을 것 같아요.
이렇게 분리하면 Service는 비즈니스 로직에만 집중할 수 있어서
전체 구조가 더 가벼워질 것 같습니다 :)
| if (!removed) { | ||
| throw new NotFoundReservationException("예약을 찾을 수 없습니다"); | ||
| } | ||
| } | ||
|
|
||
| private void validateReservationRequest(ReservationRequest request) { | ||
| if (request.name() == null || request.name().isEmpty()) { | ||
| throw new BadRequestReservationException("이름은 필수 항목입니다."); | ||
| } | ||
| if (request.date() == null || request.date().isEmpty()) { | ||
| throw new BadRequestReservationException("날짜는 필수 항목입니다."); | ||
| } | ||
| if (request.time() == null || request.time().isEmpty()) { | ||
| throw new BadRequestReservationException("시간은 필수 항목입니다."); | ||
| } | ||
| } |
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.
지금 보니까 에러 클래스는 따로 관리하고 있는데, 에러 메시지는 Service 내부에서 관리되고 있네요 ㅜㅠ
개인적으로는 에러와 메시지를 한 곳에서 함께 관리하는 편이 유지보수에도 더 좋고, 확장할 때도 안정적이라고 느껴졌어요.
그래서 error 관련 내용을 enum으로 통일해서 관리해보는 건 어떨까 조심스럽게 제안드립니다 😊
ex)
@Getter
@AllArgsConstructor
public enum FailMessage {
//400
BAD_REQUEST(HttpStatus.BAD_REQUEST, 40000, "잘못된 요청입니다."),
BAD_REQUEST_REQUEST_BODY_VALID(HttpStatus.BAD_REQUEST, 40001, "잘못된 요청본문입니다."),
BAD_REQUEST_MISSING_PARAM(HttpStatus.BAD_REQUEST, 40002, "필수 파라미터가 없습니다."),
BAD_REQUEST_METHOD_ARGUMENT_TYPE(HttpStatus.BAD_REQUEST, 40003, "메서드 인자타입이 잘못되었습니다."),
BAD_REQUEST_NOT_READABLE(HttpStatus.BAD_REQUEST, 40004, "Json 오류 혹은 요청본문 필드 오류 입니다. ");
private final HttpStatus httpStatus;
private final int code;
private final String message;
}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.
단순한 DTO 필드 검증 메시지 정도라면 필드앞에 어노테이션을 붙여서 메세지 관리를 해도 좋겠네요 :)
다만, 반복적으로 재사용될 경우에는 추천하지 않습니다.
ex)
@NotBlank(message = "이름은 필수 값입니다.") | public record ReservationResponse( | ||
| Long id, | ||
| String name, | ||
| LocalDate date, | ||
| @JsonFormat(pattern = "HH:mm") | ||
| LocalTime time |
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.
현재 Reservation 엔티티와 Response DTO의 필드 구성이 동일해 보이는데요,
DTO를 따로 분리하신 의도가 무엇일까요??
| public class ReservationService { | ||
|
|
||
| private final List<Reservation> reservationList = Collections.synchronizedList(new ArrayList<>()); | ||
| private final AtomicLong index = new AtomicLong(0); |
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.
ID 증가를 위해 Long이나 int 대신 AtomicLong을 사용하신 이유가 궁금합니다 :)
|
미션 진행하시느라 수고 많으셨습니다 !! 1차 코멘트는 여기까지 마무리할게요. |


안녕하세요 혜빈님! 이번 미션도 잘 부탁 드립니다
프로젝트 구조
궁금한 점
3, 4단계를 구현하면서 Controller의 역할이 많아져
ReservationService클래스를 만들어 데이터 저장, 비즈니스 로직, 유효성 검증을 담당하도록 분리하였습니다. 역할을 분리하면서 현재 Service가 너무 많은 책임을 지고 있지는 않은지 궁금합니다. 또한, 이와 관련해서 유효성 검증 로직을 DTO로 옮겨서 처리하고, Service는 핵심 비즈니스 로직에만 집중하도록 하는 것이 더 올바른 역할 분리인지 의견을 듣고 싶습니다!