-
Notifications
You must be signed in to change notification settings - Fork 147
[그리디] 염지환 Spring MVC 3, 4단계 미션 제출합니다. #442
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: jihwanyeom
Are you sure you want to change the base?
Conversation
- build.gradle 에 web, thymeleaf 의존성 추가 - HomeController 클래스 추가 - "home"을 반환하여 home 템플릿 렌더링
- Reservation 클래스 구현 - RoomescapeController 클래스 쿠현 - initializeReservation 예약 초기화 메서드 구현 - reservation 예약 조회 페이지 메서드 구현 - read 예약 목록을 반환하는 메서드 구현
…step3-4 # Conflicts: # build.gradle # src/main/java/roomescape/controller/ReservationController.java # src/main/java/roomescape/domain/Reservation.java
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.
안녕하세요, 지환님.
그리디의 리뷰어 루카🐳입니다!
지난 단계에선 제가 개인적인 사정으로 리뷰를 참여하지 못해 아쉽게 생각하고 있습니다.
늦게 만난만큼 많은 이야기 나눌 수 있으면 좋겠습니다.
🥰 리뷰 스타일
리뷰에 들어가기 전, 제가 리뷰를 임하는 태도와 방식에 대해서 설명 드리겠습니다.
이미 잘 돌아가는 프로덕트를 만들었다면, 정답인 코드라고 생각합니다.
그럼에도 리뷰활동을 통해서 서로의 생각을 공유하고 서로 좋은 인사이트를 얻어 코드를 더 좋게 만드는 것이 이 활동의 목표라고 생각합니다.
저희는 아직 맥락의 갭이 클 것으로 예상이 됩니다.
그 갭을 줄이기 위해 리뷰는 코드의 의도를 묻는 질문 위주로 구성될 가능성이 큽니다.
질문을 여러차례 받아도 의심하거나 상처받거나 하지 마시고, 해당 코드를 작성했을 때 생각을 가감없이 공유해주세요!
명확한 내용 전달을 위해 말을 최대한 간결하게 하려고 노력 중입니다.
리뷰 말투가 조금 딱딱해보일 수 있으나 양해부탁드려요,.
그리고 리뷰의 의도를 잘 전달하기 위해 리뷰 단게를 prefix로 붙이고 있으니 참고 부탁드려요.
‼️ Request Change: 버그를 유발하거나 컨벤션을 지키지 않아 필수적으로 수정 필요- 🤔 Comment: 같이 생각해볼만한 코드
- 👍 Approve: 동의하는 코드
🎯 리뷰 포인트
- Controller의 역할
- ID의 역할
🔮 질문의 답변
입력 검증과 예외 처리를 어떤 계층에서 처리해야 하는가?
글쎄요...
application에서 현재 계층이라고 볼만한 것은 controller와 domain이 보이는데요.
일단 답부터 하자면, 저는 두 게층에 다 검증이 있어도 괜찮다고 생각합니다.
검증이란 기본적으로 각 계층이 가지고 있는 책임을 다하기 위해 하는 것입니다.
domian은 적합한 reservation을 만들기 위한 검증이 필요할 수 있겠고,
controller는 view로부터 올바른 요청을 받기 위한 검증이 필요할 수 있겠네요.
그렇다고 꼭 모든 계층에 검증을 추가해야한다는 것은 아닙니다.
모든 코드는 관리 포인트를 추가하는 것인데요.
그만큼 유지보수 비용도 증가하게 되겠죠.
말씀하신대로 계층을 추가해야되는지 생각해보시는게 좋겠어요.
계층에 대한 책임을 정의하고,
필요한 검증 로직을 추가하고,
테스트 케이스를 추가하면 좋겠습니다.
🙇♂️ 마치며
이번 리뷰에서는 많은 내용을 드리진 않았는데요.
처음 소통이기도 하고 너무 많은 맥락을 드리면 혼란을 야기할 수 있을 것 같아 중요하다고 생각하는 주제를 몇개 드렸습니다.
지환님의 코드를 보면서 리뷰 학습을 잘하실 것 같단 인상을 받았습니다.
같이 얘기 나누고 싶은 주제가 앞으로도 많이 남아 있으니,
계속 달려가 보시죠.
## 📌 예외 | ||
|
||
| 예외 상황 | HTTP 상태 코드 | 메시지 예시 | | ||
|-----------|----------------|-------------| | ||
| 필드 누락 | `400 Bad Request` | `"예약자 이름, 날짜, 시간이 비어 있을 수 없습니다."` | | ||
| 예약 없음 | `400 Bad Request` | `"ID 99에 해당하는 예약이 존재하지 않습니다."` | | ||
|
||
--- |
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
💯 👍 100따봉 드립니다.
친절한 API 명세 정리 매우 훌륭합니다.
reservation
이란 resource를 정의하고,
reservation
에 대해 Create, Read, Delete API 명세를 잘 해두셨네요.
덕분에 이 Application이 제공하는 API를 어떻게 사용해야 할지 예측할 수 있게 되었어요.
reservations.add(new Reservation(3L, "브라운", "2023-01-03", "12:00")); | ||
} | ||
private final List<Reservation> reservations = 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.
🤔 Comment
이 index
라는 필드는 Reservation
라는 Domain Model
의 Id
필드의 값을 할당하기 위한 것이네요.
AtomicLong를 활용해서 동시에 해당 자원에 접근했을 때, 중복된 아이디 생성을 줄이려는 시도 좋습니다.
여기서 몇가지 여쭤보고 싶은 것이 있습니다.
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.
1. Reservation에게 유니크한 ID가 필요한 이유는 무엇인가?
미션 설명에 나와있거나, 예시 코드에 해당 내용이 있었을 수 있습니다.
하지만 지환님이 직접 reservation에게 id를 부여하고 id를 통해 reservation을 핸들링하면서 느낀 점에 대해서 말씀해주세요.
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.
2. AtomicLong index는 ReservationController의 필드(상태)인가?
이 클래스는 @controller라는 어노테이션이 달려있고, 지환님도 Controller라는 네이밍을 지은 것으로 보아 MVC 패턴에서 말하는 Controller의 역할을 할 것으로 보여지는데요.
이 컨트롤러가 상태로 가지고 있어야할 것은 무엇일까요?
어떤 변경에 유연하게 대처할려고 이러한 Controller를 사용하는 패턴을 따르는 것일까요?
상상해보죠.
- 우리가 사용하는 view가 json형태로 주고 받을 수 없는 view로 변경되었을 때
- 사용하는 web framework이 Spring mvc가 아닌 다른 것으로 변경 되었을 때
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에게 유니크한 ID가 필요한 이유는 무엇인가?
id 는 identifier로 탐색이나 수정, 삭제 시 객체를 효과적을 탐색하기 좋은 속성이라고 생각했습니다. 특히 예약처럼 사용자로부터 요청받은 속성이 중복될 가능성이 있는 경우 반드시 필요하다고 생각했어요.
제가 생각했을 때 이러한 identifier가 유니크 해야하는 이유는 데이터를 삭제하거나 수정할 때 데이터의 정렬 상태나 중복 여부에 따라 결과가 모호한 상황을 피하기 위해서 였습니다.
만약 reservation의 id가 1로 같은 예약이 두개인 같은 경우가 발생한다면 구현 방식에 따라하나의 예약을 지웠을 때 다른 예약이 지워지거나 두 예약이 모두 지워질 수도 있다고 생각했습니다
AtomicLong index는 ReservationController의 필드(상태)인가?
AtomicLong index는 현재 가장 마지막으로 생성된 id값이 저장되어 있고 값 삽입 시 값을 증가시키고 해당 값을 생성된 예약의 id로 설정하고 있어요.
그런 점에서 보았을 때 index 변수 자체는 사실 reservation객체의 static 변수로 관리하거나 reservation 리스트를 관리하는 일급 컬렉션의 인스턴스 변수로 관리해도 될 부분 같아요. 컨트롤러의 역할은 아니라고 생각합니다!
Controller에 대해서는 단순히 모델과 뷰 사이에서 흐름을 제어하는 역할로 생각하고 있었어서 조금 더 찾아보고 생각을 해 보았습니다.
어떤 변경에 유연하게 대처할려고 이러한 Controller를 사용하는 패턴을 따르는 것일까?
-
우리가 사용하는 view가 json형태로 주고 받을 수 없는 view로 변경되었을 때
view의 형태가 변경되어도 view가 필요로 하는 데이터는 보통 같기 때문에 Controller는 큰 수정 없이 model에서 데이터를 받아 view에 전달하는 역할을 계속 할 수 있기 때문인 것 같아요 -
사용하는 web framework이 Spring mvc가 아닌 다른 것으로 변경 되었을 때
framework마다 객체를 다루는 방식이 다르기에 동작 방식이 spring과 달라지는 경우 여러 문제가 생길 수 있을 것 같아요! 이 부분에 관해선 현재 스프링이 Controller를 다루는 방식을 찾아 보았는데 Spring은 Controller bean을 싱글톤 객체로 사용하고 있다는 걸 알게 되었어요. 그렇다면 만약 다른 방식으로 동작하는 framework로 변경 된다 해도 Controller가 큰 수정 없이 동작해야 한다고 생각하게 되었습니다.
그래서 조금 극단적인 예시도 보게 되었어요 만약 Controller객체가 요청을 할 때 마다 생겼다가 사라지는 방식의 프레임워크라면? 그렇다면 아마 reservations 나 long이 유지되지 못하는 문제가 생길수도 있겠다는 생각이 들었습니다. 따라서 Controller는 최대한 상태가 없는 방향으로 구성한다는 결론을 내리게 된 거 같아요.
아직 MVC의 개념을 Spring의 개념에 매칭시키는 것이 익숙하지 않아서 잘 생각하고 답변한 것인지 스스로 의문이 좀 들기도 하네요! 그리고 상태를 만들지 않고 Controller에서 데이터를 어떻게 다룰지도 조금 더 생각해봐야겠다고 느꼈습니다! 틀린 생각이 있다면 조언 부탁드립니다
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.
우리가 사용하는 view가 json형태로 주고 받을 수 없는 view로 변경되었을 때
view의 형태가 변경되어도 view가 필요로 하는 데이터는 보통 같기 때문에 Controller는 큰 수정 없이 model에서 데이터를 받아 view에 전달하는 역할을 계속 할 수 있기 때문인 것 같아요사용하는 web framework이 Spring mvc가 아닌 다른 것으로 변경 되었을 때
framework마다 객체를 다루는 방식이 다르기에 동작 방식이 spring과 달라지는 경우 여러 문제가 생길 수 있을 것 같아요! 이 부분에 관해선 현재 스프링이 Controller를 다루는 방식을 찾아 보았는데 Spring은 Controller bean을 싱글톤 객체로 사용하고 있다는 걸 알게 되었어요. 그렇다면 만약 다른 방식으로 동작하는 framework로 변경 된다 해도 Controller가 큰 수정 없이 동작해야 한다고 생각하게 되었습니다.
음 글쎄요? view가 필요로 하는 데이터(형식이나 종류)가 같다는 가정이였는데요.
예를 들어, 이 예약 정보를 엑셀파일로 만들어서 줘야하는 API도 존재할 수 있겠죠?
제 리뷰의 의도가 잘 전달되지 않은 것 같습니다.
제 의도를 조금 더 부연설명 드리겠습니다.
이 ReservationController
는 아래 두 가지가 특성이 있는 것 같습니다.
- http 바디에 JSON 형식으로 값을 담아서 주면 좋을 View를 위해 존재한다.
- Spring MVC를 통해 mapping되고 호출된다.
전 리뷰에서 2가지 가정을 한 이유는 이 컨트롤러는 그 2가지에 의존적일 수 밖에 없다.
이 컨트롤러가 존재하는 이유는 도메인에서 그 2가지 의존성을 최대한 떨어놓기 위함이 아닐까? 하는 의도였습니다.
그래서 아이디를 생성하거나, 검증하는 등의 비즈니스 로직은 이 컨트롤러의 책임이 아닌것 같다하는 의견이였습니다.
JSON, Spring 같이 도메인과 상관없는 요구사항이 변경되었을 때, 중요한 비즈니스 로직이 있는 코드가 변경되면 불편할테니까요!
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 List<Reservation> reservations = new ArrayList<>(); | ||
private final AtomicLong index = new AtomicLong(0); | ||
Validator validator = new Validator(); |
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 Change
학습할 키워드
bean, IoC, DI, @Autowired
Spring의 가장 대표적인 컨셉은 개발자가 작성해놓은 코드
를 IoC로 프레임웤이 그 코드를 호출해 빈 객체를 생성해서 의존성을 주입해준다
는 것인데요.
정적으로 사용해야될 필드도 존재할 수 있지만,
위에 AtomicLong 에 대한 리뷰를 토대로 이 Controller가 필드로서 가져야할 대상들을 정하고 bean으로 주입해보는 연습을 하면 좋겠습니다.
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.
bean과 Ioc 컨테이너, 의존성 주입에 대해서는 1, 2 단계 리뷰에서 공부한 기억이 있습니다! 해당 리뷰를 가져와 보자면
얕게나마 정리된 지식을 말해보자면 스프링 프레임워크는 IoC(제어 역전 : 객체가 종속성을 직접 정의하지 않고 종속성 주입을 위임하는 패턴) 컨테이너가 Bean 객체의 의존성을 주입하고, 객체를 관리하는 기능을 가지고 있다는 것을 보게 되었어요.
그리고 사용자는 Container 어노테이션을 통해 사용자가 작성한 Bean을 spring이 감지할 수 있도록 만들어 준다고 들었고, 이 Container 어노테이션의의 특수한 형태로, Controller Bean을 지정해 주는 것이 Controller 어노테이션 이라고 이해하게 되었습니다.
따라서 @Autowired
를 통한 의존성 주입을 먼저 공부하고 적용해 보게 되었습니다
먼저 Reservations 일급 컬렉션을 만든 뒤에 List를 조작하는 메서드를 추가하고, id생성과 검증 역시 해당 객체 내에서 진행하도록 하였습니다.
그 다음에는 Reservations 객체를 @Component
를 통해 Bean으로 인식하게 해 준 다음 Controller의 reservations 필드에 @Autowired
를 통한 주입을 해 주었습니다. 하지만 이렇게 아직 reservations 필드가 남아있는 상태인 것이 Controller에게 바람직한 상태인지 궁금합니다!
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.
그 다음에는 Reservations 객체를 @component를 통해 Bean으로 인식하게 해 준 다음 Controller의 reservations 필드에 @Autowired를 통한 주입을 해 주었습니다. 하지만 이렇게 아직 reservations 필드가 남아있는 상태인 것이 Controller에게 바람직한 상태인지 궁금합니다!
크게 문제가 될만한 포인트는 모르겠습니다!
저희가 지금까지 크게 취하고 있는 전략은 MVC 패턴과 레이어드 아키텍쳐인것 같은데요.
MVC패턴 관점으로 봐도 Reservations은 도메인 영역에 해당이될것이고,
레이어드 아키텍처 관점으로 봐도 Reservations은 application(business) 레이어 하위 레이어가 되겠는데요.
두 관점 모두에서 Controller는 Reservations에 의존성을 가진 아입니다. 그래서 문제로 보이는 것은 아직 없다! 입니다.
public Reservation() { | ||
|
||
} | ||
|
||
public Reservation(Long id, String name, String date, String time) { | ||
this.id = id; | ||
this.name = name; | ||
this.date = date; | ||
this.time = time; | ||
} | ||
|
||
public Reservation(Long id, Reservation reservation) { | ||
this.id = id; | ||
this.name = reservation.name; | ||
this.date = reservation.date; | ||
this.time = reservation.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.
🤔 Comment
서로 다른 Argument를 받는 사용하지 않는 생성자를 여럿 둔 이유가 있으실까요?
생성자 또한 생성 로직을 담당하는 중요한 프로덕트 코드입니다.
- 3개 중 어떤 생성자를 호출해야 온전한 reservation이 생성될 수 있는가?
- Validator를 외부에서 호출한다는 보장은 이 reservation에겐 필요가 없는가?
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.
먼저 각 생성자를 만든 이유는 다음과 같습니다
- 매개변수가 비어있는 생성자 : Jackson의 역직렬화, 객체를 json 형식으로 바꾸는 조건 중 하나가 기본 생성자를 가지고 있을 것 이었기 때문이에요.
- 속성을 전부 입력받는 생성자 : 1,2 단계에서 초기 데이터를 삽입할 때 사용했던 생성자에요. 현 단계에서는 사용할 필요가 없을 것 같아요.
- id와 예약을 입력받는 생성자 : 예약을 추가할 때 역직렬화로 받아온 예약 데이터에 id를 추가해 삽입하기 위해 만든 생성자에요
3개 중 어떤 생성자를 호출해야 온전한 reservation이 생성될 수 있는가?
현재 코드에서는 2, 3번 생성자를 사용해야만 id가진 온전한 reservation이 생성된다고 생각합니다 POST 요청을 할 때 id값은 주어지지 않으므로 1번 생성자에선 id가 없는 reservations 가 생성되게 되요.
Validator를 외부에서 호출한다는 보장은 이 reservation에겐 필요가 없는가?
지금 생각해보면 validator를 외부에서 호출 한다고 해도 객체가 스스로 값을 검증하는 것도 바람직하다고 생각합니다. 실제로 이전 미션까지는 정적 팩토리 메서드를 사용해 그 내부에서 validator를 사용했던 것 같아요. 그렇다면 여러개의 생성자를 두는 것보단. 기본 생성자를 하나 두고 다른 두 생성자는 정적 팩토리 메서드로 전환한 뒤에 검증을 적용하는 편이 좋을까요?
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.
매개변수가 비어있는 생성자 : Jackson의 역직렬화, 객체를 json 형식으로 바꾸는 조건 중 하나가 기본 생성자를 가지고 있을 것 이었기 때문이에요.
오잉 그런가요?
제가 알기로는 java의 클래스는 컴파일러가 기본 생성자를 만들어주는 것으로 알고있는데요.
실제로 해당 코드를 지워도 잘 동작할것으로 예상되는데요.
만약 해당 코드를 지웠을 때 작동하지 않는다면 다른 문제가 있는 것이 아닐까요?
속성을 전부 입력받는 생성자 : 1,2 단계에서 초기 데이터를 삽입할 때 사용했던 생성자에요. 현 단계에서는 사용할 필요가 없을 것 같아요.
사용하지 않는 코드는 과감하게 지워주세요.
생성자 또한 명세이고 해당 객체가 생성될 방식을 지환님이 구현을 하는 것입니다.
규칙은 명확할수록 좋죠.
또한, 클린코드 측면에서 사용하지 않는 코드는 정리하는 것이 좋습니다.
메서드는 있어야될 것 같아서 존재하는 것이 아니라,
메서드 시그니쳐로서 해당 코드가 외부에서 어떻게 사용되어야할지 정하는 것이니까요.
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.
지금 생각해보면 validator를 외부에서 호출 한다고 해도 객체가 스스로 값을 검증하는 것도 바람직하다고 생각합니다. 실제로 이전 미션까지는 정적 팩토리 메서드를 사용해 그 내부에서 validator를 사용했던 것 같아요. 그렇다면 여러개의 생성자를 두는 것보단. 기본 생성자를 하나 두고 다른 두 생성자는 정적 팩토리 메서드로 전환한 뒤에 검증을 적용하는 편이 좋을까요?
validator 를 사용한다 -> 정팩메(정적 팩토리 메서드 라는 뜻)를 사용한다
이 맥락이 잘 안이어지는데요.
해당 객체가 생성될 때 꼭 실행되어야하는 조건이라면 생성자나 init 블럭에 validate 로직이 존재해도 어색하지 않고,
정팩메를 사용해서 특별한 생성 규칙을 만들어야한다면, 정팩메에만 validate를 추가해도 괜찮겠죠?
또한, validate를 강한 규칙으로 삼고 싶고, 그를 생성자 내부에 넣고 싶다면, 생성자 체인기법을 사용해서 어떤 생성 로직을 실행해도 해당 validate가 실행되도록 할 수 있겠네요.
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.
제가 알기로는 java의 클래스는 컴파일러가 기본 생성자를 만들어주는 것으로 알고있는데요.
역직렬화의 조건으로 기본 생성자가 필요하다는 글을 보았고, 기본 생성자가 아닌 다른 생성자를 명시적으로 선언한 경우 반드시 기본 생성자도 만들어 주어야 한다고 보았는데요. 그런데 최근 스프링 부트 버전에선 굳이 기본 생성자를 명시하지 않아도 역직렬화에 문제가 없다고 합니다. 기본 생성자를 지웠을 때 테스트에서 문제가 생겼었는데 DTO를 적용한 뒤로는 사라졌습니다. 아마 다른 문제였을 것이라 생각합니다
사용되지 않는 코드를 지우는 건 늘 명심하지만 자주 놓치는 것 같습니다 주의하겠습니다!
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.
안녕하세요. 지환님. 리뷰 답변이 조금 늦었네요.
상황이 여러모로 꼬인 것 같네요.
지난번 리뷰에 이어서
- 에외 응답
- 컨트롤러 분리
이 두가지를 중점적으로 리뷰를 드렸습니다.
한번 더 리뷰 반영 부탁드립니다.
import org.springframework.web.bind.annotation.ResponseBody; | ||
import roomescape.domain.Reservation; | ||
import roomescape.domain.Reservations; | ||
|
||
@Controller |
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.
🤔 Comment
RestController 어노테이션과 Controller 어노테이션의 차이에 대해서 학습해보신적 있으실까요?
현재는 해당 컨트롤러가 크게 2가지 역할을 하고 있다고 느껴지는데요.
- reservation 리소스에 대한 CRUD API
- 예약 사이트 view 페이지 path로 포워딩
1번과 2번을 컨트롤러를 분리시켜 보는 것은 어떠할까요?
그러면서, 각각의 Advice도 따로 정리하면서 각각의 예외 응답도 명확하게 하면 좋을 것 같습니다.
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의 경우 @responsebody와 같은 어노테이션을 자동으로 붙여주는 Controller 어노테이션이라고 알고 있습니다 적용하려고 했으나 뷰를 반환하는 메서드 때문에 바꾸지 않았는데 이런 기준으로 분리하면 ReservationController에 @RestController를 적용할 수 있을 것 같네요!
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.
🤔 Comment
id 는 identifier로 탐색이나 수정, 삭제 시 객체를 효과적을 탐색하기 좋은 속성이라고 생각했습니다. 특히 예약처럼 사용자로부터 요청받은 속성이 중복될 가능성이 있는 경우 반드시 필요하다고 생각했어요.
앞선 리뷰에서 ID에 대한 생각을 이렇게 말씀해주셨습니다.
맞습니다. ID는 특정 엔티티를 효과적으로 탐색하기 좋게하죠.
그렇다면
id가 1L인 Reservation 객체가 2개 존재하는데,
하나는 이름이 "방탈출 지환"이고 하나는 이름이 "방탈출 루카"라면 둘은 다른 엔티티일까요?
일단, 저는 id를 부여한 순간 값의 갱신의 차이는 있지만 같은 대상이라는 생각이 듭니다.
그렇다면 이 둘이 같다는 것을 어떻게 효과적으로 표현할 수 있을까요?
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.
다른 미션에서 원시값 포장한 객체에서 equals와 hashcode를 이용해 Map에서 활용했을 때 쓴 기억이 있네요! id도 똑같은 방법으로 id를 기준으로 equals함수를 작성하고 hashcode를 id 기준으로 생성하도록 작성하면 되는 문제 같습니다. 당연히 중복이 없는 경우만 생각하고 있었어서 생각치 못했던 부분이네요!
import roomescape.exception.EmptyDataException; | ||
import roomescape.exception.NotFoundReservationException; | ||
|
||
public class Validator { |
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.
🤔 Comment
- Validator라는 네이밍은 너무 포괄적인 것 같으니, 조금 더 비즈니스 로직을 담은 이름으로 바꿔보죠.
- Validator가 상태를 같지 않고 포괄적인 로직만 갖고 있는 아이라면, 유틸 클래스로서 정적으로 만들어도 좋지 않을까요?
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.
해당 코드는 ReservationValidator라는 이름으로 모든 메서드를 static으로 선언하여 util 패키지로 빼 두었습니다! 확실히 이렇게 하면 간단하게 여러 계층에서 검증을 수행할 수 있을 것 같습니다
@ControllerAdvice | ||
public class GlobalExceptionHandler { | ||
|
||
@ExceptionHandler(EmptyDataException.class) | ||
public ResponseEntity<Void> handleEmptyDataException(EmptyDataException e) { | ||
System.out.println(e.getMessage()); | ||
return ResponseEntity.badRequest().build(); | ||
} | ||
|
||
@ExceptionHandler(NotFoundReservationException.class) | ||
public ResponseEntity<Void> handleNotFoundReservationException(NotFoundReservationException e) { | ||
System.out.println(e.getMessage()); | ||
return ResponseEntity.badRequest().build(); | ||
} | ||
|
||
} |
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.
🤔 Comment
예외 상황에 대해서 응답을 여러 방법으로 내려줄 수 있습니다.
이는 API를 작성하는 사람이 기준을 갖고 API를 사용할 사람이 잘 이해할 수 있도록 작성하면 되는데요.
그래서 이 두경우 다 400 배드리퀘스트로 응답하는 것이 지환님의 선택이라면 굳이 변경요청을하고 싶진 않습니다.
다만, RFC 2616같은 문서에서 표준화된 규칙이 존재합니다. 아직은 경험이 부족한 만큼 해당 내용들을 자세히 살펴보고 본인만의 기준을 잡아서 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.
해당 문제에 관해서는 발생시킨 두 예외중 입력값의 누락의 경우는 BadRequest가 맞다고 생각하나 사용자가 삭제를 요청한 id가 없는 것은 404 NotFound가 더 어울리는 것 같아서 테스트와 ExeptionHandler를 수정하였습니다!
안녕하세요 루카님! 리뷰가 늦어져서 죄송합니다 아무래도 상황이 잘 맞지 않아 이번 주 미션과 같이 진행하면서 리뷰가 조금 많이 미뤄진 점 죄송합니다. 리뷰에 대한 반영을 전부 완료 하였습니다! 그리고 다른 분들의 코드를 참고하여 Service 계층과 DTO 객체를 추가해 보았습니다. |
안녕하세요 루카님! 이번에 리뷰받게된 염지환이라고 합니다! 리뷰 받게되어 너무 감사해요
1, 2단계에 작성해두긴 했지만 다시 말씀드리자면 저는 스프링이 아에 처음입니다 백엔드 관련 개념도 처음 입문하는 거라 부족한 부분이 많아요 그래도 리뷰해주시는 부분은 최대한 깊게 학습하려고 노력중이에요 서슴없이 리뷰해주시면 감사하겠습니다!
3단계에서는 POST 요청을 통해 리뷰 생성 및 DELETE요청을 통한 리뷰 삭제를
4단계에서는 예외처리를 적용했습니다
이번 미션에서 가장 고민했던 점은
였습니다 자바 미션을 수행할 때는 보통 포장한 객체나 일급 컬렉션에서 검증을 수행했는데 스프링에서는 어디에서 검증을 해야 할지 갈피를 잘 못잡겠더라구요
그래서 고민 끝에 domain에 검증 클래스를 작성하고 예외는 전부 exception 패키지를 따로 만들어 검증하였습니다
미션을 끝냈다! 싶어서 다른 분들 코드를 찾아봤는데 다른 계층을 많이 만들어서 사용하셨더라고요 요구사항만 채우기 급급했는데 더 공부할 길이 멀다고 느꼈습니다... 혹시 지금 더 공부해두면 좋은 지식이 있을까요?
최대한 많이 반영해가며 학습하도록 하겠습니다 잘부탁드려요!!