Feat/#10 week4 xml 필수과제 구현 #12
Conversation
There was a problem hiding this comment.
안녕하세요! 4주차 과제 3조 코드리뷰로 참여하게 된 32기, 33기 안드로이드 수료한 김상호입니다 :)
- 코드리뷰에 담긴 질문들은 꼭 고쳐야한다고 말씀드리는게 아닙니다! 코드를 활용해보는 과정에서 생각을 담은 개발을 하기 위한 하나의 질문을 던져드린다고 생각해주세요 ㅎㅎ (고치세요!가 아니라… 이유가 궁금하거나 알고 썼나 궁금해서 질문드리는…것입니다)
- 파송송계란탁님 반갑습니다 ㅋㅌ
- "코드를 작성하면서 변수를 어느 위치에서 선언해야하는지에 대해 고민이 많은 것 같습니다." 라고 질문을 주셨는데, 잘하고 계세요! "뷰모델은 비즈니스 로직을 담당한다"라는 말을 들어보셨을 것 같은데, userId와 같은 변수들도 보통의 경우 뷰모델에서 관리해주는 것이 좋다고 생각합니다! 액티비티/프래그먼트에서는 정말 "UI에 관련된" 기능들만 두는 것이 권장됩니다. 예를 들어 리스너를 단다던지, 뷰모델에서 liveData를 observe해서 UI에 표시되는 값을 바꿔준다던지, binding, adapter과 같은 UI 컴포넌트를 연결해준다던지 와 같이요. 이외의 기능들, 변수들은 모두 뷰모델에서 관리를 해주시는 것이 권장됩니다. 지금은 로직이 간단해서 뷰모델에서 서버통신만 진행하는 것처럼 보이는데, 조건문들에 의해서 로직이 복잡해지게 되더라도 가능한만큼 뷰모델에서 최대한 관리해보는 연습을 하는게 좋을 것 같아요.
- 그리고,,, 너무 잘하십니다 ㅎㅎ 세미나에서 어느정도까지 자료를 제공해주었는지는 모르겠지만, 동민이가 얼마나 관리를 했는지도 모르겠지만 ㅎㅎ 코드가 대체적으로 굉장히 깔끔하고 변수명, xml 등 기본적인 작성이 너무 깔끔해서 보기 좋아요 ~~~~~~~ 어우 다들 넘 잘하십니다 고생 많으셨어요 :)
app/build.gradle
Outdated
| buildFeatures { | ||
| viewBinding = true | ||
| buildConfig true | ||
| } |
There was a problem hiding this comment.
buildConfig에 = 을 안붙여도 작동을 하네요! 첨 알고갑니닷
There was a problem hiding this comment.
@Marchbreeze groovy 기반 gradle에서는 = 안 붙이는게 더 자연스럽긴 합니다.
|
|
||
| class PreferenceUtil(context: Context) { | ||
| private val prefs: SharedPreferences = | ||
| context.getSharedPreferences("userId", Context.MODE_PRIVATE) |
There was a problem hiding this comment.
userId와 같이 키-벨류 에서 키값을 담당하는 값들은 따로 상수화해서 명시해주는 것이 권장됩니다!
(동일한 의미의 팁으로, 액티비티 간 Intent를 주고받을 때 사용되는 키값들도 보내는 액티비티1에서 상수화해두고, 받는 액티비티2에서는 액티비티1에서의 상수값을 import해서 사용하게 되면 두 액티비티 간 키값이 달라서 생기는 문제를 최소화할 수 있겠죠?)
| @Serializable | ||
| data class UserInfo ( | ||
| @SerialName("authenticationId") | ||
| val authenticationId: String, | ||
| @SerialName("nickname") | ||
| val nickname: String, | ||
| @SerialName("phone") | ||
| val phone: String | ||
| ) |
There was a problem hiding this comment.
ResponseUserInfoDto의 data에 해당되는 UserInfo의 경우,
data class ResponseUserInfoDto(
) {
@Serializable
data class UserInfo(
)
}
처럼 종속되어 활용할 수 있습니다! 다만 UserInfo가 Dto와 독립적으로 사용되는 경우가 더 있다면 사용하지 않는 것이 좋겠죠? 취향 차이입니다 ㅎㅎ
|
|
||
| class LoginActivity : AppCompatActivity() { | ||
|
|
||
| private val binding by lazy { ActivityLoginBinding.inflate(layoutInflater) } |
There was a problem hiding this comment.
binding은 액티비티의 생명주기와 완전히 일치하지 않기 때문에, 액티비티의 생명주기가 destroy될 때 함께 해제해주는 것이 메모리 누수를 최소화할 수 있습니다! 백패킹을 활용한다면 쉽게 대응해줄 수 있어요~
There was a problem hiding this comment.
@Marchbreeze Fragment의 경우에는 View와 객체의 생명주기가 달라서 binding을 백킹으로 둬야 안전하지만, Activity의 경우네는 View가 Activity 객체가 destroy 된 이후 자동으로 정리가 되어서 문제가 없습니다. 혹시 관련해서 메모리 릭이 난 경우 공유해주시면 감사하겠습니다.
There was a problem hiding this comment.
@l2hyunwoo 정정 감사합니다! 다른 분들 코리에서도 수정해두도록 하겠습니다. adapter나 dialog의 경우는 어떠한가요?
There was a problem hiding this comment.
adapter도 액티비티에서는 정상적으로 수거되는거 확인했던 경험이 있구요, 다이얼로그는 애가 워낙 메모리 깨지기 너무 쉬운친구여서 그냥 멤버 변수로 안 두고 사용합니다. 필요할 때만 지역변수로 만들어서 사용해요.
There was a problem hiding this comment.
@l2hyunwoo 감사합니다 쌤,,, 역시 안드 백과사전은 다르네요 또 배우고갑니닷
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
| setContentView(binding.root) | ||
| prefs = PreferenceUtil(applicationContext) |
| private fun getLoginRequestDto(): RequestLoginDto { | ||
| val id = binding.etvLoginId.text.toString() | ||
| val password = binding.etvLoginPw.text.toString() | ||
| return RequestLoginDto( | ||
| authenticationId = id, | ||
| password = password | ||
| ) | ||
| } |
There was a problem hiding this comment.
| private fun getLoginRequestDto(): RequestLoginDto { | |
| val id = binding.etvLoginId.text.toString() | |
| val password = binding.etvLoginPw.text.toString() | |
| return RequestLoginDto( | |
| authenticationId = id, | |
| password = password | |
| ) | |
| } | |
| viewModel.login(RequestLoginDto( | |
| authenticationId = binding.etLoginId.text.toString(), | |
| password = binding.etLoginPw.text.toString() | |
| ) | |
| ) |
처럼 추가적인 함수화 대신 위의 리스너에서 바로 활용해도 깔끔할 것 같아요
|
|
||
|
|
||
| class LoginViewModel : ViewModel() { | ||
| private val authService by lazy { ServicePool.authService } |
There was a problem hiding this comment.
여기서 서비스를 by lazy로 받아오는 이유는 무엇일까요?
| binding.tvMainNick.text=userInfoState.userNick | ||
| binding.tvMainId.text=userInfoState.userId | ||
| binding.tvMainPhone.text=userInfoState.userPhone |
There was a problem hiding this comment.
여기에도 binding이 반복되는 부분을 스코프 함수를 활용한다면 더 깔끔하게 코드를 작성할 수 있을 것 같아요! 깔끔해지는 것과 더불어, 다음의 함수는 binding에 관련된, 즉 UI를 직접적으로 관여하는 코드임을 암시적으로 표시해주는 기능도 해줄 수 있답니당
| private fun initViews() { | ||
| viewModel.userInfo() | ||
| } |
There was a problem hiding this comment.
이처럼 화면이 켜졌을 때 뷰모델의 함수가 실행되는 것은, 뷰모델에서 init{} 으로도 진행시킬 수 있답니다 ! 이 프래그먼트에 뷰모델이 연결되는 16번째 줄이 실행될 때 같이 실행되도록 할 수 있어요
| import retrofit2.http.GET | ||
| import retrofit2.http.Query | ||
|
|
||
| interface FollwerService { |
There was a problem hiding this comment.
영단어 오타는 sonarlint라는 안스 플러그인을 받으면 쉽게 탐지할 수 있답니다 !
| val intent = Intent(this, SignUpActivity::class.java) | ||
| startActivity(intent) |
There was a problem hiding this comment.
요기도 스코프 함수를 활용할 수 있을 것 같네요!
저도 자주 까먹어서 매번 확인합니다😅
| userId = response.headers()["location"] | ||
| userId?.let { LoginActivity.prefs.setString("userId", it) } |
| val id = binding.etvSignUpId.text.toString() | ||
| val password = binding.etvSignUpPw.text.toString() | ||
| val nickname = binding.etvSignUpNick.text.toString() | ||
| val phoneNumber = binding.etvSignUpPhone.text.toString() |
| val userRetrofit: Retrofit by lazy { | ||
| Retrofit.Builder() | ||
| .baseUrl(BASE_URL) | ||
| .client(provideOkHttpClient(HeaderInterceptor())) | ||
| .addConverterFactory(Json.asConverterFactory("application/json".toMediaType())) | ||
| .build() | ||
| } | ||
| private fun provideOkHttpClient(interceptor: HeaderInterceptor): OkHttpClient | ||
| = OkHttpClient.Builder().run { | ||
| addInterceptor(interceptor) | ||
| build() | ||
| } | ||
| class HeaderInterceptor : Interceptor { | ||
| @Throws(IOException::class) | ||
| override fun intercept(chain: Interceptor.Chain): Response = with(chain) { | ||
| val accessToken = LoginActivity.prefs.getString("userId", "") | ||
| Log.d("Login", accessToken) | ||
| val newRequest = request().newBuilder() | ||
| .addHeader("memberId", accessToken) | ||
| .build() | ||
| proceed(newRequest) | ||
| } | ||
| } |
There was a problem hiding this comment.
밑에 구현된 ServicePool와 같은 싱글톤 객체는 처음 생성된 시점에 하위 변수들을 모두 초기화해준다고 알고 있어요!!
로그인을 위해 authService에 먼저 접근하게 되면 userService도 동시에 초기화되고, 그렇게 되면 로그인 api의 응답으로 받는 유저ID를 Preference에 새로 저장하기 전에 과거에 Preference에 저장된 값을 userService가 참조하게 되는 상황이 발생하지 않나요?
제가 이 부분을 좀 특이하게 구현해놔서 혜음님 방법이 문제없다면 고쳐봐야겠어요ㅎㅎ
만약 문제가 발생한다면 userService 변수를 lazy로 초기화하는 방법을 고려해봐도 되겠네요~
| val userId = response.headers()["location"] | ||
| liveData.value = SignUpState( | ||
| isSuccess = true, | ||
| message = "회원가입 성공 유저의 ID는 $userId 입니다" |
There was a problem hiding this comment.
문자열 하드코딩은 지양해봐요~
문자열 리소스에 변수를 추가해서 출력하는 방법도 있답니다!
| @POST("member/join") //http 메소드 | ||
| fun signUp( | ||
| @Body request: RequestSignUpDto | ||
| ): Call<ResponseSignUpDto> //비동기->callback | ||
| @POST("member/login") | ||
| fun login( | ||
| @Body request : RequestLoginDto | ||
| ) : Call<ResponseLoginDto> |
There was a problem hiding this comment.
post로 api 통신을 한다는 것을 함수명에도 표시해주면 좋을 것 같네요!
그렇게 하면 추후 관리가 편해지더라구용
signUp -> postSignUp
|
|
||
| class SignUpViewModel : ViewModel() { | ||
| private val authService by lazy { ServicePool.authService } | ||
| val liveData = MutableLiveData<SignUpState>() |
There was a problem hiding this comment.
Mutable인 변수를 그대로 선언해둔다면 view에서 실수로 값을 직접변경 해버릴 수 있습니다.
아래 사진에서 사용하는 방법을 찾아보고 장점을 알고 적용해본다면 좋을 것 같아요
| class MyPageFragment : Fragment() { | ||
| private val binding: FragmentMyPageBinding | ||
| get() = requireNotNull(_binding) { "_binding이 null이 아닌 경우만 _binding 반환" } | ||
| private var _binding: FragmentMyPageBinding? = null |
There was a problem hiding this comment.
매번 이렇게 선언하기 귀찮을텐데 baseFragment를 만들어보는건 어떨까용~
There was a problem hiding this comment.
@chattymin 상속을 활용하는 방식은 당장으로서는 좋을 수 있으나, Base가 여러번 상속받는 경우로 구조를 잡을 경우 Base에서 발생하는 에러를 수정해야하는 경우 에러 발생화면 이외에도 다른 화면들도 같이 QA를 돌려야하는 경우가 발생될 수 있습니다. 그러면 의도치 않은 코드 발생도 여러곳에서 일어날 수 있겠죠? 그래서 웬만하면 상속을 하실 때에는 기능별로 작게작게 Base Class를 만들어서 상속하시는 것을 권장드립니다(상속을 안 사용하는 것도 방법중 하나)
예를 들어서 Binding 객체만 생성해서 제공해주는 베이스라면 BaseFragment가 아닌, BindingFragment 로 이름을 지을 수 있겠네요.
참고) BindingFragment
There was a problem hiding this comment.
아하 저도 의도해서 말한게 Binding객체만 생성해주는 Fragment를 말한거였습니다...!!
저도 상속받는 Fragment내부에 많은 값이 들어있다면 에러를 찾기 힘들고, Base가 무거워진다는 생각에 Binding 객체만 만들어주는 편입니다.
습관적으로 BaseFragment라고 명명하고 사용했는데 앞으론 이름을 바꿔서 BindingFragment로 더욱 직관적인 이름으로 사용해야겠습니다!! 항상 많이 배우네요. 오늘도 감사합니다 :)
안녕하세요 귀한 시간 내어 코드리뷰 남겨주시고 좋은 말씀해주셔서 감사합니다! 리뷰에 작성해주신 것들은 시간이 걸리더라도 최대한 반영할 수 있도록 하겠습니다! 그중에서도 UI와 로직을 분리해서 코드를 작성하는 연습 많이 해보겠습니다 감사합니다 :) |
| implementation 'com.squareup.retrofit2:retrofit:2.9.0' | ||
| implementation 'org.jetbrains.kotlinx:kotlinx-serialization-json:1.4.1' | ||
| implementation 'com.jakewharton.retrofit:retrofit2-kotlinx-serialization-converter:1.0.0' | ||
| // define a BOM and its version | ||
| implementation(platform("com.squareup.okhttp3:okhttp-bom:4.10.0")) | ||
| // define any required OkHttp artifacts without version | ||
| implementation("com.squareup.okhttp3:okhttp") | ||
| implementation("com.squareup.okhttp3:logging-interceptor") |
There was a problem hiding this comment.
별거 아니긴 한데 groovy와 kotlin dsl문법이 섞여있는 것 같아요!
현재는 groovy이니 이걸로 통일해도 좋을 것 가타요~
📌𝘐𝘴𝘴𝘶𝘦𝘴
📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯
📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵
xml.mp4
💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴
홈 프래그먼트 팔로워랑 비밀번호 부분을 아직 완성못했는데 완성하는대로 바로 푸쉬하겠습니다!
코드를 작성하면서 변수를 어느 위치에서 선언해야하는지에 대해 고민이 많은 것 같습니다..
(예를 들어 LoginViewModel 17 --> var userId: String? = "" )
코리 남겨주시면 열심히열심히 반영하겠습니다..!
코리 반영하기