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

Date 관련 컴포넌트 개편 #334

Merged
merged 26 commits into from
May 15, 2024
Merged

Conversation

toastmeister1
Copy link
Member

@toastmeister1 toastmeister1 commented Jan 14, 2024

Description

문제점

  • java.util.Date는 스레드에 안전하지 않고 성능 또한 좋지 못함
  • Date관련 로직이 곳곳에 산재되어 있고 작성시 매우 불편함을 느낌
  • DateUtil을 통해 아무곳에서나 시간 계산을 하는 로직이 존재

개선

  • DateUtil 제거
  • Session에 담긴 date를 String -> LocalDateTime으로 변경
  • DateUtil을 String 대신 LocalDateTime을 기반으로 로직을 수행하도록 변경
  • DateUtil에서 파싱하는 로직을 DateParser로 분리
  • presentation 모듈 Date를 파싱하는 로직을 모두 DateParser에서 하도록 수정
  • UI에 존재하는 시간 관련 계산 로직 제거

Screen Shots

Screen Shot Screen Shot

Check List

  • CI/CD 통과여부
  • Develop Mege 시 Squash and Merge 형태로 넣어주세요!
  • 1Approve 이상 Merge
  • Unit Test 작성
  • 연관된 이슈를 PR에 연결해주세요.

@toastmeister1 toastmeister1 added the 🪄 개선합시다 기능이 기존에 돌아가지만 더 나은 방향으로 개선(ex 리팩토링, 마이그레이션)하려고 할 때 label Jan 14, 2024
@toastmeister1 toastmeister1 marked this pull request as draft January 14, 2024 10:27
@toastmeister1 toastmeister1 linked an issue Jan 14, 2024 that may be closed by this pull request
3 tasks
@toastmeister1 toastmeister1 force-pushed the refactor/refactor_dateutil branch 2 times, most recently from c6029c3 to e094732 Compare January 20, 2024 07:26
@toastmeister1 toastmeister1 marked this pull request as ready for review January 20, 2024 09:07
Copy link
Member

@hoyahozz hoyahozz left a comment

Choose a reason for hiding this comment

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

귀찮은 작업임에도 테스트 코드까지 구현하시느라 고생 많으셨습니다!

이번에도 많이 배워갑니다 ㅎ.ㅎ

attendance: Attendance?
session: Session,
attendance: Attendance,
isPastSession: Boolean
): YDSAttendanceType? {
Copy link
Member

Choose a reason for hiding this comment

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

nullable 타입으로 리턴하지 않아도 될 것 같아요!

Comment on lines 39 to 40
lastAttendanceList = attendanceList.filter {
DateUtil.isPastSession(it.first.startTime)
dateUtil.isPastDate(it.first.startTime)
Copy link
Member

Choose a reason for hiding this comment

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

lastAttendanceList = attendanceList.filter { (session, _) ->
    dateUtil.isPastDate(session.startTime)
}

이렇게 바꿔주는 것도 괜찮을 것 같아용!

Copy link
Member Author

Choose a reason for hiding this comment

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

굿굿! 수정했습니다~

)
}

fun Session.toData(): SessionEntity {
fun Session.toDate(dateParser: DateParser): SessionEntity {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fun Session.toDate(dateParser: DateParser): SessionEntity {
fun Session.toData(dateParser: DateParser): SessionEntity {

@@ -21,24 +22,24 @@ data class SessionEntity(
val code: String? = null
)

fun SessionEntity.toDomain(): Session {
fun SessionEntity.toDomain(dateParser: DateParser): Session {
Copy link
Member

Choose a reason for hiding this comment

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

도메인 계층으로 매핑할 때 날짜를 LocalDateTime 형태로 파싱하는 아이디어 되게 좋은 것 같아요 !! 👍👍👍👍

Comment on lines 15 to 18
/**
* [startTime] 에서 [target] 까지 시간이 얼마나 흘렀는지를 나타내는 메서드
* 양수 (+) 일 경우 [target] 으로부터 시간이 이미 흐른상태, 음수 (-)인 경우 시간이 아직 [target] 까지 시간이 흐르지 않은상태
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* [startTime] 에서 [target] 까지 시간이 얼마나 흘렀는지를 나타내는 메서드
* 양수 (+) 일 경우 [target] 으로부터 시간이 이미 흐른상태, 음수 (-)인 경우 시간이 아직 [target] 까지 시간이 흐르지 않은상태
*/
/**
* [current] 에서 [target] 까지 시간이 얼마나 흘렀는지를 나타내는 메서드
* 양수 (+) 일 경우 [target] 으로부터 시간이 이미 흐른상태, 음수 (-)인 경우 시간이 아직 [target] 까지 시간이 흐르지 않은상태
*/

Copy link
Member Author

Choose a reason for hiding this comment

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

주석도 꼼꼼히 읽어봐 주셨군요!
체크 감사합니다 (_ _)


object DateUtil {
typealias Minute = Long
Copy link
Member

Choose a reason for hiding this comment

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

typealias 를 사용하는 아이디어 아주 신선해요!


upcomingSession?.let {
var lastSessionId = if (isUpcomingSessionIsStarted(it)) it.sessionId else it.sessionId - 1
var lastSessionId = if (dateUtil.isPastDate(upcomingSession.startTime)) it.sessionId else it.sessionId - 1
Copy link
Member

Choose a reason for hiding this comment

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

한 번만 딱 보고 읽었을 때는 startTime 이 어떤 날짜를 기준으로 past 여부를 판단하는지 이해하기 어렵다는 생각이 쪼끔 듭니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

말씀을 듣고 다시보니 충분히 그럴수 있다고 느꼈네요!
080824e 에서 수정했습니다~

@toastmeister1
Copy link
Member Author

꼼꼼히 읽어봐 주셨군요 정호님! @hoyahozz
시간내어 리뷰주셔서 감사합니다! 🙇‍♂️🙇‍♂️🙇‍♂️

Copy link
Member

@hoyahozz hoyahozz left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 :-> 👍👍👍👍👍

Comment on lines 33 to 36
fun isPastDate(
fun isPastDateFromCurrentTime(
target: LocalDateTime,
current: LocalDateTime = getCurrentTime()
): Boolean {
Copy link
Member

@hoyahozz hoyahozz Jan 27, 2024

Choose a reason for hiding this comment

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

current 매개변수에 사용자가 다른 날짜를 기입하게 되면, 더 이상 current 가 아닐 수도 있을 것 같아요!

fun isPastDate(
    evaluate: LocalDateTime,
    target: LocalDateTime
): Boolean {
    return evaluate <= target
}

fun isPastDateFromCurrentTime(
    evaluate: LocalDateTime
): Boolean {
    return isPastDate(
        evaluate = evaluate,
        target = LocalDateTime.now(),
    )
}

이런 식으로..? 함수 분리 하는건 어떤지 제안드려봅니다!
파라미터 작명이 어렵네요 ㅠ

Copy link
Member Author

Choose a reason for hiding this comment

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

음.. 맞네요 이 부분 테스트 때문에 이렇게 처리를 했었는데
말씀주신 방향대로 조금만 더 고민 해볼게요!

Copy link
Member Author

@toastmeister1 toastmeister1 Jan 27, 2024

Choose a reason for hiding this comment

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

수정을 하면서 DateUtil이 가독성이 떨어지는 이유에서도 고민을 좀 해봤는데
DateUtil에 출첵앱에서 사용하는 Domain이 섞이면서 발생한게 아닐까 해요

예를들어
GetUpcomingSessionUseCase에서 사실상 UpcomingSession 을 구하는 조건은
현재시간이 Session의 StartTime 이전 인데

[AS-IS]
스크린샷 2024-01-27 오후 7 24 31

보시면 DateUtil에서 isUpcoming을 통해 조건을 필터링 하고 있으니
[밥상차리는 방법]에 [1. 밥상을 차린다] 라고 적혀있는 느낌이네요 ㅋㅋ

[TO-BE]
스크린샷 2024-01-27 오후 7 24 43

DateUtil이 하는 기능 자체를 기준 시간으로부터 어떤 시간이 지났는지 와 같이 순수한 시간 계산에만 국한하여
함수명을 수정했습니다! (+ infix 함수를 사용해 사용법이 살짝 달라지기는 했으나 가독성을 증가했어요)

코어한 부분이라 한번 보시고 어떠신지 피드백 주시면 감사하겠습니다!

Copy link
Member

Choose a reason for hiding this comment

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

코틀린스러운 코드 너무 좋습니다 :->

한 가지 우려되는 점은, DateUtil 내 메소드들이 모드 확장 함수 형태다보니 이를 사용하기 위해선 스코프 함수의 사용이 필수적이라는 것이 조금 걸리네요!

val dateUtil = DateUtil()
val anotherDateTime = LocalDateTime.now()

with(dateUtil) { currentTime isBeforeFrom LocalDateTime.now() }
with(dateUtil) { anotherDateTime isBeforeFrom LocalDateTime.now() }
anotherDateTime.isBefore(LocalDateTime.now())

스코프 함수를 사용해야만 한다면, LocalDateTime 내에서 제공하는 isBefore 을 그대로 써도 되지 않을까? 싶은 생각도 조금 들기도 합니다!..

Copy link
Member Author

Choose a reason for hiding this comment

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

답이 많이많이 늦었군요 정호님..

LocalDateTime 내에서 제공하는 isBefore 을 그대로 써도 되지 않을까? 싶은 생각도 조금 들기도 합니다!..

넵 충분히 그렇게 생각하실 수 있다고 봅니다
먼저 DateUtil로 Scope를 만들고 내부에서 확장함수를 사용하는 이유를 말씀드리자면

첫번째로 infix로 가독성을 조금이나마 증가시키려고 한 부분이 컸는데요 시간 2개를 비교해서 연산하는 메서드들이 있을때
[시간1].isBefore([시간2]) 를 사용한다면

  • 시간1을 기준으로 시간2가 과거인지
  • 시간2를 기준으로 시간1이 과거인지
    가 헷갈리더라구요 그래서 infix로 변경하고 메서드명에 '~' 에서 부터를 나타내는 From을 붙이게 됐습니다!

다음은 이런 isBeforeFrom / isAfterFrom과 같은 메서드들이 LocalDateTime사용 시 무분별하게 노출되는것 보다는
DateUtil을 통해서만 사용하도록 강제하고, 추후에 시간관련 로직들이 이곳에 모였으면 하는 바램이였네요

위에 이 두가지를 충족할만한 뾰족한 수가 없을까요?

Copy link
Member

Choose a reason for hiding this comment

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

음 의견을 덧붙여 보자면, 현재 DateUtil 사용처 모든 곳에서 현재 시간과 비교하고 있어서 currentTime 이라는 변수도 굳이 노출될 필요가 없어 보여요
DateUtil에 LocalDateTime의 확장 함수를 만들어서 사용해 보면 어떨까요?
이름이 조금 문제였던 것 같은데.. 좋은 네이밍 있으면 추천해 주세요!

// DateUtil
fun LocalDateTime.isPast() = currentTime.isBefore(this)


// GetUpcomingSessionUseCase
return sessionRepository.getAllSession().mapCatching { sessionList ->
    sessionList.firstOrNull { session ->
        session.startTime.isPast()
    }
}

Copy link
Member

@hoyahozz hoyahozz Apr 19, 2024

Choose a reason for hiding this comment

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

클래스 내부에서 멤버 확장 함수를 사용하게 되면, 사용하는 입장에서는 해당 클래스를 사용할 때 '반드시 스코프 함수를 사용해야 한다' 라는 히스토리를 알고 있어야 하는게 마음에 계속 걸리네요 ㅠㅠ

회의 때 좀 더 자세히 얘기 나눠보면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 주신 의견들 토대로 고민해보고 회의때 한번 방안을 제의해 볼게요

Copy link
Member

@jihee-dev jihee-dev left a comment

Choose a reason for hiding this comment

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

리뷰가 너무너무너무...너무 늦어서 죄송합니다... 우선 Util 쪽만 봤는데 사용처들 더 보고 리뷰 남기겠습니다! ㅠㅠ

domain/src/main/java/com/yapp/domain/util/DateUtil.kt Outdated Show resolved Hide resolved
domain/src/test/java/DateUtilTest.kt Outdated Show resolved Hide resolved
DateUtil을 대체할 RenewDateUtil을 구현
- string Date로 계산하는것이 아닌 LocalDateTime을 기반한 RenewDateUtil 구현
- RenewDateUtil과 LocalDateTime에 대한 유닛테스트 작성
기존 String으로 되어있는 값을 LocalDateTime으로 변경
- RemoteConfig에 존재하는 공통의 출석코드가 아닌 각 세션에 존재하는 Code를 Input과 비교하여 출석하도록 수정
불필요한 코드들을 제거합니다
- 로직에서의 더 나은 가독성을 위해 함수명을 도메인(비즈니스 로직에)에 연관되지 않은 이름으로 수정합니다
- infix 함수로 가독성을 향상시킵니다
빌드 깨지는 원인 제거
 - Attendance.Status의 매핑 확장함수의 위치를 변경합니다
- YDSAttendanceType이 ResId를 가지고 있는 바람에 ViewModel 단에서 사용하지 못하는 단점이 존재
- YDSType을 ResId로 매핑하는 컴포저블을 생성
- AttendanceType을 UI대신 ViewModel에서 연산토록 AttendanceTypeMapper를 생성
- UIState 재설계 (UiState가 Domain의 모델을 가지고 있지 않도록 수정)
- AttendanceType을 UI가 아닌 SessionDetailViewModel 에서 연산하도록 함
- AttendanceType을 UI가 아닌 MemberScoreViewModel 에서 연산하도록 함
- 당일 12시까지 해당 세션을 노출하도록 수정합니다
- 반복되어 UpComingSession을 구하는 로직을 GetUpComingSessionUseCase를 사용하도록 수정
- QR인증이 제거되어 CheckQRAuthTimeUseCase -> CheckAttendanceTimeUseCase 로 네이밍 수정
@toastmeister1
Copy link
Member Author

toastmeister1 commented May 5, 2024

  • DateUtil제거

    • 오히려 가독성을 해치고 개발자 입장에서 코드 작성시 혼란이 있을 수 있어 제거
    • 시간계산 로직은 domain에 집중시키고 UI에 존재하는 시간계산 로직 제거
  • SessionDetail, MemberScore화면 UI로직(ViewModel, Screen) 개선 (DateUtil 제거하는 과정에서 필요로 했습니다)

    • Screen에 존재하는 로직들을 ViewModel로 이동시키는 작업들을 진행했습니다!
  • GetUpComfingSessionUseCase, CheckAttendanceTimeUseCase 로직 수정 (테스트 완료) -> 중요하니 꼭 확인해주세요

    • 오늘의 세션을 노출하는 부분에 당일에 해당하는 세션이 뜨지 않는 이슈가 있어 수정했어요!

Copy link
Member

@hoyahozz hoyahozz left a comment

Choose a reason for hiding this comment

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

궁금한 점 코멘트 남겼습니다!_!
주말동안 적지 않은 작업 수행하시느라 정말 고생 많으셨습니다 ㅠㅠ

import java.time.LocalDateTime
import javax.inject.Inject

class GetCurrentTimeUseCase @Inject constructor() {
Copy link
Member

Choose a reason for hiding this comment

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

현재 날짜를 리턴하는 단순한 로직같은데, useCase 를 따로 분리하신 특별한 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

이전에 이야기 했듯 나중에 출석을 서버시간으로 체크한다고 하셔서 UseCase를 만들었어요~

Copy link
Member Author

@toastmeister1 toastmeister1 May 6, 2024

Choose a reason for hiding this comment

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

추가로 GetCurrentTimeUseCase 를 쓰는곳의 로직들을 UseCase로 하나로 묶고 (Repository에서 서버시간 가져오게 함)
GetCurrentTimeUseCase와 같은 의미없는 ByPass UseCase는 지양하는게 맞다고 생각합니다

하지만 이렇게 되면 수정하는 범위가 너무 늘어나서
이건 각 화면별 코드 개선할때 진행하는게 좋을것 같아요

Comment on lines +127 to +128
enum class YDSAttendanceType {
ATTEND, TARDY, ABSENT, TBD, NO_ATTENDANCE, NO_YAPP
Copy link
Member

Choose a reason for hiding this comment

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

YDSAttendanceTypeResId를 가지고 있는 바람에 ViewModel 단에서 사용하지 못하는 단점이 존재

이 부분에 대해 제대로 이해하지 못했어요 ㅠㅠ
AttendanceTypeMapper 가 있다면 ResId 가 있더라도 ViewModel 에서 매핑이 가능하지 않나용?? ㅠㅠ

Copy link
Member Author

Choose a reason for hiding this comment

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

checkAttendanceType 메서드가 왜 UI쪽에 있는지를 보다가
YDSAttendanceType이 ResId를 가지고 있어서 ViewModel에서 사용이 어려워서 그랬구나 판단했습니다
(ViewModel에서 리소스를 사용하는건 메모리 릭 우려가 있어 권장되지 않는다는 글이 생각났네요)

그리고 YDSAttendanceType은 icon과 텍스트 resId를 가지고 있지 않게 수정한 이유는 단순 아이콘, 텍스트 이외에도 컬러를 지정할때도 사용하고 있는데요
그렇다면 YDSAttendanceType에는 또 다시 컬러의 resId가 들어가게 될것이고,
앞으로 AttendanceType에 관련된 컴포넌트들이 생긴다면 이곳에 계속 몰리는걸 우려했네요

YDSAttendanceType를 최대한 가볍게 가져가는 게 좋을것 같다는 이유였어요

UseCase 네이밍 변경으로 수정
Copy link
Member

@hoyahozz hoyahozz left a comment

Choose a reason for hiding this comment

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

코멘트 답변해주셔서 감사합니다 ㅎㅎ 너무 고생 많으셨습니다 👍👍👍

Copy link
Member

@jihee-dev jihee-dev left a comment

Choose a reason for hiding this comment

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

확인했습니다! 작업 범위가 작지 않았는데 고생 많으셨어요 ㅠㅠ 👍👍

@toastmeister1 toastmeister1 merged commit 14f512f into develop May 15, 2024
@toastmeister1 toastmeister1 deleted the refactor/refactor_dateutil branch May 15, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪄 개선합시다 기능이 기존에 돌아가지만 더 나은 방향으로 개선(ex 리팩토링, 마이그레이션)하려고 할 때
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date 관련 컴포넌트 개편
3 participants