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

Step2 Github HTTP #45

Open
wants to merge 3 commits into
base: dla3946gns
Choose a base branch
from
Open

Conversation

dla3946gns
Copy link

피드백 반영 및 2단계 작업하였습니다! 리뷰해주시면 감사하겠습니다!

Copy link
Member

@malibinYun malibinYun left a comment

Choose a reason for hiding this comment

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

2단계 미션 고생 많으셨어요!
생각해보면 좋을 점들에 코멘트 남겨두었습니다. 피드백 반영 후 다시 리뷰 요청 주세요!
또, Retrofit Test에서 실패인 케이스 또한 테스트를 작성해보세요 :) 그리고 GitRepoImpl를 포함한 모든 객체에 단위 테스트를 작성 해주세요! 💪

Comment on lines +17 to +18
val retrofitVersion = "2.9.0"
val okhttpVersion = "4.9.3"
Copy link
Member

Choose a reason for hiding this comment

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

이 변수들은 buildSrc에 위치시키는 것은 어떨까요?

Comment on lines +4 to +5

data class GitRepoDto (
Copy link
Member

Choose a reason for hiding this comment

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

Dto라는 이름보다는, 서버 요청에대한 응답 값이니, Response 정도의 이름은 어떨까요?

Copy link
Member

Choose a reason for hiding this comment

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

또, Github의 원격 Repository 정보를 불러오는 것이니, Git보단 Github이란 이름이 더 나아보입니다 :)

Comment on lines +6 to +10
internal class GitRepoImpl {
suspend fun getRepositories() : List<GitRepoDto> {
return NetworkObject.githubApi.getRepositories()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Github Repository에 대한 Repository interface를 만들고, data module에서는 해당 구현체를 만드는 방식으로 로직을 구성해보는 것은 어떨까요?
또, Retrofit 객체는 생성자로부터 주입 받게 만들어보는 것을 제안드립니다 :)

Comment on lines +8 to +9
suspend fun getRepositories() : List<GitRepoDto>
}
Copy link
Member

Choose a reason for hiding this comment

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

바로 List<~>를 반환받는 것 보다, Response<T>를 활용해보는 것은 어떨까요?
네트워크 실패 시, 더 세세하게 응답을 다룰 수 있을 거예요 :)

Comment on lines +40 to +41
.setBody(File("src/test/resources/githubRepo.json").readText())
server.enqueue(response)
Copy link
Member

Choose a reason for hiding this comment

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

json 리소스 파일의 이름은 매우 구체적인 것이 좋습니다.같은 요청이라도 성공했을 때, 실패했을 때 뿐만 아니라 여러가지의 분기가 더 있을 수 있기 때문이에요. api 수가 많아진다면 가독성에 유의해야 합니다.

respositories-200.json
respositories-404.json

또는, 각 path를 디렉토리로 만들어주는 방법도 있습니다.

src/test/resources/repositories/200.json
src/test/resources/repositories/404.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants