[1 - 3단계 방탈출 사용자 예약] 리비(이근희) 미션 제출합니다. #3
Conversation
| private void validateNonPastDate(LocalDate date) { | ||
| if (date.isBefore(LocalDate.now())) { | ||
| throw new IllegalArgumentException(date + ": 예약 날짜는 현재 보다 이전일 수 없습니다"); | ||
| } | ||
| } |
There was a problem hiding this comment.
나는 이 로직을 service 계층에서 이루어졌는데, 리비의 의견이 궁금!
체스 미션으로 예를 들어보면 piece는 현재 위치를 알 필요가 없고, piece의 위치를 아는 것은 board가 자연스러운 흐름같은데, 비슷한 맥락으로 ReservationDate가 현재 시각을 알 필요가 없지 않을까?
There was a problem hiding this comment.
ReservationDate 도메인을 어떻게 해석하는지에 따라 달라지는 문제라고 생각!
예약 날짜라는 도메인을 데이터만 담고있는 것으로 생각한다면 service계층에서의 검증이 적절한 것 같아
하지만 위같은 형태는 도메인 주도 개발이라기보다는 데이터 주도 개발에 가깝다고 생각해
도메인 주도 개발측면에서 예약 날짜는 과거일 수 없다라는 명세는 ReservationDate관련 로직이라고 생각해!
서비스는 물론 비즈니스 로직을 담는 계층이고 범용적으로 운용될 수 있지만 각각의 도메인 명세는 도메인 클래스에 있어야 자연스럽지 않나 하는 개인적인 생각이야 🧐
| private String name; | ||
| private String description; | ||
| private String thumbnail; | ||
|
|
||
| ThemeAddRequest() { | ||
|
|
||
| } |
There was a problem hiding this comment.
필드에 final을 붙여줘도 될 거 같은데 기본 생성자가 있는거 보니 역직렬화 문제를 겪은거 같군!
필드에 final 붙여주고 기본 생성자 값에 null을 넣는건 어때?
ThemeAddRequest() {
this.name = null;
this.description = null;
this.thumbnail = null;
}There was a problem hiding this comment.
그런 방법도 있군 👍
레디가 제시해준 코드가 의도를 잘 드러낼지는 의문이긴해
null을 생성자에서 초기화하는 것은 후에 리플렉션으로 값이 들어와서 바뀔 것을 아는 사람에게는 의도가 전달될 수 있지만 그렇지 않은 사람들에게는 의도가 한번 희석되지 않을까?
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.ExceptionHandler; | ||
| import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
|
|
||
| @RestControllerAdvice | ||
| public class GlobalExceptionHandler { | ||
| private final Logger log = LoggerFactory.getLogger(getClass()); |
| + " r.id as reservation_id, " | ||
| + " r.name, " | ||
| + " r.date, " | ||
| + " t.id as time_id, " | ||
| + " t.start_at as time_value , " | ||
| + " theme.id as theme_id, " | ||
| + " theme.name as theme_name, " | ||
| + " theme.description as theme_description, " | ||
| + " theme.thumbnail as theme_thumbnail " |
There was a problem hiding this comment.
텍스트 블록 사용하는게 가독성이 좀더 좋을 거 같아! 지저분한 '+'을 없앨 수 있고
String sql = """
SELECT R.ID AS 생략
"""There was a problem hiding this comment.
레디의 코멘트는 항상 도움이 되는 군 👍
부끄럽지만 텍스트 블록이라는 개념을 모르고 있었어
적용해볼게 💪🏻
| import org.springframework.jdbc.core.JdbcTemplate; | ||
| import org.springframework.jdbc.core.RowMapper; | ||
| import org.springframework.jdbc.core.simple.SimpleJdbcInsert; | ||
| import org.springframework.stereotype.Repository; | ||
| import roomescape.domain.Reservation; | ||
| import roomescape.domain.ReservationTime; | ||
| import roomescape.dto.ReservationAddRequest; | ||
| import roomescape.domain.Theme; | ||
|
|
||
| @Repository | ||
| public class ReservationDaoImpl implements ReservationDao { |
There was a problem hiding this comment.
인터페이스 구현체 네이밍에 Impl 을 붙이는건 헝가리안 표기법의 잔재라 지양해야한다고 하더라구!
https://stackoverflow.com/questions/2814805/java-interfaces-implementation-naming-convention
There was a problem hiding this comment.
오호 구현체가 한개인 경우는 Impl을 붙이는 것이 널리 통용되는 관례라고 생각했는데 아니었구나
좋은 글 추천 감사
| @@ -8,14 +9,15 @@ | |||
| import java.util.concurrent.atomic.AtomicLong; | |||
| import roomescape.domain.Reservation; | |||
| import roomescape.domain.ReservationTime; | |||
| import roomescape.dto.ReservationAddRequest; | |||
| import roomescape.domain.Theme; | |||
| import roomescape.repository.ReservationDao; | |||
|
|
|||
| public class FakeReservationDao implements ReservationDao { | |||
There was a problem hiding this comment.
나는 미션 2 진행하면서 fake 객체를 만드는 것을 지양해야겠다고 마음을 먹었는데, 리비가 이 fake 객체를 사용한 이유가 궁금해!
우선 내 의견부터 말해보자면, 만약 FakeReservationDao 테스트가 실패한다면 fake 객체 구현에 문제가 있는 것인지 아니면 실제 객체에 문제가 있는 것인지 한번에 파악이 힘들다는 것이 첫번째 이유야. 그리고 또 이번 요구사항중 인기 테마 조회 같은 요구사항이 추가되었는데, 실제 객체에선 db를 사용하기 때문에 조회 sql 쿼리를 날리지만 이 fake 객체에선 Map을 바라보기 때문에 온전히 잘 테스트가 되었나 파악이 힘든 거 같아.
다시말해 findByDateAndTheme와 findThemeOrderByReservationCount 메서드를 구현하기가 어려워져
There was a problem hiding this comment.
우선은 나도 레디와 비슷한 이유로 슬라이스 테스트를 별로 좋아하지 않는다는 점을 말해야겠으 (이전 미션에서 통합테스트로 진행했었음)
그런데 페어같은 경우에는 fake를 사용했고 이를 따라가게 된 구현이야.
관련해서 느낀 개인적인 소회가 있는데 나는 자신의 선호에 맞는 기술만을 공부하면 안되겠다고 느꼈어.
슬라이스 테스트를 싫어하는 나지만 슬라이스 테스트를 진행해야 했던 것처럼 우리는 선호하는 맥락의 개발만 진행할 수는 없을 것 같아.
여러 트레이드 오프 맥락에 대해서는 아직 학습중이야!
다만 fake 객체를 사용하면 서비스 로직을 슬라이스로 테스트 할 수 있다는 장점은 명실상부한 것 같음!
레포지토리단의 검증이 필요하면 그것은 Repository 테스트의 역할이라고 생각하고!
| } | ||
|
|
||
| private void validateNonBlank(String name) { | ||
| if (name == null || name.isEmpty()) { |
There was a problem hiding this comment.
" " 과 같은 값도 검증이 필요할 거 같은데 isEmpty() 대신 isBlank()를 사용하면 가능!
| private final LocalTime startAt; | ||
| private final Long timeId; | ||
| private final boolean alreadyBooked; |
There was a problem hiding this comment.
사소하지만 timeId가 startAt 보다 먼저 위치하는게 어때?
| public class GlobalExceptionHandler { | ||
| private final Logger log = LoggerFactory.getLogger(getClass()); | ||
|
|
||
| @ExceptionHandler(value = {NullPointerException.class, |
There was a problem hiding this comment.
여기 적혀있는 예외 클래스들이 우리가 던지는 예외만 잡는게 아니라 의도치 않은 내부 문제로도 발생할 수 있는 예외들인데 400으로 반환해도 괜찮을까?
There was a problem hiding this comment.
커스텀 예외가 등장할 필요가 있다고 생각함!
우선은 구현이 급한관계로 뛰어넘었다는 ..
| + " from reservation as r " | ||
| + " inner join theme as th " | ||
| + " on r.theme_id = th.id " | ||
| + " where r.date between dateadd('day', -7, current_date()) and dateadd('day', -1, current_date()) " |
There was a problem hiding this comment.
이렇게 dateadd 메서드를 활용하는 방법도 있구나 👍
There was a problem hiding this comment.
근데 일주일이라는 기간이 하드코딩되어있어서 서비스 맥락과 db맥락을 적절히 분리하지 못했다는 개인적인 평가야
좋지는 않은 듯..!
| @PathVariable("date") LocalDate date, | ||
| @PathVariable("themeId") Long themeId) { |
There was a problem hiding this comment.
많은 방법 중에 왜 PathVariable 을 사용하는 API 설계를 했어?
There was a problem hiding this comment.
지금 다시 짠다면 쿼리파라미터를 이용하도록 할 듯!
구현이 급해서 api 설계를 대충한 부분이야
오늘 강의 들어보니 이러한 부분이 이번 미션의 핵심이었던 것 같은데 반성 중 .. 😭
| @GetMapping("/reservations/theme-rank") | ||
| public ResponseEntity<List<Theme>> getThemeRank() { | ||
| return ResponseEntity.ok(reservationService.getThemeRanking()); | ||
| } |
There was a problem hiding this comment.
이미 피드백 강의에서 인지했겠지만, 요구사항이 조금만 변경되도 많은 수정이 필요할 것 같아!
There was a problem hiding this comment.
ThemeController에서 이걸 구현해야 할 것 같은데 왜 여기서 했어?
There was a problem hiding this comment.
자원간의 연관관계를 잘못해석했어 😂
사실 자원이라는 개념도 모르고 구현했던 것이쥐
현재는 uri가 자원의 식별자임을 알게되었고 개선이 필요한 점 인지하게 되었음 👍
| private Theme getTheme(ReservationAddRequest reservationAddRequest) { | ||
| return themeDao.findById(reservationAddRequest.getThemeId()) | ||
| .orElseThrow(() -> new IllegalArgumentException("존재 하지 않는 테마로 예약할 수 없습니다")); | ||
| } |
There was a problem hiding this comment.
리비도 "데이터베이스의 내용을 확인해야 하는 검증"은 Service에서 하는 것으로 정한거야?
There was a problem hiding this comment.
별다른 방법이 없다고 생각하긴 해!
영속성 레이어와 상호작용하는 레이어는 서비스레이어일테니까 👀
| public List<Theme> getThemeRanking() { | ||
| return reservationDao.findThemeOrderByReservationCount(); | ||
| } |
There was a problem hiding this comment.
예약에 의해 랭킹이 결정되니까 ReservationService에서 하는거야?
There was a problem hiding this comment.
그런 사고흐름이긴 했으!
그런데 그러다 보니 Reservation이 슈퍼클래스가 된 것 같더라
자원에 대한 개념을 알게 되었고 다른 크루들의 코드를 보니 어떻게 분리시키는 지 알 것 같더라
| if (themeDao.findById(id).isEmpty()) { | ||
| throw new IllegalArgumentException("해당 id를 가진 테마가 존재하지 않습니다."); | ||
| } |
There was a problem hiding this comment.
사용자의 요청에 의해 테마가 지워진거랑, 이미 테마가 지워져있는거랑 결국 테마가 없다는 결과는 동일한데, 예외로 처리한 이유가 뭐야?
There was a problem hiding this comment.
클라이언트가 있는 줄 알고 있었다는 의미라고 받아들였어
그렇다면 이는 예외상황이라고 판단한 부분이야
| time_id BIGINT, | ||
| theme_id BIGINT, |
마이그레이션 제외 링크