-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Week4] 4주차 필수 과제 #12
base: develop
Are you sure you want to change the base?
Conversation
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 OkHttpClient.Builder() | ||
.addInterceptor(loggingInterceptor) | ||
.addInterceptor(authInterceptor) | ||
.build() |
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.
릴리즈에는 필요없는 interceptor이므로 디버그 앱에만 추가해주심이 어떨까요
return OkHttpClient.Builder() | |
.addInterceptor(loggingInterceptor) | |
.addInterceptor(authInterceptor) | |
.build() | |
val builder = OkHttpClient.Builder() | |
if (BuildConfig.DEBUG) builder.addInterceptor(loggingInterceptor) | |
builder.addInterceptor(authInterceptor) | |
return builder.build() |
viewModelScope.launch { | ||
wavveRepository.getHobby().onSuccess { hobbyEntity -> | ||
_state.value = _state.value.copy( | ||
hobby = hobbyEntity.hobby | ||
) | ||
}.onFailure { } |
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.
실패하면 사용자는 아무런 피드백을 받지 못하고 빈 화면을 보게 될 것 같습니다.
앱을 사용하는 사람 입장으로 생각하면 피하고 싶은 상황일 것 같아요.
최소한 토스트로 문제가 생겼음을 보여주거나 뭔가 문제가 있으니 재시도 하라는 화면이 있으면 좋을 것 같습니다.
import javax.inject.Singleton | ||
|
||
@Singleton | ||
class User @Inject constructor( |
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 혹은 repository 수준에서 관리될 친구로 보이네요.
data로 관리한다면 UserToken 정보를 디바이스에 저장하는 객체로 변모할 것이고 (현재 프로젝트 구조 상 이름은 UserDataSource, TokenDataSource ?)
repository로 관리한다면 UserToken 정보를 저장하고, 필요한 시점에 초기화 하는 인터페이스를 제공하는 객체로 활용되겠네요. (현재 프로젝트 구조에 맞는 이름은 아마도 UserRepository? AuthRepository? SignRepository)
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.
역시 혜음.. 저같은 사람도 코드를 알아볼 수 있게 짜셨네요(mvi는 살짝 이해못함 ㅎ)
항상 코드리뷰 시간이 아니라 보면서 공부하는 느낌
@Provides | ||
fun provideLoggingInterceptor(): HttpLoggingInterceptor { | ||
return HttpLoggingInterceptor { message -> | ||
Log.d("Retrofit2", "CONNECTION INFO -> $message") |
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.
세미나에서 배운것은 이 중괄호 블럭이 없는데 이 블럭이 추가되면서 어떤 추가적인 정보를 볼 수 있나요?
|
||
@Serializable | ||
data class RequestSignUpDto( | ||
@SerialName("username") |
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.
변수명과 json key명이 같은데도 SerialName 어노테이션을 굳이 써야하나요?
@SerialName("hobby") | ||
val hobby: String | ||
){ | ||
fun toEntity() = ResponseHobbyEntity( |
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 NavController.navigateSearch( | ||
navOptions: NavOptions | ||
) { | ||
navigate(Search, navOptions) | ||
navigate(org.sopt.and.feature.search.Search, navOptions) |
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.
오잉 왜 갑자기 경로 다 줘야되는걸로 바뀌었나요
val password: String = "", | ||
var isPasswordVisible: Boolean = false, | ||
) { | ||
val isButtonEnabled: Boolean = username.isNotEmpty() && password.isNotEmpty() |
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.
이 줄이 왜 중괄호 블럭으로 따로 빠져 있나요? 저 username이랑 password를 쓰려면 따로 빼야하는 건가요?
@@ -72,9 +94,8 @@ class SignUpViewModel @Inject constructor() : ViewModel() { | |||
} | |||
|
|||
companion object { | |||
private const val MIN_PASSWORD = 8 | |||
private const val MAX_PASSWORD = 20 | |||
private const val MIN_SIGNUP_LENGTH = 1 |
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.
와 저는 상수화도 안하고 최소값1로 두지도 않았네요 혼나겠다
@Singleton | ||
class User @Inject constructor( | ||
@ApplicationContext private val context: Context | ||
) { | ||
private val sharedPreferences: SharedPreferences = |
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.
호옥시 datastore 안쓰시고 sharedPreferences 쓰시는 이유가 있나요!
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.
사소한 부분들도 string들을 다 바꾸셨네요!
) { | ||
val state by viewModel.state.collectAsStateWithLifecycle() | ||
|
||
LaunchedEffect(true) { |
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.
이런거 보면 Route를 쓰는게 좋을 거 같다는 생각이 드네요
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.
고생많았서요 혜음이~~ 클린아키텍처 적용을 해서 그런지 저랑 비슷한 부분이 많아서 보기 편했습니다 ㅎㅎ 유스케이스도 한번 사용해보셔요!
|
||
// Network | ||
implementation(platform(libs.okhttp.bom)) | ||
implementation(libs.okhttp) |
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.
okhttp랑 retrofit처럼 같은 카테고리인 친구들은 각각 libs.versions.toml 내부에서 bundles로 묶어서 한번에 디펜던시 추가해줄 수 있서요! 그러면 더 보기 깔끔할 듯합니당
tools:targetApi="31"> | ||
<activity | ||
android:name=".main.MainActivity" | ||
android:name=".feature.main.MainActivity" |
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.
오호 ui가 아니라 feature로 패키지를 구성하셨군요
새로운 네이밍 좋네요
@Provides | ||
fun provideLoggingInterceptor(): HttpLoggingInterceptor { | ||
return HttpLoggingInterceptor { message -> | ||
Log.d("Retrofit2", "CONNECTION INFO -> $message") |
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.
로그는 PR 올릴 때 삭제해주시거나 Timber로 바꿔주시면 좋을 것 같아요!!
val password: String, | ||
) | ||
|
||
fun RequestSignInEntity.toDto() = RequestSignInDto( |
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.
요거는 엔티티를 디티오로 바꾸는 거니까 엔티티가 주체잖아요!! 그래서 저라면 엔티티에 해당함수를 구현할 것 같아요
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.
고생하셨습니다. 항상 잘하셔서 리뷰라기보단 많이 배워가는 것 같아요.
override suspend fun getUserHobby(): BaseResponse<ResponseUserHobbyDto> = | ||
wavveService.getUserHobby() | ||
|
||
} |
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.
깔끔 그 자체에요. 코드 너무 좋아요
@Binds | ||
@Singleton | ||
abstract fun bindsDataSource(myDataSourceImpl: WavveDataSourceImpl): WavveDataSource | ||
} |
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.
데이터스토어 모듈을 따로 만드셨네요. 저도 분리해야겠어요.
@SerialName("token") | ||
val token: String | ||
){ | ||
fun toEntity() = ResponseSignInEntity( |
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.
objcect에 Mapper를 모아두는 방식을 안쓰고 데이터 클래스에 바로 정의하신 이유 알려주실 수 있나요??
|
||
import androidx.navigation.NavController | ||
import androidx.navigation.NavGraphBuilder | ||
import androidx.navigation.compose.composable | ||
import kotlinx.serialization.Serializable | ||
import org.sopt.and.main.MainTabRoute | ||
import org.sopt.and.feature.main.MainTabRoute | ||
|
||
fun NavController.navigateSignUp() { | ||
navigate(SignUp) |
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.
컴포즈도 보통 이렇게 네비게이션 함수를 확장으로 두고 쓰나요??
val hobby: String = "", | ||
var isPasswordVisible: Boolean = false, | ||
) { | ||
val isButtonEnabled: Boolean = username.isNotEmpty() && password.isNotEmpty() |
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.
이렇게 하면 버튼 활성화/비활성화를 관리할 때 훨씬 편할까요??
<string name="not_valid_input">유효하지 않은 입력입니다.</string> | ||
|
||
<!--signin--> | ||
<string name="signin">로그인</string> | ||
<string name="signin_placeholder">이메일 주소 또는 아이디</string> | ||
<string name="signin_username_placeholder">이름</string> | ||
<string name="signin_password_placeholder">비밀번호</string> | ||
<string name="find_id">아이디 찾기</string> | ||
<string name="reset_password">비밀번호 재설정</string> | ||
<string name="join">회원가입</string> |
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.
string 이름들이 정말 깔끔하고 직관적이네요..
Related issue 🛠
Work Description ✏️
Screenshot 📸
취미 불러오기
To Reviewers 📢