[1 - 3단계 방탈출 사용자 예약] 이든(최승준) 미션 제출합니다. #57
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| ## 예외 상황 | ||
|
|
||
| ### 예약 | ||
|
|
||
| - [x] 예약 생성 시 예약자명, 날짜, 시간에 유효하지 않은 값이 입력할 수 없다. | ||
| - null, 빈 값, 날짜 / 시간 포맷 | ||
| - [x] 중복 예약 추가는 불가능하다. | ||
| - [x] 지나간 날짜와 시간에 대한 예약 생성은 불가능하다. | ||
|
|
||
| ### 시간 | ||
|
|
||
| - [x] 시간 생성 시 시작 시간에 유효하지 않은 값이 입력할 수 없다. | ||
| - null, 빈 값, 날짜 / 시간 포맷 | ||
| - [x] 중복 시간 추가는 불가능하다. | ||
| - [x] 특정 시간에 대한 예약이 존재하면, 그 시간을 삭제할 수 없다. | ||
|
|
||
| ### 공통 | ||
|
|
||
| - [ ] 존재하지 않는 API로 요청을 보낼 수 없다. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| distributionBase=GRADLE_USER_HOME | ||
| distributionPath=wrapper/dists | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.1.1-all.zip | ||
| zipStoreBase=GRADLE_USER_HOME | ||
| zipStorePath=wrapper/dists |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package roomescape.controller; | ||
|
|
||
| import org.springframework.stereotype.Controller; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
|
|
||
| @Controller | ||
| public class ClientController { | ||
|
|
||
| @GetMapping("/") | ||
| public String readPopularThemePage() { | ||
| return "index"; | ||
| } | ||
|
|
||
| @GetMapping("/reservation") | ||
| public String readReservationPage() { | ||
| return "reservation"; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,19 @@ | ||
| package roomescape.controller; | ||
|
|
||
| import java.net.URI; | ||
| import java.time.LocalDate; | ||
| import java.util.List; | ||
| import org.springframework.format.annotation.DateTimeFormat; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.DeleteMapping; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.PostMapping; | ||
| import org.springframework.web.bind.annotation.RequestBody; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RequestParam; | ||
| import org.springframework.web.bind.annotation.RestController; | ||
| import roomescape.dto.reservation.ReservationAvailableTimeResponse; | ||
| import roomescape.dto.reservation.ReservationRequest; | ||
| import roomescape.dto.reservation.ReservationResponse; | ||
| import roomescape.service.ReservationService; | ||
|
|
@@ -31,6 +35,15 @@ public ResponseEntity<List<ReservationResponse>> readReservations() { | |
| return ResponseEntity.ok(reservationResponses); | ||
| } | ||
|
|
||
| @GetMapping("/themes/{themeId}") | ||
| public ResponseEntity<List<ReservationAvailableTimeResponse>> readAvailableTimeReservations( | ||
|
There was a problem hiding this comment. 이 부분은 List로 return하는 것도 괜찮을까요? There was a problem hiding this comment. 이전 크루의 리뷰를 복붙 한번 하겠습니당. 응답객체는 Collection이 아닌 Object 형식으로 내려주는게 유연한 설계입니다. 최근에 끄덕끄덕하면서 본 아티클인데, Don’t return arrays as top-level responses. 라는 조언이 이 리뷰에 해당합니다. There was a problem hiding this comment. 만약 지금처럼 List로 전송하는 API가 있을 때, 거기까지는 생각해보지 못했는데 정말 좋은 의견인 것 같습니다. There was a problem hiding this comment. 이때 Endpoint를 통해 버전을 관리하는 방식(ex. There was a problem hiding this comment.
이 부분이 말씀해주시는 API 버저닝과 관계있습니다 ㅎ 외부사에 API를 제공하는 경우 API 버전을 올렸다고 맞춰달라고 하는것조차 쉽지 않기 때문입니다 (상대 회사의 개발력을 투입해야함) 인하우스 기능(사내 서버만 활용하는 API)이라고 쉽냐고 물어보면 그렇지도 않습니다. 회사 전체 채널로 3개월씩 공지해도 각자의 일로 바쁘므로 하위버전 API를 계속 사용하는 팀이 나옵니다 손쉬운 해결책으로 보여도 매우 고난한 길이니 가급적 가시지 않기를 권합니다 ㅋㅋ There was a problem hiding this comment. 실무의 세계란 정말 고려할 사항들이 많네요! |
||
| @PathVariable | ||
| Long themeId, | ||
| @RequestParam | ||
| @DateTimeFormat(pattern = "yyyy-MM-dd") LocalDate date) { | ||
|
|
||
| return ResponseEntity.ok(reservationService.findReservationByDateAndThemeId(date, themeId)); | ||
| } | ||
|
|
||
| @PostMapping | ||
| public ResponseEntity<ReservationResponse> createReservation(@RequestBody ReservationRequest reservationRequest) { | ||
| ReservationResponse reservationResponse = reservationService.createReservation(reservationRequest); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| package roomescape.controller; | ||
|
|
||
| import java.net.URI; | ||
| import java.util.List; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.DeleteMapping; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.PostMapping; | ||
| import org.springframework.web.bind.annotation.RequestBody; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RequestParam; | ||
| import org.springframework.web.bind.annotation.RestController; | ||
| import roomescape.dto.theme.ThemeRequest; | ||
| import roomescape.dto.theme.ThemeResponse; | ||
| import roomescape.service.ThemeService; | ||
|
|
||
| @RestController | ||
| @RequestMapping("/themes") | ||
|
There was a problem hiding this comment. 현재는 모든 컨트롤러 코드들이 이렇게 공통 endpoint를 묶어 관리하고 있는 형태인데요. There was a problem hiding this comment. ㅋㅋ 동의합니다 |
||
| public class ThemeController { | ||
|
|
||
| private final ThemeService themeService; | ||
|
|
||
| public ThemeController(ThemeService themeService) { | ||
| this.themeService = themeService; | ||
| } | ||
|
|
||
| @GetMapping | ||
| public ResponseEntity<List<ThemeResponse>> readThemes() { | ||
| List<ThemeResponse> themeResponses = themeService.findAllThemes(); | ||
|
|
||
| return ResponseEntity.ok(themeResponses); | ||
| } | ||
|
|
||
| @GetMapping("/top") | ||
| public ResponseEntity<List<ThemeResponse>> readTopNThemes(@RequestParam int count) { | ||
|
There was a problem hiding this comment. 현재는 거의 모든 메서드의 인자에 final 처리가 되어있지 않은 상태인데요. There was a problem hiding this comment. 전 모든 파라미터 / 지역 변수 final에 대해 부정적인 편입니다 인자 final 규칙은 모든 코드에 적용해야만 비로소 효과가 생기는데 (final이 붙어 있지 않으면 가변임을 예상할 수 있음) 자동으로 처리가 안 되다 보니 팀원들에게 강요하기가 어렵습니다. 의식하고 있어도 자주 빠지기도 하구요. There was a problem hiding this comment. 저도 다 처리해줄 게 아니면 오히려 의도가 불분명한 코드가 될 수도 있다는 생각이 있는데요 인텔리제이에 자동으로 final 키워드를 추가해주는 기능이 있기도하고 개인적으로 사이드이펙트를 방지하는데에 좋다는 생각이 들어서 |
||
| List<ThemeResponse> themeResponses = themeService.findTopNThemes(count); | ||
|
|
||
| return ResponseEntity.ok(themeResponses); | ||
| } | ||
|
|
||
| @PostMapping | ||
| public ResponseEntity<ThemeResponse> createTheme(@RequestBody ThemeRequest request) { | ||
| ThemeResponse response = themeService.createTheme(request); | ||
|
|
||
| return ResponseEntity.created(URI.create("/themes/" + response.id())) | ||
| .body(response); | ||
| } | ||
|
|
||
| @DeleteMapping("/{id}") | ||
| public ResponseEntity<Void> deleteTheme(@PathVariable Long id) { | ||
| themeService.deleteTheme(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.
API URI를 최대한 RESTful하게 설계해보려고 노력했는데요.
그 중에서도
/reservations컬렉션 내부에 서브 컬렉션인/themes를 넣어준 이 URI Endpoint는 어떻게 설계하면 좋을 지 고민이 많이 되었던 부분입니다.자원을 적절하게 명시해주고 있는 지, 그리고 변경해야할 것 같다면 어떻게 변경하면 좋을 지 웨지의 의견이 궁금합니다!
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 와 theme는 종속관계로 생각하시는데 times는 별도로 다루는 이유가 있을까요?
리소스간 관계 설정에 통일성이 있었으면 좋겠습니다.
전 API 설계는 적당히 프론트랑 저희 팀원들만 알아볼 수 만 있으면 된다는 생각입니다.
그리고 전 요구사항 변경이 매우 빈번한 서비스를 관리하는 개발자기 때문에 서비스의 하위호환성을 중시하는 경항성이 있습니다. 그래서 API를 최초에 완전하게 설계하는 것에는 큰 관심이 없고 하위버전과 신규버전의 API를 호환해서 운영하는 것에 관심이 많습니다.
그래서 reservtions prefix가 있냐 마냐는 저에겐 이러든 저러든 큰 상관은 없다고 생각하나, 통일성 없는 규칙은 모든 사람(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.
결론적으로 얻고자하는건 시간까지 포함된 예약 여부와 예약 시간들인데
times가 빠지니 어색하게 느껴지긴 하네요!충분히 고려하지 못한 것 같네요 🥲 짚어주셔서 감사합니다!
/reservations/themes/{themeId}/times?date이렇게 변경해보면 좋을까요?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/themes만 있고/themes는 없음/times는 있음-> 리소스 상 theme는 reservation의 하위 개념으로 명시되는데 times는 별도 리소스로 취급한 것으로 생각
그런데 말씀주신것처럼 해당 API의 응답 결과가 themes가 아닌 theme의 시간인거 같아서 말씀주신 방향으로 수정하는게 좋을거 같네요 (뒷걸음질 치다 쥐 잡았네요ㅎ)