-
Notifications
You must be signed in to change notification settings - Fork 0
예약하기 기능을 구현했습니다. #12
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
예약하기 기능을 구현했습니다. #12
The head ref may contain hidden characters: "feature/\uC608\uC57D\uD558\uAE30"
Conversation
|
|
||
| public void reserve(StoreReserveRegisterDto storeReserveRegisterDto) { | ||
|
|
||
| StoreQueueDto storeQueueDto = new StoreQueueDto(storeReserveRegisterDto.storeReserveIdx().toString(), storeReserveRegisterDto.userIdx().toString()); |
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.
예약할 때 더블 클릭하면 어떻게 되어요??
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.
제 생각에는
Lock이 걸려 있어 동시요청이 처리되지 않아
isValidWaitingUser 메소드에서 Exception을 던질 것 같습니다.
혹시 놓친 부분이 있을까요~?
|
|
||
| UserReserveData userReserveData = UserReserveDataMapper.toEntity(reserveData, dto); | ||
|
|
||
| if (userReserveData != null) { |
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.
이게 null이 될 수 있는걸까요??
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.
함수를 통해 호출되는 부분을 사용한다고 생각하여
null Check를 진행했습니다.
로직상 절대 Null이 안될 것 같아요..
이부분은 고민이 되는데 응답값이 항상 null을 안 줄 경우
null 체크를 무시해도 괜찮을까요!?
|
|
||
| @UtilityClass | ||
| public class StoreReserveMapper { | ||
| public static StoreReserveRegisterDto toDto(StoreReserveRequestDto storeReserveRequestDto, |
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.
UtilityClass를 사용하면 static 함수로 만들어 준다고 하는데 static을 붙일 필요가 있을까요?
그리고 궁금한게 UtilityClass를 꼭 사용해야 하는 이유도 궁금합니다.
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.
RequestDto에서 생성할까 고민하다가.
Entitiy와 Request의 의존성을 제거하고 생성 단일 책임 있는 클래스를 생성할려고 만들었던 것 같습니다.
보통 레이어간의 변환을 어떻게 하시는지 궁금합니다!
| private Long modIdx; | ||
|
|
||
| @Builder | ||
| public ReserveData(Long idx, Store store, LocalDate reserveDate, String reserveTime, Integer minUserCount, Integer maxUserCount, Integer canReserveCount, Integer reservedCount, StoreReserveDataStatus reserveStatus, LocalDateTime regDatetime, Long regIdx, LocalDateTime modDatetime, Long modIdx) { |
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.
호....인자가 너무 많네요??
이거 라인별로 정리되도록 하면 좋을 것 같아요
| private Long modIdx; | ||
|
|
||
| @Builder | ||
| public UserReserveData(Long idx, User user, ReserveData reserveData, ReserveStatus reserveStatus, ReservePayType reservePayType, LocalDateTime regDatetime, Long regIdx, LocalDateTime modDatetime, Long modIdx) { |
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.
이것도...
| if (reserveTypeString == null || reserveTypeString.isEmpty()) { | ||
| throw new CatchTableException(BadRequestError.INVALID_RESERVE_PAY_TYPE); | ||
| } | ||
| return MAPPING.get(reserveTypeString.toUpperCase()); |
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.
위 결과가 null일 경우에도 INVALID_RESERVE_PAY_TYPE을 던져주는게 좋겠어요~~
| @Query(value = """ | ||
| SELECT rd | ||
| FROM ReserveData rd | ||
| JOIN FETCH rd.store s |
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.
근데 Data는 꼭 붙어야 하는 suffix일까요?ㅎㅎ
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.
음.. 다시보니 prefix로 유저와 상점을 구분해놔서 굳이.. 안붙여도 될 것 같습니다!
Reserve라는 키워드를 많이 사용할 것 같아 붙여놓긴 했는데 제거하겠습니다. ㅎㅎ
| private LocalDateTime modDatetime; | ||
|
|
||
| @Builder | ||
| public User(Long idx, String email, String password, String phone, String nickname, LocalDateTime regDatetime, LocalDateTime modDatetime) { |
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.
이것도 내려쓰기가 있는게 더 좋을 것 같아요
| StoreQueueDto storeQueueDto = new StoreQueueDto(storeReserveRequestDto.storeReserveIdx().toString(), storeReserveRequestDto.userIdx().toString()); | ||
| RLock lock = redissonClient.getLock(storeQueueDto.key()); | ||
| try { | ||
| if (lock.tryLock(1, 3, TimeUnit.SECONDS)) { |
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.
redisson을 클라에서 바로 사용하는 것 보다 클래스로 뺀 뒤에 사용하는게 더 좋을 것 같아요.
그리고 controller에서 사용하기보단 서비스에서 사용하는게 좋아보여요~~
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.
Controller보단 Service로 빼내는 이유는 락 거는 행위도
비즈니스 로직이라고 판단되어서 맞을까요??
UserReserveAggregateService 를 생성하여
RedisLockService 와
UserReserveService을 의존성을 주입 받아서 기능을 구현하도록 수정하겠습니다.
|
|
||
| @DisplayName("UserReserveService ") | ||
| @ExtendWith(MockitoExtension.class) | ||
| class UserReserveServiceTest { |
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.
예약 동시성 테스트도 진행해야 하지 않을까 싶습니다
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.
yuus95
left a comment
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.
리뷰해주셔서 감사합니다!
관련 내용 수정 및 몇가지 코맨트 남겨놨습니다.
확인 부탁드립니다!
|
|
||
| UserReserveData userReserveData = UserReserveDataMapper.toEntity(reserveData, dto); | ||
|
|
||
| if (userReserveData != null) { |
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.
함수를 통해 호출되는 부분을 사용한다고 생각하여
null Check를 진행했습니다.
로직상 절대 Null이 안될 것 같아요..
이부분은 고민이 되는데 응답값이 항상 null을 안 줄 경우
null 체크를 무시해도 괜찮을까요!?
|
|
||
| @UtilityClass | ||
| public class StoreReserveMapper { | ||
| public static StoreReserveRegisterDto toDto(StoreReserveRequestDto storeReserveRequestDto, |
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.
RequestDto에서 생성할까 고민하다가.
Entitiy와 Request의 의존성을 제거하고 생성 단일 책임 있는 클래스를 생성할려고 만들었던 것 같습니다.
보통 레이어간의 변환을 어떻게 하시는지 궁금합니다!
| @Query(value = """ | ||
| SELECT rd | ||
| FROM ReserveData rd | ||
| JOIN FETCH rd.store s |
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.
음.. 다시보니 prefix로 유저와 상점을 구분해놔서 굳이.. 안붙여도 될 것 같습니다!
Reserve라는 키워드를 많이 사용할 것 같아 붙여놓긴 했는데 제거하겠습니다. ㅎㅎ
| StoreQueueDto storeQueueDto = new StoreQueueDto(storeReserveRequestDto.storeReserveIdx().toString(), storeReserveRequestDto.userIdx().toString()); | ||
| RLock lock = redissonClient.getLock(storeQueueDto.key()); | ||
| try { | ||
| if (lock.tryLock(1, 3, TimeUnit.SECONDS)) { |
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.
Controller보단 Service로 빼내는 이유는 락 거는 행위도
비즈니스 로직이라고 판단되어서 맞을까요??
UserReserveAggregateService 를 생성하여
RedisLockService 와
UserReserveService을 의존성을 주입 받아서 기능을 구현하도록 수정하겠습니다.
|
|
||
| @DisplayName("UserReserveService ") | ||
| @ExtendWith(MockitoExtension.class) | ||
| class UserReserveServiceTest { |
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.
# Conflicts: # src/main/java/com/yscp/catchtable/application/queue/StoreQueueService.java # src/main/java/com/yscp/catchtable/application/queue/dto/StoreQueueDto.java # src/main/java/com/yscp/catchtable/exception/BadRequestError.java
|





배경
작업 내용