-
Notifications
You must be signed in to change notification settings - Fork 0
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
[1 - 6단계 - 방탈출 예약 관리] 망쵸(김주환) 미션 제출합니다. #7
base: 3juhwan
Are you sure you want to change the base?
[1 - 6단계 - 방탈출 예약 관리] 망쵸(김주환) 미션 제출합니다. #7
Conversation
* Update .gitignore Co-authored-by: 3Juhwan <[email protected]> * docs(readme): 1단계 기능 요구사항 추가 Co-authored-by: 3Juhwan <[email protected]> * feat(adminController): 어드민 메인 페이지를 응답하는 기능 구현 Co-authored-by: 3Juhwan <[email protected]> * docs(readme): 2단계 기능 요구사항 추가 Co-authored-by: 3Juhwan <[email protected]> * feat(reservationController): 저장된 예약 정보를 조회하는 API 구현 Co-authored-by: 3Juhwan <[email protected]> * docs(readme): 3단계 기능 요구사항 추가 Co-authored-by: 3Juhwan <[email protected]> * feat(reservationController): 예약을 추가하고 삭제하는 기능 구현 Co-authored-by: 3Juhwan <[email protected]> * refactor(reservationController): 예약 삭제 시 id값을 가져와서 비교하지 않고 질의하도록 변경 Co-authored-by: 3Juhwan <[email protected]> --------- Co-authored-by: HyungUk Ryu <[email protected]>
HH:mm:ss -> HH:mm
# 1단계 기능 요구사항 - 홈 화면 | ||
|
||
- [x] `/admin` 으로 `GET` 요청 시 어드민 메인 페이지를 응답한다. | ||
|
||
# 2단계 기능 요구사항 - 예약 조회 | ||
- [x] `/reservations` 으로 `GET` 요청 시 저장된 예약 정보를 응답한다. | ||
- [x] `/admin/reservation` 으로 `GET` 요청 시 예약 관리 페이지를 응답한다. | ||
|
||
# 3단계 기능 요구사항 - 예약 추가/취소 | ||
- [x] `/reservations` 으로 `POST` 요청 시 예약을 추가한다. | ||
- [x] `/reservations/{id}` 으로 `DELETE` 요청 시 해당 `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.
기능 요구사항 👍
API 명세도 있으면 좋을 듯!
@Controller | ||
@RequestMapping("/admin") | ||
public class AdminController { |
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.
어쩌다가 알게된 건데 망쵸의 의견도 궁금하네
@RequestMapping
을 통해서 리소스를 계층화 할 수 있잖아
그런데 이런 방식으로 url을 잘라놓으면 전역적으로 찾을 떄 (cmd + shift + f) 찾기 어렵다는 말도 있더라고
/admin/resrervation
을 찾을 수 없음
망쵸는 어떻게 생각!?
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.
나도 그 방식이 좋다고 생각해
다음 미션부터 @RequestMapping
보다 Mapping 애너테이션에 모든 URI를 넣으려고!
List<Reservation> reservations = reservationRepository.findAll(); | ||
List<ReservationResponseDto> reservationResponseDtos = reservations.stream() | ||
.map(ReservationResponseDto::from) | ||
.toList(); |
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는 뷰라고 생각했고 DTO는 뷰와 도메인을 모두 알고 있는 컨트롤러에서 다루어져야 한다는 개인적인 생각이야
Reservation savedReservation = reservationRepository.save(new Reservation( | ||
reservationRequestDto.name(), | ||
reservationRequestDto.date(), | ||
reservationRequestDto.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.
Dto에서 값을 꺼내와서 Reservation 객체를 생성하고 있네 👍
RequestDto에서 toEntity를 구현하는 방법이 있고 이러한 방법이 그렇게 어색하지는 않은가봐
웨지랑 나눈 이야기가 있는데 궁금하면 한번 확인해보시오!
코멘트
@DeleteMapping("/{id}") | ||
public ResponseEntity<Void> delete(@PathVariable(name = "id") long id) { | ||
reservationRepository.deleteById(id); | ||
|
||
return ResponseEntity.noContent().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.
@PathVariable
의 경로 이름(name = "id")과 변수명이 같을 경우 (long id) 생략 가능하다던데 이는 빌드와 실행 옵션에 따라 달라지는 부분이더라
설정에 따라 동작이 달라질 수 있는 코드는 좋지 않기에 지금처럼 @PathVarable
에 파라미터를 함께 전달하는 것이 좋은 것 같아 👍
다만 name은 생략할 수 있을 듯!
package roomescape.domain; | ||
|
||
import java.time.LocalDate; | ||
import java.time.LocalTime; | ||
|
||
public class Reservation { | ||
private final long id; | ||
private final String name; | ||
private final LocalDate date; | ||
private final 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.
도메인은 비즈니스 로직을 담은 협력 객체, 엔티티는 DB 테이블과 일대일 매핑되는 영속화 대상 객체
나는 레벨 1 체스 미션에서 위와 가이 학습했었어
다만 망쵸의 현행처럼 도메인과 엔티티를 엄격하게 구분하는 경우는 많지 않다고 하더라!
관련한 코멘트
public static void validateName(String name) { | ||
Objects.requireNonNull(name); | ||
if (name.length() < NAME_LENGTH_MIN || NAME_LENGTH_MAX < name.length()) { | ||
throw new IllegalArgumentException(); | ||
} | ||
} | ||
|
||
private void validateDate(LocalDate date) { | ||
Objects.requireNonNull(date); | ||
if (date.isBefore(LocalDate.now())) { | ||
throw new IllegalArgumentException(); | ||
} | ||
} | ||
|
||
private void validateTime(LocalTime time) { | ||
Objects.requireNonNull(time); | ||
if (time.isBefore(LocalTime.of(RESERVATION_TIME_MIN, 0)) | ||
|| time.isAfter(LocalTime.of(RESERVATION_TIME_MAX, 0))) { | ||
throw new IllegalArgumentException(); | ||
} |
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.
어떤 것을 검증하는지 확실한 메서드 명을 주는게 좋을 듯!?
추가적으로 Objects.requireNonNull()
은 단순히 NPE 만을 발생시키기에 (물론 발생시키지 않는 것 보다는 괜춘겠지만)
불친절하다는 생각이야 ( 나도 Objects.requireNonNull()
만 썻다가 리뷰 받음)
망쵸의 결론도 궁금하군 🤔
# 1단계 기능 요구사항 - 홈 화면 | ||
|
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을 까먹지 말라는 네오의 가르침..!
public Reservation(String name, LocalDate date, LocalTime time) { | ||
this(0L, name, date, 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.
0과 Null은 의미가 천차만별이라고 생각해
만약 아이디가 없음을 의도한 것이라면 nullable함을 설정해야된다고 생각함! (Wrapper Type)
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는 null 표현 때문에 Wrapper 타입이 거의 강제된다고 하더라고 🤔
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 Reservation(String name, LocalDate date, LocalTime time) { | ||
this(0L, name, date, 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.
추가적으로 ID는 null 표현 때문에 Wrapper 타입이 거의 강제된다고 하더라고 🤔
public record ReservationResponseDto(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.
@JsonFormat
몰랐는데 알아갑니다 🙇🏻♂️
@Repository | ||
public class H2ReservationRepositoryImpl implements ReservationRepository { |
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.
...Impl
이라는 클래스 명은 인터페이스에 대한 구현체가 단 하나일 때 사용하는 관례적 이름이라고 나는 알고 있어
지금처럼 구현체가 여러개인 경우에는 다른 이름이 좀 더 좋지 않을까 하는 개인적인 생각!
private final List<Reservation> reservations = 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.
Collections.synchronizedList(new ArrayList<>());
는 처음 보네 멀티 스레드 환경에서 동기화를 보장해주나봐 👀
새로운 것 학습하고 갑니다
@Import(InMemoryReservationRepositoryImpl.class) | ||
@WebMvcTest(ReservationController.class) | ||
class ReservationControllerTest { | ||
@Autowired |
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.
와 테스트 잘써놨다 👍
나 계층들 테스트가 제일 걱정이었는데 망쵸꺼 보면서 공부해야할 듯 💪🏻
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 javax.sql.DataSource; | ||
import java.util.List; | ||
|
||
@Primary |
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.
혹시 당신도.. 대영한
카르텔..? 농담입니다ㅎ 우선적으로 주입될 빈을 설정하기 위해서 @Primary
어노테이션을 사용해주셨군요! 굳!
validateDateTime(date, time); | ||
} | ||
|
||
public static void validateName(String 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.
아무래도 가장 고민되는 지점이죠? 우린 레벨1에서 특정 도메인의 값 검증은 해당 도메인 클래스에 위임하는게 좋다고 배웠지만. 막상 스프링 관점에서 보면 DTO에서 검증하는게 크게 나쁘지 않은거 같구요 🤔 (더군다나 스프링의 @Validation을 사용하면 훨씬 편리하고 강력하게 검증을 할 수 있는데 말이죠...ㅎ)
값에 대한 검증 책임은 DTO vs Domain
망쵸의 의견이 궁금해요
PS. 무려 7년전에도 같은 주제로 선배 개발자들이 열심히 토론하고있더라구요! 의견 역시 가지각색이고요 👀
https://stackoverflow.com/questions/42280355/spring-rest-api-validation-should-be-in-dto-or-in-entity
import static org.assertj.core.api.Assertions.assertThatCode; | ||
import static org.assertj.core.api.SoftAssertions.assertSoftly; | ||
|
||
@Transactional |
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.
@Transactional
을 사용해주셨네요! 비즈니스로직에는 따로 @transactional이 보이지 않던데 테스트 데이터 롤백용으로 사용했을까요?
테스트에서 데이터 롤백용으로 @Transactional
을 사용해야하는지는 현업자분들 사이에서도 논쟁이 오간다고해요! 망쵸는 어떤 의견인지, 사용해도 괜찮다면 단점보다 큰 장점이 무엇인지 의견이 궁금해요!
관련해서 재밌는 글을 하나 발견해서 공유드립니다! 😋
https://jojoldu.tistory.com/761
고생하셨습니다~~~