Skip to content
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

feat : 예약 선점 로직 구현 #23

Merged
merged 47 commits into from
Dec 28, 2023
Merged

feat : 예약 선점 로직 구현 #23

merged 47 commits into from
Dec 28, 2023

Conversation

dlswns2480
Copy link
Member

closed #4

⛏ 작업 상세 내용

새로 구현한 클래스

  • ReservationService
    • 해당 예약시간이 선점되었는 지 검증하는 로직
  • ReservationAsync
    • 해당 예약시간을 선점 처리하는 로직
    • 스케줄러를 통해 비동기로 실행 (현재는 7분으로 설정하지 않고 더 짧은시간으로 설정해둠. 추후에 변경 예정)
  • ReservationFacade
    • 위의 두 로직을 이용하여 응답 dto 만들어서 컨트롤러에게 응답

세부 내용

  • 예약 선점 api의 요청 dto를 변경했습니다.
    • 기존 : shopId를 pathvariable로 받고 요청 dto에 예약하려는 시간과 인원
    • 변경 : shopId를 받지 않고 요청 dto에 예약시간 id와 인원만 받도록 변경
    • 변경 이유 : 예약시간 객체 안에 해당 shop과 예약하려는 시간이 들어있으므로 예약시간 id만 받는게 효율적일 것 같습니다.
  • 예약 생성 시 생성자에 예약시간 객체 필드를 추가했습니다.
    • 예약을 생성할 때는 예약시간이 필수로 있어야하므로 생성시에 넣어주는 방향으로 잡았습니다.
  • 예약시간의 선점여부를 변경하는 로직을 도메인 안에 넣었습니다
    • ReservationTime.java의 reversePreOccupied(), reverseOccupied()

📝 작업 요약

  • 예약 선점 로직 1차 구현 (동시성 이슈 해결 제외)

☑️ 중점적으로 리뷰 할 부분

  • 전체적인 메소드 네이밍
  • 선점 로직 구현한 위에서 언급한 3개 클래스, 테스트 코드 부분
  • 도메인 로직과 비즈니스 로직이 잘 분리되었는지

- 추후에 Oauth를 통해 member 필터링
- 현재는 예제 예약시간을 직접 생성, 추후에 shop 쿼리를 통해 조회해야함
- 기존 : 시간, 인원
- 변경 : 예약시간 아이디, 인원
@dlswns2480 dlswns2480 self-assigned this Dec 27, 2023
Copy link

github-actions bot commented Dec 27, 2023

Test Results

5 tests  +5   5 ✅ +5   1s ⏱️ -1s
3 suites +3   0 💤 ±0 
3 files   +3   0 ❌ ±0 

Results for commit bd8a90f. ± Comparison against base commit 4202c8c.

♻️ This comment has been updated with latest results.

@dlswns2480 dlswns2480 linked an issue Dec 27, 2023 that may be closed by this pull request
2 tasks
@dlswns2480 dlswns2480 changed the title 예약 선점 로직 구현 feat : 예약 선점 로직 구현 Dec 27, 2023
Comment on lines +21 to +22
ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);
scheduler.schedule(reservationTime::reversePreOccupied, 10, TimeUnit.SECONDS);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정말 몰라서 여쭈는 건데, 쓰레드 1개를 더 할당해서 10분 뒤에 reversePreOccupied 메서드를 호출하는 건가요?.?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

쓰레드 1개를 더 할당해서 쓰는 스케줄러 맞습니다. 근데 여기서 설정된 시간인 10초동안 기다렸다가 스레드가 시간이 됐을 때 할당되어 작업이 실행되기 때문에 Thread.sleep()보다는 이걸 쓰는게 맞는 것 같습니다.
즉 쓰레드를 1개를 할당하고 시간을 기다리고 작업을 실행하는 것은 아니고 시간을 기다리고 스레드가 할당되어 작업이 실행됩니다~!

Copy link
Member

@hyun2371 hyun2371 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

선점 로직 구현하느라 수고 많으셨습니다~


@Component
@RequiredArgsConstructor
@Slf4j
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그를 출력하는 부분이 없는 것 같아 해당 어노테이션이 없어도 될 것 같습니다!

}

public void insertShop(Shop shop) {
this.shop = shop;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

별도의 insertShop 메서드가 아닌, 생성자에서 shop을 초기화시켜도 괜찮지 않을까 생각이 들었습니다!

NOT_EXIST_MEMBER("존재하지 않는 아이디입니다.");
NOT_EXIST_MEMBER("존재하지 않는 아이디입니다."),
IS_PRE_OCCUPIED("이미 타인에게 선점권이 있는 예약시간입니다."),
IS_OCCUPIED("이미 예약된 시간입니다."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALREADY_OCCUPIED_RESERV_TIME 이런식으로 네이밍해도 괜찮을 것 같습니다!

@@ -43,4 +46,16 @@ public ReservationTime(LocalDateTime time) {
this.time = time;
this.isOccupied = false;
}

public void reverseOccupied() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하나의 메서드 안에서 boolean값을 뒤집는 것 대신, setOccupiedTrue() setPreOccupiedFalse()로 각각 boolean값을 할당하면 코드의 가독성이 더 높아지지 않을까 생각이 됩니다!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그것도 고려해보겠습니다~!

@RequiredArgsConstructor
public class ReservationController {

private final ReservationFacade reservationFacade;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추후에 예약시간에 대한 예약을 하는 api도 구현하실 것 같습니다.
혹시 그때는 해당 Controller에 ReservationService를 주입 받으시나요? 아니면 ReservationFacade에 해당 비즈니스 로직을 구현하실건가요?.?

만약 예약 api 구현 시, 두가지 방법 중에 하나를 골라야 하는 상황밖에 없다면, Facade 패턴을 굳이 사용해야하나 의문이 듭니다
제가 잘 모르고 한 얘기라면, 해당 코멘트에 얘기해주시면 감사하겠습니다!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Facade를 주입받으려 합니다!
Facade를 쓴 이유는 예약 선점할 때, 예약 선점여부 검증과 예약 선점 스케줄러가 분리되어있어서 이 두 메서드를 호출하여 비즈니스 로직을 facade에서 구현하고 있습니다!

@kkangh00n
Copy link
Collaborator

어려운 로직 구현하시느라 너무 고생하셨습니다~~
이번 스프린트도 같이 화이팅 !^^!

@dlswns2480 dlswns2480 merged commit 492d637 into dev Dec 28, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feature : 예약 등록 로직 설계
3 participants