-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor/clean architecture #16
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
base: develop
Are you sure you want to change the base?
Conversation
cacaocoffee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드가 너무 깔끔하셔서 많이 배운거 같아요 고민하고 어려웠던 부분이 있었는데 큰 도움이 되었습니다!
꼼꼼히 챙겨보면서 나름 리뷰 남겨봤어요!
시험기간이라 바쁠텐데 고생하셨어요!
| @Singleton | ||
| @Provides | ||
| fun provideAppDatabase(@ApplicationContext context: Context): AppDatabase { | ||
| return Room.databaseBuilder(context, AppDatabase::class.java, "user_info").build() | ||
| } | ||
|
|
||
| @Singleton | ||
| @Provides | ||
| fun provideRoomDataSource(impl: RoomDataSourceImpl): RoomDataSource { | ||
| return impl | ||
| } | ||
|
|
||
| @Provides | ||
| fun provideUserInfoDao(appDatabase: AppDatabase): UserInfoDao { | ||
| return appDatabase.userInfoDao() | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 companion object로도 묶으니까 더 보기 좋은거 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가독성을 위해서 묶은 건 아니고, companion object를 사용하지 않으면 에러가 뜨더라구요!
찾아보니, 위 코드에서 @Provdies 라는 애노테이션이 붙은 메서드들은 DatabaseModule 클래스의 인스턴스를 생성하지 않고도 바로 호출하여서, Dagger에서 의존성을 제공할 때, 해당 모듈의 메서드를 인스턴스를 생성하지 않고 직접 호출할 수 있도록 해야한다고 하네요!
그래서 정적 메소드로 만들기 위해서 묶어주었습니다.
저도 hilt 사용기가 처음이라 companion object로 묶는 게 맞는지는 잘 모르겠습니다. 한 번 알아봐야 할 것 같아요 🤔
| val isMbtiValid: LiveData<Boolean> get() = _isMbtiValid | ||
|
|
||
| fun updateNickname(newNickname: String) { | ||
| _nickName.value = newNickname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
값을 넣은 뒤 검증 로직이 들어가는걸로 보여요
검증 단계를 우선하여 유효한 경우에만 값을 집어넣으면 필요한 연산의 양이 줄어들거 같아요!
_isIdValid _ispasswordValid 가 같은 요소가 어디쓰이나 했는데 데이터 바인딩으로 xml에서 쓰이는 거군요! 배워가요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
검증 단계를 우선한다는 방법이 어떤 방법일까요 ?
현재에는 데이터바인딩을 사용해서 값이 변경될 때마다 업데이트 후 검증 로직을 타서 isValid로 사용하고 있는데, 검증 단계를 우선하여 유효한 경우에 값을 집어넣는다는 말이 어떤 의미인지 잘 모르겠습니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고 이런 livedata들은 mediatorLivedata로 묶을 수 있는 것으로 알고 잇는데 저도 안써봐서 모르겠습니다
쓰게되면 어케쓰는 건지 알려주세요ㅎㅎ
| class SignUpViewModel @Inject constructor( | ||
| private val saveUserInfoUseCase: SaveUserInfoUseCase | ||
| ) : ViewModel() { | ||
| private var _nickName: MutableLiveData<String> = MutableLiveData() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loginviewmodel 에서는 MutableStateFlow 를 쓰셨는데 여기서는 따로 LiveData로 한 이유가 뭔가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅎㅎ... loginViewModel 에서 예전에 flow 과제하면서 진행해뒀던 부분이라 flow 공부 안 해가지구 ㅎㅎ...
이번엔 그냥 변수 하나하나 따로 관리를 해줬습니다!
chanubc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
띵지 쵝오><
| fun saveUserInfo(userInfoEntity: UserInfoEntity) | ||
|
|
||
| @Query("SELECT * FROM user_info LIMIT 1") | ||
| fun getUserInfo(): UserInfoEntity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| fun getUserInfo(): UserInfoEntity | |
| fun getUserInfo(): Flow<UserInfoEntity> |
room db는 flow를 사용 할 수 있는 걸로 알고 있어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가로 interface를 구현했기 때문에 RoomDataSource는 필요 없고 UserInfoDao로 바로 RoomDataSourceImpl이 가능 할것 같습니다! 어제 회의때 이야기 한 부분인데 이 부분은 저도 아직 잘 몰라서.. 함께 고민해봐요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분에 대해서도 고민해봤는데 UserInfoDao 가 db 에 직접적으로 접근할 수 있기 때문에 RoomDataSource를 통해서 추상화를 해주어야 한다고 생각했습니다!
지금 같이 가벼운 로직에는 UserInfoDao 에서 바로 Impl로 연결해도 될 것 같긴 한데 dao의 구현을 숨겨 캡슐화 하기 위해서 저는 분리하긴 했습니다
| fun toUserInfo(): UserInfo { | ||
| return UserInfo( | ||
| nickName = this.nickName, | ||
| id = this.id, | ||
| password = this.password, | ||
| mbti = this.mbti | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map 함수는 전 dto내부에 선언하는데 웹소소 처럼
따로 디렉토리 만들기도 하더라구요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
�취향 차이인 듯 합니다! 제 생각엔 각각의 장단점이 있어서 컨벤션 맞춰서 진행하면 되지 않을까 하는 생각 ㅎㅎ ?
| return impl | ||
| } | ||
|
|
||
| @Provides |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@singleton
얘도 싱글톤으로 가는 건 어떨까요?
| @@ -0,0 +1,8 @@ | |||
| package org.sopt.dosopttemplate.domain.model | |||
|
|
|||
| class UserInfo( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data class로 써도 괜찮을것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
왜 데이터클래스를 안 썼을까요 의문이네 ..
| private const val ID_PATTERN = "^(?=.*[A-Za-z])(?=.*\\d)[A-Za-z\\d]{6,10}$" | ||
| val ID_REGEX: Pattern = Pattern.compile(ID_PATTERN) | ||
|
|
||
| private const val PW_PATTERN = | ||
| "^(?=.*[A-Za-z])(?=.*\\d)(?=.*[\$@\$!%*#?&])[A-Za-z\\d\$@\$!%*#?&]{6,12}\$" | ||
| val PW_REGEX: Pattern = Pattern.compile(PW_PATTERN) | ||
|
|
||
| private const val MBTI_PATTERN = "^[a-zA-Z]{4}\$" | ||
| val MBTI_REGEX: Pattern = Pattern.compile(MBTI_PATTERN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 유효성 검사 로직들도 도메인의 usecase로 만들어주거나 따로 클래스를 분리하게 되면 뷰모델의 책임도 덜어지고, 각 유효성 검사 로직들에 대한 테스트 용이성도 증가할 것 같습니다. -by 손흥민
이슈 번호
이슈 업슴
Key Changes
스크린샷