Step2 - GitHub(HTTP) #102
Step2 - GitHub(HTTP) #102
Conversation
| } | ||
|
|
||
| @Test | ||
| fun `getRepositories 호출`() = testScope.runTest { |
There was a problem hiding this comment.
테스트명을 통해 어떤 것을 테스트하고자 하는 것이 제대로 드러나지 않는다고 생각합니다. 테스트명을 작성하실 때에는
- 검증하고자 하는 로직을 명확하게 정하고 (검증이 필요한 로직이 두 개 이상이면 필요에 따라 또 다른 함수로 분할한다)
- 그것을 함수명으로 드러내보면 어떨까요?
There was a problem hiding this comment.
아하 그러네요!! 함수명만 봤을 때 어떤 결과가 나와야 하는지 잘 유추되지 않네요
해당 내용 작성시 유의사항 1, 2 유의해서 작성하도록 하겠습니다!! 감사합니다~
|
|
||
| internal interface GithubService { | ||
|
|
||
| // 유저 목록 가져오기 |
There was a problem hiding this comment.
이 주석은 꼭 필요한 주석일까요? 간단한 주석을 남길 때에도 주석이 꼭 필요한 상황인지 고려하여 남기시는게 좋습니다. 비록 이 프로젝트는 혼자 관리하시고 계시지만 여러 사람이 contribute 하는 프로젝트라면 이렇게 작성된 주석이 관리되지 않을 가능성이 높아집니다.
클린 코드 책에서 설명하는 주석에 대한 장을 읽어보시면 좋을 것 같아요!
FYI: https://nesoy.github.io/articles/2018-01/CleanCode-Comment
There was a problem hiding this comment.
다른 플젝에서 API 버전등 API 갯수가 늘어감에 따라서 메서드만으로는 이 API가 어떤 기능을 하는지에 대해서 알기 위해서 주석을 추가했었는데 올려주신 내용 참고해서 수정하도록 하겠습니다! 감사합니다~
| import retrofit2.Retrofit | ||
| import retrofit2.converter.gson.GsonConverterFactory | ||
|
|
||
| internal object NetworkModule { |
There was a problem hiding this comment.
Step3에서 Hilt를 쉽게 적용할 수 있는 구조로 구현해주셨네요 👍
| fun toDomain(): Repository { | ||
| return Repository(fullName = this.fullName, description = this.description) | ||
| } |
There was a problem hiding this comment.
여러 레이어의 모델이 존재할 때 각 레이어 간 어떻게 모델의 값을 변환할 것인가?는 개발자마다 많은 의견이 있는 영역입니다.
저는 개인적으로 이러한 mapper 로직을 최상위 함수로 두는 것을 선호하는데요, data 모델을 domain 모델로 바꾸는 기능은 모델 자체의 역할이라고 보기 어렵기 때문입니다. data 모델을 다른 모델로 변환할 일이 또 생기더라도 원본 모델에 변화를 줄 필요는 없다고 생각합니다.
참고로 PRND 컴퍼니의 안드로이드 스타일 가이드에서도 이런 방법을 택하고 있습니다.
https://github.com/PRNDcompany/android-style-guide/blob/main/Architecture.md#model-mapping-%EC%A0%95%EC%9D%98%EA%B5%AC%EC%A1%B0
주관적인 의견이니 참고만 해주세요!
There was a problem hiding this comment.
이전에는 계속 Mapper.kt를 만들고 PRND처럼 mapper로직을 최상위 함수로 뽑아두었는데, 이번에 다른 방법으로 해보려고 작성했었습니다!!
이전 처럼 Mapper.kt 에 Mapper함수를 최상단으로 올려두고 해야겠네요ㅎㅎ 감사합니다!!
There was a problem hiding this comment.
테스트용 데이터가 6천줄이 추가되었네요...!
많은 양의 데이터를 처리할 수 있는지 자체를 테스트하는 의미가 있다고 볼 수도 있지만, 꼭 실제 API 응답을 참조하진 않아도 된다고 생각해요. 의미 있는 테스트를 구성할 수 있는 정도의 데이터만 남겨두면 어떨까요?
There was a problem hiding this comment.
넵 알겠습니다!! 해당 내용 수정하도록 하겠습니다~ 피드백 감사합니다!
안녕하세요~ 리뷰어님
Step2 반영한 내용입니다! 잘부탁드립니다!!
작업 내용