[FU-357] 날짜별 스케줄 검증로직 개선 및 리팩터링 #91
Conversation
- 예약오픈 <-> 예약중지 간 충돌 불가 - 예약확정 <-> 예약오픈,예약중지 간 충돌 가능
서비스 클래스를 간결하게 관리하고, 단일 책임 원칙(SRP)을 따르기 위해 검증 전담 클래스를 분리하였음
rheeri
changed the title
언급해줘서 고마워🫶 jira에 기록해두고 FU-349 개발할 때 반영할게! |
| + "AND ((ds.startTime < :endTime AND ds.endTime > :startTime))") | ||
| List<DailySchedule> findOverlappingSchedules(Member photographer, LocalDate date, LocalTime startTime, | ||
| LocalTime endTime); | ||
| + "AND ((ds.startTime < :endTime AND ds.endTime > :startTime))" |
There was a problem hiding this comment.
어떤 의미로 조건을 달아준건지 헷갈려서 설명 부탁해도 될까??🙏
There was a problem hiding this comment.
findConflictingSchedulesByStatuses는 추가하려는 날짜별 스케줄과 기존에 등록되어 있는 날짜별 스케줄 간의 충돌 여부를 검증하기 위해서 작성했어! 위 쿼리는 충돌하는 스케줄을 모두 조회해오는 용도이구 충돌은 아래 두 기준을 모두 만족했을 때 발생해!
- 추가하려는 스케줄과 기존 스케줄의 레벨(위계)이 같은 경우
- 추가하려는 스케줄이 기존 스케줄의 시간대와 겹치는 경우
너가 물어봐준 조건절은 (2)번에 해당하는 데이터를 필터링 하는건데, 시간대가 겹친다는 건 이런 경우야
- 기존 스케줄:
추가 오픈1월 23일 10:00 ~ 12:00 - 추가하려는 스케줄:
예약 중지1월 23일 09:00 ~ 11:00
즉, 추가하려는 스케줄의 시작시간은 기존 스케줄의 종료시간보다 이르면서, 종료시간은 기존 스케줄의 시작시간보다 늦는 상황이야
이렇게 하면 충돌하는 시간대를 찾을 수 있더라구!
There was a problem hiding this comment.
예시까지 들어서 설명해줘서 고마워!!ㅎㅎ
22번째 라인에서 ds.startTime < :endTime까지 읽고 겹치는 부분이겠다 싶었는데 뒤에 따라오는 AND ds.endTime > :startTime 이 부분을 읽고 다른의도가 있는건가? 다른 반례가 있어서 세팅해놓은건가? 싶었거든! ds.startTime < :endTime 여기까지만 쿼리문으로 줘도 충분히 말해준 2번 조건을 충족하지 않을까 싶었어!
이 부분에 대해서도 의견 부탁해!
There was a problem hiding this comment.
ds.startTime < :endTime 이 조건만으로는 시간대가 겹치지 않는 스케줄까지도 조회할 가능성이 있어!
- 기존 스케줄: 1월 23일
09:00~ 10:00 - 추가하려는 스케줄: 1월 23일 10:00 ~
11:00
위와 같은 상황처럼 충돌하지 않는 스케줄도 가져오게 되거든 🫨
| if (overlappingSchedules.size() == 1 && overlappingSchedules.get(0).getId().equals(scheduleId)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
혹시 이 부분은 겹치는 scheduleId가 하나여야만 한다는 의미로 작성된거 맞을까??
There was a problem hiding this comment.
응응 맞아! 스케줄 수정 요청 시에만 이 메서드를 호출하게 되는데, 수정하려는 시간대가 이미 등록되어 있는 자신의 스케줄과 충돌하는 경우는 예외로 취급하지 않기 위해서 작성했어
- (기존)
scheduleId=2예약오픈 1/23일 10:00 ~ 12:00 - (수정)
scheduleId=2예약오픈 1/23일 09:00 ~ 11:00
저렇게 처리해주지 않으면 위와 같은 상황에서 의도와 다르게 DAILY_SCHEDULE_OVERLAP가 발생하더라구
그래서 시간대가 겹치는 스케줄이 하나뿐이고, 그게 기존에 등록된 자기 자신이라면 무시하겠다는 의도였어!
There was a problem hiding this comment.
생각해보니까 설명 없이는 바로 이해하기 어려운 코드였던 것 같네..! 😵💫
저 부분만 메서드 추출해서 가독성 개선해봤어 🙌
체크리스트
작업 내역
이번 PR에서는 스케줄 유형에 따른 위계 구조를 명확히 하고, 같은 레벨의 스케줄 간에만 충돌 여부를 검증하도록 로직을 보완했습니다.
또한, 날짜별 스케줄은 기본 스케줄 단위와 일치하도록 설정했습니다.
스케줄 위계 구조 개선
|--- 예약 확정 스케줄
|-- 추가오픈 or 예약 중지 스케줄
|- 기본 스케줄
추가오픈과예약 중지간에 충돌 여부를 검증예약 확정스케줄은 동일한 유형의예약 확정스케줄과만 충돌 여부를 검증날짜별 스케줄 로직 보완
60분이면, 시작/종료 시간은 반드시 정시여야 함.30분이면, 시작/종료 시간은 정시 또는 30분 단위여야 함.고민한 사항
날짜별 스케줄의 등록/수정 요청 시, 요청 객체의 유효성 검증과 비즈니스 요구사항을 해치지 않는지 검증하는 과정이 복잡해짐에 따라, 자연스럽게
DailyScheduleService도 무거워졌습니다.DailyScheduleService는 스케줄 생성, 조회, 삭제, 검증 등 다양한 책임을 가지게 되었습니다.😓점점
DailyScheduleService에 메서드를 추가하는 과정이 부담스러워지고, 테스트 코드를 작성하는 데도 부담이 커지는 것을 느껴 리팩터링을 고려하게 되었습니다. 그 중에서도 검증 로직을 전담 클래스로 분리하여 API 요청으로부터 호출되는 서비스 클래스를 간결하게 관리하고, 단일 책임 원칙(SRP)을 따르는 방향을 고려했습니다. 물론 클래스를 분리함에 따라 테스트 코드도 연쇄적으로 수정해야 했지만, 오히려 아래와 같은 장점을 발견했습니다.✨ 서비스가 단일 책임 원칙을 따르게 됨
✨ 검증 로직의 재사용성이 증가함
✨ 테스트를 더 작고 집중적으로 작성할 수 있음 (예: DailyScheduleValidator에 대한 단위 테스트 작성)
리뷰 요청사항
이번 PR은 #90 에서 논의한 내용을 바탕으로 기존 PR을 보완하기 위해 작성되었습니다.
논의했던 내용이 잘 반영 되었는지 위주로 검토 부탁드립니당 🙌