[REF/#682] Abstract device info retrieval and improve data module structure #683
[REF/#682] Abstract device info retrieval and improve data module structure #683
Conversation
nhyeonii
left a comment
There was a problem hiding this comment.
고생하셨습니다! DeviceInfoProvider를 사용하면 확실히 안드로이드 관련 의존성이 분리될 수 있을것 같아요. 😀
core/common/src/main/java/com/hilingual/core/common/app/DeviceInfoProvider.kt
Show resolved
Hide resolved
nahy-512
left a comment
There was a problem hiding this comment.
버전이랑 기기 정보가 엮여있는 부분이 많았네요
작업해주신 덕에 의존성 가닥이 조금 더 명확하게 잡힌 것 같아요ㅎㅎ 고생많으셨습니다!
| private val authRemoteDataSource: AuthRemoteDataSource, | ||
| private val googleAuthDataSource: GoogleAuthDataSource, | ||
| private val systemDataSource: SystemDataSource, | ||
| private val deviceInfoProvider: DeviceInfoProvider, |
There was a problem hiding this comment.
Manager과 Provider 명칭을 구분하시는 기준이 있으실까요??
아래에는 TokenManager, UserInfoManager를 사용하셨기에 궁금해서 여쭤봅니다:)
디바이스 정보를 가져오는 것도 어쩄든 관리의 영역이라고 볼 수도 있을 것 같아서, DeviceInfoProvider 말고 DeviceInfoManager로 쓸 수도 있지 않을까 싶어서요!
There was a problem hiding this comment.
Manager와 Provider 명칭에 대해 저도 고민을 많이 했었는데요! 제가 생각하는 기준은 '상태 관리나 저장/수정 로직이 포함되는가'입니다.
TokenManager나 UserInfoManager는 데이터를 로컬에 저장하고 갱신하거나 삭제하는 등 데이터의 생명주기를 CRUD하는 성격이 강합니다.
DeviceInfoProvider는 시스템으로부터 기기 정보를 단순히 provide 받는 읽기 전용 역할이기에 Provider가 더 적합하다고 생각해요. 관리보다는 시스템 정보를 안전하게 읽어오는 통로 역할에 집중했다고 봐주시면 좋을 것 같습니다👍🏻
| @@ -0,0 +1,3 @@ | |||
| package com.hilingual.core.common.constant | |||
|
|
|||
| const val STABLE_VERSION = "2.0.0" | |||
There was a problem hiding this comment.
버전 관련 상수를 모아두는 용도로 VersionConstant로 이름붙이신 것 같은데, STABLE_VERSION으로 설정된 이유가 있는지 궁금해요! 버전 관리를 위해 앞으로 어떤 상수들이 또 생기게 될 수 있을까요? 기존의 DEFAULT_VERSION을 이번에 다 STABLE_VERSION으로 대체하신 것 같길래 여쭤봅니다!
There was a problem hiding this comment.
기존 DEFAULT_VERSION이라는 이름은 '0.0.0' 같은 단순히 빈 값을 채우는 의미로만 느껴져서, 앱이 정상적으로 동작한다고 보장할 수 있는 최소한의 기준점을 명시하고자 STABLE_VERSION으로 변경했습니다.
그리고 버전 추출 실패 시 비정상적인 버전 값 때문에 강제 업데이트 로직이 잘못 실행되는 등의 부수 효과를 방지하는 안전장치 역할을 합니다. 앞으로는 MIN_SUPPORTED_VERSION처럼 정책적인 상수들이 추가될 수 있을 것 같아요.
| packageManager.getPackageInfo(packageName, 0) | ||
| } | ||
| packageInfo.versionName | ||
| }.getOrNull() ?: STABLE_VERSION |
There was a problem hiding this comment.
DeviceInfoProviderImpl에서 사용하는 걸로 보이는데, 이외에 또 해당 확장함수가 사용될 곳이 있을까요??
디바이스 관련 정보를 앞으로 DeviceInfoProvider로 관리한다고 하면, 구현체에 한 번만 쓰일 함수이지 않을까 싶은데 Ext 파일로 로직을 분리하신 이유가 궁금합니다
There was a problem hiding this comment.
음 버전을 UI에서 불러올때 사용하는걸 의도했습니다. 정말 단순하게 버전을 불러올 뿐이라면 해당 함수를 사용하는게 더 간결하니까요! 해당 의도를 더 전달하기 위해서 Mypage는 Provider없이 사용하도록 수정하겠습니다
Hyobeen-Park
left a comment
There was a problem hiding this comment.
안그래도 앱 버전때문에 뷰모델에 @ApplicationContext 주입되는게 신경쓰여서 의견 제안해보려 했는데 바로 작업해주셨네요!! 수고하셨습니다~~!!
| @@ -0,0 +1,3 @@ | |||
| package com.hilingual.core.common.constant | |||
|
|
|||
| const val STABLE_VERSION = "2.0.0" | |||
There was a problem hiding this comment.
앞으로 버전 계속 올라갈텐데 STABLE_VERSION은 고정인가요? 아님 메이저만 반영하는 등의 기준이 있을까요?
There was a problem hiding this comment.
이 값은 고정이라기보다는 프로젝트의 기준이 되는 '최소 보장 버전'의 성격입니다. 버전 정보를 가져오지 못하는 예외 상황에서도 앱의 비즈니스 로직이 깨지지 않게 하기 위한 최소한의 가이드라인이라고 봐주시면 될 것 같습니다. 말씀하신 대로 메이저 업데이트나 중요한 정책 변경이 있을 때 이 상수도 함께 관리할 예정입니다!
| } | ||
|
|
||
| val Context.appVersionName: String | ||
| get() = runCatching { |
There was a problem hiding this comment.
여기에서 suspendRuncatching이 아닌 runcatching을 사용하신 이유가 있나요?
There was a problem hiding this comment.
appVersionName을 확장 프로퍼티로 선언했기 때문에 호출하는 쪽에서 별도의 코루틴 스코프 없이도 동기적으로 값을 바로 얻을 수 있도록 runCatching을 사용했습니다.
PackageManager를 통한 정보 조회는 I/O나 네트워크 작업이 아닌 가벼운 시스템 API 호출이라 suspend로 만들지 않아도 성능상 영향이 미비하다고 판단했고 우리 프로젝트 컨벤션인 suspendRunCatching은 비동기 처리가 필요한 Repository 레이어의 로직에서 적극 활용하는게 의도니까요ㅎㅎ
015689b to
cb02600
Compare
Daljyeong
left a comment
There was a problem hiding this comment.
DeviceInfoProvider가 생기니 코드 구조가 더 깔끔해진 것 같네요!! 수고하셨습니다🙇🏻♀️
| override fun getDeviceName(): String = Build.MODEL | ||
|
|
||
| override fun getDeviceType(): String = | ||
| if (context.resources.configuration.smallestScreenWidthDp >= 600) "TABLET" else "PHONE" |
There was a problem hiding this comment.
이전에도 있었던 코드이긴 하지만, 이번에 보다가 궁금한 점이 생겨서 댓글 남깁니다..!
smallestscreenWidthDp에 따라 디바이스 타입이 결정되는 것으로 이해했는데요, 코드를 보니 로그인 시점에만 이 부분이 호출되는 것으로 파악했습니다.
이 값이 API 호출 시 단순 통계/로그용으로만 사용되는 것인지 궁금합니다!
There was a problem hiding this comment.
현재 앱에서는 넓이를 기준으로 변하는 디자인을 채용하고 있지 않습니다. 해당 정보는 서비스 서버의 응답을 위한 코드입니다. 노션에 명세를 보면 알수있어요👍🏻
서버 자체에서 판단하는 정보이고 앰플리튜드 트래커로 추적하지 않는 정보입니다.
Related issue 🛠
Work Description ✏️
1. 앱 환경 정보 추상화 (
DeviceInfoProvider)data,presentation레이어 곳곳에서Context나Build클래스를 직접 참조하여 앱 버전이나 기기 정보를 조회하던 로직을DeviceInfoProvider인터페이스로 추상화DeviceInfoProvider인터페이스 정의DeviceInfoProviderImpl구현체 정의2.
data/config모듈 구조 개선ConfigModule하나로 묶여있던 DI 설정을 컨벤션에 맞춰DataSourceModule과RepositoryModule로 분리RemoteConfigDataSourceImpl에서 불필요하게 예외잡던runCatching블록을 제거하여 Repository 레이어에서 에러를 핸들링하도록 수정3. ViewModel 및 UI 로직 개선
Context를 직접 참조하던 방식을 버리고DeviceInfoProvider를 주입받아 스스로 버전을 체크하도록 변경async/await를 도입하여 병렬 처리로 개선UiState에 포함시켜 U로 전달하도록 변경,MyPageScreen은 더 이상Context에서 직접 버전을 조회하지 않고 순수하게 상태를 렌더링Screenshot 📸
N/A
Uncompleted Tasks 😅
N/A
To Reviewers 📢
SystemDataSource가 삭제되고DeviceInfoProvider로 대체되었습니다.DeviceInfoProvider는core/common에 인터페이스가,app모듈에 구현체가 위치합니다.