-
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
feat/#9: week4 compose 과제 구현 #12
base: develop-compose
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.
안녕하세요 유진님. 안드로이드 파트 명예 OB 이현우입니다. 만나서 반갑습니다.
앞으로의 과제들에 대한 코드리뷰를 맡게 되었습니다. 고민해보시다가 궁금한 점이 있으시면 지식 공유방/디스코드 연락 해주세요. 대면/비대면 자유형식으로 추가 피드백 가능합니다.
우선 과제 어려우셨을텐데 빠르게 과제 수행해주신점 너무 좋습니다. 관련해서 추가적으로 피드백/질문 남긴 것들이 있습니다. 질문을 남긴 경우 코드에 문제가 있어서 질문이 남긴 것이 아니고 정말 유진님의 작성 의도가 궁금해서 남긴 것이니 이 점 참고해주시면 감사하겠습니다.
앞으로 잘 부탁드리겠습니다.
추가적으로 현재 상태에 따라서 화면을 그리는 컴포저블의 렌더링 과정에 대해 이해가 필요한 것 같습니다. 해당 부분은 코드랩 혹은 안드로이드 공식 양질의 샘플들을 확인해보시면 좋을 것 같습니다.
|
||
object ApiFactory { | ||
private const val BASE_URL: String = BuildConfig.AUTH_BASE_URL | ||
lateinit var userPreference: UserPreference |
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.
ApiFactory가 userPreferences를 가지고 있어야 하는 이유가 무엇일까요? ApiFactory라는 클래스(오브젝트)는 어떤 역할을 수행하고 있을까요?
class HeaderInterceptor : Interceptor { | ||
@Throws(IOException::class) | ||
override fun intercept(chain: Interceptor.Chain): Response { | ||
if (!ApiFactory::userPreference.isInitialized) { |
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.
오 isInitialized 사용은 좋습니다! 👍🏻
class UserPreference(context: Context) { | ||
private val sharedPreferences = context.getSharedPreferences("userData", Context.MODE_PRIVATE) |
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.
이와 같은 방식은 어떡 문제를 야기할 수 있을까요? 만약에 context를 Activity나 Fragment의 context로 집어넣는다고 하면 어떤 문제가 발생될 수 있을까요?
fun saveUserId(userId: String) { | ||
with(sharedPreferences.edit()){ | ||
putString("userId", userId) | ||
apply() |
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.
apply를 적용하신 이유가 있을까요?
|
||
// 사용자 아이디 저장 | ||
fun saveUserId(userId: String) { | ||
with(sharedPreferences.edit()){ |
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.
shared preferences ktx 함수중에서 아래와 같은 함수로 더 편하게 사용하실 수 있습니다.
sharedPreferences.edit { it.putString("userId", userId) }
class LoginViewModel : ViewModel() { | ||
private val authService by lazy { ServicePool.authService } | ||
val liveData = MutableLiveData<BaseState>() | ||
val userIdLiveData = MutableLiveData<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.
이는 약간 정보가 너무 과다하게 담긴 경우인데요.
예를 한번 들어보자면, 저희가 엽기떡볶이 먹으러가자고 하지 "동대문에서 시작되고 40년동안 대한민국에서 맵기로 그 이름을 떨친 유구한 역사를 가진 엽기떡볶이" 를 먹으러가자 라고 하진 않자나요? 과도한 정보는 가독성에 악영향을 줄 수 있습니다.
@Serializable | ||
data class BaseState( | ||
@SerialName("isSuccess") | ||
val isSuccess: Boolean, | ||
@SerialName("message") | ||
val message: 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.
해당 데이터는 UI에서 사용되는 것으로 알고 있는데 이 데이터 클래스에 Serializable 붙인 이유를 알 수 있을까요?
0 -> SetHomeView(viewModel) | ||
1 -> Text(text ="Search") | ||
2 -> SetMyPageView(viewModel) |
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.
0, 1, 2가 어떤 값일까요? 이 부분도 컨텍스트가 없으면 다른 개발자분들이 알아보기 어려울 수도 있을 것 같아요(지금같이 짧은 코드에서는 이해하기 쉽겠지만, 협업 시에 문제가 발생할 수 있습니디)
viewModel.userInfo() | ||
|
||
viewModel.userInfoLiveData.observeAsState().value?.let { userInfo -> | ||
HomeView( | ||
userName = userInfo.data.nickname, | ||
userPhone = userInfo.data.phone, | ||
) | ||
} |
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 com.sopt.now.compose.ui.theme.NOWSOPTAndroidTheme | ||
|
||
@Composable | ||
fun MyPageView(userId: String, userPw: String, userName: String, userDescription: String) { | ||
fun MyPageView(userId: String, userPw: String, userPhone: String) { | ||
Log.d("myPage", "${userId}") |
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.
Log는 한번만 찍힐까요? 한번만 찍히지 않는다면 왜 그럴꺼요?
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.
이번주도 고생하셨어요~
xml에서 공통되는 부분은 빼고 코리 달았습니답
현우님이 너무 세세하게 잘 해주셔서 보고 꼭 적용하셨으면 좋겠습니다!
val userId = getString("userId", null) | ||
val userName = getString("userName", null) | ||
val userPhone = getString("userPhone", null) |
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.
이런 상수들도 따로 빼둔다면 추후 수정이 용이해집니당
try { | ||
val errorResponse = gson.fromJson(error, ResponseAuthDto::class.java) | ||
liveData.value = BaseState( | ||
isSuccess = false, | ||
message = "회원가입 실패: ${errorResponse.message}" // 에러 메시지 사용 | ||
) | ||
} catch (e: Exception) { | ||
liveData.value = BaseState( | ||
isSuccess = false, | ||
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.
내부에서 세심하게 체크해준거 너무너무 좋네요~!!
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.
명예OB 한분이 더 오시니까 제가 쓸 말이 더 없어졌네요ㅎㅎ
너무 잘하셨고 고생 많으셨어요😁
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.
주차가 지날수록 코드가 점점 더 깔끔해지시는 것 같아요...! 저도 유진언니처럼 쑥쑥 성장하는 YB가 되고싶습니다 하하
fun login(request: RequestLoginDto) { | ||
authService.login(request).enqueue(object : Callback<ResponseAuthDto> { | ||
override fun onResponse( | ||
call: Call<ResponseAuthDto>, | ||
response: Response<ResponseAuthDto>, | ||
) { | ||
if (response.isSuccessful) { | ||
val data: ResponseAuthDto? = response.body() | ||
val userId = response.headers()["location"] | ||
userIdLiveData.value = userId | ||
liveData.value = BaseState( | ||
isSuccess = true, | ||
message = "로그인 성공! 유저의 ID는 $userId 입니다." | ||
) | ||
Log.d("Login", "data: $data, userId: $userId") | ||
} else { | ||
val error = response.message() | ||
liveData.value = BaseState( | ||
isSuccess = false, | ||
message = "로그인 실패 $error" | ||
) | ||
} | ||
} | ||
|
||
override fun onFailure(call: Call<ResponseAuthDto>, t: Throwable) { | ||
liveData.value = BaseState( | ||
isSuccess = false, | ||
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.
이부분에서는 메세지 파싱 실패 경우가 없는데 일부러 안넣으신걸까요...?!
feat/#15: week6 compose 과제 구현
Related issue 🛠
Work Description ✏️
Screenshot 📸
KakaoTalk_20240503_234504327.mp4
To Reviewers 📢