Week7 by tunaunnie · Pull Request #12 · AND-SOPT-ANDROID/jiwon-yang

tunaunnie · 2024-12-17T08:38:03Z

Related issue 🛠

closed week7 #11

Work Description ✏️

MVI 패턴 적용

Screenshot 📸

지난 주차와 동일함

Uncompleted Tasks 😅

지난 주차 코드리뷰 적용

To Reviewers 📢

아직 지난 주 코리를 다 해결하지 못해서 머지가 안된상태라 이번 주에도 files changed 보기가 어려우실 것 같아요.. 이번 주차에 작업한 주요 파일은 아래와 같습니다
presentation 레이어 하의 homeScreen/HomeContract, HomeScreen, HomeViewModel, loginScreen/LoginContract, LoginScreen, LoginViewModel, mypageScreen/MypageContract, MypageScreen, MypageViewModel, signupScreen/SignUpContract, SignUpScreen, SignUpViewModel
util/base 패키지 안의 BaseViewModel, UiEvent, UiSideEffect, UiState
domain/usecase 패키지 안의 ValidateUserInputUseCase
MainActivity의 Navhost 관련 로직

입니다! 늘 성심성의껏 리뷰 달아주셔서 감사합니당

chattymin

세미나 달려오시느라 고생많으셨습니다!!
지원님이 열심히 과제해주셔서 덕분에 저도 재미있게 코드리뷰 했습니다.

앱잼 화이팅입니다 🚀

chattymin · 2024-12-19T02:56:06Z

app/src/main/java/org/sopt/and/domain/usecase/ValidateUserInputUseCase.kt

+
+import org.sopt.and.data.dataremote.model.response.ResponseGetUserHobbyDto
+import org.sopt.and.domain.repository.UserRepository
+import retrofit2.Response


domain에서는 kotlin/java를 제외한 의존성이 있으면 안돼요. 현재는 retrofit과 data 레이어의 정보가 들어있네요

domain의 존재 의의에 대해서 생각해보시면 좋을 것 같습니다!

사용하지 않는 임포트문임을 확인하여 제거했습니다. 꼼꼼한 확인과 조언 감사합니다!

chattymin · 2024-12-19T03:00:51Z

app/src/main/java/org/sopt/and/domain/usecase/ValidateUserInputUseCase.kt

+@Singleton
+class ValidateUserInputUseCase @Inject constructor() {
+    fun stringInputValidCheck(userInput: String) : Boolean {
+        return userInput.length <= 7
+    }
+
+    fun passwordValidCheck(password: String) : Boolean {
+        val pattern = "^(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)(?=.*[!@#\$%^&*]).{8,20}$".toRegex()
+        return password.matches(pattern)
+    }
+}


길이를 나타내는 7과 같은 값은 상수로 빼두는게 가독성 측면에서 좋아요. 추후 이 코드를 다른 사람이 본다면 7이라는 숫자가 어떤 의미인지 생각하기 힘들 수 있어요.

해당 useCase에서 2개의 함수를 사용하고, operator fun invoke의 형식을 사용하지 않은 이유가 있나요?? 이렇게 작성한 이유를 한번 생각해보신 후 블로그글 한번 읽어보시면 좋을 것 같아요 :)

덕분에 operator fun invoke를 사용하면 객체를 함수처럼, 이름 없이 바로 호출할 수 있다는 사실을 제대로 알아갑니다!
고민해봤는데 UserInput을 검증하는 경우가 1)유저 이름 2)유저 비밀번호 로 두 가지라, invoke를 사용하면 오히려 어느 경우에 대한 함수 처리인지 헷갈릴 것 같아 이렇게 2개의 함수명을 그대로 사용하려고 합니다. 대신 덕분에 앞으로는 PostSignUpUseCase등 하나의 함수만 포함된 usecase에서 invoke를 제대로 이해하고 적절히 활용할 수 있을 것 같습니다!

chattymin · 2024-12-19T03:02:04Z

app/src/main/java/org/sopt/and/presentation/homeScreen/HomeContract.kt

+class HomeContract {
+
+    data class HomeUiState(
+        val pagerImages: List<Int> = listOf(),


List는 불변 객체가 아닌 읽기전용 객체입니다. 그렇기 때문에 리컴포지션의 대상이 돼요.

불필요한 리컴포지션을 줄이고 성능을 더욱 높이고 싶다면 @stable, @immutable과 같은 어노테이션 혹은 ImmutableList 등을 사용하면 좋을 것 같아요

chattymin · 2024-12-19T03:04:46Z

app/src/main/java/org/sopt/and/presentation/homeScreen/HomeScreen.kt

            HomeLazyRow(
                title = "믿고 보는 웨이브 에디터 추천작",
-                images = images,
+                images = uiState.pagerImages,
                height = 230,
                width = 140,
+                onItemClick = { index ->
+                    homeViewModel.setEvent(HomeContract.HomeEvent.OnImageClicked(index))
+                }
            )


key라는 것에 대해서 알고있으신가요??
key를 사용하게 되면 불필요한 리컴포지션을 줄일 수 있어요. 한번 찾아보시고 적용해보시면 좋을 것 같습니다 :)

각 항목의 값을 고유하게 식별하는 key를 LazyRow에 추가하기만 해도 불필요한 리컴포지션을 줄일 수 있군요..! 좋은 지식 알아갑니다!

chattymin · 2024-12-19T03:07:27Z

app/src/main/java/org/sopt/and/presentation/loginScreen/LoginScreen.kt

+                horizontalAlignment = Alignment.CenterHorizontally
+            ) {
+                Image(
+                    painter = painterResource(id = R.drawable.wavve_logo),


painter내부 코드를 보면 UnStable하다고 나와있어요. 반면 imageVector를 본다면 Stable하다고 되어있어요.
그렇기 때문에 R에 있는 파일을 사용하신다면 imageVector로 사용하시는 것이 리컴포지션을 줄이고 성능을 높일 수 있어요.

비슷한 기능처럼 보여도 painterResource와 imageVector를 다르게 활용할 수 있군요! 디테일한 내용까지 제안해 주셔서 감사합니다!

chattymin · 2024-12-19T03:09:57Z

app/src/main/java/org/sopt/and/presentation/loginScreen/LoginViewModel.kt

+            is LoginContract.LoginEvent.OnUserNameChanged -> {
+                setState { copy(userName = event.userName, isUserNameValid = event.userName.length <= 7) }
+            }
+            is LoginContract.LoginEvent.OnPasswordChanged -> {
+                setState { copy(password = event.password, isPasswordValid = event.password.length <= 8) }
+            }


여기서도 길이에 해당하는 부분은 상수로 빼서 관리하면 좋을 것 같아요.

여기서도 길이에 대한 검증을 진행하고, 위에있는 useCase에서도 길이에 대한 검증을 수행하고 있는 것 같아요. 만약 길이가 7에서 9로 변경되었다면 2개의 코드를 수정해야할 것 같네요. 어떻게 하면 좋을까요?

회원가입 시나 로그인 시나 사용자가 입력한 username/password에 대한 길이 제한 조건은 똑같아서, 동일한 usecase를 불러와서 같은 함수로 isValid 를 처리한 후 setState 해주는 방식으로 수정했습니다!

이전에는 ViewModel에서 바로 Contract의 setEvent를 불러와 상태를 바꿔주는 방법과, Usecase를 활용하는 방법 두 개가 어떨 때 다르게 활용할 수 있는 건지 분간이 잘 안 갔는데 조금 고민해보니 UI에 띄우는 상태 (화면에 올라가는 데이터 값들)을 간단하게 바꾸는 작업인지 vs 혹은 조금 더 복잡한 비즈니스 로직인지 를 고민하면 구분이 조금 더 수월한 것 같습니다! 좋은 조언 감사합니다 :)

chattymin · 2024-12-19T03:11:40Z

app/src/main/java/org/sopt/and/presentation/loginScreen/LoginViewModel.kt

+        } catch (e: Exception) {
+            sendSideEffect(LoginContract.LoginSideEffect.ShowSnackbar("로그인 요청 중 오류가 발생했습니다."))


tryCatch도 정말 좋은 방법이죠!
하지만 해당 방법을 사용할 때에는 catch를 하는 Exception에 대해서 명시적으로 표시해줘야 합니다.

다른 사람이 이 코드를 보았을 때에 어떤 이유로 오류가 발생할 가능성이 있는지 알지 못하고, 각각 상황에 맞는 대처를 해야할 수 도 있으니까요.
catch를 여러개 사용하더라도 Exception을 각각 나타내주는게 더 좋을 것 같습니다

chattymin · 2024-12-19T03:21:54Z

app/src/main/java/org/sopt/and/ui/components/BottomBar/NavIcon.kt

+                "profile" -> {
+                    navController.navigate(Route.MypageScreen(userName = "")) /*username을 여기다 어떻게 넣어주지?*/
+                }


음 굳이 넘겨줄 필요가 있을까요??

서버통신을 통해 받아오거나, 로컬에 저장해두었다가 불러오는 방법이 더욱 적합해보이네요.
바텀네비를 통해 움직이는 특성상 home에서 profile로, search에서 profile로 이동하는 두가지 경우 모두 있어요.
그렇기 때문에 이전화면에서 profie로 이동하라 때 사용자 이름을 받아오는건 조금 위험한 방법이 될 수 있을 것 같아요

NavBar에도 일반적인 Screen처럼 ViewModel을 만들어서, uiState로부터 userName을 받아오는 방식으로 수정했습니다! MyPage에서 로그인 성공 시에 로컬 DB에 사용자 정보를 담아두었던 것을 그대로 불러와서 사용했던 방식(userInfoLocalDataSource 사용)을 참고했습니다! 그런데 ui가 아니라 NavBar과 같은 컴포넌트에도 뷰모델을 붙여서 관리하는 방식이 적합한지(일반적인지)에 대해서는 조금 더 고민해보아야 할 것 같습니다

chattymin · 2024-12-19T03:23:14Z

app/src/main/java/org/sopt/and/ui/components/MypageScreen/MyPageProfileSection.kt

            Spacer(modifier = Modifier.width(10.dp))
            Text(
-                "${deliveredEmail}님",
+                "${deliveredUserHobby}를 즐기는\n${deliveredUserName}님",


정말 사소하긴 하지만 deliverdUserHobby가 null이라면 "null를 즐기는 ~" 이렇게 될거에요.
이런 상황도 고려해서 코드를 작성하신다면 더욱 사용자 친화적인 개발자가 될 수 있을 것 같아요 :)

jihyunniiii

고생하셨습니다

jihyunniiii · 2024-12-25T03:50:17Z

app/build.gradle.kts

+hilt {
+    enableAggregatingTask = false
 }


이 부분을 왜 추가해야 하는지, 어떻게 하면 제거할 수 있을지 찾아보시면 좋을 것 같아요

jihyunniiii · 2024-12-25T03:51:12Z

app/src/main/java/org/sopt/and/data/dataremote/network/ApiFactory.kt

+
+object ApiFactory {
+//    private const val BASE_URL: String = BuildConfig.BASE_URL
+    private const val BASE_URL: String = "http://223.130.135.50:8085"


BASE_URL 숨겨주세요

jihyunniiii · 2024-12-25T03:51:49Z

app/src/main/java/org/sopt/and/di/NetworkModule.kt

+                val request = chain.request().newBuilder()
+                    .addHeader("Accept", "*/*")
+                    .build()
+                chain.proceed(request)
+            }


왜 이런 Interceptor를 추가하셨나요?

tunaunnie added 20 commits November 13, 2024 02:12

apply MVVM architecture

cda17c8

api connecting

113b9fa

[feat] 유저 등록 API 연동

71d677f

feature #week4 : 회원가입 통신 성공

ee9d131

fix #week4 : signup 후 login 이동 시 navigate 로직 수정

841d04e

fix #week4 : 네이게이션 폴더링 구조 수정

72b791b

feat #week4 : login Get API 구현 완료

5cbd2ee

feat #week4 : 로그인 성공 후 네비게이션

18cd80e

feat #week4 : 취미 조회 API 연결

26c22c1

fix #week4 : NavHost 전체 innerPadding 제거

0512b01

fix #week4 : BaseUrl 숨기기

f6f4b09

initial commit

32230ce

fix #week6 : 폴더 구조 변경

20ecf8f

fix #week6 : DTO request 인자 이름 변경

c3ff78e

fix #week6 : baseurl 숨기기

51c3193

feature #week7 : home 및 login 화면 contract 구현

4f4f579

Merge branch 'week6_assignment' into week7

539a84e

feat #week7 : mypage contract 구현

b365474

feat #week7 : Navhost에서 navigate 함수 인자 매칭 수정

2df94d5

fix #week7 : 주석 제거 및 코드 정리

13077d8

tunaunnie requested review from angryPodo, chattymin, jihyunniiii and t1nm1ksun and removed request for t1nm1ksun December 17, 2024 13:31

chattymin reviewed Dec 19, 2024

View reviewed changes

jihyunniiii reviewed Dec 25, 2024

View reviewed changes

tunaunnie added 2 commits December 29, 2024 14:40

feat #week7 : 코드 리뷰 반영

6f78b7d

week6 : domain -data 의존성 제거

9c48a65

feat #week8 : 코드 리팩토링

4cff767

		} catch (e: Exception) {
		sendSideEffect(LoginContract.LoginSideEffect.ShowSnackbar("로그인 요청 중 오류가 발생했습니다."))

Conversation

tunaunnie commented Dec 17, 2024

Related issue 🛠

Work Description ✏️

Screenshot 📸

Uncompleted Tasks 😅

To Reviewers 📢

Uh oh!

chattymin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihyunniiii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chattymin left a comment •

edited

Loading