로빈 1단계 제출 #8

robinjoon · 2024-05-02T13:43:08Z

원본 PR

이번단계 변경 사항

- 상태코드를 200 -> 201 변경 - Location 헤더 추가 Co-authored-by: zangsu <zangsu_@naver.com>

- 상태코드를 200 -> 204 변경 Co-authored-by: zangsu <zangsu_@naver.com>

Co-authored-by: zangsu <zangsu_@naver.com>

…시 에러 발생 구현 Co-authored-by: zangsu <zangsu_@naver.com>

Co-authored-by: zangsu <zangsu_@naver.com>

- 어차피 자원이 존재하지 않는 것은 동일하므로 문제가 없다고 결정 Co-authored-by: zangsu <zangsu_@naver.com>

Co-authored-by: zangsu <zangsu_@naver.com>

구현 Co-authored-by: zangsu <zangsu_@naver.com>

Co-authored-by: zangsu <zangsu_@naver.com>

- 테마 이름, 설명, 썸네일 이미자가 비어 있을 경우 에러 - 중복된 이름의 테마 생성 요청시 에러 Co-authored-by: zangsu <zangsu_@naver.com>

Co-authored-by: zangsu <zangsu_@naver.com>

- 이미 구현된 기능의 체크 리스트 누락된 것 작성 Co-authored-by: zangsu <zangsu_@naver.com>

Co-authored-by: zangsu <zangsu_@naver.com>

Step3 2

작업 컴퓨터 변경을 위한 PR

- 이름이 비어있지 않아야 한다는 조건은 모든 도메인이 공통적으로 가질 가능성이 그렇지 않을 가능성보다 훨씬 높다고 판단함.

- 식별자가 null 인 경우 이 getId 메서드를 호출할 때 예외가 발생하는 것이 타당하다고 판단.

- 변수명을 변경하는 것 대신 메서드를 분리하는 것이 더 가독성이 향상된다고 판단

- 이미 작성함.

- Controller 는 서비스의 코드를 그대로 호출하므로 테스트 작성이 필요하지 않다고 판단 - 단, 기존의 잘 동작하는 테스트 코드를 삭제하지는 않음

… 생각이 듭니다." 삭제 - 각자 추구하는 방향이 달라 페어 프로그래밍이 끝난 후 각자 개선하는 것으로 합의

- 도메인으로 이동할 수 있는 책임을 이동

- 각자 추구하는 방향이 달라 페어 프로그래밍이 끝난 후 각자 개선하는 것으로 합의

- 4~6 단계 인증 인가가 들어갈 때 고민이 해소 될 것이라 판단

hyxrxn

잘 읽었습니닷
다음 단계도 화이팅!

hyxrxn · 2024-05-03T05:13:41Z

README.md

+    - [x] 중복된 이름의 테마 생성 요청시 에러
+    - [x] 예약이 있는 테마를 삭제 요청시 에러
+
+- [ ] 사용자 예약 기능 추가


체크를 잊었나요....?!? ㅋㅋㅋㅋ

hyxrxn · 2024-05-03T05:14:45Z

src/main/java/roomescape/config/TimeFormatterConfig.java

+    private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd");
+    private static final DateTimeFormatter TIME_FORMATTER = DateTimeFormatter.ofPattern("HH:mm");


저도 이런걸 추가하려다 말았는데... 로빈은 이 부분을 따로 만들어둔 이유가 뭔가요?

저도 이런걸 추가하려다 말았는데... 로빈은 이 부분을 따로 만들어둔 이유가 뭔가요?

응답이나 요청의 시간 포맷이 API 마다 다른 경우는 거의 없다고 판단.

따라서 전역적으로 시간 포맷을 결정해야 한다고 생각했음.

만약, 특정 API에서 시간 포맷이 달라진다면 그곳에서만 추가적인 설정이 가능하기 때문에 대응이 가능함.

예외를 잡을 수 있음

Spring에서 핸들러의 메서드 파라미터를 역직렬화하는 과정에서 오류가 있다면 지정된 예외를 던짐.

그리고 그 예외는 ControllerAdvice 에서 잡을 수 있음.

따라서 잘못된 포맷이 입력되었을 경우의 예외 처리를 할 수 있음.

hyxrxn · 2024-05-03T05:15:36Z

src/main/java/roomescape/controller/AvailableTimeController.java

+import roomescape.service.AvailableTimeService;
+
+@RestController
+@RequestMapping("/availableTimes")


새로 컨트롤러를 만들었네요...!
저는 시간 목록을 준다는 의미에서 /times에서 관리하도록 두었는데요,
이렇게 나누면 좋은 점이 뭔가요?

새로 컨트롤러를 만들었네요...! 저는 시간 목록을 준다는 의미에서 /times에서 관리하도록 두었는데요, 이렇게 나누면 좋은 점이 뭔가요?

좋은 점이라기 보단, 기존의 방탈출 시작 시간 자원과 이 API가 제공해야하는 자원이 전혀 다른 자원이라고 생각했습니다. 왜냐면 이 API가 제공하는 자원은 "예약" 이라는 행위의 영향을 받기 때문입니다.

그런데 이 결정이 일반적인 URI 설계는 아닌 것 같아서 확인해보는 중입니다.

hyxrxn · 2024-05-03T05:21:28Z

src/main/java/roomescape/controller/ReservationController.java

+        return ResponseEntity.created(URI.create("/reservations/" + saved.id()))
+                .body(saved);


지금은 바디만 필요한 상태인데 URI까지 담아서 리턴하는 이유가 있을까요?

@ResponseStatus(HttpStatus.CREATED) 를 사용하면 ResponseEntity 없이도 201을 넘길 수 있답니닷...!
저는 이번에 알아서 이걸 사용해봤는데... 로빈은 알고 있지만 안썼을 수도 있겠다 싶어서 궁금하네용

지금은 바디만 필요한 상태인데 URI까지 담아서 리턴하는 이유가 있을까요?

@ResponseStatus(HttpStatus.CREATED) 를 사용하면 ResponseEntity 없이도 201을 넘길 수 있답니닷...! 저는 이번에 알아서 이걸 사용해봤는데... 로빈은 알고 있지만 안썼을 수도 있겠다 싶어서 궁금하네용

현재 테스트코드에 반영되었는지는 모르겠지만(워낙 시간이 없었어서...) Controller의 단위 테스트에서 상태코드를 검증할 수 있다는 장점도 있고, 가독성도 나쁘지 않아서 이렇게 했습니다.

hyxrxn · 2024-05-03T05:22:38Z

src/main/java/roomescape/controller/ThemeController.java

+    public List<ThemeResponse> findAndOrderByPopularity(@RequestParam LocalDate start,
+                                                        @RequestParam LocalDate end,
+                                                        @RequestParam int count) {


오오 세개를 다 받아서 했군요!
오늘 수업 때 잠깐 생각했지만... 변경에 정말 용이한 코드인 것 같네요!

hyxrxn · 2024-05-03T05:39:05Z

src/main/java/roomescape/service/ReservationService.java

    }

    public ReservationResponse save(ReservationRequest reservationRequest) {
-        Reservation saved = reservationRepository.save(reservationRequest);


저는 로직이 추가되지 않았더라도 레파지토리에 이 dto가 그대로 가면 안된다고 생각했는데, 첫 미션에서는 request를 그대로 보냈었네요
이유가 있나요?

저는 로직이 추가되지 않았더라도 레파지토리에 이 dto가 그대로 가면 안된다고 생각했는데, 첫 미션에서는 request를 그대로 보냈었네요 이유가 있나요?

woowacourse/spring-roomescape-admin#155 (comment) 이전 미션 리뷰어도 동일한 질문을 주셨는데요, 답변 참고해 주시면 될 것 같아요!

오오.... 저는 모 크루와 이야기해본 결과 아직 입력이 되지 않은 정보는 당연히 null이면 된다! 라는 결론을 얻었답니다
그래서

public Reservation toReservation() { return new Reservation( null, new Name(name), new Theme(themeId, null, null, null), new ReservationDate(date), new ReservationTime(timeId, (LocalTime) null)); }

이런 코드도 있어요.... ㅋㅋㅋㅋㅋㅋ
그냥 이런 생각도 있다는....!!!

hyxrxn · 2024-05-03T06:07:32Z

src/main/java/roomescape/repository/JdbcTemplateReservationRepository.java

        KeyHolder keyHolder = new GeneratedKeyHolder();
-        save(reservation, keyHolder);
+        save(beforeSaved, keyHolder);


이 메서드는 그냥 길어서 뺀건가요???

이 메서드는 그냥 길어서 뺀건가요???

네. 메서드가 길어지는데, 읽기 좋은 단위로 분리할 수 있을 것 같아서 분리했습니다.

hyxrxn · 2024-05-03T06:10:04Z

src/main/java/roomescape/repository/JdbcTemplateThemeRepository.java

+
+    @Override
+    public List<Theme> findAll() {
+        return jdbcTemplate.query("select ID, NAME, DESCRIPTION, THUMBNAIL from THEME", themeRowMapper);


이 쿼리문 대소문자가 좀 특이하네요......?!

이 쿼리문 대소문자가 좀 특이하네요......?!

인텔리제이 자동 생성 기능을 사용하기 위해서는 스키마를 등록해줘야 하는데, 중간에 그 작업을 해서 이전에 제가 작성한 코드와 인텔리제이가 자동 생성한 쿼리가 섞여서 이렇게 되었어요.

다음 PR에서 바꾸려구요

hyxrxn · 2024-05-03T06:11:06Z

src/main/java/roomescape/service/AvailableTimeService.java

+    //todo : 메서드 개선
+    public List<AvailableTimeResponse> findByThemeAndDate(LocalDate date, long themeId) {
+        Set<Long> alreadyReservedTimeIds = reservationRepository.findAll().stream()
+                .filter(reservation -> reservation.isDateOf(date))
+                .filter(reservation -> reservation.isThemeOf(themeId))
+                .map(Reservation::getReservationTime)
+                .map(ReservationTime::getId)
+                .collect(Collectors.toSet());


예약을 db에서 전부 가져오는 것부터가... 문제이지 않을지...........

예약을 db에서 전부 가져오는 것부터가... 문제이지 않을지...........

맞아요. 원래대로면 절대 이렇게 안할텐데 이번 미션에 다른 중요한 일들이랑 겹치는 바람에 시간이 없어서 이렇게 되었어요. ㅠㅠ

hyxrxn · 2024-05-03T06:12:46Z

src/main/java/roomescape/dto/AvailableTimeResponse.java

+
+import java.time.LocalTime;
+
+public record AvailableTimeResponse(long id, LocalTime startAt, boolean isBooked) {


저는 time 도메인에 booked를 추가했는데 이 방법이 더 좋아보이네요!

robinjoon and others added 30 commits April 30, 2024 14:30

feat: 이전 미션 코드 가져오기

64335cd

docs: 요구사항 변경에 따른 README.md 수정

9d575fe

fix: 시간 생성 API 응답 변경

258c9b6

- 상태코드를 200 -> 201 변경 - Location 헤더 추가 Co-authored-by: zangsu <zangsu_@naver.com>

fix: 예약 생성 API 응답 변경

dbb1b8b

- 상태코드를 200 -> 201 변경 - Location 헤더 추가 Co-authored-by: zangsu <zangsu_@naver.com>

fix: 예약, 예약 시간 삭제 API 응답 변경

202216d

- 상태코드를 200 -> 204 변경 Co-authored-by: zangsu <zangsu_@naver.com>

fix: 누락된 설정 파일 추가

bae62c9

Co-authored-by: zangsu <zangsu_@naver.com>

fix: hh:MM 형식이 아닌 시간 생성 요청시 예외 처리

fc59282

Co-authored-by: zangsu <zangsu_@naver.com>

refactor : Repository 가 DTO를 사용하지 않도록 변경

65733d0

Co-authored-by: zangsu <zangsu_@naver.com>

feat : ReservationTime 의 startAt 에 대한 null 검증 추가 및 존재하지 않는 시간에 예약 생성 …

1bbadab

…시 에러 발생 구현 Co-authored-by: zangsu <zangsu_@naver.com>

feat : 중복된 예약 시간 생성 요청 시 에러 구현

a14d390

Co-authored-by: zangsu <zangsu_@naver.com>

feat : 예약이 있는 예약 시간을 삭제 요청 시 에러 구현

2da2b7a

Co-authored-by: zangsu <zangsu_@naver.com>

docs : 존재하지 않는 자원을 삭제하는 경우 예외처리 삭제

4f222f4

- 어차피 자원이 존재하지 않는 것은 동일하므로 문제가 없다고 결정 Co-authored-by: zangsu <zangsu_@naver.com>

feat: 동일한 날짜와 시간에 예약 생성 요청 시 에러 구현

6a4ebb7

Co-authored-by: zangsu <zangsu_@naver.com>

feat: ISO 8601 표준에 따른 YYYY-MM-dd 포맷에 해당하지 않는 날짜가 포함된 예약 생성 요청 시 에러

60e2f88

구현 Co-authored-by: zangsu <zangsu_@naver.com>

feat: 지나간 날짜와 시간의 예약 요청 시 에러 구현

80b2371

Co-authored-by: zangsu <zangsu_@naver.com>

feat: 이름이 비어있는 예약 요청 시 에러 구현

a5ca6d4

Co-authored-by: zangsu <zangsu_@naver.com>

refactor: 커스텀 예외 도입

36fe37e

Co-authored-by: zangsu <zangsu_@naver.com>

docs: api 명세 수정

3e37f25

Co-authored-by: zangsu <zangsu_@naver.com>

docs: api 명세 추가

9d83ffe

Co-authored-by: zangsu <zangsu_@naver.com>

docs: 기능 목록 추가

6f1be1a

Co-authored-by: zangsu <zangsu_@naver.com>

feat: 변경된 페이지로 반환하도록 수정

c75c485

Co-authored-by: zangsu <zangsu_@naver.com>

feat: 테마 CRD 구현

4d4c985

- 테마 이름, 설명, 썸네일 이미자가 비어 있을 경우 에러 - 중복된 이름의 테마 생성 요청시 에러 Co-authored-by: zangsu <zangsu_@naver.com>

feat: 기존 에약 API 에 테마 추가

44c1974

Co-authored-by: zangsu <zangsu_@naver.com>

docs: 누락된 문서 작성

af7f7e6

- 이미 구현된 기능의 체크 리스트 누락된 것 작성 Co-authored-by: zangsu <zangsu_@naver.com>

docs: 이미 해결한 Todo 삭제

31df8bd

Co-authored-by: zangsu <zangsu_@naver.com>

docs: 이미 해결한 Todo 삭제

564ecaf

Co-authored-by: zangsu <zangsu_@naver.com>

fix: equals 메서드가 id만으로 비교하도록 수정

550f4cd

Co-authored-by: zangsu <zangsu_@naver.com>

fix: 중복 예약 검증 로직이 테마도 확인하도록 변경

d1c9bee

Co-authored-by: zangsu <zangsu_@naver.com>

docs: 사용자 예약 기능 및 API 명세 추가

1cfec9a

Co-authored-by: zangsu <zangsu_@naver.com>

feat: 사용자 예약 기능 구현

37c3bc1

Co-authored-by: zangsu <zangsu_@naver.com>

robinjoon and others added 18 commits May 2, 2024 11:50

Merge pull request #2 from robinjoon/step3-2

b55ca91

Step3 2

refactor: ExceptionType 네이밍 변경

2789dae

Merge pull request #3 from zangsu/robinjoon

c6f2992

작업 컴퓨터 변경을 위한 PR

refactor: ResultSet 사용 방식 변경

6ec052d

docs: 해결한 미션 가이드 Todo 삭제

ad587b5

docs: "TODO : NAME_EMPYT 재사용 고민" 삭제

ddf88b1

- 이름이 비어있지 않아야 한다는 조건은 모든 도메인이 공통적으로 가질 가능성이 그렇지 않을 가능성보다 훨씬 높다고 판단함.

docs: "//todo : long 으로 변경" 삭제

e36c497

- 식별자가 null 인 경우 이 getId 메서드를 호출할 때 예외가 발생하는 것이 타당하다고 판단.

docs: "//todo : 변수명 고민" 삭제

3961bb6

- 변수명을 변경하는 것 대신 메서드를 분리하는 것이 더 가독성이 향상된다고 판단

docs: "//TODO : 테스트 생성" 삭제

1378a92

- 이미 작성함.

docs: "//Todo 테스트코드 작성" 삭제

093f68b

- Controller 는 서비스의 코드를 그대로 호출하므로 테스트 작성이 필요하지 않다고 판단 - 단, 기존의 잘 동작하는 테스트 코드를 삭제하지는 않음

docs: "todo [로빈] 이 테스트를 작성하다 보니 Response dto 에도 NotNull 검증은 필요할 것 같다는…

1024c7e

… 생각이 듭니다." 삭제 - 각자 추구하는 방향이 달라 페어 프로그래밍이 끝난 후 각자 개선하는 것으로 합의

docs: "TODO : 도메인에게 넘길 수 있을 것 같은데" 삭제

258466c

- 도메인으로 이동할 수 있는 책임을 이동

docs: "Todo : [로빈] Response 가 변환 로직을 가지고 있으면 아래 코드도 간단해 질 것 같음" 삭제

8c4efbc

- 각자 추구하는 방향이 달라 페어 프로그래밍이 끝난 후 각자 개선하는 것으로 합의

refactor : 도메인으로 이동할 수 있는 책임을 이동

c7c5033

docs: "Todo 상태코드 고민해보기"

0391803

- 4~6 단계 인증 인가가 들어갈 때 고민이 해소 될 것이라 판단

refactor: getter 대신 도메인 로직을 사용하도록 변경

c7f2dad

docs: 해결된 todo 삭제

2416896

style: 컨벤션 적용

e8e7a6f

hyxrxn reviewed May 3, 2024

View reviewed changes

robinjoon added 11 commits May 4, 2024 16:00

docs: 누락된 체크 표시 추가

62cef24

docs: 리팩토링 대상 목록 정리

1561f7e

refactor: RowMapper 구현 클래스 추가 및 sql 스타일 통일

c9b2f37

refactor: Stream 대신 SQL을 사용하도록 수정

92e8519

style: 길이가 긴 SQL 문을 텍스트 블록으로 표현

a1ed68a

test: 인기 테마 조회 기능 테스트 추가

5d02747

test: 누락된 Repository 테스트 추가

eb38c7d

docs: 리팩토링 목록 수정

9881fc7

feat: 테마의 thumbnail 이 url 형태인지 검증하는 기능 추가

239c241

refactor: 정적 페이지를 반환하는 컨트롤러 이름 수정

20fddde

refactor: 예약 가능한 시간 조회 API 명세 수정

e12b17e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

로빈 1단계 제출 #8

로빈 1단계 제출 #8

robinjoon commented May 2, 2024 •

edited

Loading

hyxrxn left a comment

hyxrxn May 3, 2024

robinjoon May 4, 2024

hyxrxn May 3, 2024

robinjoon May 4, 2024

hyxrxn May 3, 2024

robinjoon May 4, 2024

hyxrxn May 3, 2024

robinjoon May 4, 2024

hyxrxn May 3, 2024

hyxrxn May 3, 2024

robinjoon May 4, 2024

hyxrxn May 4, 2024

hyxrxn May 3, 2024

robinjoon May 4, 2024

hyxrxn May 3, 2024

robinjoon May 4, 2024

hyxrxn May 3, 2024

robinjoon May 4, 2024

hyxrxn May 3, 2024

		private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd");
		private static final DateTimeFormatter TIME_FORMATTER = DateTimeFormatter.ofPattern("HH:mm");

		return ResponseEntity.created(URI.create("/reservations/" + saved.id()))
		.body(saved);


		import java.time.LocalTime;

		public record AvailableTimeResponse(long id, LocalTime startAt, boolean isBooked) {

로빈 1단계 제출 #8

Are you sure you want to change the base?

로빈 1단계 제출 #8

Conversation

robinjoon commented May 2, 2024 • edited Loading

hyxrxn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinjoon commented May 2, 2024 •

edited

Loading