Conversation
nhyeonii
requested review from
Hyobeen-Park and
nahy-512
and removed request for
a team
…at-centralization
angryPodo
left a comment
angryPodo
left a comment
There was a problem hiding this comment.
굿! 이슈 의도에 맞게 잘 수정됐네요ㅎㅎ 리뷰로 남긴 코멘트는 필수는 아닙니다만 수정하면 더 완성도 높은 PR이 될것 같습니다👍🏻
core/common/src/main/java/com/hilingual/core/common/util/DateFormatter.kt
Outdated
Show resolved
Hide resolved
| val instant = Instant.ofEpochMilli(pastTime) | ||
| val localDateTime = LocalDateTime.ofInstant(instant, ZoneId.systemDefault()) | ||
| DATE_FORMATTER.format(localDateTime) | ||
| DateFormatters.KOREAN_SHORT_DATE.format(localDateTime) |
There was a problem hiding this comment.
LocalDate가 아닌 LocalDateTime을 포매팅 하기 위해서 확장 함수를 사용하지 않고 DateFormatters.KOREAN_SHORT_DATE 객체를 직접 호출하고 있어요. DateFormatter.kt에 LocalDateTime에 대한 오버로딩 함수를 추가하면 호출부의 패턴을 일관되게 유지 할 수 있어요.
// DateFormatter.kt
fun LocalDateTime.toKoreanShortDate(): String = this.format(DateFormatters.KOREAN_SHORT_DATE)
// FormatRelativeTime.kt
localDateTime.toKoreanShortDate()There was a problem hiding this comment.
오 ..!! 이 부분 사실 저도 통일감 있게 가지고 가고 싶어서 고민했던 부분이였는데요..!! 말씀하신 대로 LocalDateTime에 대한 확장 함수를 추가하면 호출부가 훨씬 일관되고 깔끔해지는거 같아요 !!
core/common/src/main/java/com/hilingual/core/common/util/DateFormatter.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
이건 관점이 궁금해서 남기는 코멘트인데요.🤔
만약 다른 개발자가 다른 형태로 파싱을 하고 싶은 경우에는 (나현이는 "YYYY.MM.dd" 형태, 저는 "HH:mm:SS"로) 어떻게 하는게 좋다고 생각하나요?
먼저 저의 의견은 DateFormtter.kt를 만든 핵심 목적이 날짜 포맷 문자열이 여기저기 흩어지는걸 방지 하기 위함이니, 패턴을 인자로 받는 범용 함수를 열어두면 표준화가 깨진다고 생각합니다. 따라서 새로운 포맷이 필요하다면 객체에 상수로 추가해 재사용성을 높이도록 하는게 좋다고 생각합니다.
다른 관점으로는.. 정말 일회성으로만 쓰이는 포맷이 많다면 패턴을 직접 받는 함수를 사용할 수 있다고 생각합니다. 그러나 일관성을 깨뜨린다고 생각이 드네요.
There was a problem hiding this comment.
저도 기본적으로는 포맷 문자열이 여기저기 흩어지는 걸 막는 게 이 유틸의 핵심 목적이라고 생각해서 말씀하신 것처럼 패턴을 인자로 받는 범용 함수는 지양하는 쪽에 동의합니다 !!
그래서 여러 화면 / 여러 로직에서 재사용될 가능성이 있는 포맷이라면
DateFormatters 객체에 명시적인 Formatter 상수로 추가하는 게 맞다고 봅니다.
이 방식이 프로젝트에서 허용된 날짜 표현을 자연스럽게 문서화하는 효과도 있다고 생각해요.
저 역시 정말 일회성인 경우는 DateTimeFormatter.ofPattern()을 직접 쓰되 주석으로 왜 커스텀이 필요한지 간단히 적어두거나 한 파일 내에서만 쓰인다면 private val로 분리하는 것도 방법이겠지만 .. 의도적으로 사용 범위를 제한해서 이건 예외적인 표현이다라는 점이 드러나게 하는 게 중요할 것 같아요. 이미 날짜 포맷을 위한 유틸이 존재하는 상황에서 이를 사용하지 않는다면 오히려 왜 이 포맷만 예외인지를 이해하기 어려워질 수 있다는 생각도 듭니다..!
There was a problem hiding this comment.
사용하지 않는 import 정리해주세요!
| ) | ||
| } | ||
|
|
||
| private val DATE_FORMATTER: DateTimeFormatter = |
There was a problem hiding this comment.
요기도 formatter 없애면서 사용하지 않는 import문이 생겼을 것 같은데 확인 부탁드려요~!
Hyobeen-Park
left a comment
Hyobeen-Park
left a comment
There was a problem hiding this comment.
왕 훨씬 더 깔끔하고 좋네요! 수고하셨습니다~~!!
nahy-512
left a comment
nahy-512
left a comment
There was a problem hiding this comment.
고생하셨습니다~!
생각보다 변경사항이 많이 없다 싶어서 생각해봤는데, 서버에서 시간을 포맷을 맞춰서 문자열로 내려주고 있는 경우도 많은 것 같아서 그렇게 느꼈나봐요ㅎㅎ
나중에는 서버랑 소통해서 주고받는 모든 날짜도 LocalDate or LocalDateTime으로 관리해보면 좋을까요?
|
|
||
| /** | ||
| * LocalDate를 한국어 짧은 날짜 형식으로 변환합니다. | ||
| * @return "M월 d일" 형식의 문자열 (예: "2월 6일") |
There was a problem hiding this comment.
이렇게 변환 예시 써주신 게 되게 좋은 것 같아요!! 세심하다🥹
iOS랑도 얘기해야해서 ㅋㅋ...진행해주실래요? |
There was a problem hiding this comment.
수고하셨습니당!!
확실히 여러 곳에서 사용되던 date format을 중앙화하니 깔끔해졌네요 🙌🏻
to reviewers에 함수 네이밍과 관련해 남겨주신 내용을 보고, 아무래도 full, short로 모든 의미를 담기에는 어렵지 않을까 싶어 고민을 해보기도 했었는데요..! PR 올려주신 후에 주석을 함께 달아주셔서 지금 방식도 괜찮다고 느꼈습니다.
추후 포맷 종류가 더 늘어나서 네이밍을 고민해야 하는 시점이 온다면 그 때 규칙을 한 번 논의해 봐도 좋을 것 같아요!
타 파트 쪽에 여유가 있다면 한 번 같이 정리해보는 건 괜찮을 것 같아요 😃 |
|
머지합시다! |
5 participants
PR chekList
Related issue 🛠
Work Description ✏️
Uncompleted Tasks 😅
To Reviewers 📢
core 모듈의 util 패키지에 DateFormatter를 만들어서 작업했어요! 함수 네이밍을 toKoreanFullDate, toKoreanShortDate로 했는데 혹시 더 괜찮은 네이밍이 있다면 추천 부탁드리겠습니다.