-
Notifications
You must be signed in to change notification settings - Fork 43
Step2 - GitHub(HTTP) #102
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
Step2 - GitHub(HTTP) #102
Changes from all commits
68d9709
6e7799d
6f195d1
610c621
7cd5863
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,16 @@ | ||
| # Step2(2023-08-30) # | ||
| * GitHub(HTTP) | ||
| * [x] GET, https://api.github.com/repositories | ||
| * [x] full_name, description 필드만 | ||
| * [x] Repository 목록을 가져오는 기능은 data 모듈에 구현되어야 한다. | ||
| * [x] data 모듈의 구현체는 모두 internal class로 선언한다. | ||
| * [x]HTTP 요청을 통해 가져오는 구현체에 대한 테스트 코드를 작성한다. | ||
|
|
||
| # Step1(2023-08-29) # | ||
| * [x] 순수 코틀린 모듈인 domain 모듈을 만든다. | ||
| * [x] 순수 코틀린 모듈인 data 모듈을 만든다. | ||
| * [x] data 모듈은 domain 모듈에 의존해야 한다. | ||
| * [x] app 모듈은 domain 모듈에 의존해야 한다. | ||
|
|
||
| * GitHub(모듈 분리) | ||
| * [x] 순수 코틀린 모듈인 domain 모듈을 만든다. | ||
| * [x] 순수 코틀린 모듈인 data 모듈을 만든다. | ||
| * [x] data 모듈은 domain 모듈에 의존해야 한다. | ||
| * [x] app 모듈은 domain 모듈에 의존해야 한다. | ||
| * 1차 피드백 | ||
| * [x] Android Studio linebreak 설정 추가 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package com.nextstep.edu.data | ||
|
|
||
| internal object ApiClient { | ||
| const val BASE_URL = "https://api.github.com/" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package com.nextstep.edu.data | ||
|
|
||
| import com.nextstep.edu.data.model.RepositoryResponse | ||
| import retrofit2.http.GET | ||
|
|
||
| internal interface GithubService { | ||
|
|
||
| // 유저 목록 가져오기 | ||
| @GET("repositories") | ||
| suspend fun getRepositories(): List<RepositoryResponse> | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package com.nextstep.edu.data.di | ||
|
|
||
| import com.nextstep.edu.data.ApiClient | ||
| import com.nextstep.edu.data.GithubService | ||
| import okhttp3.OkHttpClient | ||
| import retrofit2.Retrofit | ||
| import retrofit2.converter.gson.GsonConverterFactory | ||
|
|
||
| internal object NetworkModule { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Step3에서 Hilt를 쉽게 적용할 수 있는 구조로 구현해주셨네요 👍 |
||
|
|
||
| fun provideRetrofit(): Retrofit = Retrofit.Builder() | ||
| .baseUrl(ApiClient.BASE_URL) | ||
| .client(provideOkHttpClient()) | ||
| .addConverterFactory(GsonConverterFactory.create()) | ||
| .build() | ||
|
|
||
| private fun provideOkHttpClient(): OkHttpClient = OkHttpClient.Builder().build() | ||
|
|
||
| fun provideGithubService(retrofit: Retrofit): GithubService = retrofit.create(GithubService::class.java) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package com.nextstep.edu.data.model | ||
|
|
||
| import com.google.gson.annotations.SerializedName | ||
| import com.nextstep.edu.domain.model.Repository | ||
|
|
||
| internal data class RepositoryResponse( | ||
| @SerializedName("full_name") | ||
| val fullName: String, | ||
|
|
||
| @SerializedName("description") | ||
| val description: String | ||
| ) { | ||
| fun toDomain(): Repository { | ||
| return Repository(fullName = this.fullName, description = this.description) | ||
| } | ||
|
Comment on lines
+13
to
+15
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여러 레이어의 모델이 존재할 때 각 레이어 간 어떻게 모델의 값을 변환할 것인가?는 개발자마다 많은 의견이 있는 영역입니다. 참고로 PRND 컴퍼니의 안드로이드 스타일 가이드에서도 이런 방법을 택하고 있습니다. 주관적인 의견이니 참고만 해주세요!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이전에는 계속 Mapper.kt를 만들고 PRND처럼 mapper로직을 최상위 함수로 뽑아두었는데, 이번에 다른 방법으로 해보려고 작성했었습니다!! |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package com.nextstep.edu.data.repository | ||
|
|
||
| import com.nextstep.edu.data.GithubService | ||
| import com.nextstep.edu.data.model.RepositoryResponse | ||
| import com.nextstep.edu.domain.repository.RemoteRepository | ||
| import com.nextstep.edu.domain.model.Repository | ||
|
|
||
| internal class RemoteRepositoryImpl( | ||
| private val githubService: GithubService | ||
| ): RemoteRepository { | ||
| override suspend fun getRepositories(): List<Repository> { | ||
| return githubService.getRepositories().map(RepositoryResponse::toDomain) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package com.nextstep.edu.data | ||
|
|
||
| import com.google.common.truth.Truth.assertThat | ||
| import com.nextstep.edu.data.model.RepositoryResponse | ||
| import kotlinx.coroutines.ExperimentalCoroutinesApi | ||
| import kotlinx.coroutines.test.StandardTestDispatcher | ||
| import kotlinx.coroutines.test.TestScope | ||
| import kotlinx.coroutines.test.runTest | ||
| import okhttp3.mockwebserver.MockResponse | ||
| import okhttp3.mockwebserver.MockWebServer | ||
| import org.junit.jupiter.api.BeforeEach | ||
| import org.junit.jupiter.api.Test | ||
| import retrofit2.Retrofit | ||
| import retrofit2.converter.gson.GsonConverterFactory | ||
| import java.io.File | ||
|
|
||
| @ExperimentalCoroutinesApi | ||
| class GithubServiceTest { | ||
|
|
||
| private val testDispatcher = StandardTestDispatcher() | ||
| private val testScope = TestScope(testDispatcher) | ||
|
|
||
| private lateinit var server: MockWebServer | ||
| private lateinit var service: GithubService | ||
|
|
||
| @BeforeEach | ||
| fun setup() { | ||
| server = MockWebServer() | ||
| service = Retrofit.Builder() | ||
| .addConverterFactory(GsonConverterFactory.create()) | ||
| .baseUrl(server.url("")) | ||
| .build() | ||
| .create(GithubService::class.java) | ||
| } | ||
|
|
||
| @Test | ||
| fun `getRepositories 호출`() = testScope.runTest { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 테스트명을 통해 어떤 것을 테스트하고자 하는 것이 제대로 드러나지 않는다고 생각합니다. 테스트명을 작성하실 때에는
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아하 그러네요!! 함수명만 봤을 때 어떤 결과가 나와야 하는지 잘 유추되지 않네요 |
||
| // given : githubRepositories.json 파일이 주어졌을 때 | ||
| val response = MockResponse() | ||
| .setBody(File("src/test/resources/githubRepositories.json").readText()) | ||
| server.enqueue(response) | ||
|
|
||
| // when : getRepositories() 를 요청 하면 | ||
| val actual = service.getRepositories() | ||
|
|
||
| // then : 결과의 첫번째의 full_name은 mojombo/grit 이다. | ||
| val expected = RepositoryResponse( | ||
| fullName = "mojombo/grit", | ||
| description = "**Grit is no longer maintained. Check out libgit2/rugged.** Grit gives you object oriented read/write access to Git repositories via Ruby." | ||
| ) | ||
| assertThat(actual[0]).isEqualTo(expected) | ||
| } | ||
|
|
||
| @Test | ||
| fun `getRepositories 호출2`() = testScope.runTest { | ||
| // given : githubRepositories.json 파일이 주어졌을 때 | ||
| val response = MockResponse() | ||
| .setBody(File("src/test/resources/githubRepositories.json").readText()) | ||
| server.enqueue(response) | ||
|
|
||
| // when : getRepositories() 를 요청 하면 | ||
| val actual = service.getRepositories() | ||
|
|
||
| // then : 결과의 두번째의 full_name은 wycats/merb-core 이다. | ||
| val expected = RepositoryResponse( | ||
| fullName = "wycats/merb-core", | ||
| description = "Merb Core: All you need. None you don't." | ||
| ) | ||
| assertThat(actual[1]).isEqualTo(expected) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 주석은 꼭 필요한 주석일까요? 간단한 주석을 남길 때에도 주석이 꼭 필요한 상황인지 고려하여 남기시는게 좋습니다. 비록 이 프로젝트는 혼자 관리하시고 계시지만 여러 사람이 contribute 하는 프로젝트라면 이렇게 작성된 주석이 관리되지 않을 가능성이 높아집니다.
클린 코드 책에서 설명하는 주석에 대한 장을 읽어보시면 좋을 것 같아요!
FYI: https://nesoy.github.io/articles/2018-01/CleanCode-Comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 플젝에서 API 버전등 API 갯수가 늘어감에 따라서 메서드만으로는 이 API가 어떤 기능을 하는지에 대해서 알기 위해서 주석을 추가했었는데 올려주신 내용 참고해서 수정하도록 하겠습니다! 감사합니다~