Skip to content
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] Compose 필수 과제 #12

Merged
merged 7 commits into from
Jun 7, 2024
Merged

Conversation

arinming
Copy link
Member

@arinming arinming commented May 3, 2024

🧩 Issue number

이슈 번호 : #10

✨ Summary

컴포즈는 XML 뷰모델 그대로 쓰고, Screen 로직만 변경했어요
XML에 시간 많이 쓴 만큼.... 컴포즈는 수월했던 것 같ㅌ습니다

🔍 PR Type

  • 필수 XML
  • 필수 Compose
  • 심화 XML
  • 심화 Compose
  • 도전 XML
  • 도전 Compose

📷 Screenshot

week4_compose.mp4

📔 Other Information

인터셉트 너무 어렵지만 일단 피알 올리고 천천히 공부해보겠습니다 ㅜㅜ!!!!!!!! 팟팅.

Copy link

@SYAAINN SYAAINN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체적으로 xml 과 동일하네요! 얼른 같이 인터셉터 마스터해서 구현 완료해봅시다!

Comment on lines +96 to 98
// Coil
implementation 'io.coil-kt:coil-compose:2.6.0'
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음부터 모르는 녀석이 등장.. Coil 이 어떤 기능일까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미지를 불러오는 라이브러리입니다! Glide 역할과 같아용 ㅎ ㅎ

Comment on lines +12 to +13
.addHeader("memberId", "514")
.build()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인터셉터 기능 활용이 잘 안돼서 멤버아이디를 강제로 집어넣은걸까요??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

맞....습니당 ㅜㅠ 리팩토링하면서 수정하겠습니다,,,! Sharedpreferences로 구현해보려구욥!

mutableStateOf(
textId.length in 6..10 && textPw.length in 8..12 && textNickname.isNotEmpty() && !textNickname.contains(
id.length in 6..10 && password.length in 8..12 && nickname.isNotEmpty() && !nickname.contains(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우리 상수들은 이제 그만.. 놓아주는 건 어떨까요?! ㅋㅋㅋㅋㅋ

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

흐엉어 ㅜㅜ 항상 놓치는 상수화.. 꼼꼼히 봐주셔서 감사해요

Copy link
Member

@junseo511 junseo511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xml에서 한번 진행되고 오니까 쉽죠?!
이게 뷰모델의 힘입니다~~👍
저희 한번 이제 비동기처리랑 코루틴이라는 부분을 천천히 배워볼까요 🤭

app/build.gradle Outdated
Comment on lines 92 to 97
// Glide
implementation 'com.github.bumptech.glide:glide:4.13.0'
annotationProcessor 'com.github.bumptech.glide:compiler:4.13.0'

// Coil
implementation 'io.coil-kt:coil-compose:2.6.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

둘다 추가하신 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glide는 삭제하도록 하게씁니다..!

Copy link

@yeonjeen yeonjeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

역시 컴포즈 장인!! 항상 좋은 코드 보고 많이 배워갑니다~

app/build.gradle Outdated

// Glide
implementation 'com.github.bumptech.glide:glide:4.13.0'
annotationProcessor 'com.github.bumptech.glide:compiler:4.13.0'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glide랑 coil 둘 다 사용하신 건가용??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어머나 코일만 사용하도록 하겠습니다!

import retrofit2.Response

class HomeViewModel : ViewModel() {
private val followerService by lazy { ServicePool.followerService }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

필요할 때만 초기화 할 수 있어서 좋은 것 같아용 짱!!

name = "${follower.firstName} ${follower.lastName}",
description = follower.email
)
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 시도해보지는 않았지만 데이터 매핑 로직은 util 클래스로 분리하는게 좋다고 들어서 나중에 같이 도전해봐용:)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋아용! 이번 주차 과제로 해보게씁니다

class SignUpViewModel : ViewModel() {
private val authService by lazy { ServicePool.authService }

private val _signUpState = MutableStateFlow(SignUpState(isSuccess = false, message = ""))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

보면 livedata 보다는 state를 사용하시는 편이신데 이유가 있으실까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • LiveData 가 아닌 StateFlow 를 사용하신 이유가 있을까요?
  • StateFlow 를 사용함으로써 얻을 수 있는 이점이 무엇일까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전에 클린 아키텍처 공부하면서 간단히 정리한 내용인데욤..!

  • LiveData를 통해 데이터 업데이트
    • 액티비티, 프래그먼트의 생명주기를 인식 → 메모리 누수 X
    • Observer를 통해 객체에 알려 UI와 데이터 상태 보장
    • LiveData는 Android 플랫폼에 종속적이기 때문에 UI가 없는 곳에서 LiveData를 사용하기가 어려움
      • 즉, 아키텍처 패턴 관점에서는 단점이 존재
      • Presentation 레이어에서는 사용 가능하나, Domain에서는 불가능
  • StateFlow를 통해 데이터 업데이트
    • SharedFlow의 한 종류로, LiveData와 가장 가까움. LiveData를 대체 가능
    • 항상 오직 하나의 값을 가지고 있는 생성자로 초기상태가 필요함
    • 여러 collector를 지원하며 flow를 공유할 수 있음
      • collector의 개수에 상관 없이 구독하는 것의 최신 값을 받는다

따라서 StateFlow가 UI에 상관없이 구독하는 최신 값을 받아서 StateFlow로 구현하는 것을 지향하고 이씁니다. 잘못된 정보가 있다면 공유 부탁드려욤 🥹🥹🥹🥹

Copy link

@murjune murjune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요. 아린님 리뷰어로 함께하게 된 명예OB 이준원입니다. 앞으로 잘 부탁드립니다.

양이 상당히 많았을 텐데 과제 구현하시느라 고생하셨습니다!
제가 활동 기수였다면.. 저는 못했을 것 같아요 너무 대단하십니다 👍👍👍

저는 개발자라면 내가 작성한 코드에 대해 남에게 설명할 수 있어야한다고 생각해요
그래서, 단순한 지식 공유 보다는 물음표 살인마 로 아린님이 어떤 의도로 코드를 작성하신 것인지�구체적으로 질문을 드렸습니다.

제가 남겨드린 코멘트를 모두 반영하지 않아도 좋아요!!
아린님이 판단하기에 나 이 정도면 많이 성장했다! 라고 생각하실 때 머지하셔도 됩니다 :D


개인적으로 객체지향의 사실과 오해 라는 책을 추천드려요! 객체지향적인 사고를 하시데 많은 도움이 될 겁니다 💪

Comment on lines 78 to 80
id.length in 6..10 && password.length in 8..12 && nickname.isNotEmpty() && !nickname.contains(
" "
) && textMbti.length == 4
) && phone.matches(Regex("^010-\\d{4}-\\d{4}\$"))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

유효성 검사는 viewmodel 에서 해야하지 않을까요?

Copy link

@murjune murjune May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서버 통신하기 전에, 내부적으로 유효성 검사 좋네요 👍

Comment on lines +25 to +28
object : Callback<ResponseSignUpDto> {
override fun onResponse(
call: Call<ResponseSignUpDto>,
response: Response<ResponseSignUpDto>,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매 번 서버 통신을 할 때마다, Callback 의 익명 객체를 만들어주고 있는데요..!
함수 or 객체로 분리할 수 있을까요??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 너무 좋은 방법이네용 한번 해보겠습니당 🥹!

Comment on lines 38 to 47
} else {
val error = response.code()
_signUpState.update {
SignUpState(
isSuccess = false,
message = "회원가입 실패 : $error"
)
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

400.. 500 번대의 모든 에러가 해당 에러 메세지를 보여줄텐데요..

사용자에게 어떤 이유로 회원가입에 실패했는지 case 마다 알려줄 수 있을까요??

ex1. 정해진 id 양식이 아닙니다
ex2. 닉네임에 빈값을 넣을 수 없습니다

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네엡 저도 케이스별로 에러메시지를 띄우는 건 필수....라고 생각합니다 ㅜㅜ 얼른 구현하겠습니다!

class SignUpViewModel : ViewModel() {
private val authService by lazy { ServicePool.authService }

private val _signUpState = MutableStateFlow(SignUpState(isSuccess = false, message = ""))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • LiveData 가 아닌 StateFlow 를 사용하신 이유가 있을까요?
  • StateFlow 를 사용함으로써 얻을 수 있는 이점이 무엇일까요?

class SignUpViewModel : ViewModel() {
private val authService by lazy { ServicePool.authService }

private val _signUpState = MutableStateFlow(SignUpState(isSuccess = false, message = ""))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error mesage 와 success message 를 분리해보는건 어떨까요?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 동일한 error 가 중복해서 발생할 경우에는 View 로 Event가 전달되지 않고 있어요..!

어떻게 개선해볼 수 있을까요?

2024-05-07.1.28.43.mov

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 섬세하게 예외를 발견해주시다니 정말 감동임니다...

Comment on lines 35 to 36
inline fun <reified T> createBase(): T = baseRetrofit.create(T::class.java)
inline fun <reified T> createFollower(): T = followerRetrofit.create(T::class.java)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동료개발자가 createBase, createFollower 라는 네이밍을 보고, Retrofit 객체를 만드는걸 알 수 있을까요??

Suggested change
inline fun <reified T> createBase(): T = baseRetrofit.create(T::class.java)
inline fun <reified T> createFollower(): T = followerRetrofit.create(T::class.java)
inline fun <reified T> createBaseRetrofit(): T = baseRetrofit.createRetrofit(T::class.java)
inline fun <reified T> createFollower(): T = followerRetrofit.create(T::class.java)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 네이밍 제안 너무 감사해요 섬세한 개발자가 되게씁니다!!!!!

@@ -0,0 +1,6 @@
package com.sopt.now.compose.data.model

data class SignUpState(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 class는 왜 Dto postfix를 안붙이셨는지 궁금합니다 👀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기는 API 통신에서 사용하지 않는 데이터 모델이라서 저렇게 명시했는데, 어떻게 작성하는 것이 좋을까요? 🥺

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dto 들과 같은 패키지에 있어 헷갈렸네요 😅

@@ -1,7 +1,7 @@
package com.sopt.now.compose.data
package com.sopt.now.compose.data.model

data class Profile(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 class는 왜 Dto postfix를 안붙이셨는지 궁금합니다 👀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요 프로필은 view에 직접적으로 사용되고 API에는 사용되지 않아서 이렇게 명시했는데, 준원님은 어떻게 네이밍하시는지 궁금합니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 data 레이어에서만 사용하는 model 이였군요!
dto 들과 같은 패키지에 있어 헷갈렸네요 😅

)

@Serializable
data class UserInfo(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 class는 왜 Dto postfix를 안붙이셨는지 궁금합니다 👀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

붙여야 겠네욥...!!!!!!!

Comment on lines +75 to +84
// Lifecycle Viewmodel
implementation 'androidx.lifecycle:lifecycle-viewmodel-ktx:2.7.0'

// Fragment && Activity
implementation 'androidx.fragment:fragment-ktx:1.6.2'
implementation 'androidx.activity:activity-ktx:1.9.0'

// Retrofit
implementation 'com.squareup.retrofit2:retrofit:2.9.0'
implementation 'org.jetbrains.kotlinx:kotlinx-serialization-json:1.5.1'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석으로 구분해주셨네요! 💯

Copy link
Member Author

@arinming arinming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 늦게 답변 달아서 죄송합니다 🥹🥹🥹🥹🥹🥹🥹🥹🥹 합세를 컴포즈로 하고 있는데, 합세 하기 전에 리팩토링 했다면 훨씬 좋은 코드를 쓸 수 있었을텐데 하는 아쉬움이 있네욥.... 좋은 조언들 너무너무 감사하고 리팩토링하면서 반영하도록 하겠습니다!

app/build.gradle Outdated

// Glide
implementation 'com.github.bumptech.glide:glide:4.13.0'
annotationProcessor 'com.github.bumptech.glide:compiler:4.13.0'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어머나 코일만 사용하도록 하겠습니다!

app/build.gradle Outdated
Comment on lines 92 to 97
// Glide
implementation 'com.github.bumptech.glide:glide:4.13.0'
annotationProcessor 'com.github.bumptech.glide:compiler:4.13.0'

// Coil
implementation 'io.coil-kt:coil-compose:2.6.0'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glide는 삭제하도록 하게씁니다..!

Comment on lines +96 to 98
// Coil
implementation 'io.coil-kt:coil-compose:2.6.0'
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미지를 불러오는 라이브러리입니다! Glide 역할과 같아용 ㅎ ㅎ

Comment on lines 35 to 36
inline fun <reified T> createBase(): T = baseRetrofit.create(T::class.java)
inline fun <reified T> createFollower(): T = followerRetrofit.create(T::class.java)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 네이밍 제안 너무 감사해요 섬세한 개발자가 되게씁니다!!!!!

Comment on lines 36 to 37
inline fun <reified T> createFollower(): T = followerRetrofit.create(T::class.java)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reified으로 컴파일 타임에 타입 정보를 유지해줘서 가능한 거군욥..! 처음 알았습니다 ㅜㅜ 배워갑니다 🤩



class SignUpViewModel : ViewModel() {
private val authService by lazy { ServicePool.authService }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위에 HomeViewModel과 동일한 이유입니당 🫣

class SignUpViewModel : ViewModel() {
private val authService by lazy { ServicePool.authService }

private val _signUpState = MutableStateFlow(SignUpState(isSuccess = false, message = ""))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이전에 클린 아키텍처 공부하면서 간단히 정리한 내용인데욤..!

  • LiveData를 통해 데이터 업데이트
    • 액티비티, 프래그먼트의 생명주기를 인식 → 메모리 누수 X
    • Observer를 통해 객체에 알려 UI와 데이터 상태 보장
    • LiveData는 Android 플랫폼에 종속적이기 때문에 UI가 없는 곳에서 LiveData를 사용하기가 어려움
      • 즉, 아키텍처 패턴 관점에서는 단점이 존재
      • Presentation 레이어에서는 사용 가능하나, Domain에서는 불가능
  • StateFlow를 통해 데이터 업데이트
    • SharedFlow의 한 종류로, LiveData와 가장 가까움. LiveData를 대체 가능
    • 항상 오직 하나의 값을 가지고 있는 생성자로 초기상태가 필요함
    • 여러 collector를 지원하며 flow를 공유할 수 있음
      • collector의 개수에 상관 없이 구독하는 것의 최신 값을 받는다

따라서 StateFlow가 UI에 상관없이 구독하는 최신 값을 받아서 StateFlow로 구현하는 것을 지향하고 이씁니다. 잘못된 정보가 있다면 공유 부탁드려욤 🥹🥹🥹🥹

class SignUpViewModel : ViewModel() {
private val authService by lazy { ServicePool.authService }

private val _signUpState = MutableStateFlow(SignUpState(isSuccess = false, message = ""))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 섬세하게 예외를 발견해주시다니 정말 감동임니다...

Comment on lines +25 to +28
object : Callback<ResponseSignUpDto> {
override fun onResponse(
call: Call<ResponseSignUpDto>,
response: Response<ResponseSignUpDto>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 너무 좋은 방법이네용 한번 해보겠습니당 🥹!

Comment on lines 38 to 47
} else {
val error = response.code()
_signUpState.update {
SignUpState(
isSuccess = false,
message = "회원가입 실패 : $error"
)
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네엡 저도 케이스별로 에러메시지를 띄우는 건 필수....라고 생각합니다 ㅜㅜ 얼른 구현하겠습니다!

@arinming arinming requested review from junseo511, a team, murjune, SYAAINN and yeonjeen and removed request for a team and murjune May 23, 2024 14:40
@arinming arinming merged commit 3a2cd25 into develop-compose Jun 7, 2024
Copy link

@murjune murjune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arinming 추가적인 남겨주신 질문 comment 달아놨습니다 😎

@@ -1,7 +1,7 @@
package com.sopt.now.compose.data
package com.sopt.now.compose.data.model

data class Profile(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 data 레이어에서만 사용하는 model 이였군요!
dto 들과 같은 패키지에 있어 헷갈렸네요 😅

@@ -0,0 +1,6 @@
package com.sopt.now.compose.data.model

data class SignUpState(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dto 들과 같은 패키지에 있어 헷갈렸네요 😅

): Call<ResponseSignInDto>

@GET("member/info")
fun memberInfo(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전 getter() 함수를 넘어서 get 이라는 prefix 를 붙이는 컨벤션도 좋아하지 않는데요!

이에 대해 정말 여러가지 이유가 있지만, 간단하게 한가지만 말씀드려볼게요 ㅎ ㅎ
getter, setter 는 자바빈패턴으로부터 유래되었어요. 자바빈패턴에서는 getter() 는 필드 값을 그대로 반환하고 있기에 해당 객체가 해당 field 값이 있음을 암시하고 있다고 생각해요.

이는 클라이언트 입장에게 객체의 세부사항을 노출하는 것이기에 저는 선호하지 않습니다. 특히, 자바가 아닌 코틀린에서는 프로퍼티를 사용하고 있기에 더더욱 굳이 get prefix를 사용해야할까?? 라는 생각을 합니다. 😄

POJO 를 통해 getter 를 널리 퍼트린 Java 진영에서도 새로나온 record 문법을 보면, get prefix 를 권장하지 않습니다.

fun getA() // 해당 객체가 A라는 필드를 가지고 있겠군..! 

단순 네이밍이 아닌 getter() 사용을 지양해야하는 이유는 다음 블로그를 참고해주세요!

getter를 사용하는 대신 객체에 메시지를 보내자

): Call<ResponseSignInDto>

@GET("member/info")
fun memberInfo(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

참고로, get 을 메서드 네이밍에 사용하지 말자는 이야기는 yegor bugayenko 의 엘레강트 오브젝트 책에서 유명해진 이야기 입니다.

이 책도 추천드리는데요! 바쁘시면
저자의 블로그글을 한 번 읽어보시는 것도 좋을 것 같네요 😄

import retrofit2.Response

class HomeViewModel : ViewModel() {
private val followerService by lazy { ServicePool.followerService }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미 ServicePool 에서 전역 서비스 객체를 선언해두셔서 여쭤본 겁니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants