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

week7: 7차 필수과제 진행 (compose) #20

Open
wants to merge 2 commits into
base: develop-compose
Choose a base branch
from

Conversation

junseo511
Copy link
Member

@junseo511 junseo511 commented Jun 6, 2024

📌𝘐𝘴𝘴𝘶𝘦𝘴

📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯

  • Hilt 추가
  • Hilt 마이그레이션
  • 레포지토리 추가

📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵

image

💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴

@junseo511 junseo511 added 🍯 [FEAT] 새로운 기능 구현 🔮 [준서] 오늘도 사I로 파이팅을 외치며 📕 [필수 과제] 필수 과제 🧑‍🎨 [COMPOSE] labels Jun 6, 2024
@junseo511 junseo511 requested a review from a team June 6, 2024 17:19
@junseo511 junseo511 self-assigned this Jun 6, 2024
@junseo511 junseo511 changed the title Feat/week compose7 week7: 7차 필수과제 진행 (compose) Jun 6, 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.

안녕하세요 명예오비 이준원입니다!
과제 요구사항도 아닌데 Hilt를 스스로 공부하고 적용해주신 점이 인상 깊었습니다. 💯
열심히 하시는 모습을 보니 저 또한 자극을 받네요 🔥
준서님한테 도움이 되었으면 좋겠�는 부분을 위주로 리뷰를 남겼습니다.

질문이나 궁금한 점이 있으면 언제든지 DM 주세요 ㅎ ㅎ

@@ -3,6 +3,8 @@ plugins {
id 'org.jetbrains.kotlin.android'
id 'kotlin-parcelize'
id 'org.jetbrains.kotlin.plugin.serialization' version '1.9.0'
id 'kotlin-kapt'
id 'com.google.dagger.hilt.android'
Copy link

Choose a reason for hiding this comment

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

Hilt를 도입해주셨군요!! 💯
Koin 이라는 DI 라이브로리도 있는데 Hilt 를 추가해주신 이유와 Hilt 의 장점에 대해 설명해주실 수 있을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

구글에서 공식적으로 Hilt를 권장하고 있는 것으로 알고 있습니다!
일단 어노테이션만 사용하면 돼서 그 이상의 많은 코드가 필요하지 않아 편리하다는 장점, 생명주기를 관리하기 쉽다는 장점이 있는 것으로 알아요...!

Comment on lines +18 to +19
lateinit var appContext: Context
private set
Copy link

Choose a reason for hiding this comment

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

appContext 를 전역상수로 선언해두신 이유가 있을까요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

음 이친구 왜 이렇게 해놨을까요...?! ㅋㅋㅋㅋㅋㅋ 😅

retrofitBuilder: Retrofit.Builder,
@AuthBaseUrl url: String,
): AuthService {
return retrofitBuilder.baseUrl(url).build().create(AuthService::class.java)
Copy link

Choose a reason for hiding this comment

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

Suggested change
return retrofitBuilder.baseUrl(url).build().create(AuthService::class.java)
return retrofitBuilder.baseUrl(url).build().create()

클래스 타입을 넘겨주지 않아도 됩니다!


@Module
@InstallIn(SingletonComponent::class)
object NetworkModule {
Copy link

Choose a reason for hiding this comment

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

현재 네트워크 Config 설정, Retrofit 생성, Service 생성 등 Network모듈에서 너무 많은 일을 하고 있는 것 같습니다. 객체 분리를 해볼 수 있을까요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

허허 지금 돌아보니 이 부분이 보이는군요...! NetworkModule과 서비스 생성 등으로 분리할 수 있겠네용!!

fun provideRetrofit(url: String): Retrofit {
@Singleton
@Provides
fun provideRetrofit(): Retrofit.Builder {
Copy link

Choose a reason for hiding this comment

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

provideRetrofit 이지만 실제로는 Retrofit.Builder 를 생성하고 있습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

아...!

    @Provides
    @Singleton
    fun provideRetrofit(okHttpClient: OkHttpClient): Retrofit = Retrofit.Builder()
        .baseUrl(BASE_URL)
        .client(okHttpClient)
        .addConverterFactory(json.asConverterFactory(CONTENT_TYPE.toMediaType()))
        .build()

요런 느낌이 낫겠네요...!

fun getUserInfo(userId: Int) {
_userId.value = userId
fun getUserInfo(userId: Int) {
if (userId == 0) {
Copy link

Choose a reason for hiding this comment

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

  • 0이라는 매직넘버가 어떤 의미인지 동료개발자가 알 수 있을까요??🥲
  • 0이 넘어온다면 에러가 발생한 것으로 보입니다. stub값을 보여주는 것보다는 문제가 생겼다는 것을 개발자/사용자가 인지할 수 있어야하지 않을까요?

Comment on lines 47 to 48
mainViewModel.getFriendsInfo()

Copy link

Choose a reason for hiding this comment

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

이 부분도 LaunchEffect 안에 넣어주세요! Composable 함수 내부에 SideEffect 가 아닌 영역에서 뷰모델 함수를 호출하면 왜 안될까요?

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 +53 to +54
route = Screen.MyPage.route + "/{userId}",
arguments = listOf(navArgument("userId") { type = NavType.IntType }),
Copy link

Choose a reason for hiding this comment

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

    data object MyPage : Screen(route = "myPage") {
        fun route(userId: Int): String = "myPage/$userId"
    }

>> Screen.MyPage.route(userId)

이런식으로 리팩토링해볼 수 있겠습니다 💪

Copy link
Member Author

Choose a reason for hiding this comment

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

오...! 이부분 고민이 참 많았는데 감사합니다 o0o

Comment on lines 38 to 41
var username by remember { mutableStateOf("") }
var password by remember { mutableStateOf("") }
var nickname by remember { mutableStateOf("") }
var phoneNumber by remember { mutableStateOf("") }
Copy link

Choose a reason for hiding this comment

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

해당 상태 변수들은 ViewModel 에 있는게 더 적절할 것 같습니다!
Composable 함수가 Stateful하면 어떤 단점이 있을까요?

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 +23 to +35
fun performSignUp(request: SignUpRequest) {
viewModelScope.launch {
runCatching {
authRepository.signUp(request)
}.onSuccess {
if (it.code() in 200..299) {
_signUpMessage.value = SUCCESS_SIGN_UP + "/" + it.headers()["LOCATION"]
} else {
_signUpMessage.value = it.errorBody()?.string()?.split("\"")?.get(5)
}
}.onFailure {
if (it is HttpException) {
_signUpMessage.value =
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

@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.

앱잼 준비를 하다보니 답변이 늦었네요...! 그동안 감사했습니다 :)
즐거운 개발생활 되시길 기원합니다 👏👏👏

@@ -3,6 +3,8 @@ plugins {
id 'org.jetbrains.kotlin.android'
id 'kotlin-parcelize'
id 'org.jetbrains.kotlin.plugin.serialization' version '1.9.0'
id 'kotlin-kapt'
id 'com.google.dagger.hilt.android'
Copy link
Member Author

Choose a reason for hiding this comment

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

구글에서 공식적으로 Hilt를 권장하고 있는 것으로 알고 있습니다!
일단 어노테이션만 사용하면 돼서 그 이상의 많은 코드가 필요하지 않아 편리하다는 장점, 생명주기를 관리하기 쉽다는 장점이 있는 것으로 알아요...!

Comment on lines +18 to +19
lateinit var appContext: Context
private set
Copy link
Member Author

Choose a reason for hiding this comment

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

음 이친구 왜 이렇게 해놨을까요...?! ㅋㅋㅋㅋㅋㅋ 😅


@Module
@InstallIn(SingletonComponent::class)
object NetworkModule {
Copy link
Member Author

Choose a reason for hiding this comment

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

허허 지금 돌아보니 이 부분이 보이는군요...! NetworkModule과 서비스 생성 등으로 분리할 수 있겠네용!!

fun provideRetrofit(url: String): Retrofit {
@Singleton
@Provides
fun provideRetrofit(): Retrofit.Builder {
Copy link
Member Author

Choose a reason for hiding this comment

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

아...!

    @Provides
    @Singleton
    fun provideRetrofit(okHttpClient: OkHttpClient): Retrofit = Retrofit.Builder()
        .baseUrl(BASE_URL)
        .client(okHttpClient)
        .addConverterFactory(json.asConverterFactory(CONTENT_TYPE.toMediaType()))
        .build()

요런 느낌이 낫겠네요...!

return Retrofit.Builder()
.baseUrl(url)
.client(okHttpClient)
.addConverterFactory(json.asConverterFactory(CONTENT_TYPE.toMediaType()))
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 +13 to +23
class AuthRepository
@Inject
constructor(
private val authService: AuthService,
) {
suspend fun signUp(request: SignUpRequest): Response<SignUpResponse> {
return authService.signUp(request)
}

suspend fun signIn(request: SignInRequest): Response<SignInResponse> {
return authService.signIn(request)
Copy link
Member Author

Choose a reason for hiding this comment

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

Dto로 반환하는게 맞는듯 하네요...!

Comment on lines 47 to 48
mainViewModel.getFriendsInfo()

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 +53 to +54
route = Screen.MyPage.route + "/{userId}",
arguments = listOf(navArgument("userId") { type = NavType.IntType }),
Copy link
Member Author

Choose a reason for hiding this comment

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

오...! 이부분 고민이 참 많았는데 감사합니다 o0o


fun getUserInfo(userId: Int) {
_userId.value = userId
fun getUserInfo(userId: Int) {
Copy link
Member Author

Choose a reason for hiding this comment

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

updateUserInfo도 괜찮은 것 같네요...!

Comment on lines 38 to 41
var username by remember { mutableStateOf("") }
var password by remember { mutableStateOf("") }
var nickname by remember { mutableStateOf("") }
var phoneNumber by remember { mutableStateOf("") }
Copy link
Member Author

Choose a reason for hiding this comment

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

상태 변경이 일어날 때마다 리컴포지션이 일어날 것 같슴다...!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍯 [FEAT] 새로운 기능 구현 📕 [필수 과제] 필수 과제 🔮 [준서] 오늘도 사I로 파이팅을 외치며 🧑‍🎨 [COMPOSE]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants