-
Notifications
You must be signed in to change notification settings - Fork 77
[Spring Data JPA] 김진학 미션 제출합니다. #126
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
base: iampingu99
Are you sure you want to change the base?
Changes from 20 commits
ab543f9
eff71de
da43ffd
e6cef74
9cad9dd
dc58c31
aad3799
b39a720
d6225a4
4eaca4a
0827b2d
cbda84c
5f18f38
d5748cf
9313fb6
40890a3
918a6c7
169e0e7
51ccab8
4dbc06c
9fffdc2
f70b93f
44b821d
6daf5ba
7abfe2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package roomescape.reservation; | ||
|
||
import roomescape.waiting.Waiting; | ||
import roomescape.waiting.WaitingWithRank; | ||
|
||
public record MyReservationResponse( | ||
Long id, | ||
String theme, | ||
String date, | ||
String time, | ||
String status | ||
) { | ||
public static MyReservationResponse from(Reservation reservation) { | ||
return new MyReservationResponse( | ||
reservation.getId(), | ||
reservation.getTheme().getName(), | ||
reservation.getDate(), | ||
reservation.getTime().getValue(), | ||
"예약"); | ||
} | ||
|
||
public static MyReservationResponse from(WaitingWithRank waitingWithRank) { | ||
Waiting waiting = waitingWithRank.getWaiting(); | ||
return new MyReservationResponse( | ||
waiting.getId(), | ||
waiting.getTheme().getName(), | ||
waiting.getDate(), | ||
waiting.getTime().getValue(), | ||
waitingWithRank.getRank() + 1 + "번째 예약대기" | ||
); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
import jakarta.persistence.GenerationType; | ||
import jakarta.persistence.Id; | ||
import jakarta.persistence.ManyToOne; | ||
import roomescape.member.Member; | ||
import roomescape.theme.Theme; | ||
import roomescape.time.Time; | ||
|
||
|
@@ -21,24 +22,18 @@ public class Reservation { | |
private Time time; | ||
@ManyToOne(fetch = FetchType.LAZY) | ||
private Theme theme; | ||
@ManyToOne(fetch = FetchType.LAZY) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Member 객체를 넣게 되는 경우 Reservation 과 Member의 객체 관계를 명확히하여 그래프 탐색이나 연관 객체의 변경에 대한 감지에 용이합니다. 현재는 예약 비즈니스 로직에서 Member 의 id 만을 다루기도 하고 조회 쿼리가 대부분이며 추가 쿼리가 필요하다고는 하나, 영속성 컨텍스트에서 조회될 수 있으므로 Member 객체를 가지기 보단 Long 객체를 가지는 것이 좋아보입니다. |
||
private Member member; | ||
|
||
public Reservation(Long id, String name, String date, Time time, Theme theme) { | ||
this.id = id; | ||
this.name = name; | ||
this.date = date; | ||
this.time = time; | ||
this.theme = theme; | ||
public Reservation() { | ||
} | ||
|
||
public Reservation(String name, String date, Time time, Theme theme) { | ||
public Reservation(String name, String date, Time time, Theme theme, Member member) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 지금 로직은
이에 대한 의견을 들려주세용 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 현재의 예약 테이블과 예약 테이블에서 중복적인 요소를 제거하기 위해 날짜, 시간, 테마 필드를
분리를 통해 날짜, 시간, 테마를 매개변수로 받던 메서드들이 스케줄의 주키만을 받아 동작하여 가독성이 좋을 것으로 생각됩니다. 또한 날짜, 시간, 테마 조회가 많이 발생하므로 이를 복합키로 만들기 보다는 새 테이블로 분리해서 조인이 필요하지만 주키의 인덱싱을 사용하는 것이 더 좋을 것이라는 생각이 듭니다. 다른 의견도 궁금합니다 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 질문한 의도는 해당 요소와 같을때는 중복적 + 커버링 인덱스로 검색을 커버 가능 ( 물론, 예약 관련된 내용들이 더 추가될 가능성이 있다면 안할거 같긴 해요. DB 는 최대한 수비적으로 ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 추가로, 이를 엔티티로 분리하는 방법도 있고 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
말씀해주신 내용이 잘 이해가 되지 않는데 조금 더 자세하게 이야기해 주실 수 있으실까요?
|
||
this.name = name; | ||
this.date = date; | ||
this.time = time; | ||
this.theme = theme; | ||
} | ||
|
||
public Reservation() { | ||
|
||
this.member = member; | ||
} | ||
|
||
public Long getId() { | ||
|
@@ -60,4 +55,8 @@ public Time getTime() { | |
public Theme getTheme() { | ||
return theme; | ||
} | ||
|
||
public Member getMember() { | ||
return member; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,12 +25,16 @@ public List<ReservationResponse> list() { | |
return reservationService.findAll(); | ||
} | ||
|
||
@GetMapping("/reservations-mine") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 경로를 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 서비스의 역할에 대해서 고민한 결과입니다. 만약에 일행의 토큰을 받아 예약을 조회하는 경우가 있다면, 조회할 사용자의 id 와 파라미터로 토큰을 받아 id 와 토큰의 정보가 일치하는지 검사한 후에 예약 목록을 반환하며, 추가로 비회원 사용자가 회원인 일행의 토큰을 받아 조회하는 것보다는 공유하는 방법이 더 적절할 것 같습니다. |
||
public ResponseEntity<List<MyReservationResponse>> readAllByMember(LoginMember loginMember) { | ||
List<MyReservationResponse> response = reservationService.readAllByMember(loginMember.email()); | ||
return ResponseEntity.ok(response); | ||
} | ||
|
||
@PostMapping("/reservations") | ||
public ResponseEntity create(@RequestBody ReservationRequest reservationRequest, LoginMember loginMember) { | ||
reservationRequest.checkName(loginMember.name()); | ||
ReservationResponse reservation = reservationService.save(reservationRequest); | ||
|
||
return ResponseEntity.created(URI.create("/reservations/" + reservation.getId())).body(reservation); | ||
ReservationResponse reservation = reservationService.save(reservationRequest, loginMember.email()); | ||
return ResponseEntity.created(URI.create("/reservations/" + reservation.id())).body(reservation); | ||
} | ||
|
||
@DeleteMapping("/reservations/{id}") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,23 @@ | ||
package roomescape.reservation; | ||
|
||
import java.util.List; | ||
import org.springframework.data.jpa.repository.Query; | ||
import java.util.Optional; | ||
import org.springframework.data.jpa.repository.EntityGraph; | ||
import org.springframework.data.repository.CrudRepository; | ||
import roomescape.member.Member; | ||
import roomescape.theme.Theme; | ||
import roomescape.time.Time; | ||
|
||
public interface ReservationRepository extends CrudRepository<Reservation, Long> { | ||
@Query("SELECT r FROM Reservation r JOIN FETCH r.theme JOIN FETCH r.time") | ||
|
||
@EntityGraph(attributePaths = {"member"}) | ||
Optional<Reservation> findWithMemberByDateAndTimeAndTheme(String date, Time time, Theme theme); | ||
|
||
@EntityGraph(attributePaths = {"time", "theme"}) | ||
List<Reservation> findAll(); | ||
|
||
@EntityGraph(attributePaths = {"time", "theme"}) | ||
List<Reservation> findByMember(Member member); | ||
|
||
List<Reservation> findByDateAndThemeId(String date, Long themeId); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,37 +1,19 @@ | ||
package roomescape.reservation; | ||
|
||
public class ReservationResponse { | ||
private Long id; | ||
private String name; | ||
private String theme; | ||
private String date; | ||
private String time; | ||
|
||
public ReservationResponse(Long id, String name, String theme, String date, String time) { | ||
this.id = id; | ||
this.name = name; | ||
this.theme = theme; | ||
this.date = date; | ||
this.time = time; | ||
} | ||
|
||
public Long getId() { | ||
return id; | ||
} | ||
|
||
public String getName() { | ||
return name; | ||
} | ||
|
||
public String getTheme() { | ||
return theme; | ||
} | ||
|
||
public String getDate() { | ||
return date; | ||
} | ||
|
||
public String getTime() { | ||
return time; | ||
public record ReservationResponse( | ||
Long id, | ||
String name, | ||
String theme, | ||
String date, | ||
String time | ||
) { | ||
public static ReservationResponse from(Reservation reservation) { | ||
return new ReservationResponse( | ||
reservation.getId(), | ||
reservation.getName(), | ||
reservation.getTheme().getName(), | ||
reservation.getDate(), | ||
reservation.getTime().getValue() | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,56 @@ | ||
package roomescape.reservation; | ||
|
||
import java.util.List; | ||
import java.util.Optional; | ||
import java.util.stream.Stream; | ||
import org.springframework.stereotype.Service; | ||
import roomescape.member.Member; | ||
import roomescape.member.MemberRepository; | ||
import roomescape.theme.Theme; | ||
import roomescape.theme.ThemeRepository; | ||
import roomescape.time.Time; | ||
import roomescape.time.TimeRepository; | ||
import roomescape.waiting.WaitingRepository; | ||
|
||
@Service | ||
public class ReservationService { | ||
private final ReservationRepository reservationRepository; | ||
private final TimeRepository timeRepository; | ||
private final ThemeRepository themeRepository; | ||
private final MemberRepository memberRepository; | ||
private final WaitingRepository waitingRepository; | ||
|
||
public ReservationService(ReservationRepository reservationRepository, TimeRepository timeRepository, | ||
ThemeRepository themeRepository) { | ||
ThemeRepository themeRepository, MemberRepository memberRepository, | ||
WaitingRepository waitingRepository) { | ||
this.reservationRepository = reservationRepository; | ||
this.timeRepository = timeRepository; | ||
this.themeRepository = themeRepository; | ||
this.memberRepository = memberRepository; | ||
this.waitingRepository = waitingRepository; | ||
} | ||
|
||
public ReservationResponse save(ReservationRequest reservationRequest) { | ||
Time time = timeRepository.findByIdOrThrow(reservationRequest.getTime()); | ||
Theme theme = themeRepository.findByIdOrThrow(reservationRequest.getTime()); | ||
Reservation reservation = new Reservation(reservationRequest.getName(), reservationRequest.getDate(), time, | ||
theme); | ||
public List<MyReservationResponse> readAllByMember(String email) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 코드도 아래와 비슷하게 정답은 아니지만 public List<MyReservationResponse> readAllByMember(final String email) {
final Member member = memberRepository.findByEmailOrThrow(email);
final var reservations = getMemberReservation(member);
final var waitingWithRanks = getMemberWaiting(member);
return List.copyOf(Stream.concat(
reservations.stream().map(MyReservationResponse::from),
waitingWithRanks.stream().map(MyReservationResponse::from))
.toList());
} 이와같으면 좀 더 깔끔 해질거 같네요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 지난번 리뷰에서 알려주신 레이어 분리 방식이 너무 마음에 들었습니다. 공유해주신 지속 성장 가능한 소프트웨어를 만들어가는 방법 글을 읽으며, 레이어를 분리하는 것만으로 해결하려다 보니 메서드 분리를 잊고 있었네요. 수정해보겠습니다. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 해당 아티클을 읽으며 영향을 받았는데
무조건 레이어 기반 클래스들을 계속 만들어 분리하다보니 1:1 과 같은 형태가 될 수도 있더라구요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 단순히 메서드 호출을 위임하는 형태의 분리보다는, 현재 요구사항을 명확하게 만족하는 코드가 더 중요하다는 관점을 갖게 되었습니다. 하지만 일관성에 대한 고민은 여전히 남아 있습니다. 특정 도메인에서는 분리와 확장의 필요성이 명확해 4단계 레이어 구조를 적용했지만, 다른 도메인에서는 굳이 필요하지 않아 3단계로 유지했을 때, 설계의 균형이 맞지 않는 느낌이 들었습니다. 해당 아티클에도 레이어들의 규칙이 명시되어 있는데, 이는 어떻게 균형을 맞추는게 좋을까요? |
||
Member member = memberRepository.findByEmailOrThrow(email); | ||
List<MyReservationResponse> reservations = reservationRepository.findByMember(member).stream() | ||
.map(MyReservationResponse::from).toList(); | ||
List<MyReservationResponse> waitings = waitingRepository.findAllWithRankByMemberId(member.getId()).stream() | ||
.map(MyReservationResponse::from).toList(); | ||
return List.copyOf(Stream.concat(reservations.stream(), waitings.stream()).toList()); | ||
iampingu99 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
public ReservationResponse save(ReservationRequest reservationRequest, String email) { | ||
Member member = memberRepository.findByEmailOrThrow(email); | ||
Time time = timeRepository.findByIdOrThrow(reservationRequest.time()); | ||
Theme theme = themeRepository.findByIdOrThrow(reservationRequest.theme()); | ||
|
||
Reservation reservation = new Reservation( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 부분에서 Option.ofNullable 부분을 private method 로 빼도 좋을거 같네요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 조건식을 메서드로 추출하면 네이밍을 통해 조건의 의미를 쉽게 파악할 수 있는 것처럼, 해당 동작도 private method 로 분리하는 것이 동작의 의미를 명확하게 전달할 수 있을 것 같습니다. 수정해보겠습니다 👍 |
||
Optional.ofNullable(reservationRequest.name()).orElse(member.getName()), reservationRequest.date(), | ||
time, theme, member); | ||
|
||
Reservation savedReservation = reservationRepository.save(reservation); | ||
|
||
return new ReservationResponse(savedReservation.getId(), savedReservation.getName(), | ||
savedReservation.getTheme().getName(), savedReservation.getDate(), | ||
savedReservation.getTime().getValue()); | ||
return ReservationResponse.from(savedReservation); | ||
} | ||
|
||
public void deleteById(Long id) { | ||
|
@@ -39,8 +59,7 @@ public void deleteById(Long id) { | |
|
||
public List<ReservationResponse> findAll() { | ||
return reservationRepository.findAll().stream() | ||
.map(it -> new ReservationResponse(it.getId(), it.getName(), it.getTheme().getName(), it.getDate(), | ||
it.getTime().getValue())) | ||
.map(ReservationResponse::from) | ||
.toList(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
package roomescape.waiting; | ||
|
||
import jakarta.persistence.Entity; | ||
import jakarta.persistence.FetchType; | ||
import jakarta.persistence.GeneratedValue; | ||
import jakarta.persistence.GenerationType; | ||
import jakarta.persistence.Id; | ||
import jakarta.persistence.ManyToOne; | ||
import roomescape.member.Member; | ||
import roomescape.theme.Theme; | ||
import roomescape.time.Time; | ||
|
||
@Entity | ||
public class Waiting { | ||
@Id | ||
@GeneratedValue(strategy = GenerationType.IDENTITY) | ||
private Long id; | ||
private String name; | ||
private String date; | ||
@ManyToOne(fetch = FetchType.LAZY) | ||
private Time time; | ||
@ManyToOne(fetch = FetchType.LAZY) | ||
private Theme theme; | ||
@ManyToOne(fetch = FetchType.LAZY) | ||
private Member member; | ||
|
||
public Waiting() { | ||
} | ||
|
||
public Waiting(String name, String date, Time time, Theme theme, Member member) { | ||
this.name = name; | ||
this.date = date; | ||
this.time = time; | ||
this.theme = theme; | ||
this.member = member; | ||
} | ||
|
||
public Long getId() { | ||
return id; | ||
} | ||
|
||
public String getName() { | ||
return name; | ||
} | ||
|
||
public String getDate() { | ||
return date; | ||
} | ||
|
||
public Time getTime() { | ||
return time; | ||
} | ||
|
||
public Theme getTheme() { | ||
return theme; | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.