-
Notifications
You must be signed in to change notification settings - Fork 1
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
달력 조회 API를 작성 #50
base: dev
Are you sure you want to change the base?
달력 조회 API를 작성 #50
Conversation
Resolves: #38
Resolves: #38
…edule> 조회 기능 추가 Resolves: #38
- 전략 + 플라이웨이트 패턴 적용 테스트 포함 Resolves: #38
- CalendarServiceUtil.java - CalendarServiceTest.java Resolves: #38
Resolves: #38
- CalendarServiceUtil.java - CalendarServiceUtilTest.java Resolves: #38
- 검색 조건이 반복 주에 속하지 않은 케이스에 대한 버그 수정을 위해 getNearestDateUsingWeekInterval 메서드 추가 구현 Resolves: #38
- 기존의 Util Class의 메서드로 존재하던 구조 대신 전략 패턴 적용해서 클래스 단위로 분리 - 이에 따른 테스트 수정 Resolves: #38
Resolves: #38
- CalendarStrategy의 ScheduleRepository 연관관계 제거 - 주석 추가 Resolves: #38
Resolves: #38
Resolves: #38
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.
개발하시느라 고생많으셨습니다! 😄
전체적인 코드는 잘 작성하였습니다! 👍🏼
하지만 컨벤션 및 함수로 분리하면 좋을 것 같은 내용들이 있었습니다.
해당 내용들은 자세히 작성해두었고, 더 필요한 부분은 노션에다가 따로 작성해놨습니다!
고생 많으셨고 해당 내용 수정해주시면 감사합니다!
application/wypl-core/src/main/java/com/wypl/wyplcore/calendar/CalendarServiceUtil.java
Outdated
Show resolved
Hide resolved
application/wypl-core/src/main/java/com/wypl/wyplcore/calendar/CalendarServiceUtil.java
Outdated
Show resolved
Hide resolved
application/wypl-core/src/main/java/com/wypl/wyplcore/calendar/CalendarServiceUtil.java
Outdated
Show resolved
Hide resolved
application/wypl-core/src/main/java/com/wypl/wyplcore/calendar/service/CalendarService.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/wypl/jpacalendardomain/calendar/repository/ScheduleRepositoryCustomImpl.java
Show resolved
Hide resolved
...-domain/src/main/java/com/wypl/jpacalendardomain/calendar/repository/ScheduleRepository.java
Show resolved
Hide resolved
...re/src/test/java/com/wypl/wyplcore/calendar/service/strategy/CalendarStrategyConfigTest.java
Outdated
Show resolved
Hide resolved
LocalDate date = getMaxDate(searchStartDate, schedule.getRepetitionStartDate()).with( | ||
TemporalAdjusters.nextOrSame(DayOfWeek.of(dayOfWeek))); | ||
|
||
for(; !date.isAfter(searchEndDate); date = date.plusWeeks(schedule.getWeekInterval())) { |
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.
!date.isAfter(searchEndDate)
이런 조건식을 메서드로 분리해보면 어떨까요?
|
||
// searchStartDate를 해당 주의 월요일로 설정 | ||
LocalDateTime searchStartMonday = LocalDateTime.of(searchStartDate.with(TemporalAdjusters.previousOrSame(DayOfWeek.MONDAY)), LocalTime.of(0, 0)); | ||
LocalDateTime scheduleStartMonday = LocalDateTime.of(schedule.getRepetitionStartDate().with(TemporalAdjusters.previousOrSame(DayOfWeek.MONDAY)), LocalTime.of(0, 0)); |
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.
schedule.getRepetitionStartDate().with(TemporalAdjusters.previousOrSame(DayOfWeek.MONDAY)
해당 함수도 메서드로 분리하면 어떨까요?!
매개변수로 두 개만 넘기는 형식으로 하면 좋을 것 같아요.
# Conflicts: # application/wypl-core/build.gradle
Resolves: #38
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 final ScheduleRepository scheduleRepository; | ||
|
||
private final Map<CalendarType, CalendarStrategy> calendarStrategyMap; |
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.
혹시 개인적인 의견이지만 calendarStrategies
로 수정하는게 어떨까요?! 변수명에 자료구조 형식이 드러나는 것이 좋아보이지 않습니다!
public CalendarSchedulesResponse findCalendar(AuthMember authMember, long calendarId, | ||
CalendarFindRequest calendarFindRequest) { |
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.
public CalendarSchedulesResponse findCalendar(AuthMember authMember, long calendarId, | |
CalendarFindRequest calendarFindRequest) { | |
public CalendarSchedulesResponse findCalendar( | |
AuthMember authMember, | |
long calendarId, | |
CalendarFindRequest calendarFindRequest | |
) { |
이런 형식으로 매개변수를 작성해보는건 어떨까요?!
|
||
public class CalendarServiceUtil { | ||
|
||
public static LocalDate getMaxDate(LocalDate date1, LocalDate date2) { |
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.
음 혹시 getNewestDate
는 어떨까요?? 아래 내용도 getOldestDate
도 괜찮을 것 같다고 생각합니다!
해당 반환값이 Number
형식이 아닌 날짜 형식이기 때문에 메서드명을 수정하시는게 어떨까 생각합니다!
@Bean | ||
public Map<CalendarType, CalendarStrategy> calendarStrategyMap() { | ||
return calendarStrategyMap; | ||
} |
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.
CalendarService
에서 피드백을 준 내용을 토대로 수정한다면 해당 @Bean
의 이름이 수정되어야 합니다!
public WyplResponseEntity<ScheduleInfoCreateResponse> addSchedule(@Authenticated AuthMember authMember, | ||
@RequestBody ScheduleCreateRequest scheduleCreateRequest) { |
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.
해당 내용도 위에는 있지만!
public WyplResponseEntity<ScheduleInfoCreateResponse> addSchedule(@Authenticated AuthMember authMember, | |
@RequestBody ScheduleCreateRequest scheduleCreateRequest) { | |
public WyplResponseEntity<ScheduleInfoCreateResponse> addSchedule( | |
@Authenticated AuthMember authMember, | |
@RequestBody ScheduleCreateRequest scheduleCreateRequest | |
) { |
와 같이 수정하는게 더 보기 편하다고 생각하는데 어떠신가요?
if (gapOfWeek % weekInterval == 0) | ||
return searchStartDate; |
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.
단 한줄이여도 괄호 부탁드립니다. 🙏
LocalDate date = getMaxDate(searchStartDate, schedule.getRepetitionStartDate()).with( | ||
TemporalAdjusters.nextOrSame(DayOfWeek.of(dayOfWeek))); | ||
|
||
for (; !date.isAfter(searchEndDate); date = date.plusWeeks(schedule.getWeekInterval())) { |
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.
해당 조건절도 isBefore
로 처리가 될 것 같은데 한번만 확인 부탁드립니다!
LocalDateTime nearestEndDateTime = LocalDateTime.of( | ||
searchStartDate.withDayOfYear(schedule.getEndDateTime().getDayOfYear()), | ||
schedule.getEndDateTime().toLocalTime()); |
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.
schedule.getEndDateTime().getDayOfYear()
를 캡슐화해서 메서드 체이닝이 단 한번만 호출해서 데이터를 가져오도록 캡슐화를 진행하면 좋을 것 같습니다!
public Schedule toObject() { | ||
return Schedule.builder() | ||
.title(this.title) | ||
.description(this.description) | ||
.startDateTime(this.startDateTime) | ||
.endDateTime(this.endDateTime) | ||
.repetitionStartDate(this.repetitionStartDate) | ||
.repetitionEndDate(this.repetitionEndDate) | ||
.repetitionCycle(this.repetitionCycle) | ||
.dayOfWeek(this.dayOfWeek) | ||
.weekInterval(this.weekInterval) | ||
.build(); | ||
} |
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.
toObject
인데 반환값은 Schedule
입니다! 어떠한 역할을 하는 메서드일까요? 궁금합니다!
@@ -34,6 +34,7 @@ subprojects { | |||
compileOnly 'org.projectlombok:lombok' | |||
testImplementation 'org.projectlombok:lombok' | |||
annotationProcessor 'org.projectlombok:lombok' | |||
testAnnotationProcessor 'org.projectlombok:lombok' |
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.
대신 넣어주셔서 감사합니다! 💯
📑 개요
#38
와플 v1을 개선하며 바뀐 반복되는 일정에 대한 로직의 변화에 따라 달력 조회 기능을 새롭게 구현했습니다.
AS-IS
TO-BE
다음 기준에 따라서 전략 패턴을 적용했습니다.
✅ PR 체크리스트
🚀 상세 작업 내용
📁 ETC