[1 - 6단계 - 방탈출 예약 관리] 리비(이근희) 미션 제출합니다. #6
Conversation
* docs: 커밋 메시지 템플릿 공동 작업자 설정 Co-authored-by: SeongjuLee <[email protected]> * feat: "/admin" 요청 시 어드민 메인 페이지가 응답하는 기능 구현 Co-authored-by: SeongjuLee <[email protected]> * test: admin 메인 페이지 응답 테스트 오류 수정 Co-authored-by: SeongjuLee <[email protected]> * test: 예약 관리 페이지 응답 테스트 작성 Co-authored-by: SeongjuLee <[email protected]> * feat: Reservation entity 구현 Co-authored-by: SeongjuLee <[email protected]> * feat: 예약 관리 페이지 응답 기능 구현 Co-authored-by: SeongjuLee <[email protected]> * test: 예약 추가 및 삭제 테스트 작성 Co-authored-by: SeongjuLee <[email protected]> * feat: ReservationDto 구현 Co-authored-by: SeongjuLee <[email protected]> * feat: 예약 추가 기능 구현 Co-authored-by: Seo현ngjuLee <[email protected]> * feat: 예약 삭제 기능 구현 Co-authored-by: SeongjuLee <[email protected]> * refactor: 관련된 컨트롤러 기능끼리 응집 개선 Co-authored-b현y: SeongjuLee <[email protected]> * refactor: 컨트롤러 공통 리소스 계층화 개선 Co-authored-by: SeongjuLe선e <[email protected]> * refactor: 컨트롤러 메서드 이름 가독성 개선 Co-authored-by: SeongjuL선ee <[email protected]> * refactor: 주석 제거 개선 Co-authored-by: SeongjuL선ee <[email protected]> * docs: 커밋 메시지 템플릿 삭제 * docs: API 명세서 작성 * refactor: RestController 응답 생성 시 ResponseEntity를 사용하도록 개선 * refactor: 컨트롤러 매핑 경로 개선 * style: dto 클래스명 구체화 개선 * feat: ReservationResponseDto 구현 * refactor: 컨트롤러 응답 시 Dto를 반환하도록 개선 * test: 테스트 코드 가독성 개선 * test: 테스트 클래스 분리 개선 * fix: Reservation, Time 클릭 시 url 요청 경로 수정 * test: 불필요한 테스트 컨테이너 격리 제거 개선 * refactor: 엔티티, DTO의 아이디 속성을 Wrapper 타입으로 변경 * docs: 도메인 명세 작성 * feat: Name 도메인 구현 * style: DisplayName 컨벤션 통일 (마침표 제거) * feat: ReservationTime 도메인 구현 * feat: Name 도메인 Getter 프로퍼티 추가 * feat: ReservationTime 도메인 Getter 프로퍼티 추가 * refactor: 엔티티가 포장 클래스를 이용, 비즈니스 로직을 담도록 개선 * feat: ReservationRepository 구현 * feat: 예약간 시간 충돌 확인 기능 구현 * refactor: Reservation 단건 조회 Optional 개선 및 deleteById 예외 상황 구현 * feat: 시간이 겹치는 예약이 있는지 조회하는 기능 구현 * feat: ReservationService 구현 * refactor: 레이어드 아키텍쳐 적용 개선 * refactor: DTO 클래스 이름 변경 개선 * refactor: requestDTO - toEntity 기능 구현 * refactor: Controller -> ViewController 클래스 이름 구체화 개선 * style: 코드 컨벤션 통일 개선 * refactor: 해당하는 Id 찾을 수 없음 예외 NoSuchElementException으로 변경 개선 * test: MemoryReservationRepositoryTest 작성 * chore: Mockito 의존성 추가 * test: 시간 겹침에 따른 예약 저장 실패, 성공 테스트 케이스 작성 * feat: ReservationTime NonNull검증 추가 구현 * refactor: 예외 메시지 구체화 개선 * docs: 기능 구현 목록 최신화 --------- Co-authored-by: SeongjuLee <[email protected]>
| @GetMapping() | ||
| public String showAdminMainPage() { |
There was a problem hiding this comment.
뒤에 괄호 없애는게 조금 더 깔끔할 거 같은 느낌!
@GetMapping() -> @GetMapping
| private final String name; | ||
|
|
||
| public Name(String name) { | ||
| validate(name); | ||
| this.name = name; | ||
| } | ||
|
|
| public long getId() { | ||
| return id; | ||
| } |
There was a problem hiding this comment.
[질문]
id의 필드는 Long (래퍼클래스)인데 id의 getter는 long(원시타입)인 이유가 궁금해
There was a problem hiding this comment.
약간의 의도를 담았는데 그 의도가 희석되었을 수도 있겠네
Reservation의 Id는 Null이 될 수도 있고 값을 담고 있을 수도 있어.
그런데 getId를 하는 시점에서는 id가 널이면 안된다는 의도를 담고 싶었고 결과적으로 위와 같은 메서드 형태가 등장하게 되었네 🤔
| @RequestMapping("/reservations") | ||
| @RestController |
There was a problem hiding this comment.
리비만의 애노테이션 순서 컨벤션이 있는지 궁금해
There was a problem hiding this comment.
없움 🫠
다른 거 신경쓰다가 놓친 부분이네
관련해서 아는 게 아무것도 없다 보니까 크루들과 토의 해보아야겠다는 생각이 드네 ㅎㅎ
짚어줘서 땡큐! 안 짚어줬으면 그냥 넘어갔을 듯
| import java.time.LocalTime; | ||
| import roomescape.entity.Reservation; | ||
|
|
||
| public class ReservationRequest { |
There was a problem hiding this comment.
dto를 record가 아닌 class로 설정한 이유가 궁금해
There was a problem hiding this comment.
사실 별차이 없다고 생각해
데이터 객체를 만드는 귀찮음을 감수하느냐 그렇지 않느냐가 현재에서 record를 고민하는 맥락이라고 생각함!
그런데 나는 눈에 보이는 쪽이 괜찮다는 생각이 들었네 🤔
record를 잘모르는 사람으로써 record 키워드를 보면 어떤 생성자가 있고 getter가 있는지 setter가 있는지 잘 기억이 나지 않아서리..
| date VARCHAR(255) NOT NULL, | ||
| time VARCHAR(255) NOT NULL, |
There was a problem hiding this comment.
date랑 time을 varchar로 두는 것보다 date 타입이랑 time 타입으로 두는건 어때?
There was a problem hiding this comment.
그러한 타입이 실제로 존재하는지도 몰랐다는 ㅋㅋ
VARCHAR에서의 변환도 부드럽게 이루어지더라
보면서 우와 신기하다라고만 했었음
관련해서 학습해봐야겠으!
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.boot.test.context.SpringBootTest; | ||
|
|
||
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT) |
There was a problem hiding this comment.
DEFINED_PORT로 설정해두면 application과 동시에 테스트를 실행할 수 없는 문제점이 발생할 수 있을 거 같아!
There was a problem hiding this comment.
test/resources/application.yml 에서 테스트용 포트 정의해주고, 테스트에선
@LocalServerPort
int port;
@BeforeEach
void beforeEach() {
RestAssured.port = port;
}이렇게 설정해주면 RestAssured 포트도 변경할 수 있어!
| private void validateNonNull(String name) { | ||
| if (name == null) { | ||
| throw new NullPointerException("예약자 이름은 Null이 될 수 없습니다"); | ||
| } | ||
| } |
There was a problem hiding this comment.
굳이 이 메서드가 없어도 validateLength 에서 name.length() 메서드를 호출할 때 NPE가 발생할 거 같은데 만약 name이 null이라면 그대로 NPE를 발생시키는게 아니라 IllegalArgumentException으로 처리하는건 어때?
There was a problem hiding this comment.
NPE에 메시지를 담는다는 맥락에서 단순 NPE발생보다는 효율적이라는 개인적인 생각!
다만 레디말처럼 다른 예외로 변환할 수도 있겠네
고민해보아야겠으!
| private void validateNonNull(LocalDateTime startTime) { | ||
| if (startTime == null) { | ||
| throw new NullPointerException("예약 시간은 Null이 될 수 없습니다"); | ||
| } | ||
| } |
There was a problem hiding this comment.
name의 validateNonNull이랑 동일한 이야기!
| public boolean isAnyReservationConflictWith(Reservation reservation) { | ||
| DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss"); | ||
| String startDateTime = reservation.getStartDateTime().format(formatter); | ||
| String endDateTime = reservation.getEndDateTime().format(formatter); | ||
|
|
||
| String sql = "select exists (" + | ||
| " select 1 " + | ||
| " from reservation " + | ||
| " where ? between (date || ' ' || time) and dateadd('HOUR', ?, (date || ' ' || time)) " + | ||
| " or ? between (date || ' ' || time) and dateadd('HOUR', ?, (date || ' ' || time)) " + | ||
| ") as exists_overlap;"; | ||
|
|
||
| boolean conflict = jdbcTemplate.queryForObject(sql, Boolean.class, endDateTime, | ||
| ReservationTime.RESERVATION_DURATION_HOUR, startDateTime, ReservationTime.RESERVATION_DURATION_HOUR); | ||
| return conflict; | ||
| } |
There was a problem hiding this comment.
예외 처리를 열심히 했네 👍
다만, 이 메서드가 너무 구체적이라는 생각이 드네! 재사용성을 생각해서 repository의 메서드는 좀 범용적인 것이 좋다고 생각해. 예를 들어 특정 범위에 있는 예약을 조회하는 방법이 있을 것 같아. 추가로 해당 sql문의 성능적인 측면도 궁금하고! 효율적인 sql문일까?
There was a problem hiding this comment.
해당 쿼리에 대한 평가가 절실했는데 망쵸가 달아주었군 😎
확실히 레포지토리레이어가 알기에 지나치게 비즈니스로직 종속적인 것 같아 좋은 피드백 땡큐!
sql문의 성능적인 측면은 어떨까.. 🤔
내가 sql 쌩초보이기도 해서 잘 모르겠긴 해.
다만 내가 아는 지식내에서 판단을 해볼게
우선 나는 sql의 성능은 Join의 횟수에서 많이 달라진다고 생각해
해당 sql문의 where절이 같은 연산을 여러번 하는 등 비효율적인 것 같지만 이는 큰 영향은 아니라고 생각함!
| this.reservationService = reservationService; | ||
| } | ||
|
|
||
| @GetMapping() |
There was a problem hiding this comment.
확실히 ()는 생략해도 좋을듯!
그리고 @RequestMapping으로 코드 중복을 줄이는 것도 좋은데, 메서드마다 URI 정보를 모두 포함해 주는 것은 어때? URI로 검색했을 때 어떤 메서드에서 처리하는 요청인지 찾을 수 있게!
No description provided.