Step3 : Github(UI) #99

minhyukseul · 2023-08-28T10:05:36Z

3단계 기능구현해보았습니다.
화면단의 기능요구사항 명세가 없어서 임의로 구현을 해보았습니다.
compose, mvi 적용을 해보았는데요.
좀더 나은 방향으로 리팩토링하거나 놓친 부분, 그리고 제가 잘못 이해하고 적용한 부분에 대해서 많은 리뷰 부탁드리겠습니다.

wisemuji

안녕하세요 성현님!

구현을 잘 해주셨습니다 💯
고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.
컴포즈와 MVI 패턴을 적용해주셨는데요, MVI 구현에 써드파티 라이브러리를 사용하셔서 이 구조에 대한 피드백은 깊게 드리지 않았습니다

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 🙏 💪

wisemuji · 2023-08-29T08:59:02Z

app/src/main/java/camp/nextstep/edu/github/GithubRepositoriesViewModel.kt

+                        }
+                    }
+                }.getOrElse {
+                    reduce { state.copy(status = UiStatus.Failed("로딩 실패")) }


단순 로딩 실패보다 더 의미있는 에러 메시지를 남겨보면 어떨까요?

해당 상황에서 어떤 이유로 에러가 발생했는데 throwable의 message를 넘기는 식으로 수정해보았습니다.

wisemuji · 2023-08-29T09:13:28Z

app/src/main/java/camp/nextstep/edu/github/MainActivity.kt

+            GitHubMainTheme {
+                Box(modifier = Modifier.fillMaxSize()) {
+                    val navController = rememberNavController()
+                    NavHost(navController = navController, startDestination = "Main") {


Compose 네비게이션을 활용해서 구성해주셨군요! 지금은 Main 네비게이션 하나밖에 없지만, 나중에 앱이 커지게 되면 네비게이션 Route를 적절하게 관리할 방법을 찾고 싶어질 수 있어요.

NowInAndroid에서 네비게이션을 어떻게 관리하는지 보시면 도움이 될 것 같습니다.

https://github.com/android/nowinandroid

정보 감사합니다. 사실 해당 mission이 Main밖에 없어 이 부분에 대해서 더 보지 못하였는데요. 참고로 주신 리파지토리 보도록 하겠습니다.

wisemuji · 2023-08-29T09:14:19Z

app/src/main/java/camp/nextstep/edu/github/theme/Theme.kt

+import androidx.compose.runtime.Composable
+
+@Composable
+fun GitHubMainTheme(content: @Composable () -> Unit) {


사실 현재는 MaterialTheme을 그대로 사용하셔도 무방합니다. 컬러, 폰트 등 프리셋이 생기면 그때 추가해도 괜찮아보여요.

원래 프리셋 등으로 해서 적용해보는 식으로 할까 하다가 다른 속성에 대해서 적용을 해볼까 하다가 말았는데요. 현업에서 MaterialTheme보다는 커스텀한 상황을 많이 쓰겠죠? 제가 컴포즈를 잘 몰라 문의드립니다.

현업에서는 디자인 시스템 프리셋을 정의하는 경우가 많고, MaterialTheme을 그대로 따라가기에는 아이폰과 같은 디자인인 경우가 많아서 대부분 커스텀으로 정의하긴 합니다. 현재 미션과는 크게 관련 없으니 참고만 해주세요!

wisemuji · 2023-08-29T09:16:51Z

app/src/main/java/camp/nextstep/edu/github/ui/UiStatus.kt

+sealed class UiStatus {
+
+    object Loading : UiStatus()
+    object Success : UiStatus()


단순 성공 / 실패 상태만 명시하기보다 이 UI 자체에 데이터의 흐름을 담아보면 어떨까요?

사용하신 MVI 라이브러리에 대한 이해도가 낮은 상태라 적절하지 않은 피드백일 수 있습니다.

현재는 state는 명시적으로 로딩 중과 성공 실패에대한 상태를 넘기고 데이터의 경우 추가적으로 넘길 수 있도록 구현하였는데요. Status 자체에 데이터를 넘기는 식으로 하는게 더 좋을까요?

어떤 것이 더 좋다라고 이야기하기보다 UI 상태 자체에 데이터를 포함시킬 수 있단 의미입니다. 예를 들어 로딩 상태일때는 별다른 데이터를 받지 않아도 되지만, 성공 상태일때는 레포지토리 정보를 받는 것을 보장할 수 있습니다.

다음 자료들이 도움이 되면 좋겠어요!

문서: https://developer.android.com/topic/architecture/ui-layer?hl=ko#define-ui-state

예시 코드: https://github.com/android/nowinandroid/blob/000c21f06570edc7f71387f5f7d2b9a89eb2de67/feature/foryou/src/main/java/com/google/samples/apps/nowinandroid/feature/foryou/OnboardingUiState.kt#L24

참고로 이번 미션 학습 목표와는 거리가 있으니 꼭 반영하진 않으셔도 됩니다.

wisemuji · 2023-08-29T09:20:34Z

core/data/src/main/java/camp/nextstep/edu/github/data/network/di/DataModule.kt

+        }
+
+        @Provides
+        fun provideGithubService(client: OkHttpClient): GithubService {


지금은 GithubService 하나만 존재하지만 나중에 다른 네트워크 서비스가 추가될 때를 대비하여 Retrofit 객체과 GithubService 의존성은 분리하는게 좋습니다.

수정해보았는데요. 의도하신대로 적용된게 아니라면 언제든 말씀 주십시오~

wisemuji · 2023-08-29T09:21:24Z

core/data/src/main/java/camp/nextstep/edu/github/data/network/di/DataModule.kt

+
+@InstallIn(SingletonComponent::class)
+@Module
+abstract class DataModule {


DataModule로 되어있지만 실제로는 네트워크 의존성 추가 작업까지 하고 있습니다. 나눠보면 어떨까요?

수정해보았습니다. 맞게 적용된지 잘 모르겠네요.

잘 적용해주셨습니다 😊

wisemuji · 2023-08-29T09:21:46Z

core/data/src/main/java/camp/nextstep/edu/github/data/network/GithubService.kt

-interface GitHubService {
-    @GET("/repositories")
+interface GithubService {
+    @GET("repositories")


/를 제거하신 이유가 있을까요?

Retrofit의 introduction에 보니 앞에 /가 붙지 않아 제거했습니다.

wisemuji

안녕하세요 성현님! 리뷰 반영을 잘 해주셨습니다 💯

조금 더 고민해보면 좋을 내용을 코멘트로 남겼습니다.
다음 리뷰 요청에서는 머지하도록 하겠습니다.

얼마 남지 않은 미션 완주까지 조금만 더 화이팅이이에요!

wisemuji · 2023-08-29T14:03:13Z

app/src/main/java/camp/nextstep/edu/github/ui/UiStatus.kt

+sealed class UiStatus {
+
+    object Loading : UiStatus()
+    object Success : UiStatus()


어떤 것이 더 좋다라고 이야기하기보다 UI 상태 자체에 데이터를 포함시킬 수 있단 의미입니다. 예를 들어 로딩 상태일때는 별다른 데이터를 받지 않아도 되지만, 성공 상태일때는 레포지토리 정보를 받는 것을 보장할 수 있습니다.

다음 자료들이 도움이 되면 좋겠어요!

문서: https://developer.android.com/topic/architecture/ui-layer?hl=ko#define-ui-state

예시 코드: https://github.com/android/nowinandroid/blob/000c21f06570edc7f71387f5f7d2b9a89eb2de67/feature/foryou/src/main/java/com/google/samples/apps/nowinandroid/feature/foryou/OnboardingUiState.kt#L24

참고로 이번 미션 학습 목표와는 거리가 있으니 꼭 반영하진 않으셔도 됩니다.

wisemuji · 2023-08-29T14:03:32Z

core/data/src/main/java/camp/nextstep/edu/github/data/network/di/DataModule.kt

+
+@InstallIn(SingletonComponent::class)
+@Module
+abstract class DataModule {


잘 적용해주셨습니다 😊

wisemuji · 2023-08-29T14:04:30Z

core/data/src/main/java/camp/nextstep/edu/github/data/network/di/NetworkModule.kt

+@Module
+abstract class NetworkModule {
+
+    companion object {


이제 꼭 companion object 에 정의하지 않아도 됩니다. NetworkModule 자체를 object로 만들어보면 어떨까요?

Hilt를 처음써봐서 abstract에 companion으로 정의가 되어야만 작동이 되는줄 알았습니다. 변경해보겠습니다~

wisemuji · 2023-08-29T14:05:54Z

...data/src/test/java/camp/nextstep/edu/github/data/network/GithubRepositoriesDataSourceTest.kt

@@ -53,13 +53,16 @@ class GithubRepositoriesDataSourceTest {
    }

    @Test
-    fun `GitHub repository 실제 데이터 체크`() = runTest {
+    fun `Github repository 실제 데이터 체크`() = runTest {


DM으로 말씀드렸듯이 MockWebServer를 사용하였기에 실제 서버의 동작을 확인하는 것은 의미가 없습니다. 테스트 더블이라는 키워드가 왜 나왔는지 다음 글이 도움이 되길 바래요!

https://tecoble.techcourse.co.kr/post/2020-09-19-what-is-test-double/

참고로 주신 링크 확인해보도록 하겠습니다 감사합니다~

wisemuji · 2023-08-29T14:08:30Z

app/src/main/java/camp/nextstep/edu/github/theme/Theme.kt

+import androidx.compose.runtime.Composable
+
+@Composable
+fun GitHubMainTheme(content: @Composable () -> Unit) {


현업에서는 디자인 시스템 프리셋을 정의하는 경우가 많고, MaterialTheme을 그대로 따라가기에는 아이폰과 같은 디자인인 경우가 많아서 대부분 커스텀으로 정의하긴 합니다. 현재 미션과는 크게 관련 없으니 참고만 해주세요!

wisemuji

안녕하세요 성현님!
리뷰 반영 잘 해주셨습니다! 💯

아키텍처와 의존정 주입에 대한 이해가 충분히 있으셔서 구조상 크게 피드백 드릴 부분이 없었습니다.
짧은 시간이였지만 저도 좋은 인사이트를 많이 얻어갑니다.

미션 완주 축하드립니다 👏

minhyukseul added 2 commits August 28, 2023 15:29

feat: Github repository 메인 페이지 개발 작업 중

9bb5efa

feat: step3 기능 구현완료

65b0bee

wisemuji requested changes Aug 29, 2023

View reviewed changes

refactor: 리뷰사항에 따른 변경 적용

2a48792

wisemuji requested changes Aug 29, 2023

View reviewed changes

refactor: 리뷰사항 반영

c8e9aa4

wisemuji approved these changes Aug 30, 2023

View reviewed changes

wisemuji merged commit f2aeb7b into next-step:minhyukseul Aug 30, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step3 : Github(UI) #99

Step3 : Github(UI) #99

minhyukseul commented Aug 28, 2023

wisemuji left a comment

wisemuji Aug 29, 2023

minhyukseul Aug 29, 2023

wisemuji Aug 29, 2023

minhyukseul Aug 29, 2023

wisemuji Aug 29, 2023

minhyukseul Aug 29, 2023

wisemuji Aug 29, 2023

wisemuji Aug 29, 2023

minhyukseul Aug 29, 2023

wisemuji Aug 29, 2023

wisemuji Aug 29, 2023

minhyukseul Aug 29, 2023

wisemuji Aug 29, 2023

minhyukseul Aug 29, 2023

wisemuji Aug 29, 2023

wisemuji Aug 29, 2023

minhyukseul Aug 29, 2023

wisemuji left a comment

wisemuji Aug 29, 2023

wisemuji Aug 29, 2023

wisemuji Aug 29, 2023

minhyukseul Aug 30, 2023

wisemuji Aug 29, 2023

minhyukseul Aug 30, 2023

wisemuji Aug 29, 2023

wisemuji left a comment

Step3 : Github(UI) #99

Step3 : Github(UI) #99

Conversation

minhyukseul commented Aug 28, 2023

wisemuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisemuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisemuji left a comment

Choose a reason for hiding this comment