[둘리] 3, 4단계 쇼핑 장바구니 제출합니다. #33
Conversation
# Conflicts: # app/src/main/java/woowacourse/shopping/view/productlist/ProductListActivity.kt
| @@ -1,3 +1,3 @@ | |||
| package woowacourse.shopping.model | |||
|
|
|||
| data class CartProductModel(val id: Int, val name: String, val imageUrl: String, val count: Int, val totalPrice: Int) | |||
| data class CartProductModel(var isChecked: Boolean, val id: Int, val name: String, val imageUrl: String, var count: Int, val price: Int) | |||
There was a problem hiding this comment.
data class에서 isChecked 변수를 variable로 정의하셨네요.
var로 선언하게되면 외부에서 isChecked에 접근하여 얼마든지 상태를 변경할 수 있기 때문에, isChecked 를 참조하고 있는 곳에 예상치 못한 동작이 발생할 수 있어요.
외부에서 변경하지 못하도록 개선해보면 어떨까요?
There was a problem hiding this comment.
valuable로 수정하고 프레젠터에서 리스트의 아이템을 복사해서 다시 넣어주도록 수정했습니다!
| private fun setUpMockServer() { | ||
| val thread = Thread { mockWebServer = MockServer() } | ||
| thread.start() | ||
| thread.join() | ||
| } |
There was a problem hiding this comment.
MockWebServer를 초기화하는 코드를 잘 구현해주셨네요. 👍
다만, WebServer를 제어하는 역할이 View에서 하는 것이 적절할까요?
MVP 패턴에서의 View의 역할에 대해서 다시 한번 고민해보세요. (이하 관련 내용 동일)
There was a problem hiding this comment.
MockWebServer를 구동시키는 것이 View의 역할이 아닌 것 같다고 생각하고 있었으나, Presenter에게 MockWebServer의 url을 넘겨주어야해서 어떻게 수정해야할지 잘 모르겠습니다.
실제 서버라면 Server를 켤 필요 없이 서버의 url을 넘겨주면 될텐데요...
이런 경우에는 어떻게 분리해야할까요..?
There was a problem hiding this comment.
MockWebServer의 url이 아닌 MockWebServer를 넘기도록 구현해도 되지 않을까요?
There was a problem hiding this comment.
지금은 MockWebServer을 사용하니까 그래도 되겠네요!
| override val cartSystemResult: LiveData<CartSystemResult> | ||
| get() = _cartSystemResult | ||
| override val cartPageStatus: LiveData<CartPageStatus> | ||
| get() = _cartPageStatus | ||
| override val isCheckedAll: LiveData<Boolean> | ||
| get() = _isCheckedAll |
There was a problem hiding this comment.
라이브데이터는 흔히 backing property 기법을 사용합니다.
backing property는 관련 변수끼리 같은 곳에 위치시키는 것이 좋습니다.
| // 남은 자리 페이지 뒷 상품으로 다시 채우기 | ||
| if (nextItemExist) addNextProduct() | ||
| view.showChangedItems() |
There was a problem hiding this comment.
주석으로 이 로직이 무엇을 하는지 잘 기재해주셔서 이해하기가 수월하네요!
이렇게 명확하게 어떤 일을 하는지 알 수 있다면, 함수로 분리해보면 어떨까요?
There was a problem hiding this comment.
addNextProduct로 이미 한번 분리하여 한 줄로 작성한 코드라 더 분리할 방법을 잘 모르겠습니다!
There was a problem hiding this comment.
private fun fillProduct(nextItemExist: Boolean) {
// 남은 자리 페이지 뒷 상품으로 다시 채우기
if (nextItemExist) addNextProduct()
view.showChangedItems()
}간단하게 이렇게 작성할 수 있지 않을까요?
There was a problem hiding this comment.
함수에서 boolean값을 받아와서 처리할 수 있겠군요!
다음 미션 코드에서 적용하겠습니다!!ㅎㅎ
| private lateinit var productDetailBinding: ActivityProductDetailBinding | ||
| private lateinit var dialogBinding: DialogCountBinding | ||
| private lateinit var dialog: AlertDialog |
There was a problem hiding this comment.
lateinit var를 잘 활용해주셨네요. 👍
lateinit var도 좋지만 by lazy를 활용해볼수 있을 것 같아요.
두 지연초기화의 차이점에 대해서 학습해보시고 개선해보면 어떨까요? (이하 관련 내용 동일)
There was a problem hiding this comment.
lateinit var는 말그대로 나중에 초기화해주는 것이고 null이 가능하다는 점,
by lazy는 그 변수를 사용할 때 {}내부 구문을 실행하여 사용한다는 점에서 차이가 있습니다.
차이에 대해서는 알고있으나 언제 lateinit var을 사용하고 by lazy를 사용해야하는지 잘 모르겠습니다!
되도록 by lazy로 고쳐두었으나 ProductListActivity의 cartCountInAppBar는 초기화 로직이 단순하지 않다고 판단하여 lateinit var로 유지했습니다!
There was a problem hiding this comment.
차이점을 잘 설명해주셨네요. 👍
말씀하신 차이점 외에 가장 큰 차이점이 있다면, lateinit var는 지연초기화 이후 값을 얼마든지 새로운 값으로 덮어쓸 수 있습니다.
반면에 by lazy는 한번 초기화가 되면, 그 이후에는 값을 변경할 수 없습니다.
그렇기 때문에 가능하다면 by lazy를 활용하는 것이 더 좋습니다. 😄
There was a problem hiding this comment.
값을 변경할 수 있느냐, 없느냐로 판단할 수도 있겠네요..! 감사합니다 ㅎㅎ
| } | ||
|
|
||
| interface Presenter { | ||
| val count: LiveData<Int> |
There was a problem hiding this comment.
count를 LiveData 타입으로 정의하셨네요. 👍
count를 Presenter 인터페이스에 꼭 정의할 필요가 있을까요?
There was a problem hiding this comment.
액티비티에서는 추상화된 presenter를 타입으로 받고 있는데 인터페이스에 정의하지 않더라도 count를 사용할 수 있나요?
xml에 LiveData type의 count variable을 선언해주고 프레젠터에서 LiveData타입의 count를 넘기도록 한 후, TextView에서 쓰도록 수정했는데, error: cannot find symbol 에러가 나왔습니다.ㅠㅠ
override fun setUpDialogBinding(product: ProductModel, count: LiveData<Int>) {
dialogBinding.lifecycleOwner = this
dialogBinding.presenter = presenter
dialogBinding.product = product
dialogBinding.count = count
}There was a problem hiding this comment.
지금 구조에서는 Presenter에서 LiveData를 쓰다보니 인터페이스에 꼭 정의를 해야겠군요. 😅
Presenter에서의 LiveData는 익숙지 않네요.
There was a problem hiding this comment.
MVVM 구조를 배운다면 이런 경우가 발생하지 않을까요?
LiveData를 사용하면서 어쩔 수 없이 구조가 이렇게 되는데, 이 구조는 MVP가 아닌 것 같다는 생각이 들면서도 방법이 영 떠오르지 않았습니다...
아키텍처란 어렵네요🥲
| binding.btnAdd.setOnClickListener { | ||
| onItemClick.onProductClickAddFirst(item.product.id, 1) | ||
| binding.textCount.text = "1" | ||
| binding.btnAdd.visibility = View.INVISIBLE | ||
| binding.layoutAddBtns.visibility = View.VISIBLE | ||
| } | ||
| binding.btnPlus.setOnClickListener { | ||
| onItemClick.onProductUpdateCount(item.product.id, Integer.parseInt(binding.textCount.text.toString()) + 1) | ||
| binding.textCount.text = (Integer.parseInt(binding.textCount.text.toString()) + 1).toString() | ||
| } | ||
| binding.btnMinus.setOnClickListener { | ||
| onItemClick.onProductUpdateCount(item.product.id, Integer.parseInt(binding.textCount.text.toString()) - 1) | ||
| binding.textCount.text = (Integer.parseInt(binding.textCount.text.toString()) - 1).toString() | ||
| } |
There was a problem hiding this comment.
bind 함수에서 클릭리스너를 정의해주셨네요.
다만, 어떤 클릭리스너는 init 블록에서 정의하고, 어떤 클릭리스너는 bind 함수에 정의하고 있네요.
클릭리스너는 ViewHolder 생성 시 한 번 만 등록하도록 개선해보면 어떨까요?
There was a problem hiding this comment.
생성자에서 onPlusClick, onMinusClick 람다로 묶어 Function1 타입으로 넘겨주었습니다!
xml 코드에서 invoke를 통해 함수를 실행하였습니다.
| tools:text="10,000원" /> | ||
|
|
||
|
|
||
| <androidx.appcompat.widget.AppCompatButton |
| override fun find(id: Int): CartProduct? { | ||
| return cartProducts.find { it.id == id } | ||
| } |
There was a problem hiding this comment.
테스트 코드에는 가급적이면 로직을 정의하지 않는 것이 좋아요.
만약 실제 코드에서 find 함수의 구현부가 변경된다면, 이 테스트코드는 어떻게 될까요? (이하 관련 내용 동일)
There was a problem hiding this comment.
최대한 단순하게 변경해보았는데 이게 맞는지 잘 모르겠습니다 ㅎㅎ...
로직을 지우고 테스트를 진행하니 원활하게 작동 안되는 부분이 있어서 몇가지를 제외하고는 삭제했습니다!
| } | ||
|
|
||
| @Test | ||
| fun 업데이트_하려는_상품의_개수가_1부터_100사이면_업데이트할_수_있다() { |
There was a problem hiding this comment.
요구사항에 1부터 100사이까지만 업데이트할 수 있다는 부분은 없는데, 이 요구사항은 무엇인가요?
There was a problem hiding this comment.
최대를 정해두어야할 것 같아 임의로 100으로 정했는데 명세에 없는 내용이므로 삭제했습니다!
|
앱을 구동해보면 아래와 같은 버그가 존재해요.
|
A. Presenter에서는 대체로 View의 함수 실행 여부에 대한 테스트를 작성합니다. |
|
안녕하세요, 리뷰어님! 최근 미션을 진행하느라 수정이 늦었습니다. |
| if (mark < 0) return false | ||
| return true |
There was a problem hiding this comment.
| if (mark < 0) return false | |
| return true | |
| return mark > -1 |
이렇게 하셔도 됩니다만, 이 부분도 로직이기 때문에 없는 것이 좋긴 합니다. 😅
A. 의도하고 기능을 구현한 점은 매우 좋습니다! 👍 |
A. 이전 화면으로 돌아오는 경우가 좌상단의 "<-" 버튼이 아닌, 백버튼을 눌렀을 때입니다. 😅 |
안녕하세요 리뷰어님! 이번 미션은 시간이 조금 걸렸네요🥲
최대한 데이터, 뷰 바인딩을 사용하여 액티비티의 코드를 간략하게 작성하고, 프레젠터의 코드를 나눠보려고 노력했습니다.
그리고 이번 3,4단계에 와서 프레젠터를 수정하고 테스트를 진행해보니 테스트가 어려운 함수가 있는 것 같다고 느꼈습니다.
view의 함수를 실행하는지 안하는지 검사할 수 밖에 없는 코드가 있는데, 제가 코드를 더 좋게 짤 수 있는 부분인지 이게 최선인지 잘 모르겠습니다.ㅠㅠ
이번 리뷰도 잘 부탁드리고 날카로운 피드백 부탁드립니다!!