Conversation
kimjw2003
left a comment
There was a problem hiding this comment.
수고하셨습니다 리뷰한것만 적용해주시고 나중에 ViewModel 생기면 @hiltviewmodel 꼭 넣기!
|
|
||
| @Provides | ||
| @Singleton | ||
| fun provideFlintApi(retrofit: Retrofit): FlintApi = |
There was a problem hiding this comment.
p1
해당 내용은 ApiModule이라는 싱글톤 object 파일에다가 넣어주시면 어떨까요?
FlintApi 뿐만 아니라 다른 Api도 사용할수 있으니까
nahy-512
left a comment
There was a problem hiding this comment.
고생 많으셨습니당ㅎㅎ
작업 너무 잘 해주셨는데, 이번 설정에서 미리 추가해주시면 좋겠는 내용들이 있어서 리젝드립니당
| } else { | ||
| HttpLoggingInterceptor.Level.NONE | ||
| } |
There was a problem hiding this comment.
p1
저희 그 FlintApplication에서 timber로 디버그만 로그 설정을 해둬서, 이 부분 빼도 크게 상관없을 것 같습니다!
There was a problem hiding this comment.
p1
ServiceModule도 필요할 것 같아요!
FlintApi랑 retrofit.create 설정해줄 용도로
There was a problem hiding this comment.
p1.5)
MVVM 구조까지 원래 제가 설정해보려 했었는데ㅎㅎ
예시용으로 차민님 파트인 onboarding으로 presentation 안에 폴더 하나 만들고, OnboardingScreen이랑 OnboardingViewModel도 한번 만들고, @inject 예시 넣어보면 좋을 것 같아요!
giovannijunseokim
left a comment
There was a problem hiding this comment.
Hilt 세팅 고생 많으셨습니다! 👍
코드를 보며 궁금한 점들을 리뷰로 남겨보았습니다.
답변 후 다시 리뷰 요청 부탁드립니다. 🙇
| connectTimeout(20, TimeUnit.SECONDS) | ||
| writeTimeout(20, TimeUnit.SECONDS) | ||
| readTimeout(20, TimeUnit.SECONDS) |
There was a problem hiding this comment.
p2: 이 부분은 어떤 의도로 설정된 것일까요?
There was a problem hiding this comment.
네트워크 불안정 상황 대비, 이미지/파일 업로드 고려해서 두었습니다
There was a problem hiding this comment.
최대 20초까지 로딩 뷰를 보여줄 수도 있다는 건데, 이 때 사용자가 20초를 기다려야 하는 점이 우려되네요.
이 부분은 서버 팀과 협의가 필요하지 않을까요? 서버 개발자분들께서 API의 평균 응답 시간이나 최대 허용 시간 등을 알 수도 있으니까요.
There was a problem hiding this comment.
Hilt 세팅이 너무 늦어지는 것 같다면 해당 내용 별도 이슈로 빼신 후 작업하시는 것도 좋을 것 같습니다. 🙂
| fun provideRetrofit( | ||
| okHttpClient: OkHttpClient, | ||
| json: Json | ||
| ): Retrofit = Retrofit.Builder() | ||
| .baseUrl(BuildConfig.BASE_URL) | ||
| .client(okHttpClient) | ||
| .addConverterFactory(json.asConverterFactory("application/json".toMediaType())) | ||
| .build() |
There was a problem hiding this comment.
p2: 사실상 이 함수 내에서 HttpLoggingInterceptor, OkHttpClient, Json을 전부 생성해줘도 되지 않나요?
어떤 기준으로 함수를 분리한 것인지 궁금합니다.
There was a problem hiding this comment.
재사용성과 단일책임원칙을 위해서 함수를 분리했습니다. 향후확장성도 고려해
There was a problem hiding this comment.
재사용에 관해서는 현재 재사용 되고있지 않은데, 추후 어떤 상황에 재사용할 일이 생길까요?
함수가 짧게 유지되면서 한 가지 일만 하도록 만들어져 가독성 측에서는 좋은 것 같습니다. 👍
|
|
||
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| object ServiceModule { |
There was a problem hiding this comment.
p2: Api들을 제공하는 모듈인 것 같아요.
ServiceModule이라는 네이밍이 필요할까요?
There was a problem hiding this comment.
FlintApi랑 retrofit.create 설정해줄 용도로 만들었는데 어떤 네이밍이 적절할까요?
There was a problem hiding this comment.
네이밍은 항상 어렵죠. 저라면 NetworkModule 내부에 두거나, ApiModule이라는 이름을 사용할 것 같아요.
그 외의 파일에서는 어떻게 네이밍했는지 확인해보겠습니다.
- Network 작업을 위해 필요한 객체들을
NetworkModule에서 제공하고 Repository들을RepositoryModule에서 주고 있어요.
ServiceModule라는 이름을 사용하기엔
Service가 무엇인지 구체적으로 정의되지 않은 것 같고- 실제로
Service객체를 제공하고 있지도 않아요.
때문에 어색한 이름이라는 인상을 받았습니다.
|
좋은 의견 감사합니다! 확인해보니 |
giovannijunseokim
left a comment
There was a problem hiding this comment.
추가 코멘트 남겼으니 확인 후 머지해주시면 좋을 것 같습니다. 🙂
추가적으로 각 리뷰를 반영했다면 해당 내용이 담긴 커밋을 답변에 남겨주시면 확인이 편할 것 같아요. 리뷰마다 커밋이 나눠진다면 더욱 좋을 것 같구요. 👍
이 부분은 앱잼을 진행하며 조바심 갖지 않으셔도 됩니다. Hilt 세팅 고생하셨습니다. 🔥
| connectTimeout(20, TimeUnit.SECONDS) | ||
| writeTimeout(20, TimeUnit.SECONDS) | ||
| readTimeout(20, TimeUnit.SECONDS) |
There was a problem hiding this comment.
최대 20초까지 로딩 뷰를 보여줄 수도 있다는 건데, 이 때 사용자가 20초를 기다려야 하는 점이 우려되네요.
이 부분은 서버 팀과 협의가 필요하지 않을까요? 서버 개발자분들께서 API의 평균 응답 시간이나 최대 허용 시간 등을 알 수도 있으니까요.
| connectTimeout(20, TimeUnit.SECONDS) | ||
| writeTimeout(20, TimeUnit.SECONDS) | ||
| readTimeout(20, TimeUnit.SECONDS) |
There was a problem hiding this comment.
Hilt 세팅이 너무 늦어지는 것 같다면 해당 내용 별도 이슈로 빼신 후 작업하시는 것도 좋을 것 같습니다. 🙂
| fun provideRetrofit( | ||
| okHttpClient: OkHttpClient, | ||
| json: Json | ||
| ): Retrofit = Retrofit.Builder() | ||
| .baseUrl(BuildConfig.BASE_URL) | ||
| .client(okHttpClient) | ||
| .addConverterFactory(json.asConverterFactory("application/json".toMediaType())) | ||
| .build() |
There was a problem hiding this comment.
재사용에 관해서는 현재 재사용 되고있지 않은데, 추후 어떤 상황에 재사용할 일이 생길까요?
함수가 짧게 유지되면서 한 가지 일만 하도록 만들어져 가독성 측에서는 좋은 것 같습니다. 👍
|
|
||
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| object ServiceModule { |
There was a problem hiding this comment.
네이밍은 항상 어렵죠. 저라면 NetworkModule 내부에 두거나, ApiModule이라는 이름을 사용할 것 같아요.
그 외의 파일에서는 어떻게 네이밍했는지 확인해보겠습니다.
- Network 작업을 위해 필요한 객체들을
NetworkModule에서 제공하고 Repository들을RepositoryModule에서 주고 있어요.
ServiceModule라는 이름을 사용하기엔
Service가 무엇인지 구체적으로 정의되지 않은 것 같고- 실제로
Service객체를 제공하고 있지도 않아요.
때문에 어색한 이름이라는 인상을 받았습니다.
📮 관련 이슈
📌 작업 내용
🫛 To. 리뷰어
Hilt 세팅을 처음 해보는데,
봐주시면 감사하겠습니다!