[쇼핑 주문 1, 2 단계] 토바에 미션 제출합니다. #104
Conversation
- shimmer_background 색상 추가 - shimmer 효과를 위한 item_shimmer_product.xml 레이아웃 추가
메인 화면에서 데이터를 불러오는 동안 스켈레톤 UI를 표시하도록 기능을 추가했습니다. - `MainViewModel`에 로딩 상태를 관리하는 `_isLoading` LiveData를 추가했습니다. - `loadInitial` 함수 호출 시 로딩 상태를 true로 변경하고, 데이터 로드 완료 후 false로 변경합니다. - `MainActivity`에서 `isLoading` LiveData를 관찰하여 로딩 상태에 따라 스켈레톤 UI와 상품 목록 RecyclerView의 가시성을 조절합니다. - `activity_main.xml`에 ShimmerFrameLayout을 활용한 스켈레톤 UI를 추가했습니다.
- `ProductState`에 `increaseCartQuantity2` 함수를 추가하여 장바구니 수량 증가 시 서버와 동기화하도록 변경했습니다. - `MainViewModel`에서 상품 추가 시 `cartRepository.addCart`를 호출하여 서버에 장바구니 정보를 전달합니다. - `MainViewModelFactory`에서 `DefaultCartRepository`를 주입받도록 수정했습니다. - `MainActivity`에서 `ViewModelProvider.Factory`에 `DefaultCartRepository`를 전달하도록 변경했습니다. - `ProductUiState`의 `canIncreaseCartQuantity` 함수를 `increaseCartQuantity`로 변경하고, 반환 타입을 `ProductState`로 수정했습니다.
장바구니 목록 조회 API 응답을 위한 데이터 클래스들을 추가합니다. - `CartsResponse`: 페이지네이션 정보를 포함한 전체 응답 - `Content`: 장바구니 상품 정보 - `Product`: 상품 상세 정보 - `Pageable`: 페이지네이션 관련 정보 - `SortX`: 정렬 관련 정보
상품 목록을 불러올 때 장바구니에 담긴 상품의 수량 정보를 함께 가져와 UI에 반영하도록 수정했습니다. - `loadNextPage` 함수에서 상품 정보 로드 후 `loadCartAndMerge` 함수를 호출합니다. - `loadCartAndMerge` 함수는 장바구니 정보를 로드하여 `mergeProductWithCart` 함수를 통해 상품 정보와 병합합니다. - `mergeProductWithCart` 함수는 각 상품에 대해 장바구니 정보를 찾아 `ProductState`에 반영합니다. - 로딩 상태 관리 및 오류 로깅 방식을 개선했습니다.
상품 목록과 장바구니 목록을 동시에 로드하고 병합하도록 로직을 변경했습니다. - `loadNextPage` 함수를 `loadProductsAndCarts`로 변경하여 상품과 장바구니 정보를 함께 처리합니다. - `loadCartAndMerge` 함수를 `mergeProductsWithCarts`로 변경하고, 상품과 장바구니 정보를 병합하여 `ProductState` 목록을 생성합니다. - 로딩 상태 관리 및 오류 처리 방식을 개선하여 `setLoading` 및 `handleError` 함수를 사용합니다. - 장바구니 수량 증가 시, 이미 장바구니에 담긴 상품이면 수량 업데이트 API를, 아니면 추가 API를 호출하도록 수정했습니다.
장바구니에 담긴 특정 상품을 삭제하는 기능을 추가합니다.
- `CartDataSource.kt`: `android.util.Log` import 제거 - `RecommendFragment.kt`: `android.util.Log` import 제거
- 상품 조회 기록 저장 시점을 상품 상세 화면 진입 시점으로 변경했습니다. - 상품 상세 화면 종료 시 `MainActivity`에서 조회 기록을 동기화하도록 변경했습니다. - `MainActivity`의 `onSelectProduct`, `onClickHistory`에서 `saveHistory` 대신 `handleNavigateDetailEvent`를 호출하도록 수정했습니다.
… 인스턴스 생성 시점으로 이동, 불필요한 필드 제거
…RecommendScreen로네이밍 변경
app/ProductResponse.kt
Outdated
| import kotlinx.serialization.Serializable | ||
|
|
||
| @Serializable | ||
| data class ProductResponse( |
There was a problem hiding this comment.
refactor: 중복 ProductResponse 클래스 삭제
해당 부분이 실수로 중복으로 들어간 것 같습니다... ㅠㅠ 찾아서 제거하였습니다!
| @@ -1,4 +1,4 @@ | |||
| package woowacourse.shopping.data.network.entity | |||
| package woowacourse.shopping.data.network.response | |||
There was a problem hiding this comment.
Entity는 데이터베이스에서 사용되는 개체이며 테이블 구조를 매핑하고, Response는 서버로부터 받은 데이터를 담는 객체로 네트워크 통신 결과를 표현합니다.
Entity는 DB와 관련된 곳에, Response는 서버와 관련된 곳에 두는 게 좋을 것 같습니다!
MockServer로부터 응답 받는 객체의 이름이 Entity라는 이름이 어색하다고 느껴진다는 의미로 이해하여 Entity라는 접미사를 Response로 변경해주었습니다!
refactor: ProductEntity, ProductPageEntity 이름 변경
| @Serializable | ||
| data class Content( | ||
| val id: Long, | ||
| val product: Product, | ||
| val quantity: Int, | ||
| ) { |
There was a problem hiding this comment.
현재 response 패키지 하위의 products와 carts에서 각각 동일한 이름의 Content 클래스를 사용하고 있습니다.
다른 패키지에 같은 이름의 Content가 존재할 경우, import 시 충돌이 발생할 수 있습니다.
따라서 중첩 클래스로 변경하여 충돌을 방지하는 것이 좋다는 의미로 이해하여 반영하였습니다.
refactor: 응답 클래스의 Content 클래스를 중첩 클래스로 변경
| data class SortX( | ||
| val empty: Boolean, | ||
| val sorted: Boolean, | ||
| @SerialName("unsorted") | ||
| val unSorted: Boolean, | ||
| ) |
There was a problem hiding this comment.
완전히 똑같은 개념을 다른 패키지에 또 만들 이유 없습니다! 하나의 클래스를 재사용하는 방식이 좋을 것 같아, common 패키지로 공통된 클래스를 이동시켰습니다. refactor: 공통된 Pageable, SortX를 common 패키지로 이동 해당 사항을 반영한 커밋입니다!
| val content: List<Content>, | ||
| val empty: Boolean, | ||
| val first: Boolean, | ||
| val last: Boolean, | ||
| val number: Int, | ||
| val numberOfElements: Int, | ||
| val pageable: Pageable, | ||
| val size: Int, | ||
| val sort: SortX, | ||
| val totalElements: Int, | ||
| val totalPages: Int, |
There was a problem hiding this comment.
모든 응답 데이터를 사용하는 것은 아닙니다. 현재는 content와 last 필드만 사용하고 있습니다. 그래서 나머지 필드를 제거한 상태로 테스트해봤는데, 화면에 데이터가 나타나지 않고 아래와 같은 오류가 발생했습니다!
kotlinx.serialization.json.internal.JsonDecodingException: Encountered an unknown key 'totalElements' at offset 4240 at path: $
Use 'ignoreUnknownKeys = true' in 'Json {}' builder or '@JsonIgnoreUnknownKeys' annotation to ignore unknown keys.
이를 해결하는 방법은 두 가지가 있습니다.
- Json { ignoreUnknownKeys = true }로 JSON 파서를 설정하여 모든 알 수 없는 키를 무시하는 방법
- 각 응답 데이터 클래스에 @JsonIgnoreUnknownKeys 어노테이션을 붙이는 방법
초기에는 문서를 참고해 두 번째 방법인 어노테이션 방식으로 처리했습니다. 그러나 이 방법은 다음과 같은 단점이 있었습니다.
- 불필요한 필드가 있는 모든 데이터 클래스에 일일이 어노테이션을 붙여야 한다는 점
- 해당 어노테이션이 실험적 API로, @OptIn(ExperimentalSerializationApi::class)를 함께 사용해야 한다는 점
반면, 첫 번째 방법인 Json { ignoreUnknownKeys = true }는 JSON 빌드를 생성할 때 한 번만 설정하면 됩니다. 이후 해당 빌더를 사용하는 모든 곳에 공통적으로 적용됩니다. 다만, 모든 알 수 없는 키가 무시된다는 특성상 실수로 중요한 키를 누락해도 인지하지 못할 가능성은 있습니다.
하지만 어노테이션 방식이 실험적이라는 점을 고려하여, 저는 경고 없이 안정적으로 동작하는 첫 번째 방법을 선택하여 반영했습니다!
refactor: Kotlinx Serialization 설정 변경, ignoreUnknownKeys 설정을 Json 인스턴스 생성 시점으로 이동, 불필요한 필드 제거
| android:background="@color/turquoise" | ||
| android:padding="10dp" | ||
| android:gravity="center" | ||
| android:onClick="@{() -> vm.onChangeScreen()}" |
There was a problem hiding this comment.
onChangeScreen() 화면이 바뀌고 나서 실행되는 이벤트 같습니다. 하지만 실제로는 화면 변경을 요청하는 것이므로 requestNavigationToRecommendScreen로 함수명을 변경하였습니다!
refactor: CartUiEvent.NavigationToRecommendScree, requestNavigationToRecommendScreen로네이밍 변경
| historyRepository.saveHistory(productId) { | ||
| _uiEvent.postValue(MainUiEvent.NavigateToDetail(productId, state.lastSeenProductId)) | ||
| } |
There was a problem hiding this comment.
최근 본 상품을 상품 페이지에서 저장하도록 변경하였습니다!
refactor: 상품 조회 기록 저장 로직 변경 및 상품 상세 화면 종료 시 기록 동기화
| val targetIndex = targetIndex(cartId) | ||
| val targetItem = items[targetIndex] | ||
|
|
||
| val mutableItems = items.toMutableList() | ||
| mutableItems[targetIndex] = targetItem.modifyChecked(check) | ||
|
|
||
| return copy(items = mutableItems) |
There was a problem hiding this comment.
불변 컬렉션을 수정, 변경하는 상황에서 사용하게 되었던 것 같습니다... 해당 부분 불변성을 유지하도록 변경하였습니다!
refactor: MutableList로 변환했던 리스트를 불변성 유지하도록 수정
| @@ -4,27 +4,26 @@ import woowacourse.shopping.domain.Quantity | |||
| import woowacourse.shopping.domain.product.Product | |||
|
|
|||
| data class ProductState( | |||
| val cartId: Long? = null, | |||
There was a problem hiding this comment.
“동일한 상품임에도 장바구니 아이디가 달라질 수 있다는 뜻 같다"는 말은, 현재 구조가 서버와 너무 강하게 결합되어 있다는 뜻으로 이해하였습니다.
그리고 장바구니 아이디는 API 통신에는 필요하지만, UI에 보여주는 데는 불필요한 정보 같은데,
그런 정보가 UI 상태인 CartState에 들어간 건 불필요한 결합이 아닌가 싶습니다.
혹시 제가 의미를 다르게 해석한 거라면, 더 직관적으로 설명해 주실 수 있을까요?
| data class CartState( | ||
| val cartId: Long, | ||
| val name: String, | ||
| val imgUrl: String, | ||
| val price: Price, | ||
| val quantity: Quantity, | ||
| val checked: Boolean, | ||
| ) { |
There was a problem hiding this comment.
페어와 함께 이야기해본 결과, 지금처럼 UI 상태에 로직이 몰리는 구조는 결국 도메인 모델의 역할을 약하게 만들고 있다는 점에 동의하게 되었습니다.
우리가 만들고 있는 것은 단순한 UI가 아니라 ‘장바구니’라는 도메인 개념과 그 안의 비즈니스 규칙이기 때문에, 도메인 모델에 책임을 두고 UI는 그것을 표현하는 역할로 분리하는 게 맞다는 결론을 내렸습니다.
레벨1에서 배운 도메인 설계나 책임 분리 같은 기초가 다시 중요하게 느껴졌고, 앞으로도 UI 상태에 로직을 넣기보단 도메인 모델을 먼저 중심에 놓는 방향으로 가야겠다고 생각했습니다!
해당 부분을 리팩토링한 커밋이 2단계로 함께 묶여 있었던 것 같습니다...!
refactor: CartState, CartUiState, ShoppingCart 및 Quantity 모델 개선
| <TextView | ||
| android:id="@+id/textView_product_price" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginStart="16dp" | ||
| android:layout_marginTop="6dp" | ||
| android:layout_marginEnd="16dp" | ||
| android:layout_marginBottom="16dp" | ||
| android:text="@{@string/format_thousand_won(model.item.priceValue)}" | ||
| android:textColor="@color/black" | ||
| android:textSize="16sp" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toBottomOf="@id/textView_product_name" | ||
| tools:text="10,000원" /> |
There was a problem hiding this comment.
item_recommend에 대한 UI를 그릴 때 가로 크기를 어디까지 그리는게 의도일까요?
지금의 TextView는 wrap_content이기 때문에 계속해서 늘어날 수가 있어요.
만약 컨테이너 크기를 최대 154까지 하고 싶은 것이라면, ConstraintLayout의 너비가 고정되고 각 자식 View는 match_parent로 그릴 수 있을 것 같아요.
| @@ -192,6 +209,33 @@ class MainViewModel( | |||
| setLoading(false) | |||
| } | |||
|
|
|||
| val productEventHandler = | |||
There was a problem hiding this comment.
구현하다 실수로 가장 아래에 위치된 것 같은데, Handler를 정의하는 것은 뷰모델이 아닌 Activity(UI)가 할 일로 보여요.
| val recommendEventHandler = | ||
| object : RecommendAdapter.Handler { | ||
| override fun showQuantity(productId: Long) { | ||
| increaseRecommendProductQuantity(productId) | ||
| } | ||
| } |
There was a problem hiding this comment.
실수인줄 알았는데.. 전부 이 패턴으로 작성을 해주셨네요 😓
이벤트 핸들러를 이곳에, 그리고 뷰모델이 구현한 이유는 무엇일까요?
| val binding = FragmentRecommendBinding.bind(view) | ||
| setUpBinding(binding) | ||
| setUpRecyclerView(binding) | ||
| viewModel.loadRecommendProduct() |
| @@ -73,10 +76,11 @@ class CartActivity : AppCompatActivity() { | |||
| when (supportFragmentManager.findFragmentById(R.id.fragment_container_view)) { | |||
| is CartListFragment -> { | |||
| supportFragmentManager.commit { | |||
| replace(R.id.fragment_container_view, OrderCompleteFragment()) | |||
| replace(R.id.fragment_container_view, RecommendFragment()) | |||
There was a problem hiding this comment.
현재 어느 상태에 있고 다음으로 어느 화면으로 이동해야 할지 판단하는 것은 Fragment를 꺼내와서 비교하는 것이 아닌, 뷰모델에서 상태를 판단하여 결정을 해야 하지 않을까요?
안녕하세요, 페로로!
레벨 1 블랙잭에 이어서 레벨 2에서 다시 만나 뵙게 되어서 영광입니다. 🙇♀️
부족하지만 많이 배워가도록 하겠습니다...!
시간이 부족하여 미완성으로 제출하게 되었습니다. 😭
다음 리뷰 요청 때, 구현하지 못한 부분과 테스트 코드도 함께 반영하여 제출하도록 하겠습니다.
현재 테스트 코드 깨지는 곳이 많습니다. 구현을 우선시 하다 보니, 테스트 코드를 챙기지 못했습니다. 죄송합니다 🥲
2단계의 프로그래밍 요구사항 중 "서버 통신을 위한 JSON 직렬화 라이브러리를 선택하고 PR에 선택 이유를 남긴다."라는 부분이 있습니다.
저희는 Kotlinx.serialization을 선택했습니다. Jetbrains에서 만든 라이브러리이며, 코틀린의 확장 라이브러리라는 점에서 신뢰가 높다고 판단하였습니다. 또한 gson은 reflection 기반으로 런타임에 작동하지만, Kotlinx.serialization는 컴파일 타임에 serialization 코드 생성하기 때문에 속도가 빠르고 메모리 사용이 적습니다. Kotlinx.serialization는 java의 reflection에 의존하지 않아서 jvm 말고도 kotlin multiplatform에서 사용이 가능하기 때문에 선택했습니다.
페어의 코드를 기반으로 해당 미션을 진행했습니다.
쇼핑 카트 미션 마이그레이션 코드를 제외한 커밋들의 Files Changed링크 함께 첨부합니다! 리뷰 잘 부탁드립니다! 🙇♀️
쇼핑 주문 1단계 커밋
쇼핑 주문 2단계 커밋
셀프 체크리스트
README.md에 기능 목록을 정리하고 명확히 기술하였는가?스크린샷
default.mp4