[Spring Data JPA] 정민주 미션 제출합니다. #153
Conversation
There was a problem hiding this comment.
민주님 먼저 미션을 수행하시느라 고생하셨습니다.
pr 요청시 남긴 의문에 대한 이야기를 나눠보면 좋을 것 같습니다.
scheme.sql 내에 name 필드가 정의되어있어 Reservation에서 name 필드를 제거하지 못했습니다.
Reservation이 Member 필드를 가지고 있기 때문에 name 필드는 불필요해 보이는데 .. 삭제해도 괜찮을까요
member_id를 가지고 계시는 상황이기에 삭제해도 괜찮을 것 같습니다.
waiting과 reservation이 각각 다른 repository에서 관리되어서
"내가 이미 예약 성공한 시간 및 테마"에 대해 "예약 대기"를 거는 것이 허용되는 문제가 발생하고 있습니다.
따로 명시된 구현 조건엔 없어서 그냥 넘기긴 했는데 .. 수정하는게 나을까요?!
저는 검증하는 것이 좋다고 생각하지만 추가적인 것은 민주님이 자유롭게 하시면 좋을 것 같습니다. 이미 자신이 예약한 방탈출에 같은 유저가 예약 대기가 가능한 것은 자연스럽지 않는 흐름인것 같긴합니다.
기존 구현된 코드에선 controller가 repository를 직접 호출하고 있었습니다.
이때문에 controller에서 어떤 도메인은 controller를 어떤 도메인은 service를 주입받고 있는데
이 상황은 그닥 문제가 되지 않는게 맞을까요??
통일성이 없어보여 고쳐보려 시도했다가 .. 그냥 코드가 더 깔끔해지는 것 말곤 이득이 없는 것 같아 일단 내버려두긴 했습니다 :(
이 부분도 민주님의 선택이 될 것 같은데요. 저는 단순 위임을 하는 상황이라도 service layer를 만드는 편인데 그 이유는 다른 코드와의 통일성도 있고 추가적으로 present layer에서 db와 연관된 작업을 하는 것은 책임이 비대해진다고 느껴져서 입니다. 또한 새롭게 추가되는 비즈니스 로직에 대해서도 유연하게 대응이 가능해서 왠만하면 저는 service layer를 만드는 편입니다.
리뷰 남겼는데 천천히 확인하고 반영해주세요!
| public Member() { | ||
| } |
There was a problem hiding this comment.
jpa를 사용하기 위해 추가한 기본생성자는 public 접근 제어자 보다는 protected 접근제어자가 좋을 것 같습니다.
| public Member findByName(String name) { | ||
| return memberRepository.findByName(name) | ||
| .orElseThrow(() -> new IllegalArgumentException("존재하지 않는 회원")); | ||
| } |
There was a problem hiding this comment.
현재 해당 method는 reservation을 생성하는 메소드에서 사용하는데 유저의 존재 여부를 검증 후 불러올 때에는 name 보다는 id를 이용하는 것이 좋습니다. 그 이유는 name 같은 경우에는 동명이인이 있을 수 있기 때문에 잘못된 유저 정보를 가져올 수 있기 때문에 id 처럼 유일한 정보를 바탕으로 객체 정보를 불러오는 것이 좋습니다.
| public Reservation() { | ||
|
|
||
| } |
There was a problem hiding this comment.
여기도 protected로 접근제어자를 변경해주면 좋을 것 같습니다.
|
|
||
| return new ReservationResponse(reservation.getId(), reservationRequest.getName(), reservation.getTheme().getName(), reservation.getDate(), reservation.getTime().getValue()); | ||
| Member member = memberService.findByName(reservationRequest.getName()); |
There was a problem hiding this comment.
현재 해당 save 메소드를 호출하는 controller를 보면 agrumentResolver를 통해 member를 불러오던데 굳이 reservationRequest에 member 이름 넣고 다시 service에서 해당 member 다시 찾는 것은 불필요할 것 같은데
save 메소드의 파라미터로 member를 같이 받아서 이용하는 것은 어떤가요?
| return new ReservationResponse(reservation.getId(), reservationRequest.getName(), theme.getName(), | ||
| reservation.getDate(), | ||
| time.getValue()); |
There was a problem hiding this comment.
이 부분으노 MyReservationResponse와 같이 정적 팩토리 메소드나 생성자를 이용하면 service 코드에서 dto변환 코드를 간결하게 관리할 수 있을 것 같습니다.
| } | ||
|
|
||
| @DeleteMapping("/themes/{id}") | ||
| public ResponseEntity<Void> deleteTheme(@PathVariable Long id) { | ||
| themeDao.deleteById(id); | ||
| themeRepository.softDeleteById(id); |
There was a problem hiding this comment.
제가 생각하는 jpa의 장점 중 하나는 1차캐시에서의 더티채킹이라고 생각하는데 지금처럼 바로 query를 직접 db로 날려서 상태를 변경하기 보다는 theme 객체에서 변경해보는 것은 어떤가요?
| @Modifying | ||
| @Query("UPDATE Theme t SET t.deleted = true WHERE t.id = :id") | ||
| void softDeleteById(Long id); |
| public TimeService(TimeRepository timeRepository, ReservationRepository reservationRepository) { | ||
| this.timeRepository = timeRepository; | ||
| this.reservationRepository = reservationRepository; |
There was a problem hiding this comment.
timeService에 save와 deleteById의 메소드의 경우에는 @Transactional을 추가해주는 것이 좋을 것 같습니다.
| private void checkAvailabilityOfCreate(Member member, String date, Time time, Theme theme) { | ||
| boolean alreadyExists = waitingRepository.existsByMemberAndDateAndTimeAndTheme(member, date, time, theme); | ||
| if (alreadyExists) { | ||
| throw new IllegalStateException("이미 동일한 대기 항목이 존재합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 예외처리를 exceptionController를 통해서 global excpetion으로 관리하는 것은 어떤가요?
| Member member = memberRepository.findByName(request.getName()) | ||
| .orElseThrow(() -> new IllegalArgumentException("회원 없음")); |
There was a problem hiding this comment.
여기도 create 메소드에서 파라미터로 argumentResolver로 불러온 Member 받으면 해당 부분은 불필요할 것 같습니다.
안녕하세요 :) 미션 완료하여 pr 올립니다.
아래 내용은 미션 중 궁금했던, 고민했던 내용입니다!
1
scheme.sql 내에 name 필드가 정의되어있어 Reservation에서 name 필드를 제거하지 못했습니다.
Reservation이 Member 필드를 가지고 있기 때문에 name 필드는 불필요해 보이는데 .. 삭제해도 괜찮을까요/
2
waiting과 reservation이 각각 다른 repository에서 관리되어서
"내가 이미 예약 성공한 시간 및 테마"에 대해 "예약 대기"를 거는 것이 허용되는 문제가 발생하고 있습니다.
따로 명시된 구현 조건엔 없어서 그냥 넘기긴 했는데 .. 수정하는게 나을까요?!
3
기존 구현된 코드에선 controller가 repository를 직접 호출하고 있었습니다.
이때문에 controller에서 어떤 도메인은 controller를 어떤 도메인은 service를 주입받고 있는데
이 상황은 그닥 문제가 되지 않는게 맞을까요??
통일성이 없어보여 고쳐보려 시도했다가 .. 그냥 코드가 더 깔끔해지는 것 말곤 이득이 없는 것 같아 일단 내버려두긴 했습니다 :(