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.

Sign up for GitHub

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

Jump to bottom

러닝 상태를 관리하는 RunningDataManager를 모듈로 분리 #77

Merged
merged 4 commits into from
Dec 8, 2022
Merged

러닝 상태를 관리하는 RunningDataManager를 모듈로 분리 #77

merged 4 commits into from
Dec 8, 2022

Conversation

yonghanJu
Copy link
Member

@yonghanJu yonghanJu commented Dec 6, 2022 •
edited
Loading

😎 작업 내용

  • 러닝 상태를 관리하는 RunningDataManager를 모듈로 분리

🧐 변경된 내용

  • RunningDataManager 를 재사용하기 위해 프로젝트 의존성을 없에고 presentation이 runningdata 모듈을 단방향으로 알도록 변경

🥳 동작 화면

  • 동작 화면 없음

🤯 이슈 번호

  • 이슈 없음

기타

@yonghanJu yonghanJu added the ⛔️머지금지❗️⛔️ 확인할 것이 있어요!! 머지 금지!! label Dec 6, 2022
soopeach
soopeach reviewed Dec 6, 2022
View reviewed changes
Copy link
Member

@soopeach soopeach left a comment

Choose a reason for hiding this comment

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

고생 하셨어용~~!

presentation/src/main/java/com/whyranoid/presentation/running/RunningActivity.kt
@@ -72,7 +75,7 @@ internal class RunningActivity :
}

observeStateOnMapReady()
naverMap.addOnCameraChangeListener { reason, animated ->
naverMap.addOnCameraChangeListener { reason, _ ->
if (reason == CameraUpdate.REASON_GESTURE) {
Copy link
Member

Choose a reason for hiding this comment

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

네이버 맵 자체에 CameraChangeListener라는게 있군용..!!! CameraUpdate.REASON_GESTURE 요 속성도 네이버 맵 SDK의 속성인가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 이 함수는 병희님이 작성하셨던 부분인데 맞는 것 같습니다

presentation/src/main/java/com/whyranoid/presentation/running/RunningUtil.kt
Comment on lines +16 to +35
fun RunningHistoryModel.toRunningHistoryUiModel() =
RunningHistoryUiModel(
historyId = historyId,
date = startedAt,
startedAt = startedAt,
finishedAt = finishedAt,
totalRunningTime = totalRunningTime,
pace = pace,
totalDistance = totalDistance
)

fun RunningHistoryModel.toRunningHistory() =
RunningHistory(
historyId = historyId,
startedAt = startedAt,
finishedAt = finishedAt,
totalRunningTime = totalRunningTime,
pace = pace,
totalDistance = totalDistance
)
Copy link
Member

Choose a reason for hiding this comment

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

model, Response, UiModel간 타입 캐스팅 확장함수들은 각 모델들의 클래스파일에 같이 존재하고 있는 것으로 기억하는데 요 친구들만 이렇게 분리하신 이유가 있으신가요??!

Copy link
Member Author

Choose a reason for hiding this comment

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

이 아이들은 모듈 간 분리 전에 의존성만 빼는 과정에서 나온 애들인데요

분리 되어야 하는 핵심 로직에서 Ui 모델 대신에 해당 모듈에서 사용할 모델을 따로 만들어서 교체해줬고 이후 커밋에서 피처 모듈에 넣었습니다

runningdata/build.gradle Outdated
targetCompatibility JavaVersion.VERSION_1_8
}
kotlinOptions {
jvmTarget = '1.8'
Copy link
Member

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.

넵 피드백 감사합니다!

runningdata/build.gradle Outdated
buildTypes {
release {
minifyEnabled false
proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro'
Copy link
Member

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.

넵 새롭게 만들 모듈이라 기본 값으로 작은 따옴표가 붙어있네요 내일 병희님 컨펌 받으면서 수정할게욧

runningdata/build.gradle Outdated
}

android {
namespace 'com.whyranoid.runningdata'
Copy link
Member

Choose a reason for hiding this comment

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

요것도 큰 따옴표..!

runningdata/build.gradle Outdated
Comment on lines 2 to 3
id 'com.android.library'
id 'org.jetbrains.kotlin.android'
Copy link
Member

Choose a reason for hiding this comment

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

요것도 큰 따옴표..!

runningdata/src/main/java/com/whyranoid/runningdata/RunningDataManager.kt

data class Paused(override val runningData: RunningData) : RunningState
fun getInstance(): RunningDataManager {
return INSTANCE ?: RunningDataManager().also {
Copy link
Member

Choose a reason for hiding this comment

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

크... also 깔끔하네용~!

settings.gradle Outdated
Comment on lines 25 to 28
include ':data'
include ':presentation'
include ':signin'
include ':runningdata'
Copy link
Member

Choose a reason for hiding this comment

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

그러고 보니 세팅 그레들쪽도 가능하면 ' ' -> " " 로 변경해볼까용?

@yonghanJu yonghanJu force-pushed the feat/runningdatamanager_module branch from 6cccf06 to a92fc56 Compare December 7, 2022 02:13
@yonghanJu yonghanJu force-pushed the feat/runningdatamanager_module branch from a92fc56 to 5700fe6 Compare December 7, 2022 07:05
@yonghanJu yonghanJu force-pushed the feat/runningdatamanager_module branch from 5700fe6 to 8b9c7df Compare December 7, 2022 07:11
@yonghanJu yonghanJu added 리뷰요청 병희 용한 and removed ⛔️머지금지❗️⛔️ 확인할 것이 있어요!! 머지 금지!! labels Dec 7, 2022
peter-j0y
peter-j0y approved these changes Dec 8, 2022
View reviewed changes
Copy link
Member

@peter-j0y peter-j0y left a comment

Choose a reason for hiding this comment

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

모듈 분리 고생하셨어요!

presentation/src/main/java/com/whyranoid/presentation/running/RunningUtil.kt
Comment on lines +19 to +20
date = startedAt,
startedAt = startedAt,
Copy link
Member

Choose a reason for hiding this comment

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

같은 startedAt이라는 데이터를 받는데 date와 startedAt이 다른 이유는 무엇인가요?

presentation/src/main/java/com/whyranoid/presentation/util/converters/UnitConverters.kt

import android.content.Context

object PxDpUtil {
object UnitConverters {
Copy link
Member

Choose a reason for hiding this comment

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

저번에 말했던 걸 반영해주셨네요!

runningdata/src/main/java/com/whyranoid/runningdata/model/RunningFinishData.kt
data class RunningFinishData(
val runningHistory: RunningHistoryModel,
val runningPositionList: List<List<RunningPosition>>
) : java.io.Serializable
Copy link
Member

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.

모듈을 분리해서 외부에 지원하기 위해 다른 의존성을 최대한 배제하려고 의존성을 제거 했습니다.

@yonghanJu yonghanJu merged commit 71768c2 into develop Dec 8, 2022
@soopeach soopeach deleted the feat/runningdatamanager_module branch December 8, 2022 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soopeach soopeach soopeach approved these changes

@bngsh bngsh bngsh approved these changes

@peter-j0y peter-j0y peter-j0y approved these changes

Assignees
No one assigned
Labels
리뷰완료 병희 용한
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yonghanJu @peter-j0y @bngsh @soopeach