Step2 - GitHub(HTTP) #108

oyj7677 · 2023-09-03T16:18:06Z

No description provided.

- GitHub Repository 목록 수신로직 구현 - MockWebServer를 이용한 테스트 코드 작성

…o step2

- 불필요한 의존성 제거

oyj7677 · 2023-09-04T16:01:17Z

안녕하세요 남재님. 가장 마지막 미션을 함께 해주셔서 감사합니다.
부족한 점이 많아 미션 진행이 조금 느렸습니다. 비록 강의 기간은 끝났지만 마지막 미션 끝까지 함께해주시면 감사드리겠습니다.

namjackson

안녕하세요 영재님!
2단계 미션 고생하셨습니다!
몇가지 고민할법한 포인트들 코멘트 남겼으니, 확인해주세요!

namjackson · 2023-09-05T02:47:06Z

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

+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext

 class MainActivity : AppCompatActivity() {


UI의 경우는 Step3의 미션입니다 ㅠ,ㅠ! 관련 변경사항은 step3진행하면서 적용해주시면 좋을거같아요!

namjackson · 2023-09-06T15:13:35Z

github-domain/src/main/java/com/example/github_domain/GithubDomain.kt

+class GithubDomain {
+}


불필요한 객체는 제거해주셔도 좋아요 😄

namjackson · 2023-09-06T15:15:21Z

github-domain/src/main/java/com/example/github_domain/GithubRepoData.kt

@@ -0,0 +1,7 @@
+package com.example.github_domain


패키지 네이밍 룰을 참고해보아요
https://developer.android.com/kotlin/style-guide?hl=ko#package_names

namjackson · 2023-09-06T15:16:09Z

github-domain/src/main/java/com/example/github_domain/GithubRepoData.kt

+
+data class GithubRepoData(
+    val id: String,
+    val full_name: String,


_보다는 카멜케이스를 활용해주세요

namjackson · 2023-09-06T15:18:36Z

github-data/src/main/java/com/example/github_data/room/Injector.kt

+package com.example.github_data.room
+
+import com.example.github_data.GitHubRepositoryImpl
+import com.example.github_domain.GitHubRepository
+
+object Injector {
+    fun providesGithubRepoRepository(githubRepoDao: GithubRepoDao): GitHubRepository {


외부의 Injector 구현 👍
하지만 외부에서 GithubRepoDao라는 구현체를 꼭 알아야할까요?

namjackson · 2023-09-06T15:24:42Z

app/src/test/java/camp/nextstep/edu/github/GitHubViewModelTest.kt

+import java.io.File
+
+class GitHubViewModelTest {
+    val server = MockWebServer()


viewModel 테스트도 좋지만,

Repository 목록을 가져오는 기능은 data 모듈에 구현되어야 한다.

데이터모듈에서 테스트코드를 작성해보면 어떨까요?

namjackson · 2023-09-06T15:25:51Z

app/src/test/java/camp/nextstep/edu/github/GitHubViewModelTest.kt

+    val server = MockWebServer()
+
+
+    private lateinit var service: GitHubApi


api를 테스트하기 보단 GitHubRepositoryImpl를 테스트하는게 의미있지않을까요?

namjackson · 2023-09-06T15:26:12Z

app/src/test/java/camp/nextstep/edu/github/GitHubViewModelTest.kt

+    fun `MockWebServer를 활용하여 getGitHubInfo() 요청을 검증한다`() = runTest(UnconfinedTestDispatcher()) {
+        // given : MockResponse을 활용해 서버 응답을 세팅해둔다.
+        val response = MockResponse()
+            .setBody(File("src/test/resources/githubRepo.json").readText())


정상케이스의 테스트도 좋지만,
빈리스트,
에러 등의 케이스도 고민해보아요!

namjackson · 2023-09-06T15:28:04Z

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

+    }
+
+    private fun initViewModel() {
+        val githubRepoDao = GithubRepoDatabase.getInstance(this)!!.GithubRepoDao()


!! 는 null일 경우, 에러가 발생하니 사용을 지양하는편이 좋아요
유연한 null처리방법을 활용해보아요

namjackson · 2023-09-06T15:28:41Z

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

+
+    private fun initViewModel() {
+        val githubRepoDao = GithubRepoDatabase.getInstance(this)!!.GithubRepoDao()
+        val githubViewModelFactory = GithubViewModelFactory(githubRepoDao)


dao라는 데이터 레이어의 구현체를 app모듈에서 알아야할까요? 추가로 고민하면 좋을거같아요!

namjackson · 2023-09-06T15:29:56Z

리뷰가 조금 늦었네요 ㅠㅠ
미션 기간 상관없이, 리뷰요청해주시면 미션 리뷰할 예정이니, 끝까지 도전해보아요 💪 👍

- 변수값 카멜케이스 사용 - 저장소 패턴 적용 - 구현체 internal class 적용 - GitHubRepositoryImpl 테스트 케이스 추가 - UI관련 코드 삭제

oyj7677 · 2023-09-11T16:02:08Z

테스트 케이스관련한 조언에 대해서 어떤 식으로 테스트 해야할 지 감이 잡히질 않습니다. 또한, 통신에 관련되어 테스트를 진행할 때 테스트코드의 함수명을 어떤 식으로 지어야 할지 조언을 주시면 감사하겠습니다.

GitHubService관련 리뷰 에 대해서 저의 생각입니다. 싱글톤을 사용하는 이유라 하면 보편적으로 데이터가 있는 클래스의 객체를 여러개 생성하는 것을 방지하여 데이터의 동기화(?)를 시켜줍니다. 또한, 객체가 여러개 생성되는 것을 방지하여 메모리에 대해 이점을 가질 수 있습니다.
GitHubService의 경우 데이터를 관리하기 보다는 다수의 곳에서 객체를 생성 할 경우에도 메모리에 여러번 쌓이는 것을 방지하여 메모리에 대한 이점을 가져오기 위해 사용한다고 생각합니다.

dataSource를 추상화하라는 조언에는 저장소 패턴을 사용해 보았습니다. 말씀해주신 조언에 맞는 조치일까요? 또한, 저장소 패턴을 처음 사용해보는 것이라 많이 미숙한 것 같습니다. 해당 부분도 조언 해주시면 감사하겠습니다.

namjackson

안녕하세요 영재님!
피드백 반영 잘해주셨네요!
몇가지 추가적인 코멘트 남겼으니, 확인해주세요 😄

namjackson · 2023-09-15T10:03:21Z

github-data/src/main/java/com/example/github_data/room/GithubRepoEntity.kt

@@ -0,0 +1,22 @@
+package com.example.github_data.room


패키지네이밍에서 _ 보다는 . 을 활용해 구분해보면 어떨까요?
패키지명은 소문자와 하나의 단어를 사용하기를 권장합니다 😄

com.example.github.data com.example.github.domain

좋지 못한 네이밍 습관을 고쳐주셔서 감사합니다.

namjackson · 2023-09-15T10:10:06Z

github-data/src/main/java/com/example/github_data/repository/remote/RemoteGithubDataSource.kt

+interface RemoteGithubDataSource {
+    suspend fun getGitHubRepo(): List<GithubRepoEntity>
+}


local과 Remote 인터페이스가 동일하다면,, 하나로 추상화해도 좋을거같네요 😄

Remote에서 모든 데이터를 받아오는 것으로 수정되어 인터페이스에 차이가 생겼습니다.
추후 어떤 데이터를 다른 구성으로 저장할 수도 있겠다 싶어 모든 항목을 받아오는 것으로 변경해보았습니다.

namjackson · 2023-09-15T10:30:07Z

github-data/src/main/java/com/example/github_data/GitHubRepositoryImpl.kt

+internal class GitHubRepositoryImpl(private val githubRepoDao: GithubRepoDao): GitHubRepository {
+    override fun insertGitHubRepository() {
+        CoroutineScope(Dispatchers.IO).launch {
+            val response = GitHubService.getGitHubService().getGitHubInfo()


#108 (comment) 에 대해서 저의 생각입니다. 싱글톤을 사용하는 이유라 하면 보편적으로 데이터가 있는 클래스의 객체를 여러개 생성하는 것을 방지하여 데이터의 동기화(?)를 시켜줍니다. 또한, 객체가 여러개 생성되는 것을 방지하여 메모리에 대해 이점을 가질 수 있습니다.
GitHubService의 경우 데이터를 관리하기 보다는 다수의 곳에서 객체를 생성 할 경우에도 메모리에 여러번 쌓이는 것을 방지하여 메모리에 대한 이점을 가져오기 위해 사용한다고 생각합니다.

싱글턴 패턴은 인스턴스 관리에 매우 유리하지만, 관리측면에서 주의해야합니다!
하지만 이전 코드처럼 싱글턴 객체에서 직접 접근하는 구조라면, GitHubService객체에 강한 의존관계를 가지게 됩기때문에 코멘트 드린부분입니다!
dataSource를 추상화함으로써 잘 수정해주셨네요 👍

namjackson · 2023-09-15T10:31:25Z

github-data/src/main/java/com/example/github_data/mapper/Mapper.kt

+import com.example.github_data.room.GithubRepoEntity
+import com.example.githubdomain.GithubRepoData
+
+fun mapperToGithubRepo(githubRepoEntityList : List<GithubRepoEntity>): List<GithubRepoData> {


확장함수의 형태로 작성해보면 어떨까요?

확장함수를 하나의 파일에 몰아서 작성하였습니다. 확장함수로 작성하니 재사용이나 코드의 가독성면에서도 좋은 것 같습니다.

namjackson · 2023-09-15T10:33:00Z

github-data/src/main/java/com/example/github_data/repository/GithubRepositoryImpl.kt

+        val localData = localGithubDataSource.getGitHubRepo()
+        return if(localData.isNullOrEmpty()) {
+            mapperToGithubRepo(remoteGithubDataSource.getGitHubRepo())
+        } else {
+            mapperToGithubRepo(localData)
+        }


레파지토리 패턴을 잘활용해주셨네요 👍
현재 로직상 localData는 항상 NullOrEmpty 가 나오진 않을까요?

해당 부분 수정하였습니다. 현재 코드에서는 Remote가 업데이트 됐을때 상황을 처리하지 못합니다.
Remote데이터가 업데이트 되는 상황은 자동으로 처리하는 것이 아니라 추가적은 동작(최신 버전 업데이트 하기)을 통해 기존 local 데이터와 비교 후 저장 혹은 local데이터 전부 삭제 후 새로운 값을 저장하는 방법을 생각해 봤습니다.
두가지 방법중 많이 사용되는 방법이 있을까요? 혹시 두 방법이 자주 사용되지 않는다면 다른 방법이 있을까요?

namjackson · 2023-09-15T10:36:40Z

...b-data/src/main/java/com/example/github_data/repository/remote/RemoteGithubDataSourceImpl.kt

+internal class RemoteGithubDataSourceImpl(githubService: GitHubApi? = null): RemoteGithubDataSource {
+    // retrofit통신으로 데이터 받아옴.
+    private val gitHubService = githubService ?: GitHubService.getGitHubService()


GitHubApi를 nullable한 객체로 받아올필요가 있을까요?
+
생성시점에 주입받을수 있는구조를 사용해보아요!
RemoteGithubDataSourceImpl는 GitHubService에 강한 의존이 가진 코드는 아닐까요?
(싱글톤 관련 코멘트 남긴부분 참고해보아요!)

Suggested change

internal class RemoteGithubDataSourceImpl(githubService: GitHubApi? = null): RemoteGithubDataSource {

// retrofit통신으로 데이터 받아옴.

private val gitHubService = githubService ?: GitHubService.getGitHubService()

internal class RemoteGithubDataSourceImpl(private val githubService: GitHubApi = GitHubService.getGitHubService() ): RemoteGithubDataSource {

GitHubApi를 nullable한 객체로 받아왔던 건 테스트를 위해서였습니다. 하지만 조언해주신 방법으로 구성해도 테스트 코드에 지장이 없음을 확인하여 수정했습니다.

namjackson · 2023-09-15T10:46:21Z

...b-data/src/main/java/com/example/github_data/repository/remote/RemoteGithubDataSourceImpl.kt

+
+    @OptIn(DelicateCoroutinesApi::class)
+    override suspend fun getGitHubRepo(): List<GithubRepoEntity> {
+        return GlobalScope.async(Dispatchers.IO) {


Retrofit의 경우에는 suspend만 붙여줘도 내부적으로 스케줄러를 관리해주고있습니다

namjackson · 2023-09-15T10:51:36Z

github-data/src/test/kotlin/GithubDataTest.kt

+    }
+
+    @Test
+    fun `MockWebServer를 활용하여 githubRepository의 기능을 검증한다1`() = runTest(UnconfinedTestDispatcher()) {


통신에 관련되어 테스트를 진행할 때 테스트코드의 함수명을 어떤 식으로 지어야 할지 조언을 주시면 감사하겠습니다.

저는 개인적으로
given/when/then 표기법으로 테스트케이스 네이밍을 정하는 편입니다.
emptyGithubRepo.json을 내려줄때, GitHubRepoData를 받아오면, 빈값이 나와야한다
같은 구체적인 네이밍을 활용하곤합니다 😄

테스트케이스가 많아지면, 구현부보다는 네이밍만으로 관리되기때문에, 테스트케이스이름을 조금더 구체적으로 작성해보면 좋을거같아요!

아직 네이밍에 어려움이 많습니다 ㅜㅜ
given/when/then 표기법을 통해 네이밍을 할 수 있다는 점 알려주셔서 감사합니다.
이번 개선사항에서는 해당 표기법을 사용해 보았습니다.

namjackson · 2023-09-15T10:53:55Z

github-data/src/test/kotlin/GithubDataTest.kt

+
+@OptIn(ExperimentalCoroutinesApi::class)
+@RunWith(RobolectricTestRunner::class)
+class GithubDataTest {


GithubDataTest는 어떤 테스트를 의미할까요?
테스트 코드는 기존 코드와 동일한 패키지 네이밍과
테스트할 클래스명+"Test"라는 네이밍으로
하나의 테스트클래스에서는 하나의 클래스만 테스트하도록 작업해보아요!

namjackson · 2023-09-15T10:56:24Z

github-data/src/test/kotlin/GithubDataTest.kt

+        val context = ApplicationProvider.getApplicationContext<Context>()
+
+        val githubRepoDao = GithubRepoDatabase.getInstance(context)!!.GithubRepoDao()


불필요한 android의 context를 참조하고 있네요,
mock 이나 페이크 객체를 활용해보면 어떨까요?

local 저장소의 경우 Fake 저장소 구현하여 사용했습니다. Remote 저장소의 경우 Fake 저장소가 필요없는 것 같아서 기존 구현된 저장소를 사용하였습니다.

- 패키지명 변경 - LocalDataSource, RemoteDataSource 인터페이스 수정 - local Data 저장 로직 추가 - GlobalScope 제거 - 테스트 케이스 네이밍 수정 - 오류 테스트 케이스 추가

oyj7677 · 2023-09-18T15:47:40Z

항상 부족한 부분 많이 알려주셔서 감사합니다.

oyj7677 and others added 7 commits August 30, 2023 00:04

Create README.md

5c378e9

step2 - GitHub(HTTP)

678ced4

- GitHub Repository 목록 수신로직 구현 - MockWebServer를 이용한 테스트 코드 작성

README 업데이트

e0d1e86

Merge branch 'oyj7677' into step2

1c2ac73

코루틴 테스트 코드 어노테이션 추가

5ab40f8

Merge branch 'step2' of https://github.com/oyj7677/android-github int…

8dc801b

…o step2

data모듈

daf059b

- 불필요한 의존성 제거

namjackson requested changes Sep 6, 2023

View reviewed changes

oyj7677 added 2 commits September 12, 2023 00:33

step2 - 1차 개선사항

222dd0b

- 변수값 카멜케이스 사용 - 저장소 패턴 적용 - 구현체 internal class 적용 - GitHubRepositoryImpl 테스트 케이스 추가 - UI관련 코드 삭제

제거된 코드 정리

759b5b6

namjackson requested changes Sep 15, 2023

View reviewed changes

oyj7677 added 2 commits September 19, 2023 00:30

step2 - 3차 개선사항

d262f9b

- 패키지명 변경 - LocalDataSource, RemoteDataSource 인터페이스 수정 - local Data 저장 로직 추가 - GlobalScope 제거 - 테스트 케이스 네이밍 수정 - 오류 테스트 케이스 추가

코드 정리 및 테스트 코드 주석 수정

e515420

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step2 - GitHub(HTTP) #108

Step2 - GitHub(HTTP) #108

oyj7677 commented Sep 3, 2023

oyj7677 commented Sep 4, 2023

namjackson left a comment

namjackson Sep 5, 2023

namjackson Sep 6, 2023

namjackson Sep 6, 2023

namjackson Sep 6, 2023

namjackson Sep 6, 2023

namjackson Sep 6, 2023

namjackson Sep 6, 2023

namjackson Sep 6, 2023

namjackson Sep 6, 2023

namjackson Sep 6, 2023

namjackson commented Sep 6, 2023

oyj7677 commented Sep 11, 2023

namjackson left a comment

namjackson Sep 15, 2023

oyj7677 Sep 18, 2023

namjackson Sep 15, 2023

oyj7677 Sep 18, 2023

namjackson Sep 15, 2023

namjackson Sep 15, 2023

oyj7677 Sep 18, 2023

namjackson Sep 15, 2023

oyj7677 Sep 18, 2023

namjackson Sep 15, 2023

oyj7677 Sep 18, 2023

namjackson Sep 15, 2023

namjackson Sep 15, 2023

oyj7677 Sep 18, 2023

namjackson Sep 15, 2023

namjackson Sep 15, 2023

oyj7677 Sep 18, 2023

oyj7677 commented Sep 18, 2023

		val server = MockWebServer()


		private lateinit var service: GitHubApi

		val context = ApplicationProvider.getApplicationContext<Context>()

		val githubRepoDao = GithubRepoDatabase.getInstance(context)!!.GithubRepoDao()

Step2 - GitHub(HTTP) #108

Are you sure you want to change the base?

Step2 - GitHub(HTTP) #108

Conversation

oyj7677 commented Sep 3, 2023

oyj7677 commented Sep 4, 2023

namjackson 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

namjackson commented Sep 6, 2023

oyj7677 commented Sep 11, 2023

namjackson 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

oyj7677 commented Sep 18, 2023