-
Notifications
You must be signed in to change notification settings - Fork 44
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
base: dla3946gns
Are you sure you want to change the base?
Conversation
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.
2단계 미션 고생 많으셨어요!
생각해보면 좋을 점들에 코멘트 남겨두었습니다. 피드백 반영 후 다시 리뷰 요청 주세요!
또, Retrofit Test에서 실패인 케이스 또한 테스트를 작성해보세요 :) 그리고 GitRepoImpl를 포함한 모든 객체에 단위 테스트를 작성 해주세요! 💪
val retrofitVersion = "2.9.0" | ||
val okhttpVersion = "4.9.3" |
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.
이 변수들은 buildSrc에 위치시키는 것은 어떨까요?
|
||
data class GitRepoDto ( |
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.
Dto라는 이름보다는, 서버 요청에대한 응답 값이니, Response 정도의 이름은 어떨까요?
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.
또, Github의 원격 Repository 정보를 불러오는 것이니, Git보단 Github이란 이름이 더 나아보입니다 :)
internal class GitRepoImpl { | ||
suspend fun getRepositories() : List<GitRepoDto> { | ||
return NetworkObject.githubApi.getRepositories() | ||
} | ||
} |
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.
Github Repository에 대한 Repository interface를 만들고, data module에서는 해당 구현체를 만드는 방식으로 로직을 구성해보는 것은 어떨까요?
또, Retrofit 객체는 생성자로부터 주입 받게 만들어보는 것을 제안드립니다 :)
suspend fun getRepositories() : List<GitRepoDto> | ||
} |
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.
바로 List<~>를 반환받는 것 보다, Response<T>를 활용해보는 것은 어떨까요?
네트워크 실패 시, 더 세세하게 응답을 다룰 수 있을 거예요 :)
.setBody(File("src/test/resources/githubRepo.json").readText()) | ||
server.enqueue(response) |
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.
json 리소스 파일의 이름은 매우 구체적인 것이 좋습니다.같은 요청이라도 성공했을 때, 실패했을 때 뿐만 아니라 여러가지의 분기가 더 있을 수 있기 때문이에요. api 수가 많아진다면 가독성에 유의해야 합니다.
respositories-200.json
respositories-404.json
또는, 각 path를 디렉토리로 만들어주는 방법도 있습니다.
src/test/resources/repositories/200.json
src/test/resources/repositories/404.json
피드백 반영 및 2단계 작업하였습니다! 리뷰해주시면 감사하겠습니다!