[WEEK4] 4주차 필수과제 구현 #10
Conversation
HAJIEUN02
requested review from
kamja0510,
jihyunniiii,
hyeeum and
1971123-seongmin
| value = email, | ||
| onValueChange = { onEmailChange(it) } | ||
| value = username, | ||
| onValueChange = { onUsernameChange(it) } |
There was a problem hiding this comment.
람다 안에서 추가 동작이 필요하지 않으면 onValueChange = onUserNameChange처럼 바로 넘겨도 됩니다
이렇게 하면 Stable 상태로 봐서 퍼포먼스가 좋아진다고 하네요
| val userHobby: StateFlow<String> = _userHobby | ||
|
|
||
| init { | ||
| getUserHobby() |
There was a problem hiding this comment.
ViewModel에서 init 사용이 좋지 않다고 하네요
UI초기화 할 때 오래 걸리거나 핸들링 하기 어렵고 테스트도 힘들다고 하더라고요
| override var accessToken: String | ||
| get() = pref.getString(ACCESS_TOKEN, "") ?: "" | ||
| set(value) = pref.edit { putString(ACCESS_TOKEN, value) } |
There was a problem hiding this comment.
과제이므로 구현까지 하실 필요는 없는데요. 그냥 간단하게 리뷰 남깁니다.
SharedPreferences에 저장하는 값은 디바이스에 평문으로 저장됩니다. 지금 토큰을 평문으로 디바이스에 적고 있는 건데요. 이는 보안에 문제가 될 수 있습니다.
공격자가 이 토큰을 디바이스에서 얻어내는 순간 그 토큰을 가지고 서버에 유저인 척 요청을 보낼 수 있게 됩니다.
그러므로 이렇게 사용자를 식별할 수 있는 데이터는 왠만하면 별도의 암호화 과정을 거친 이후 디바이스에 저장해야 합니다. 이를 위해 암호화된 SharedPreferences 도 제공하고 있으니 확인 한번 해보셔요.
https://developer.android.com/reference/androidx/security/crypto/EncryptedSharedPreferences
There was a problem hiding this comment.
암호화된 SharedPreference가 있는 것을 처음 알았는데 저도 리뷰를 보면서 함께 배워갑니다 감사합니다 :)
| @GET("/$USER/$HOBBY") | ||
| suspend fun getMyHobby(): NullableBaseRespone<ResponseHobbyDto> |
There was a problem hiding this comment.
코드 스타일에 대한 질문인데요.
이렇게 변수명으로 url을 적는 것이 좋은 방법인지는 고민해볼 주제인 듯 합니다.
보통 api 명세는 get /user/hobby 요청으로 사용자 취미 목록을 받는다.는 식으로 전달받게 될 텐데요.
지금과 같은 형태로 사용하면 /user/hobby 를 에디터에서 검색해봐도 getMyHooby 함수를 찾기는 어렵습니다.
또한, 나중에 /user/like 와 같은 새로운 요구사항이 들어올 때 작업자가 AuthService에 USER라는 필드가 있었다는 걸 인지하기도 어렵구요.
어떻게 판단하시나요?
| import org.sopt.and.presentation.util.BaseResponse | ||
| import org.sopt.and.presentation.util.NullableBaseRespone |
There was a problem hiding this comment.
패키지 구조를 나누고 계시는 것 같아서요.
그렇다면 data 모듈이 presentation 모듈에 의존성을 가지고 있는게 좋지 않아보입니다.
There was a problem hiding this comment.
헉 감사합니다! util 패키지를 프로젝트 단에 두는 것이 좋을까요 아니면 base 패키지를 만들어서 data 패키지 내부에 두는 것이 좋을까요?
There was a problem hiding this comment.
제가 @HAJIEUN02 님이 어떤 아키텍처를 생각하시는지 몰라서요. 위치는 의도하시는 대로 두시면 될 것 같아요.
There was a problem hiding this comment.
저는 data, domain, presentation 별로 각 레이어에 필요한 util 패키지를 따로 둡니다! 참고하셔요!
| override var accessToken: String | ||
| get() = pref.getString(ACCESS_TOKEN, "") ?: "" | ||
| set(value) = pref.edit { putString(ACCESS_TOKEN, value) } |
There was a problem hiding this comment.
암호화된 SharedPreference가 있는 것을 처음 알았는데 저도 리뷰를 보면서 함께 배워갑니다 감사합니다 :)
| private val _userHobby = MutableStateFlow<String>("") | ||
| val userHobby: StateFlow<String> = _userHobby |
There was a problem hiding this comment.
해당 화면에서 필요한 데이터값(상태값)등은 state data class를 통해 관리해보는 건 어떨까요?
뷰모델은 로직과 관련된 부분들에 집중하고 데이터 값들도 따로 관리해보면 좋을 것 같아요 : -)
| connectTimeout(10, TimeUnit.SECONDS) | ||
| writeTimeout(10, TimeUnit.SECONDS) | ||
| readTimeout(10, TimeUnit.SECONDS) |
| @Auth | ||
| fun provideAuthInterceptor(interceptor: AuthInterceptor): Interceptor = interceptor | ||
|
|
||
| @ExperimentalSerializationApi |
There was a problem hiding this comment.
혹시 이 어노테이션은 어떻게 사용되는걸까요..! (진짜 몰라서 물어보는..)
| import org.sopt.and.domain.model.UserLoginEntity | ||
| import org.sopt.and.domain.repository.AuthRepository | ||
|
|
||
| class PostUserLoginUseCase( |
There was a problem hiding this comment.
이거는 저도 다른분께 배웠던 내용인데요!
domain의 경우 data/presentation layer를 몰라야하므로(분리되어야하므로)
post/patch 같이 data layer와의 의존도가 없는 네이밍을 하는 것을 지향한다고 합니다 : - )
| set(value) = pref.edit { putString(USER_PASSWORD, value) } | ||
| override var accessToken: String | ||
| get() = pref.getString(ACCESS_TOKEN, "") ?: "" | ||
| set(value) = pref.edit { putString(ACCESS_TOKEN, value) } | ||
|
|
||
| companion object { |
| override fun intercept(chain: Interceptor.Chain): Response { | ||
| val originalRequest = chain.request() | ||
| val authRequest = | ||
| if (localStorage.isLogin) originalRequest.newAuthBuilder() else originalRequest |
There was a problem hiding this comment.
자동로그인 구현하려고 만들어놨는데 자동 로그인 구현은 아직 안했어요(?)
ㅋㅋㅋㅋㅋ이 인터셉터 코드 이용 + 로그인할 때 로컬에 isLogin = true로 변경해주기만 하면 구현 완성입니당
|
|
||
| @Qualifier | ||
| @Retention(AnnotationRetention.BINARY) | ||
| annotation class Auth |
| writeTimeout(10, TimeUnit.SECONDS) | ||
| readTimeout(10, TimeUnit.SECONDS) | ||
| addInterceptor(authInterceptor) | ||
| if (DEBUG) addInterceptor(loggingInterceptor) |
| Timber.tag("[회원가입]").d("이메일 변경 :${event.email}") | ||
| is RegisterContract.RegisterEvent.UsernameChanged -> { | ||
| setState(currentUiState.copy(username = event.username)) | ||
| Timber.tag("[회원가입]").d("이름 변경 :${event.username}") |
| <string name="register_password">Wavve 비밀번호 설정</string> | ||
| <string name="register_password_information">비밀번호는 8~20자 이내로 영문 대소문자, 숫자, 특수문자 중 3가지 이상 혼용하여 입력해 주세요.</string> | ||
| <string name="register_title">이름과 비밀번호, 취미 입력만으로\nWavve를 즐길 수 있어요!</string> | ||
| <string name="register_username_hint">HA JIEUN</string> |
| setState(currentUiState.copy(email = event.email)) | ||
| Timber.tag("[회원가입]").d("이메일 변경 :${event.email}") | ||
| is RegisterContract.RegisterEvent.UsernameChanged -> { | ||
| setState(currentUiState.copy(username = event.username)) |
There was a problem hiding this comment.
아무리 찾아봐도 currentUiState가 선언도 없고, UiState 파일도 없는데 currentUiState 어떻게 사용하는 거에요??
There was a problem hiding this comment.
BaseViewModel에 있을 듯한 느낌!
| Timber.tag("[회원가입]").d("Email 검사 ${currentUiState.email}") | ||
| return currentUiState.email.matches(REGEX_EMAIL.toRegex()) | ||
| } | ||
| private fun checkIsValidUsername(): Boolean = (currentUiState.username.length <= MAX_LENGTH) |
There was a problem hiding this comment.
빈칸으로 회원가입해도 되버릴거 같아요!(제 코드도 그럴 거 같은..)
| localDataStorage.userPassword = currentUiState.password | ||
| Timber.tag("[회원가입]").d("비밀번호 저장 : ${localDataStorage.userPassword}") | ||
| } | ||
| private fun checkIsValidHobby(): Boolean = (currentUiState.hobby.length <= MAX_LENGTH) |
There was a problem hiding this comment.
isValid 3개가 결국 로직은 다 같으니까 하나로 묶는건 별로인가요?
| @@ -2,6 +2,5 @@ package org.sopt.and.data.datasource.local | |||
|
|
|||
| interface WaveLocalDataSource { | |||
| var isLogin: Boolean | |||
| var userEmail: String | |||
| var userPassword: String | |||
| var accessToken: String | |||
There was a problem hiding this comment.
토큰을 따로 인터페이스로 정의해 두셨네요. 혹시 이유를 알려주실 수 있으실까요??
|
|
||
| companion object { | ||
| const val FILE_NAME = "AuthSharedPreferences" | ||
| const val AUTO_LOGIN = "AutoLogin" | ||
| const val USER_EMAIL = "UserEmail" | ||
| const val USER_PASSWORD = "UserPassword" | ||
| const val ACCESS_TOKEN = "AccessToken" | ||
| } |
There was a problem hiding this comment.
상수화 정말 철저하시네요. 네이밍도 완전 직관적이라 좋아요
| override fun intercept(chain: Interceptor.Chain): Response { | ||
| val originalRequest = chain.request() | ||
| val authRequest = | ||
| if (localStorage.isLogin) originalRequest.newAuthBuilder() else originalRequest |
There was a problem hiding this comment.
아래 Request.newAuthBuilder() 함수를 이용해서 Header에 토큰을 넣잖아요?! 근데 isLogin이 true(로그인된 유저)이냐 false(로그인 안된 유저)이냐에 따라서 request header에 token을 넣어주냐 안 넣어주냐를 구분한 거에요! 로그인한 유저의 경우에만 토큰이 존재할테니까용
| ) : MyRepository { | ||
| override suspend fun getMyHobby(): Result<UserHobbyEntity> = | ||
| runCatching { | ||
| myRemoteDataSource.getMyHobby().result.toUserHobbyEntity() |
| writeTimeout(10, TimeUnit.SECONDS) | ||
| readTimeout(10, TimeUnit.SECONDS) | ||
| addInterceptor(authInterceptor) | ||
| if (DEBUG) addInterceptor(loggingInterceptor) |
| username = username, | ||
| password = userPassword, | ||
| hobby = userHobby | ||
| ) |
There was a problem hiding this comment.
objcet에 mapper 모아서 정의해 두는 방법보다 이게 더 좋을까요??
| with(waveLocalDataStorage) { | ||
| accessToken = token | ||
| isLogin = true | ||
| } |
There was a problem hiding this comment.
뷰모델에 토큰을 저장하는 부분을 따로 두셨네요. 혹시 어디에서 사용하기 위해 추가하셨나요??
| import org.sopt.and.presentation.util.BaseResponse | ||
| import org.sopt.and.presentation.util.NullableBaseRespone |
There was a problem hiding this comment.
저는 data, domain, presentation 별로 각 레이어에 필요한 util 패키지를 따로 둡니다! 참고하셔요!
| @SerialName("no") | ||
| val userId : Long |
| import retrofit2.http.POST | ||
|
|
||
| interface AuthService { | ||
| @POST("/$USER") |
There was a problem hiding this comment.
Base Url에 /까지 추가해주시는 것도 좋을 것 같아요 !
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| abstract class RepositoryModule { | ||
| @Binds |
| fun toRequestUserLoginDto() = RequestUserLoginDto( | ||
| username = username, | ||
| password = userPassword | ||
| ) |
There was a problem hiding this comment.
mapper 함수를 여기에 선언하게 되면서 domain레이어가 data 레이어에 의존성을 가지게 되었어요!
클린 아키텍처에서 의존성의 방향은 어떻게 되어야 할까요?
이를 지키기 위해선 어떤 식으로 수정을 해야할까요?
| import org.sopt.and.domain.model.UserLoginEntity | ||
| import org.sopt.and.domain.repository.AuthRepository | ||
|
|
||
| class PostUserLoginUseCase( |
| @@ -2,12 +2,16 @@ package org.sopt.and.presentation.ui.auth.login | |||
|
|
|||
| import dagger.hilt.android.lifecycle.HiltViewModel | |||
| import org.sopt.and.data.datasource.local.WaveLocalDataSource | |||
There was a problem hiding this comment.
여기도 ~ 클린 아키텍처에서 의존성에 대해 고민해봅시다 !
| setState(currentUiState.copy(email = event.email)) | ||
| Timber.tag("[회원가입]").d("이메일 변경 :${event.email}") | ||
| is RegisterContract.RegisterEvent.UsernameChanged -> { | ||
| setState(currentUiState.copy(username = event.username)) |
There was a problem hiding this comment.
BaseViewModel에 있을 듯한 느낌!
There was a problem hiding this comment.
어디까지가 비즈니스 로직인지 고민해보면 좋을 것 같아요 ~ (어디까지를 뷰모델에 담고 어디까지를 domain에 담을지 !!)
물론 이번주 세미나에서 고민해보는 시간? 알려주는 시간? 을 가질 것임
| import kotlinx.serialization.Serializable | ||
|
|
||
| @Serializable | ||
| data class BaseResponse<T>( |
There was a problem hiding this comment.
BaseResponse에 대해서 고민해봅시다.
지금 같은 명세에서는 어떤 걸 BaseResponse의 형태로 쓰면 좋을지를요
Related issue 🛠
Work Description ✏️
Screenshot 📸
XRecorder_05112024_115538.mp4
Uncompleted Tasks 😅
To Reviewers 📢
My뷰 같은 경우에는 BaseViewModel을 사용하기에는 아직 상호작용하는 부분이 많지 않아서 StateFlow를 이용해 간단하게 구현했습니다.
조만간 로그인 뷰와 회원가입 뷰에 MVI를 적용해 깔끔하게 로직을 수정하겠습니다.