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

[refactor] 종일 일정의 endDateTime을 11시 59분에서 다음 날 00시 00분으로 변경한다. #614

Conversation

summerlunaa
Copy link
Collaborator

  • 🔀 PR 제목의 형식을 잘 작성했나요? e.g. [feat] PR을 등록한다.
  • 💯 테스트는 잘 통과했나요?
  • 🏗️ 빌드는 성공했나요?
  • 🧹 불필요한 코드는 제거했나요?
  • 💭 이슈는 등록했나요?
  • 🏷️ 라벨은 등록했나요?
  • 💻 git rebase를 사용했나요?
  • 🌈 알록달록한가요?

작업 내용

종일 일정의 endDateTime을 11시 59분에서 다음 날 00시 00분으로 변경한다.

주의사항

  • 일정 타입 판단 로직이 혹시 잘못 되었는지 꼼꼼히 확인 부탁드립니다.

Closes #592

@summerlunaa summerlunaa added backend 백엔드 refactor 리팩터링 labels Sep 19, 2022
@summerlunaa summerlunaa self-assigned this Sep 19, 2022
Copy link
Collaborator

@hyeonic hyeonic left a comment

Choose a reason for hiding this comment

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

안녕하세요 파랑! 매트입니다 😃

생각보다 변경점이 적어서 금방 적용할 것 같네요! 추가로 merge 되고 main에 배포 되는 시점에 Dabase startDateTimeendDateTime에 대한 UPDATE도 이뤄져야 하니 염두해두세요!

LocalTime startTime = LocalTime.from(startDateTime);
LocalTime endTime = LocalTime.from(endDateTime);
return ChronoUnit.MINUTES.between(startTime, endTime) % ONE_HOUR;
return startTime.equals(MIDNIGHT) && endTime.equals(MIDNIGHT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

MIDNIGHT00:00을 뜻하는 건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes `LocalTime'에서 상수로 관리하더라구요!

Comment on lines +31 to +34
public IntegrationSchedule toIntegrationSchedule(final Long internalCategoryId) {
return new IntegrationSchedule(id, internalCategoryId, summary, getStartDateTime(), getEndDateTime(),
description, CategoryType.GOOGLE);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

dto 라고 생각했던 객체 내부에 로직이 들어간 부분은 조금 걸리긴 하네요 ㅜㅠ 로직은 훨씬 깔끔해진 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넣을까말까 고심하다 다른 곳에서는 toEntity라는 메서드로 비슷한 로직이 존재해서 넣어보았습니다.. 근데 저도 별로 맘에 안 듦^^ㅠ
더 좋은 방법 있음 얘기해주세요 매리후~

Copy link
Collaborator

Choose a reason for hiding this comment

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

딱히 계산로직이 들어간건 아니고, 단순히 toEntity() 처럼 변환 로직이 들어간거라서 큰 문제 없을 것 같습니다! DTO -> Domain 방향으로 의존하는 것 이니까요 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 변환 로직이 들어간 부분이여서 괜찮을것 같아요~
domain이 Dto를 아는게 아닌 dto가 domain을 아는 것이라 의존성도 괜찮을것 같아요🙂

Copy link
Collaborator

@devHudi devHudi left a comment

Choose a reason for hiding this comment

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

안녕하세요, 파랑!
00:00 ~ 23:59 -> 00:00 ~ 00:00 으로 변경해주신 것 잘 확인했습니다.
저는 딱히 바뀔부분 없는 것 같아서 바로 Approve 드릴게요!

Comment on lines +31 to +34
public IntegrationSchedule toIntegrationSchedule(final Long internalCategoryId) {
return new IntegrationSchedule(id, internalCategoryId, summary, getStartDateTime(), getEndDateTime(),
description, CategoryType.GOOGLE);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

딱히 계산로직이 들어간건 아니고, 단순히 toEntity() 처럼 변환 로직이 들어간거라서 큰 문제 없을 것 같습니다! DTO -> Domain 방향으로 의존하는 것 이니까요 😄

Copy link
Collaborator

@gudonghee2000 gudonghee2000 left a comment

Choose a reason for hiding this comment

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

파랑 작성해주신 코드 잘보았습니다~
11시 59분에서 다음날 00시로 종일의 기준을 바꾸니
로직이 훨씬 단순해지고 이해하기가 더욱 쉬워진것 같아요😊

그리고 코드를 잘 짜주셔서 이해가 잘되었네요😊
바로 approve하겠습니다!

Comment on lines +31 to +34
public IntegrationSchedule toIntegrationSchedule(final Long internalCategoryId) {
return new IntegrationSchedule(id, internalCategoryId, summary, getStartDateTime(), getEndDateTime(),
description, CategoryType.GOOGLE);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 변환 로직이 들어간 부분이여서 괜찮을것 같아요~
domain이 Dto를 아는게 아닌 dto가 domain을 아는 것이라 의존성도 괜찮을것 같아요🙂

@summerlunaa summerlunaa force-pushed the feature/592-change-alldays-endtime branch from 495f3fe to 99cb2b4 Compare September 20, 2022 04:12
@summerlunaa summerlunaa merged commit ea8939d into woowacourse-teams:develop Sep 20, 2022
@summerlunaa summerlunaa deleted the feature/592-change-alldays-endtime branch September 20, 2022 04:17
@hyeonic hyeonic mentioned this pull request Sep 22, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend 백엔드 refactor 리팩터링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants