-
Notifications
You must be signed in to change notification settings - Fork 56
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
[Spring Data JPA] 정상희 미션 제출합니다. #109
Conversation
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.
리뷰가 늦어져서 죄송합니다..🙇♂️
JPA도 잘 활용하시고 미션 하시느라 고생하셨어요!
몇가지 코멘트 남겨두었으니 확인해주세요!!
추가로 남겨주신 질문에 대해 답변드릴게요.
List<Waiting> findWaitingsWithRankByMemberId(Long memberId);
이렇게 만들고 실행하는데 제 로컬에서는 아무 오류가 발견 되지 않았어요...! 제가 제대로 재현한게 맞죠..?
남겨주신 오류는 @Query
에 사용된 바인딩 파라미터가 메서드에 있는 파라미터와 달라서 발생하는 오류이긴 합니다.
질문 2)
관리자가 어드민 화면에서 예약을 생성하는 경우 name 값을 string으로 전달합니다. 저 요구사항의 경우 관리자 페이지에서 name만으로 예약을 추가한다면 무조건 name으로만 Member를 조회할 수밖에 없을 것 같은데 동명이인의 경우는 고려하지 않는 걸까요??
예리하시네요! 미션 설계가 그렇게 되어 있지만, 상희님의 고려사항은 굉장히 중요한 것 같습니다. 이름처럼 unique하지 않은 값으로 조회를 할 때, 항상 고려해야하는 사항입니다!
어드민 페이지에서 추가하면 회원 id를 조회하기 위해 입력 받은 name으로 조회를 하면 이름이 같은 회원이 있다면 오류가 발생하겠네요. 조회 조건에 권한을 추가하여 조금은 오류 발생 가능성을 줄일 수 있을 것 같아요.
말씀대로 동명이인에 대한 예외 가능성이 존재하지만, 우선 미션에서는 고려하지 않고 진행 하시죠!
|
||
@Entity | ||
@Getter | ||
public class Waiting { |
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.
심화 단계도 구현 하셨군요!👍👍
고생하셨습니다.
다만, Waiting 객체를 따로 두어야 하는지 의문이에요! Reservation에 '상태' 필드를 추가하여 대기중인 예약과 일반 예약을 구분할 수 있지 않을까 생각이 듭니다.
Reservation과 구분 지은 이유가 있다면 알려주세요!
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.
미션에서 waiting 클래스를 통한 repository 쿼리가 주어져서 자연스럽게 둘을 분리하게 된 것도 있고, 둘의 특성이 약간 다른 것 같아서 분리했습니다..!
reservation은 특성상 예약이 확정되면 그 뒤로 그 상태를 유지하는 데이터인 반면,
waiting은 예약이 확정되기 전까지 대기 순번에 따라 동적으로 변하고, 예약 가능 상태가 되면 대기 목록에서 제거되거나 예약으로 전환되는 데이터라 약간 둘의 생성 및 유지 특성이 다르다고 생각했습니다.
그래서 waiting 클래스를 reservation과 분리했을 때의 장단점을 고민해봤습니다.
장점 :
- 대기 목록에 요구사항이 추가될 때 수정에 용이하며 각각의 책임과 행동이 명확해짐
단점
- 관리 포인트가 두 곳에 분산되어 관리하기 어려움 (실제로 리뷰 반영할 때 두 곳을 모두 수정해야 했음)
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.
좋습니다. 어떤 부분을 고려하고 나누었는지 이해했어요!
상희님 의견대로 "대기예약"이라는 도메인에 요구사항이 추가되는 상황을 고려한다면, 분리하는 것이 매우 유리 하겠네요.
장단점과 함께 상희님 의견을 제시하신 점이 인상 깊어요. 감사합니다👍
…basic-roomescape-playground into sanghee-jpa # Conflicts: # src/main/java/roomescape/auth/AuthRoleInterceptor.java # src/main/java/roomescape/member/Member.java # src/main/java/roomescape/member/MemberService.java # src/main/java/roomescape/reservation/MyReservationResponse.java # src/main/java/roomescape/reservation/Reservation.java # src/main/java/roomescape/reservation/ReservationController.java # src/main/java/roomescape/reservation/ReservationRepository.java # src/main/java/roomescape/reservation/ReservationService.java # src/main/java/roomescape/waiting/Waiting.java # src/main/java/roomescape/waiting/WaitingController.java # src/main/java/roomescape/waiting/WaitingService.java
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.
빠르게 잘 반영해주셨군요!
고생 많으셨습니다👍👍
JPA관련 미션은 충분히 소화하신 것 같아요.
이번 미션은 이만 머지하겠습니다!
Reservation reservation = new Reservation(reservationRequest.name(), reservationRequest.date(), time, theme, member); | ||
reservationRepository.save(reservation); | ||
|
||
return new ReservationResponse(reservation.getId(), reservationRequest.name(), reservation.getTheme().getName(), reservation.getDate(), reservation.getTime().getTime()); |
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 변환 로직은 Service
보단 해당 dto로 옮기는걸 선호해요!
비즈니스를 담당하는 Service
의 역할과 거리가 멀고, Service
는 여러 요구사항에 따라 쉽게 복잡해져서 최대한 간결하게 유지하는 걸 좋아합니다.
이전 리뷰처럼, dto에 위임해도 괜찮아보여요!
protected Waiting() { | ||
|
||
} |
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.
중요한건 아닙니다!
Lombok을 사용한 김에 @NoArgsConstructor(access = AccessLevel.PROTECTED)
요걸로 대체할 수도 있습니다~
if (waitingRequest.date() == null | ||
|| waitingRequest.theme() == null | ||
|| waitingRequest.time() == null) { | ||
return ResponseEntity.badRequest().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.
JPA와는 다른 주제지만, 외부 요청 값에 대한 일반적인 검증(Null, empty, length 검증)들은 validation 의 도움을 받을 수도 있어요.
취향에 따라 사용유무가 갈리는 것 같긴 한데, 알아두면 편리할 것 같아서요!
안녕하세요:)
리팩토링을 조금 해야할 것 같은데 미션 기한이 어제까지라 테스트 통과만 한 상태로 올렸습니다 ㅜ.ㅜ
테스트 통과하는 도중에 생긴 궁금증이 있는데요
질문 1)
WaitingRepository
에서List<Waiting> findWaitingsWithRankByMemberId(**_@Param("memberId")_** Long memberId);
이 굵은 부분을 추가하지 않으면 6단계 테스트에서
이런 오류가 발생하더라구요..? 근데 견본 코드이기도 하고 다른 사람들의 코드를 참고해보니 견본 코드 그대로 쓴 사람도 많던데 차이점이 왜 발생하는지 궁금합니다.
질문 2)
5단계에 존재하는 요구사항 해결 방향성 제시 문장입니다.
저 요구사항의 경우 관리자 페이지에서 name만으로 예약을 추가한다면 무조건 name으로만 Memberr를 조회할 수밖에 없을 것 같은데 동명이인의 경우는 고려하지 않는 걸까요??