[둘리] 2단계 재화 미션 제출합니다. #30
Conversation
# Conflicts: # app/src/main/java/woowacourse/shopping/view/cart/CartPresenter.kt
| private val retrofitService = Retrofit.Builder() | ||
| .baseUrl(serverRepository.getServerUrl()) | ||
| .addConverterFactory(GsonConverterFactory.create()) | ||
| .build() | ||
| .create(CartApi::class.java) |
There was a problem hiding this comment.
Repository마다 중복된 코드가 있네요,
분리해보면 어떨까요?
There was a problem hiding this comment.
RetrofitGenerator라는 클래스를 생성하여 url과 service에 따른 retrofitService를 반환하도록 했습니다!
| callback(cartProducts.subList(mark, mark + rangeSize)) | ||
| import woowacourse.shopping.domain.repository.ServerStoreRespository | ||
|
|
||
| class CartRemoteRepository(serverRepository: ServerStoreRespository, private val failureCallback: (String?) -> Unit) : CartRepository { |
There was a problem hiding this comment.
failureCallback이 Repository단에서 관리될 이유가 있을까요?
failureCallback은 호출하는 함수마다 달라야 하진 않을까요?
There was a problem hiding this comment.
Presenter에서 DataResult를 활용하여 관리하도록 했습니다.
| }) | ||
| } | ||
| companion object { | ||
| private const val SERVER_ERROR_MESSAGE = "서버와의 통신이 원활하지 않습니다." |
There was a problem hiding this comment.
다국어 지원해야하는 경우에는 에러메세지는 어떻게 처리할수 있을까요?
에러는 Exception이나, 다른 클래스로 분리해서 전달하면 어떨까요?
There was a problem hiding this comment.
에러는 DataResult 클래스를 생성하여 Presenter에서 관리하도록 했습니다.
다국어 지원의 경우에는 string.xml의 localization을 활용할 수 있다고 생각했으나, 시간상의 문제로 6시 안에 수정하지는 못했습니다.
다음 리뷰 요청 때 함께 반영하여 올리겠습니다!
There was a problem hiding this comment.
다국어 지원뿐아니라, 특정 에러마다 다른 동작을 할수도 있는구조가 되면 좋을거같아요 👍
There was a problem hiding this comment.
DataResult의 개수를 늘려서 우선 서버 통신이 아예 실패한 경우, 성공은 했으나 isSuccessful이 false의 경우로 나누어 다른 view의 함수를 실행하도록 수정했습니다!
| import woowacourse.shopping.domain.model.CartProduct | ||
|
|
||
| interface CartApi { | ||
| @Headers("Authorization: Basic ZG9vbHlAZG9vbHkuY29tOjEyMzQ=") |
There was a problem hiding this comment.
동적으로 token이 변경된다면 어떻게 대응할수 있을까요?
(okhttp interceptor 개념을 활용해보아요)
There was a problem hiding this comment.
token을 내부에서 저장해둔다면 repository를 사용하여 저장한 token값을 넘겨 헤더를 유동적으로 추가할 수 있을 것 같습니다.
이번 미션에서는 token이 변경될 일이 없기 때문에 object로 선언하여, Header를 추가해야하는 api의 경우 object의 함수로부터 값을 가져올 수 있도록 했습니다.
추가로, 보통 앱에서 로그인 유지 기능을 구현할 때 어떻게 구현하는지 궁금합니다.
저는 내부 저장소에 base64로 인코딩된 값을 저장하는 것과, 지금처럼 이메일-비밀번호를 저장하여 사용할 때마다 encoding을 하여 사용하는 방법 두 가지를 생각했는데요.
이메일-비밀번호가 그대로 노출될 수 있는 것도 껄끄럽고 base64된 값을 저장하는 것도 껄끄럽게 느껴집니다.🥲
There was a problem hiding this comment.
보통 아이디/비밀번호이 노출되는 방식의 값을 서버에서 처리하지, 안드로이드에선 저장하지는 않습니다.
서버에서 발행된 인증토큰을 관리하도록 하는데, 이부분 또한 암호화하여 보관하곤하니다.
이번 미션 특성상 token이 변경되지는 않지만, 보통은 그렇지 않기때문에, token을 처리하는 클래스를 만들어도 좋을거같아요!
| binding.recyclerCart.adapter?.notifyDataSetChanged() | ||
| } | ||
|
|
||
| override fun showChangedItem(position: Int) { | ||
| runOnUiThread { | ||
| binding.recyclerCart.adapter?.notifyItemChanged(position) | ||
| } | ||
| binding.recyclerCart.adapter?.notifyItemChanged(position) |
There was a problem hiding this comment.
notifyDataSetChanged~ 는 adapter의 중요한 로직이예요,
외부로 노출되어 실행되기 보단, list를 adapter로 전달하고,
변경시에 adapter내부에서 알아서 실행해주면 어떨까요?
There was a problem hiding this comment.
view에 update함수를 추가하여 adapter에 list를 전달하고, adapter 내부에서 변경점을 판단하여 notifyDataSetChanged를 하도록 수정했습니다!
There was a problem hiding this comment.
변경된 코드가 올라오지 않은거 같아요!
notifyItemChanged~ 는 adapter내부에서만 동작하게 해주세요 😄
| cartItems.addAll(items) | ||
| addBottomPagination() | ||
| _isCheckedAll.postValue(getIsCheckedAll()) | ||
| view.showChangedItems() |
There was a problem hiding this comment.
cartItems의 참조를 이용하고 계시네요,
참조를 이용하면 어떤 문제가 생길까요?
cartItems을 View로 전달하면 어떨까요?
There was a problem hiding this comment.
참조를 어디서 하는지 개발자 외에는 알기 어려운 문제가 있을 것 같습니다.
또한 그래서는 안되겠지만 View에서 받은 list를 조작했을 때 Presenter가 알기 어렵습니다.
요약하면 관리가 어려운 문제로 통칭할 수 있을 것 같습니다!
notifyDataSetChanged~ 리뷰 내용과 함께, View에 새 items를 넘겨 adapter에서 notifyDataSetChanged를 호출하도록 수정했습니다.!
| private val _cash = MutableLiveData(0) | ||
| override val cash: LiveData<Int> | ||
| get() = _cash |
There was a problem hiding this comment.
둘리님은 MVP패턴에서 LiveData를 사용해보니 어떠신거 같나요?
There was a problem hiding this comment.
MVP가 아닌 것 같습니다...🥲
LiveData가 편해서 사용하긴 했지만, interface에 LiveData 변수를 선언해야 View에서 사용할 수 있어서 MVP에 어긋난다고 느꼈습니다!
There was a problem hiding this comment.
추후에 mvvm패턴을 적용해보시면 장점을 느끼실수 있으실거예요! 👍
| @@ -0,0 +1,5 @@ | |||
| package woowacourse.shopping.domain.model | |||
|
|
|||
| data class OrderCartItemsDTO(val orderCartItemDtos: List<OrderCartItemDTO>) { | |||
There was a problem hiding this comment.
DTO는 도메인인가요? Repository Interface들이 지금 도메인 모듈에 위치해있는데, DTO를 도메인 모듈로 봐야할지, app 모듈로 봐야할지 헷갈립니다.
DTO를 정의하신 이유는 무엇일까요? 네트워크등의 데이터를 전달받기위한 용도일까요?
그렇다면 도메인레이어에서 DTO를 알고있을 필요가 있을까요?
DTO의 책임은 데이터 레이어에서만 있으면 좋을거같아요!
Repository에서는 도메인의 모델로 정의해도 좋을거 같아요
There was a problem hiding this comment.
받은 데이터 값을 DTO -> 도메인의 모델로 변환하여 리턴하도록 수정했습니다!!
| import kotlinx.parcelize.Parcelize | ||
|
|
||
| @Parcelize | ||
| data class OrderCartProductsModel(val orderProducts: List<OrderCartProductModel>) : Parcelable { |
There was a problem hiding this comment.
저는 되도록 View에서 사용하는 Model을 따로 생성하여, xxxModel이라고 이름을 지었는데요. 만약 DTO의 내용 그대로로 View를 그릴 수 있다면 Model을 따로 생성하는 것이 좋을지, DTO를 그대로 사용해도 되는지 헷갈립니다.
둘리님이 생각하는 Model과 DTO의 책임은 무엇일까요?
도메인의 모델과 View의 모델, DTO 등을 나눈 이유는 한번더 고민해보아도 좋을거 같아요!
There was a problem hiding this comment.
책임이라는 키워드로 계속 고민해보겠습니다!
View를 그리기 위한 값들을 가지는 모델, 비즈니스 로직에서 사용하는 모델, 서버 통신 때 사용하는 모델로 구분하려고 했는데 이전 코드에서는 많이 혼합해서 사용했던 것 같습니다.🥲
There was a problem hiding this comment.
기존 모델을 나눈이유를 한번더 생각해보면 좋을거같아요!
함께 사용하고 있는 구조라면,
api의 변경으로 데이터모델이 변경된다면,
그 영향은 도메인레이어, ui레이어까지 전파되진 않을까요?
There was a problem hiding this comment.
생각해보니 그렇네요..! 데이터 모델이 변경되면 Mapper정도만 바꿔주면 되고 더 많은 파일을 변경할 필요가 없을 것 같습니다!
| } | ||
| import woowacourse.shopping.domain.repository.ServerStoreRespository | ||
|
|
||
| class ProductRemoteRepository(serverRepository: ServerStoreRespository, private val failureCallback: (String?) -> Unit) : ProductRepository { |
There was a problem hiding this comment.
기존 Repositroy에서 많은 부분이 변경되었네요,
테스트를 위해 or 준비되지않은 서버api를 토대로 개발하기위해
Mock서버를 다시 사용하려면 어떻게 처리할수 있을까요?
데이터 모듈의 아키텍쳐를 고민해보셔도 좋을거같아요
https://developer.android.com/topic/architecture/data-layer#architecture
There was a problem hiding this comment.
추가적으로 데이터 모듈 관련 테스트 코드를 작성해주셔도 좋을거같아요!
There was a problem hiding this comment.
시간상 이번 리뷰 요청 때는 반영하지 못했는데 다음 리뷰 때는 반영하겠습니다!
협업 이전에 MockServer를 사용할 때 Repository는 MockServer의 baseUrl을 넘겨서 처리했었는데, View에서 MockServer 객체를 가지게 되어 View 역할에 어긋나는 느낌을 받았습니다.
따라서 이번 미션에서는 View에서 baseUrl을 꺼내오는 작업을 하지 않도록 Repository를 통으로 넘겨줬습니다.
테스트할 때는 MockServer객체를 테스트 클래스에서 갖고, ServerRepository 인터페이스를 MockServer의 url를 저장하고 꺼내주는 형태로 구현해서 넣어준다면 어떨까요?
There was a problem hiding this comment.
어떤 서버에서 어떻게 데이터를 가져올지는 RemoteDataSource의 구현체가 담당할일이라고 생각이 들어요
둘리님 말씀대로 테스트할 때는 MockServer객체를 테스트하도록 하면 좋을거같아요 !
# Conflicts: # app/src/main/java/woowacourse/shopping/view/productlist/ProductListPresenter.kt # domain/src/main/java/woowacourse/shopping/domain/model/ProductsWithCartItemDTO.kt
| class CartRemoteRepository( | ||
| serverRepository: ServerStoreRespository, | ||
| ) : CartRepository { | ||
|
|
||
| private val retrofitService = | ||
| RetrofitGenerator.create(serverRepository.getServerUrl(), CartApi::class.java) |
There was a problem hiding this comment.
retrofitService라는 네이밍보다는, CartService가 더 어울리지 않을까요?
CartService를 생성시점에 주입받아도좋을거같아요 😄
There was a problem hiding this comment.
네이밍 수정했습니다!
CartService를 생성 시점에 주입 받으려면 View에서 생성해주어야하는데, View에서 service를 생성해도 괜찮을까요?
| import kotlinx.parcelize.Parcelize | ||
|
|
||
| @Parcelize | ||
| data class OrderCartProductsModel(val orderProducts: List<OrderCartProductModel>) : Parcelable { |
There was a problem hiding this comment.
기존 모델을 나눈이유를 한번더 생각해보면 좋을거같아요!
함께 사용하고 있는 구조라면,
api의 변경으로 데이터모델이 변경된다면,
그 영향은 도메인레이어, ui레이어까지 전파되진 않을까요?
| if (original.size < items.size) { // 더보기 클릭한 경우 | ||
| notifyItemRemoved(original.size - 1) | ||
| notifyItemRangeInserted(original.size - 1, items.size - original.size + 1) | ||
| return | ||
| } | ||
| val updatedItems = (newItems - original.toSet()).toList() | ||
| for (item in updatedItems) { | ||
| notifyItemChanged(items.indexOf(item)) | ||
| } | ||
| } |
There was a problem hiding this comment.
diffutil 적용해서 리팩터링했습니다!
| retrofitService.requestCartItems().enqueue(object : retrofit2.Callback<List<CartProduct>> { | ||
| override fun onResponse( | ||
| call: Call<List<CartProduct>>, | ||
| response: Response<List<CartProduct>>, | ||
| ) { | ||
| if (!response.isSuccessful) { | ||
| onFailure(call, Throwable(SERVER_ERROR_MESSAGE)) | ||
| return | ||
| } | ||
| response.body()?.let { cartProducts -> | ||
| callback(cartProducts) | ||
| } | ||
| } | ||
| } | ||
| executeGetRequest(request, ::callBackWrapper) | ||
| } | ||
|
|
||
| override fun isExistByMark(mark: Int, callback: (Boolean) -> Unit) { | ||
| val request = Request.Builder() | ||
| .url("$baseUrl/cart-items") | ||
| .addHeader("Authorization", USER_EMAIL_INFO) | ||
| .build() | ||
|
|
||
| fun callBackWrapper(cartProducts: List<CartProduct>) { | ||
| callback(cartProducts.size > mark) | ||
| } | ||
| executeGetRequest(request, ::callBackWrapper) | ||
| override fun onFailure(call: Call<List<CartProduct>>, t: Throwable) { | ||
| throw t | ||
| } | ||
| }) |
There was a problem hiding this comment.
네트워크 요청시마다, 매번 중복된 코드가 생겨나네요,
중복된 코드를 제거해보아도 좋을거같아요!
| }) | ||
| } | ||
| companion object { | ||
| private const val SERVER_ERROR_MESSAGE = "서버와의 통신이 원활하지 않습니다." |
There was a problem hiding this comment.
다국어 지원뿐아니라, 특정 에러마다 다른 동작을 할수도 있는구조가 되면 좋을거같아요 👍
| @@ -57,4 +58,7 @@ dependencies { | |||
| implementation("com.squareup.okhttp3:okhttp:4.10.0") | |||
| implementation("com.squareup.okhttp3:mockwebserver:4.10.0") | |||
| implementation("org.json:json:20210307") | |||
|
|
|||
| implementation("com.squareup.retrofit2:retrofit:2.9.0") | |||
| implementation("com.squareup.retrofit2:converter-gson:2.9.0") | |||
There was a problem hiding this comment.
사실 구현에 급급해 사용해본적 있는 컨버터를 사용했습니다...
찾아보니 kotlinx-serialiation을 많이 사용하는 것 같습니다! 이 라이브러리는 default value를 지정해줄 수 있음을 알게 되었지만,
하지만 저는 null값으로 정보가 잘 전달되었는지 확인하고 있고 이미 default value를 지정해준 상태라 굳이 바꿀 필요성을 못느껴 바꾸지 않았습니다..!
| @@ -1,5 +1,5 @@ | |||
| package woowacourse.shopping.domain.model | |||
|
|
|||
| data class Product(val id: Int, val name: String, val imageUrl: String, val price: Price) { | |||
| data class Product(val id: Int, val name: String, val price: Price, val imageUrl: String) { | |||
| companion object | |||
| @@ -1,5 +1,5 @@ | |||
| package woowacourse.shopping.domain.model | |||
|
|
|||
| data class Product(val id: Int, val name: String, val imageUrl: String, val price: Price) { | |||
| data class Product(val id: Int, val name: String, val price: Price, val imageUrl: String) { | |||
There was a problem hiding this comment.
api 명세서는 정해진 약속이지만,
스펙 변경등의 이유로 다르게 값이 전달될수도 있을거같아요!
name등이 null 로 오거나, 해당 항목이 포함되지않는다면 어떤일이 일어날까요?
There was a problem hiding this comment.
실제로 두 서버의 명세가 조금 달라서 boolean값이 false로 받아진 적이 있었는데요!
잘못된 값이 들어와서 앱이 작동하는데 영향을 끼칠 수 있습니다.
DTO의 프로퍼티들을 nullable로 만들고, null체크하는 프로퍼티를 선언해서 null인 경우 DataResult.WrongResponse를 넘기도록 수정했습니다!
| binding.recyclerCart.adapter?.notifyDataSetChanged() | ||
| } | ||
|
|
||
| override fun showChangedItem(position: Int) { | ||
| runOnUiThread { | ||
| binding.recyclerCart.adapter?.notifyItemChanged(position) | ||
| } | ||
| binding.recyclerCart.adapter?.notifyItemChanged(position) |
There was a problem hiding this comment.
변경된 코드가 올라오지 않은거 같아요!
notifyItemChanged~ 는 adapter내부에서만 동작하게 해주세요 😄
| private val _cash = MutableLiveData(0) | ||
| override val cash: LiveData<Int> | ||
| get() = _cash |
There was a problem hiding this comment.
추후에 mvvm패턴을 적용해보시면 장점을 느끼실수 있으실거예요! 👍
| } | ||
| import woowacourse.shopping.domain.repository.ServerStoreRespository | ||
|
|
||
| class ProductRemoteRepository(serverRepository: ServerStoreRespository, private val failureCallback: (String?) -> Unit) : ProductRepository { |
There was a problem hiding this comment.
어떤 서버에서 어떻게 데이터를 가져올지는 RemoteDataSource의 구현체가 담당할일이라고 생각이 들어요
둘리님 말씀대로 테스트할 때는 MockServer객체를 테스트하도록 하면 좋을거같아요 !
|
안녕하세요, 리뷰어님! 최대한 피드백을 반영해보았습니다. |
| private val cartService = | ||
| RetrofitGenerator.create(url, CartApi::class.java) |
There was a problem hiding this comment.
cartService를 생성시점에 주입받아서 사용하면
의존성 문제나, 테스트하기 수월할거 같아요 😄
| override fun onResponse( | ||
| call: Call<List<CartProductDTO>>, | ||
| response: Response<List<CartProductDTO>>, | ||
| ) { | ||
| if (!response.isSuccessful) { | ||
| callback(DataResult.NotSuccessfulError) | ||
| return | ||
| } | ||
| response.body()?.let { cartProducts -> | ||
| if (!cartProducts.all { it.isNotNull }) { | ||
| callback(DataResult.WrongResponse) | ||
| return | ||
| } | ||
| callback(DataResult.Success(cartProducts)) | ||
| } | ||
| } | ||
|
|
||
| override fun onFailure(call: Call<List<CartProductDTO>>, t: Throwable) { | ||
| callback(DataResult.Failure) | ||
| } | ||
| }) |
There was a problem hiding this comment.
다만 궁금한 점이 네트워크 요청 시 코드 부분인데요! 처음에는 Callback을 상속받는 abstract class를 생성하여 중복을 줄여보려했는데
지금 onResponse에서는 함수마다 조금씩 코드가 달라서 어떻게 줄여야할지 잘 모르겠습니다.🥲
제너릭을 활용해서 만들어보면 어떨까요?
| class RecentViewedDbDataSource(context: Context, url: String) : RecentViewedDataSource { | ||
| private val dbHelper = RecentViewedDBHelper(context, url) |
There was a problem hiding this comment.
RecentViewedDbDataSource 생성시 context 필요하면,
유닛테스트하려면 어떻게해야할까요?
마찬가지로, context, url을 받기보단
dbHelper를 생성시점에 받으면 좋을거 같아요
| } | ||
|
|
||
| fun CartProductDTO.toDomain(): CartProduct { | ||
| if (!isNotNull) throw IllegalArgumentException() |
There was a problem hiding this comment.
mapper에서 Exception이 발생할것이라고 예상할수 있을까요?
| val isNotNull: Boolean | ||
| get() = orderId != null && | ||
| orderedDateTime != null && | ||
| products != null && | ||
| products.all { it?.isNotNull ?: false } && | ||
| totalPrice != null |
There was a problem hiding this comment.
gson에 대한 문제는 방지할수 있지만,
모든 dto코드에 isNotNull가 추가되고, 관리되어야하는 구조가 되었네요
There was a problem hiding this comment.
isNotNull이 아니었으면 제네릭을 활용해서 Callback을 상속받는 추상 클래스를 만들 수 있었겠네요...
하지만 서버에서 못받아와서 null인 데이터가 디폴트값이 된다면, 클라이언트에서 이 값이 null이라서 디폴트값인지 진짜 서버에서 넘어온 값인지 어떻게 판단할 수 있을까요?
null이 확실해서 판단하기 편할 것이라고 생각했는데, isNotNull 프로퍼티만 아니면 통신 코드를 줄일 수 있었겠다는 생각이 드니 고민이 됩니다.ㅜㅜ
| override fun areItemsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean { | ||
| val oldItem = oldList[oldItemPosition] | ||
| val newItem = newList[newItemPosition] | ||
| return oldItem.type == newItem.type | ||
| } | ||
|
|
||
| override fun areContentsTheSame(oldItemPosition: Int, newItemPosition: Int): Boolean { |
There was a problem hiding this comment.
areItemsTheSame, areContentsTheSame가 무엇을 의미하는지
한번더 확인해도 좋을거같아요!
| class Failure(val message: String) : DataResult<Nothing>() | ||
| object Failure : DataResult<Nothing>() | ||
| object NotSuccessfulError : DataResult<Nothing>() | ||
| object WrongResponse : DataResult<Nothing>() |
There was a problem hiding this comment.
실패의 경우에는 failure 로 한번더 묶어서 사용해도 좋을거같단생각이 들어요 😄
|
재화 미션 기간동안, 의미있는 시간이 되셧길바랍니다! |
안녕하세요, 리뷰어님! 처음 뵙겠습니다! 둘리입니다. :)
이번 미션에서 Retrofit을 사용하면서, DTO 개념을 사용했는데요.
아무래도 기존에 모델을 생성하던 것과 DTO가 섞이다보니 개념적으로 많이 헷갈려서 저 개인적으로도 조금 만족스럽지 못한 코드입니다.
질문
아키텍처 구조의 모델의 구조가 많이 헷갈리는 미션이었습니다.☺️
날카로운 피드백 부탁드립니다. 감사합니다.