[쇼핑 주문 1, 2 단계] 시아 미션 제출합니다. #113
Conversation
- 장바구니 리포지토리에서 아이템 fetch시 CartResponse를 콜백하도록 수정
|
|
||
| </com.facebook.shimmer.ShimmerFrameLayout> | ||
|
|
||
| <com.facebook.shimmer.ShimmerFrameLayout |
| private val goods = mutableListOf<Goods>() | ||
| private var page: Int = 1 | ||
| private val _isFullLoaded = MutableLiveData(false) | ||
| val isFullLoaded: LiveData<Boolean> get() = _isFullLoaded | ||
| private val _goodsWithCartQuantity = MutableLiveData<List<CartItem>>() | ||
| val goodsWithCartQuantity: LiveData<List<CartItem>> get() = _goodsWithCartQuantity | ||
| private var _totalCartItemSize: MutableLiveData<String> = MutableLiveData("0") | ||
| val totalCartItemSize: LiveData<String> get() = _totalCartItemSize | ||
| private val _navigateToCart = MutableSingleLiveData<Unit>() | ||
| val navigateToCart: SingleLiveData<Unit> get() = _navigateToCart | ||
| private val _recentlyViewedGoods: MutableLiveData<List<Goods>> = MutableLiveData() | ||
| val recentlyViewedGoods: LiveData<List<Goods>> get() = _recentlyViewedGoods |
| onFail = { throwable -> | ||
| throw (throwable) | ||
| }, |
There was a problem hiding this comment.
이 상태라면 예외가 발생했을 때 앱이 터지지 않을까요?
이는 사용자 경험 측면에서 가장 좋지 않은 케이스입니다!
There was a problem hiding this comment.
a4317fc
차후에 수정한다고 하고 수정을 못했었네요 ㅠㅠ
| private fun navigateGoodsDetails(goods: Goods) { | ||
| val intent = GoodsDetailsActivity.newIntent(this, goods.toUi()) | ||
| intent.putExtra(EXTRA_SOURCE, SOURCE_GOODS_LIST) | ||
| intent.putExtra(CART_KEY, viewModel.findCart(goods)?.toUiModel()) |
There was a problem hiding this comment.
상품 정보 페이지로 넘어가는데 장바구니에서 아이템을 찾는 이유는 무엇인가요?
There was a problem hiding this comment.
장바구니에서 아이템을 찾아서, 해당 아이템이 있으면 차후에 장바구니 추가 시에 그 아이템을 갱신하도록 만들기 위해 찾게 했습니다!
| fun login(basicKey: String) { | ||
| Authorization.setBasicKey(basicKey) | ||
| cartRepository.checkValidBasicKey(basicKey, { response -> | ||
| when { | ||
| response == 200 -> Authorization.setLoginStatus(true) | ||
| else -> Authorization.setLoginStatus(false) | ||
| } | ||
| }, {}) | ||
| } | ||
|
|
||
| fun onCartClicked() { | ||
| if (!Authorization.isLogin) { | ||
| _navigateToLogin.setValue(Unit) | ||
| } else { | ||
| _navigateToCart.setValue(Unit) | ||
| } | ||
| } |
There was a problem hiding this comment.
로그인이라는 기능을 넣은 것은 좋은 전략이지만 덕분에 고민해야 할 것들이 늘어버려 정작 중요한 것을 놓친것만 같은 기분이네요 😭
| goodsRepository.fetchCategoryGoods( | ||
| 10, | ||
| goods.category, | ||
| { goodsResponse -> | ||
| val allGoodsList = goodsResponse.content.map { CartItem(it.toDomain(), 0) } | ||
|
|
||
| filterRecommendedGoods(allGoodsList) | ||
| }, | ||
| { | ||
| loadDefaultRecommendedGoods() | ||
| }, |
There was a problem hiding this comment.
여러 인자가 넘어가는 경우 named argument를 추가해 가독성을 높여볼 수도 있겠습니다.
| fun selectAllItemsFromServer() { | ||
| cartRepository.fetchAllCartItems({ allItems -> | ||
| selectedCartMap.clear() | ||
| allItems.toCartItems().forEach { selectedCartMap[it.id] = it } | ||
| _isAllSelected.value = true | ||
| updateTotalPriceAndCount() | ||
| }, {}) | ||
| } |
There was a problem hiding this comment.
이 함수는 어떤 일을 하길래 selectAllItemsFromServer이런 이름이 붙게 되었을까요?
There was a problem hiding this comment.
현제 페이지 뿐만 아니라 모든 페이지의 아이템을 일괄적으로 선택하는 함수입니다!
There was a problem hiding this comment.
뷰모델은 Repository를 통해 데이터를 가져오고 있습니다.
API를 사용하는지, Local DB를 사용하는지 뷰모델은 알 길이 없죠.
FromServer라는 네이밍은 더 적절한 것으로 변경해보면 좋을 것 같아요!
| object ApiEndpoints { | ||
| const val PRODUCTS = "products" | ||
| } |
| interface RetrofitService { | ||
| @GET("/products") | ||
| fun requestProducts( | ||
| @Header("accept") accept: String = "*/*", |
There was a problem hiding this comment.
만약 공통으로 사용되는 필수 Header가 있다면 OkHttpInterceptor를 활용해볼 수도 있겠습니다!
There was a problem hiding this comment.
bd1cca9
인터셉터는 처음 써보는데, 도움이 많이 되었습니다!
| data class CartResponse( | ||
| @SerializedName("content") | ||
| val content: List<CartContent>, | ||
| @SerializedName("empty") | ||
| val empty: Boolean, | ||
| @SerializedName("first") | ||
| val first: Boolean, | ||
| @SerializedName("last") | ||
| val last: Boolean, | ||
| @SerializedName("number") | ||
| val number: Int, | ||
| @SerializedName("numberOfElements") | ||
| val numberOfElements: Int, | ||
| @SerializedName("pageable") | ||
| val pageable: PageableX, | ||
| @SerializedName("size") | ||
| val size: Int, | ||
| @SerializedName("sort") | ||
| val sort: SortXX, | ||
| @SerializedName("totalElements") | ||
| val totalElements: Int, | ||
| @SerializedName("totalPages") | ||
| val totalPages: Int, | ||
| ) |
There was a problem hiding this comment.
플러그인을 통해 자동으로 생성한 객체를 사용하다 보니, 모든 정보를 가져오도록 긁어오게 되었습니다. 또한 임의로 필드를 삭제 하면 JSON 에서 객체로 변환하는 과정에서 문제가 생길 것 같아 유지하였습니다.
There was a problem hiding this comment.
필드를 삭제하면 정말 문제가 생길까요?
만약 서버에서 내려주지 않으면 어떻게 될지 고민해봐도 좋겠어요!
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
Walkthrough이 변경 사항은 안드로이드 쇼핑 카트 애플리케이션의 신규 기능 개발, 데이터 계층, UI, 테스트, 빌드 구성 등 전반적인 구조를 대대적으로 추가 및 개편한 것입니다. 주요 내용은 다음과 같습니다: Room 기반 로컬 DB 및 DAO, 레포지토리, 원격 데이터 소스, Retrofit 서비스, 데이터 매퍼, 도메인 및 UI 모델, ViewModel과 팩토리, 다양한 Fragment 및 Activity, RecyclerView 어댑터, Data Binding 어댑터, 싱글 이벤트 LiveData, 테스트 픽스처와 페이크 레포지토리, LiveData 유틸, 각종 리소스(XML 레이아웃, 드로어블 등), 그리고 광범위한 UI 및 ViewModel 테스트 코드가 신규로 추가되었습니다. 또한, 빌드 스크립트와 매니페스트, README 문서도 프로젝트 구조와 기능에 맞게 전면 수정 및 확장되었습니다. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 72
♻️ Duplicate comments (21)
app/src/main/java/woowacourse/shopping/data/util/ApiEndpoints.kt (1)
3-5: 이전 리뷰 코멘트를 확인해보세요.이전 리뷰에서
PRODUCTS상수가 사용되지 않는다는 지적이 있었습니다.코드베이스에서 이 상수가 실제로 어떻게 사용되고 있는지 확인해보는 것은 어떨까요? 다음과 같은 질문들을 스스로에게 해보세요:
- 이 상수가 정말 필요한가요?
- 현재 사용되지 않는다면, 향후 사용 계획이 있나요?
- 사용되지 않는 코드를 유지하는 것의 장단점은 무엇인가요?
프로젝트에서 API 엔드포인트를 어떻게 관리하고 있는지 다시 살펴보고, 이 접근법이 정말 필요한지 판단해보세요.
app/src/main/java/woowacourse/shopping/data/carts/dto/CartResponse.kt (1)
5-28: DTO 설계 원칙에 대해 생각해보세요이전 리뷰어가 "모든 정보가 필요한가요?"라는 좋은 질문을 했습니다. 이를 바탕으로 다음을 고려해보세요:
- 필요성 검토: 현재 앱에서 실제로 사용하는 필드는 어떤 것들인가요?
- 성능 영향: 불필요한 데이터를 파싱하고 메모리에 유지하는 비용은 얼마나 될까요?
- 유지보수성: API가 변경될 때 영향받는 필드들을 어떻게 관리할 건가요?
다음 질문들도 생각해보세요:
- DTO와 도메인 모델을 분리하는 이유는 무엇인가요?
- "필요한 만큼만" 파싱하는 방법이 있을까요?
app/src/main/java/woowacourse/shopping/feature/goods/GoodsViewModel.kt (5)
53-61: 로그인 로직을 별도의 ViewModel로 분리하는 것을 고려해보세요.GoodsViewModel에 로그인 기능을 포함시킨 것은 단일 책임 원칙(SRP)에 위배됩니다. LoginViewModel을 별도로 만들어 관심사를 분리하는 것이 어떨까요?
105-117: 메서드 순서를 호출 순서에 맞게 재배열하세요.코드의 가독성을 위해
setTotalCartItemSize와bindCartCache메서드의 순서를 호출되는 순서대로 배치하는 것이 좋습니다.
31-37: 이벤트 관리를 위해 sealed class 활용을 고려해보세요.여러 개의 SingleLiveData 대신 sealed class를 사용하여 하나의 LiveData로 이벤트를 관리하면 더 깔끔한 구조가 될 수 있습니다.
예시:
sealed class GoodsEvent { object NavigateToCart : GoodsEvent() object NavigateToLogin : GoodsEvent() }
83-85:⚠️ Potential issue예외 처리를 개선하세요.
예외를 그대로 throw하면 앱이 크래시될 수 있습니다. 사용자 경험을 위해 적절한 에러 처리가 필요합니다.
에러 상태를 LiveData로 관리하거나, 사용자에게 적절한 피드백을 제공하는 방법을 고려해보세요.
173-189:⚠️ Potential issue실패 콜백에서 적절한 에러 처리를 구현하세요.
현재 에러 콜백이 비어있어 실패 시 아무런 처리가 되지 않습니다. 사용자에게 실패를 알리거나 재시도 로직을 추가하는 것이 필요합니다.
최소한 로그를 남기거나 에러 상태를 UI에 전달할 수 있는 방법을 구현해보세요.
app/src/main/java/woowacourse/shopping/data/util/RetrofitService.kt (1)
21-21: 공통 헤더 처리 방식에 대해 고민해보세요.이전 리뷰에서 언급된
OkHttpInterceptor활용 제안을 다시 생각해보세요.accept헤더가 모든 메서드에서 반복되고 있는데, 이를 어떻게 개선할 수 있을까요?app/src/main/java/woowacourse/shopping/feature/cart/CartFragment.kt (1)
165-168: 바인딩 null 처리를 잘 해주셨습니다.Fragment 생명주기에 맞춰 바인딩을 올바르게 해제하고 있습니다.
app/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsActivity.kt (2)
78-78: FLAG_ACTIVITY_CLEAR_TOP 사용 목적을 문서화해주세요.이 플래그가 최근 본 상품 네비게이션에서 어떤 역할을 하는지 코멘트로 설명해주시면 코드의 의도가 더 명확해집니다.
// 최근 본 상품에서 다른 상품 클릭 시 기존 스택을 정리하고 새로운 상품 화면으로 이동 intent.flags = Intent.FLAG_ACTIVITY_CLEAR_TOP
29-54: 🛠️ Refactor suggestiononCreate 메서드가 너무 많은 책임을 가지고 있습니다.
현재 onCreate에서 뷰 바인딩, 인텐트 파싱, 의존성 주입, 뷰모델 초기화를 모두 처리하고 있습니다. 이를 작은 메서드들로 분해하면 가독성과 테스트 가능성이 향상됩니다.
메서드를 더 작은 단위로 분해해보세요:
override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) setupBinding() val (goodsUiModel, cartUiModel) = extractIntentData() ?: return setupViewModel(goodsUiModel, cartUiModel) setupObservers() } private fun extractIntentData(): Pair<GoodsUiModel, CartUiModel?>? { val goodsUiModel = IntentCompat.getParcelableExtra(intent, GOODS_KEY, GoodsUiModel::class.java) ?: return null val cartUiModel = IntentCompat.getParcelableExtra(intent, CART_KEY, CartUiModel::class.java) return goodsUiModel to cartUiModel } private fun setupViewModel(goodsUiModel: GoodsUiModel, cartUiModel: CartUiModel?) { // ViewModel 초기화 로직 }app/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModel.kt (5)
3-3: 디버그용 Log import가 남아있습니다과거 리뷰에서 언급되었듯이 로그들을 정리해야 합니다. 프로덕션 코드에서는 불필요한 Log import를 제거하는 것이 좋습니다.
62-62: 디버그 로그들을 정리해주세요시간이 부족했더라도 프로덕션 코드에 디버그 로그가 남아있으면 코드 품질에 영향을 줍니다. 이런 로그들은 어떤 목적으로 사용되었는지 생각해보고 정리해보세요.
Also applies to: 67-67
31-31: 빈 콜백들이 필요한지 검토해보세요여러 곳에서 빈 콜백
{}을 사용하고 있습니다. 이런 콜백이 정말 필요한지, 아니면 API 설계를 개선할 수 있는지 생각해보세요. 특히 에러 처리가 필요한 부분에서 빈 콜백을 사용하는 것이 적절한지 고민해보시기 바랍니다.Also applies to: 68-68, 92-92
76-81: 🛠️ Refactor suggestionViewModel에서 UI 메시지를 생성하는 것이 적절한지 고민해보세요
ViewModel이 R.string 리소스를 직접 참조하여 UI 메시지를 만들고 있습니다. 이런 방식이 ViewModel의 책임과 맞는지 생각해보시고, 다른 방법은 없을지 고민해보세요.
85-85: 🛠️ Refactor suggestion함수명을 능동적으로 변경해보세요
onClickMostRecentlyGoodsSection처럼onXxx형태의 함수명은 수동적인 의미입니다. ViewModel은 명령을 내리는 능동적인 객체인데, 이런 네이밍이 ViewModel의 역할과 맞는지 생각해보시고 더 적절한 이름을 고려해보세요.app/src/main/java/woowacourse/shopping/feature/goods/GoodsActivity.kt (1)
165-170: 장바구니 정보 전달 로직에 대한 설명기존 리뷰어님의 질문에 대해: 상품 상세 화면으로 이동할 때 해당 상품의 장바구니 정보를 함께 전달하는 것은 상세 화면에서 현재 장바구니에 담긴 수량을 표시하기 위한 것으로 보입니다.
이런 의도라면 코드에 주석을 추가하여 목적을 명확히 하면 좋겠습니다. 다만, 이 정보를 Intent로 전달하는 대신 상세 화면에서 직접 조회하는 방식도 고려해볼 수 있습니다.
app/src/main/java/woowacourse/shopping/feature/cart/CartViewModel.kt (4)
38-44: 중복된 프로퍼티 정리 필요
leftPageEnable과isLeftPageEnable,rightPageEnable과isRightPageEnable이 동일한 backing field를 참조하고 있습니다. 하나로 통일하는 것이 좋겠습니다.
46-46: 사용되지 않는 프로퍼티 제거
endPage프로퍼티가 정의되어 있지만 실제로 사용되지 않습니다. 제거를 고려해보세요.
73-83: Named argument로 가독성 향상여러 인자가 전달되는 경우 named argument를 사용하면 가독성이 향상됩니다.
- goodsRepository.fetchCategoryGoods( - 10, - goods.category, - { goodsResponse -> + goodsRepository.fetchCategoryGoods( + size = 10, + category = goods.category, + onSuccess = { goodsResponse -> val allGoodsList = goodsResponse.content.map { CartItem(it.toDomain(), 0) } filterRecommendedGoods(allGoodsList) }, - { + onError = { loadDefaultRecommendedGoods() }, )
230-237: 혼란스러운 함수명
selectAllItemsFromServer라는 이름은 서버에서 선택한다는 의미로 오해될 수 있습니다. 실제로는 서버에서 모든 아이템을 가져와 로컬에서 선택하는 것이므로 더 명확한 이름이 필요합니다.- fun selectAllItemsFromServer() { + fun fetchAndSelectAllItems() { cartRepository.fetchAllCartItems({ allItems -> selectedCartMap.clear() allItems.toCartItems().forEach { selectedCartMap[it.id] = it } _isAllSelected.value = true updateTotalPriceAndCount() }, {}) }
🧹 Nitpick comments (103)
app/src/main/res/drawable/ic_close_white.xml (1)
1-5: 코드 포맷팅과 컬러 리소스 사용을 개선해보세요.현재 한 줄로 작성된 XML은 가독성이 떨어집니다. 또한 하드코딩된 색상값 대신 컬러 리소스를 사용하는 것이 더 좋은 방법입니다.
XML 포맷팅을 개선하고 컬러 리소스 사용을 고려해보는 것은 어떨까요? 예를 들어,
android:tint="#FFFFFF"대신android:tint="@android:color/white"또는 별도의 컬러 리소스를 사용하면 일관성과 유지보수성이 향상됩니다.어떤 방법으로 XML 구조를 더 명확하게 만들 수 있을지 생각해보세요.
app/src/main/res/drawable/ic_close_gray.xml (1)
1-5: 일관성 있는 아이콘 리소스 관리를 고려해보세요.이 아이콘은
ic_close_white.xml과 비슷한 구조를 가지고 있네요. 두 파일을 비교해보면서 다음을 생각해보세요:
- 포맷팅의 일관성: 두 아이콘 파일이 같은 형식을 사용하고 있나요?
- 컬러 관리:
#AAAAAA같은 하드코딩된 색상값을 어떻게 개선할 수 있을까요?- 코드 중복: 같은 path 데이터를 사용하는 아이콘들을 어떻게 더 효율적으로 관리할 수 있을까요?
Android에서 벡터 drawable을 관리하는 모범사례에 대해 찾아보고, 프로젝트에 어떻게 적용할 수 있는지 생각해보세요.
app/src/main/res/drawable/ic_shopping_cart.xml (1)
1-5: 색상 정의 최적화 제안
현재android:tint="#FFFFFF"과android:fillColor="@android:color/white"으로 하드코딩된 흰색을 사용하고 있는데, 재사용성과 테마 일관성을 위해 색상 리소스나 테마 어트리뷰트(?attr/colorOnSurface등)로 분리하는 것을 고려해보세요.app/src/main/java/woowacourse/shopping/data/carts/dto/CartItemRequest.kt (1)
1-6: DTO 필드 매핑 검토
서버 API 스펙에서 JSON 필드 네이밍이productId/quantity와 일치하는지 확인하셨나요? snake_case 로 전달된다면 GSON 의@SerializedName("product_id")등을 추가해주시는 것이 좋습니다.
또한quantity가 음수로 들어올 가능성은 없는지, 비즈니스 요구사항에 맞게 유효성 검증 필요성을 검토해보시면 좋겠습니다.app/src/main/res/drawable/skeleton_card_background.xml (1)
1-6: 코너 반경 재사용성 향상 제안
android:radius="8dp"가 하드코딩 되어 있는데, 여러 곳에서 같은 값을 사용한다면dimens.xml에 정의하여 재사용성과 일관성을 높이는 방안을 고려해보세요.app/src/main/res/drawable/cart_item_background.xml (1)
1-12: corners 속성 간소화 가능
모든 코너가 같은 반경(4dp)을 사용하고 있으므로<corners android:radius="4dp" />하나로 정의를 간소화할 수 있습니다.app/src/main/java/woowacourse/shopping/domain/model/Goods.kt (1)
1-9: 도메인 객체 유효성 검증 고려
price에 음수 값이 들어올 가능성은 없나요? 도메인 모델 생성 시require(price >= 0)같은 간단한 검증을 추가하여 비즈니스 규칙을 확실히 지키는 방법을 검토해보시면 좋겠습니다.app/src/main/java/woowacourse/shopping/data/util/mapper/toDomain.kt (1)
1-1: 파일명 검사 억제 대신 파일명을 확장 함수에 맞춰보시겠어요?
현재@file:Suppress("ktlint:standard:filename")로 파일명 검사 경고를 억제하고 있는데, 확장 함수명과 일치하도록ContentExtensions.kt등으로 파일명을 변경하면 suppression 없이도 깔끔하게 관리할 수 있습니다.app/src/main/java/woowacourse/shopping/data/carts/dto/CartQuantity.kt (1)
6-7:SerializedName어노테이션 중복 검토
프로퍼티명이 JSON 키와 동일하므로@SerializedName("quantity")를 생략해도 무방합니다. 실제로 제거해도 직렬화 결과가 동일한지 확인해 보시겠어요?app/src/main/res/layout/dialog_login.xml (1)
8-8: 빈autofillHints대신 적절한 속성 사용 고려
android:autofillHints=""로 비워두기보다는, 오토필을 비활성화할 경우android:importantForAutofill="no"등을 활용해 의도를 명확히 표현하는 편이 낫습니다.app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsClickListener.kt (1)
5-7: KDoc 추가로 인터페이스 역할 명시
fun interface GoodsClickListener에 간단한 KDoc을 붙여서 클릭 이벤트의 용도와 파라미터goods의 의미를 문서화하면 가독성과 유지보수에 도움이 됩니다.app/src/main/java/woowacourse/shopping/feature/login/LoginError.kt (1)
3-7: 에러별 사용자 메시지 매핑 함수 추가 제안
sealed class LoginError내부에fun message(): String같은 추상 메서드를 선언하고 각object에서 구체 메시지를 반환하도록 구현하면, 뷰단에서 에러 처리 로직을 더 간결하게 작성할 수 있습니다.app/src/main/res/layout/activity_cart.xml (2)
4-6: 빈<data>블록 제거 고려
현재<data>가 비어있습니다. 데이터 바인딩 변수 사용 계획이 없다면 불필요한 블록을 제거해 레이아웃을 간결하게 유지하세요.
8-15: 컨테이너 ID 명명 개선 제안
fragment_container대신cartFragmentContainer등 기능을 명확히 나타내는 식별자를 사용하면 유지보수성이 향상됩니다.app/src/main/java/woowacourse/shopping/data/goods/dto/SortX.kt (1)
5-12: 클래스 명명 일관성 검토:SortX
SortX라는 네이밍이 직관적이지 않습니다.SortDto또는SortingInfo등 역할이 드러나는 이름으로 변경을 고려해보세요.app/src/main/java/woowacourse/shopping/data/util/mapper/CartResponseMapper.kt (1)
6-6: 매핑 함수 가독성 향상 제안
한 줄로 작성된 람다식 대신 개행과 파라미터 명시를 통해 가독성을 높일 수 있습니다. 예:fun CartResponse.toCartItems(): List<CartItem> = content.map { cartContent -> CartItem( product = cartContent.product.toDomain(), quantity = cartContent.quantity, id = cartContent.id ) }app/src/main/java/woowacourse/shopping/data/carts/dto/SortXX.kt (1)
5-12: 클래스 명명 일관성 검토:SortXX
SortX와 마찬가지로SortXX네이밍이 중복되어 보입니다.CartSortDto등 모듈별 역할이 드러나는 이름을 사용하는 것이 좋습니다.app/src/main/java/woowacourse/shopping/feature/GoodsUiModel.kt (1)
6-12: 기본값(default) 설정 검토 제안
현재name,price,thumbnailUrl,id모두 기본값("",0)을 갖고 있는데, 매핑 오류나 누락된 데이터가 기본값으로 대체되어 버릴 위험이 있습니다. 필수 필드에 대해서는 기본값 대신 명시적 생성자 파라미터 또는require검증을 통해 안전성을 높여 보는 것을 권장드립니다.app/src/main/java/woowacourse/shopping/feature/QuantityChangeListener.kt (1)
5-9: 함수형 인터페이스(fun interface) 고려 제안
Kotlin의fun interface로 선언하면 람다로 간결하게 사용할 수 있어 가독성과 사용 편의성이 올라갑니다.fun interface QuantityChangeListener { fun onIncrease(cartItem: CartItem) fun onDecrease(cartItem: CartItem) }이 방향을 검토해 보시는 건 어떨까요?
app/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsAlertMessage.kt (1)
5-9: 데이터 유효성 검증 고려 제안
quantity값이 음수가 될 경우를 방지하기 위한 간단한 검증(require(quantity >= 0) { "수량은 0 이상이어야 합니다." }등)을 추가하면 잘못된 흐름을 미리 차단할 수 있습니다.app/src/main/java/woowacourse/shopping/data/carts/CartFetchError.kt (1)
3-7: 에러 분류 확장성 검토 제안
현재Server(HTTP 5xx)와Network만 처리하고 있는데, HTTP 4xx(클라이언트 오류)나 인증 실패 같은 케이스를 별도 타입(ClientError)으로 구분하면 향후 유지보수성과 핸들링 명확성이 높아질 수 있습니다. 이 점을 한번 고려해 보셨나요?app/src/main/java/woowacourse/shopping/data/carts/dto/CartContent.kt (1)
6-13: 장바구니 아이템 DTO 구현이 깔끔합니다.GSON 직렬화 어노테이션과 데이터 클래스 구조가 잘 설계되었습니다. 다만
quantity필드에 대한 유효성 검증을 고려해보세요.수량에 대한 유효성 검증을 추가하고 싶다면 다음과 같이 고려해볼 수 있습니다:
data class CartContent( @SerializedName("id") val id: Int, @SerializedName("product") val product: Content, @SerializedName("quantity") - val quantity: Int, + val quantity: Int, ) +{ + init { + require(quantity > 0) { "수량은 0보다 커야 합니다" } + } +}app/src/main/java/woowacourse/shopping/data/goods/RecentSeenGoodsEntity.kt (1)
7-13: Room 엔티티 구조가 올바르게 정의되었습니다.테이블 어노테이션과 컬럼 정의가 적절합니다. 최근 본 상품 기능을 위해 조회 시간 정보를 추가하는 것을 고려해보세요.
최근 본 상품의 순서를 관리하기 위해 timestamp 필드를 추가해보는 것은 어떨까요?
@Entity(tableName = "recent_seen_goods") data class RecentSeenGoodsEntity( @PrimaryKey(autoGenerate = true) val id: Long = 0, @ColumnInfo(name = "goods_id") val goodsId: String, + @ColumnInfo(name = "viewed_at") + val viewedAt: Long = System.currentTimeMillis(), )이렇게 하면 DAO에서 최근 순서대로 정렬할 수 있습니다.
app/src/androidTest/java/woowacourse/shopping/feature/cart/CartActivityTest.kt (1)
11-18: 기본적인 UI 테스트 구조는 올바릅니다.액티비티 실행과 RecyclerView 표시 확인은 좋은 시작점입니다. 하지만 더 의미있는 테스트를 위해 사용자 시나리오를 추가하는 것을 고려해보세요.
테스트 커버리지를 확장해보는 것은 어떨까요? 예를 들어:
@Test fun 장바구니_아이템_표시_테스트() { // 테스트 데이터 준비 val testRepository = FakeCartRepository().apply { addTestCartItems() } ActivityScenario.launch(CartActivity::class.java) // RecyclerView 표시 확인 onView(withId(R.id.rv_cart_items)).check(matches(isDisplayed())) // 첫 번째 아이템 표시 확인 onView(withText("테스트 상품")).check(matches(isDisplayed())) } @Test fun 장바구니_빈_상태_테스트() { // 빈 장바구니 상태에서의 UI 테스트 }이런 식으로 실제 사용자 시나리오를 테스트하면 더 유용할 것 같습니다.
app/src/main/java/woowacourse/shopping/data/carts/dto/PageableX.kt (1)
5-18: 클래스 명명 규칙과 설계 개선점을 고려해보세요.현재 구현은 기능적으로 올바르지만 몇 가지 개선할 수 있는 부분이 있습니다:
PageableX와SortXX의 명명 규칙이 일관성이 없어 보입니다. 더 의미있는 이름을 생각해보세요.- 페이지네이션 매개변수에 대한 검증 로직을 고려해보셨나요?
- 이 클래스의 목적과 사용법에 대한 문서화가 도움이 될 것 같습니다.
어떤 명명 규칙을 따르는 것이 좋을지, 그리고 데이터 검증이 필요한 경우는 언제인지 생각해보세요.
app/src/main/java/woowacourse/shopping/feature/cart/adapter/CartViewHolder.kt (1)
14-16: 인터페이스가 선언되었지만 사용되지 않고 있습니다.
CartClickListener인터페이스를 만들었는데, 이것을 어떻게 활용할 계획인가요?다음을 고려해보세요:
- 이 ViewHolder에서 클릭 이벤트를 어떻게 처리할 건가요?
- 인터페이스를 ViewHolder에 전달하는 방법은?
- 실제 클릭 리스너 설정은 어디서 해야 할까요?
참고: Android RecyclerView의 아이템 클릭 처리 패턴을 학습해보세요.
app/src/main/java/woowacourse/shopping/data/goods/dto/Pageable.kt (1)
12-17: 데이터 무결성에 대해 생각해보세요.페이지네이션 데이터에서
paged와unpaged필드가 모두 존재합니다.질문해보세요:
- 이 두 필드가 항상 반대 값을 가져야 하나요?
- 만약 그렇다면, 하나의 필드로도 충분하지 않을까요?
- API 스펙을 다시 확인해서 실제로 두 필드가 모두 필요한지 검토해보세요.
힌트: 서버 응답을 확인하고 불필요한 중복 데이터는 제거하는 것이 좋습니다.
app/src/main/java/woowacourse/shopping/feature/CartUiModel.kt (1)
13-13: 확장 함수 네이밍을 더 명확하게 만들어보세요
toUiModel()함수명이 너무 일반적이지 않나요?toCartUiModel()처럼 더 구체적인 이름을 고려해보시는 것은 어떨까요?이렇게 하면 다른 도메인 모델들에도 비슷한 확장 함수를 만들 때 혼동을 줄일 수 있을 것 같은데, 어떻게 생각하시나요?
app/src/main/java/woowacourse/shopping/feature/cart/adapter/CartSkeletonAdapter.kt (1)
8-10: 스켈레톤 아이템 수를 고정값으로 하신 이유가 있나요?현재
skeletonCount = 5로 고정되어 있는데, 이것이 모든 화면 크기나 상황에서 적절할까요?화면 크기나 실제 데이터 크기에 따라 동적으로 조정할 수 있는 방법을 고민해보세요. 어떤 방식으로 더 유연하게 만들 수 있을까요?
app/src/main/res/layout/activity_goods.xml (1)
23-29: 커스텀 바인딩 어댑터의 구현을 확인해보세요.
app:cartItems와app:moreButtonVisible커스텀 바인딩 어댑터를 사용하고 있습니다. 이러한 바인딩 어댑터들이 어떻게 RecyclerView의 동작을 제어하는지 구현 방식을 생각해보세요. 특히cartItems가 어떻게 RecyclerView 어댑터와 연결되는지 고민해보시면 좋겠습니다.app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsSkeletonAdapter.kt (2)
9-9: 하드코딩된 값의 개선 방안을 고려해보세요.
skeletonCount = 20이 하드코딩되어 있습니다. 이 값이 변경될 가능성이나 다른 곳에서도 사용될 가능성을 고려해보세요. 어떤 방법으로 이 값을 관리하는 것이 더 좋을까요?
33-35: ViewHolder 클래스의 접근성을 검토해보세요.ViewHolder의
binding속성이val로 선언되어 있습니다. 이것이 적절한 접근 제어자인지 생각해보세요. 외부에서 이 속성에 접근할 필요가 있나요? 캡슐화 관점에서 어떻게 개선할 수 있을까요?app/src/main/java/woowacourse/shopping/data/goods/dto/GoodsResponse.kt (2)
5-28: DTO 클래스의 완성도를 높여보세요.현재 DTO가 기본적인 구조는 갖추고 있지만, 몇 가지 개선할 수 있는 부분들이 있습니다:
- 클래스와 필드에 대한 문서화가 없습니다. 이 DTO가 어떤 API 응답을 나타내는지 주석으로 설명해보세요.
- 필드들의 유효성 검증이나 기본값 처리를 고려해보세요.
SortX클래스명이 일관성 있는 네이밍인지 검토해보세요.어떤 방법으로 이러한 부분들을 개선할 수 있을까요?
6-27: SerializedName 사용의 일관성을 확인해보세요.모든 필드에
@SerializedName어노테이션을 사용하고 있습니다. 이것이 항상 필요한지 생각해보세요. JSON 키와 Kotlin 프로퍼티명이 동일한 경우에도 어노테이션이 필요할까요? 프로젝트의 일관성을 위해 어떤 정책을 적용하는 것이 좋을까요?app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSource.kt (1)
6-6: 메서드명의 일관성을 검토해보세요.
fetchRecentGoodsIds와loggingRecentGoods의 메서드명 패턴이 다릅니다. 하나는 명사형(fetch), 다른 하나는 동명사형(logging)을 사용하고 있습니다. 인터페이스 내에서 일관된 네이밍 컨벤션을 적용하려면 어떻게 개선할 수 있을까요?app/src/main/res/layout/cart_bottom_bar.xml (1)
64-64: 문자열 포맷팅에서 고려해볼 점이 있습니다.현재
@{@string/goods_price(viewModel.totalPrice)}방식으로 가격을 표시하고 있는데, 이 방법의 장단점을 생각해보셨나요? 다른 방식들과 비교했을 때 어떤 차이점이 있을지 고민해보시기 바랍니다.특히 다국화나 통화 형식이 변경될 가능성을 고려할 때, 어떤 접근 방식이 더 유연할지 생각해보세요.
app/src/main/java/woowacourse/shopping/feature/goods/GoodsViewModelFactory.kt (2)
17-17: 에러 메시지의 일관성을 고려해보세요.예외 메시지가 한국어로 작성되어 있습니다. 프로젝트 전체적으로 에러 메시지의 언어 정책이 어떻게 되어 있는지 확인해보시기 바랍니다.
다른 ViewModelFactory들과 일관된 에러 메시지 처리 방식을 사용하고 있는지 점검해보세요. 특히 개발자용 로그와 사용자용 메시지를 구분하는 것에 대해 생각해보시기 바랍니다.
8-18: 팩토리 패턴의 확장성에 대해 생각해보세요.현재 구조에서 새로운 ViewModel이 추가된다면 어떻게 처리해야 할지 고민해보셨나요?
Android에서 제공하는 다른 DI 방식들(예: Hilt, Dagger)과 비교했을 때 현재 접근 방식의 장단점은 무엇인지 분석해보시기 바랍니다.
app/src/main/res/layout/fragment_cart.xml (1)
35-79: 페이지네이션 UI의 사용성을 재고해보세요.현재 페이지네이션이 버튼 방식으로 구현되어 있습니다.
모바일 환경에서 페이지네이션의 다른 패턴들(무한 스크롤, pull-to-refresh 등)과 비교했을 때 현재 방식의 장단점은 무엇인지 분석해보세요. 사용자 경험 측면에서 어떤 개선 방안이 있을지 고민해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonViewHolder.kt (2)
16-25: 팩토리 메서드 패턴의 일관성을 확인해보세요.companion object를 사용한 팩토리 메서드를 구현하고 있습니다.
프로젝트의 다른 ViewHolder들도 같은 패턴을 따르고 있는지 확인해보세요. 일관된 코드 스타일을 유지하는 것이 팀 개발에서 왜 중요한지, 그리고 이런 패턴의 장점은 무엇인지 생각해보시기 바랍니다.
1-26: 더 나은 ViewHolder 구현을 위한 고민해볼 점들이 있습니다.현재 구현이 기능적으로는 문제없어 보이지만, 확장성과 유지보수성 측면에서 개선할 여지가 있을지 생각해보세요.
예를 들어, 바인딩 로직이 복잡해진다면 어떻게 구조화할 것인지, 또는 여러 타입의 "More" 버튼이 필요하다면 어떻게 대응할 것인지 고민해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/feature/login/LoginActivity.kt (1)
29-30: 하드코딩된 문자열을 상수로 추출해보세요."AccountInfo"와 "basicKey" 같은 문자열이 하드코딩되어 있습니다.
다음을 고려해보세요:
- 이 키들이 다른 곳에서도 사용된다면?
- 오타가 발생하면 어떻게 찾을 수 있을까요?
- 리팩토링할 때 놓치는 부분이 있을까요?
companion object나 별도의 Constants 클래스를 만들어 관리하는 방법을 생각해보세요.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/HorizontalSelectionViewHolder.kt (1)
13-25: 팩토리 메서드 패턴의 장단점을 생각해보세요.companion object에서 ViewHolder 인스턴스를 생성하는 패턴을 사용하고 있습니다.
이 패턴의 장점들을 생각해보세요:
- 생성 로직이 캡슐화되는가?
- 매개변수 전달이 명확한가?
그리고 고려할 점들도 있습니다:
- lifecycleOwner와 viewModel을 매번 전달해야 하는가?
- 이런 의존성들을 다른 방식으로 주입할 수 있을까?
- RecyclerView.Adapter에서 이런 정보들을 관리하는 게 더 적절하지 않을까?
다른 ViewHolder 생성 패턴들도 조사해보세요.
app/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsActivityTest.kt (2)
22-27: 테스트 데이터 관리 패턴을 개선해보세요.테스트용
GoodsUiModel을 직접 생성하고 있습니다.다음을 고려해보세요:
- 다른 테스트에서도 비슷한 테스트 데이터가 필요하다면?
- 테스트 데이터의 일관성을 어떻게 보장할까요?
- 실제 데이터와 유사한 테스트 데이터를 만들려면?
Test Fixture나 Test Data Builder 패턴에 대해 알아보세요. 테스트 코드의 재사용성과 유지보수성을 높일 수 있습니다.
41-61: 테스트 커버리지를 검토해보세요.현재 기본적인 UI 표시와 종료 기능만 테스트하고 있습니다.
추가로 테스트할 만한 시나리오들을 생각해보세요:
- 수량 변경 기능은?
- 장바구니 담기 기능은?
- 에러 상황 처리는?
- 최근 본 상품 기능은?
테스트 피라미드에서 UI 테스트의 역할과 범위에 대해 생각해보세요. 어떤 것들은 Unit Test나 Integration Test에서 다루는 게 더 적절할 수 있습니다.
app/src/main/java/woowacourse/shopping/feature/cart/recommend/RecommendFragment.kt (1)
71-72: UI 요소 숨기기 로직을 개선할 수 있을까요?현재 특정 UI 요소들을 하드코딩으로 숨기고 있습니다. 이런 접근 방식의 문제점을 생각해보세요:
- 이 Fragment가 다른 곳에서 재사용된다면?
- 비슷한 로직이 다른 Fragment에서도 필요하다면?
- UI 상태가 더 복잡해진다면?
더 유연하고 재사용 가능한 방법들을 고민해보세요. 예를 들어:
- Fragment 생성자나 arguments를 통한 설정
- 인터페이스를 통한 커스터마이징
- 별도의 설정 객체 사용
app/src/main/java/woowacourse/shopping/feature/cart/CartViewModelFactory.kt (1)
12-18: 타입 안전성과 오류 메시지를 개선해보세요현재 구현에서 몇 가지 고려해볼 점들이 있습니다:
타입 안전성:
@Suppress("UNCHECKED_CAST")를 사용하고 있는데, 이를 더 안전하게 처리할 방법이 있을까요? Kotlin의 타입 시스템을 활용해보세요.오류 메시지: "알 수 없는 뷰 모델 클래스 입니다"라는 메시지가 적절한가요?
- 이 메시지를 보게 될 사람은 누구일까요?
- 더 구체적이고 도움이 되는 정보를 포함할 수 있을까요?
확장성: 현재는 CartViewModel만 지원하는데, 미래에 다른 ViewModel이 추가된다면 어떻게 대응하시겠어요?
이런 관점에서 코드를 다시 살펴보고 개선점을 찾아보세요.
app/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendViewHolder.kt (2)
14-17: Data Binding 최적화를 고려해보세요현재
bind메서드에서executePendingBindings()을 호출하고 있습니다. 이것이 정말 필요한지 생각해보세요:
- RecyclerView에서
executePendingBindings()의 역할은 무엇인가요?- 언제 이것이 성능에 도움이 되고, 언제 불필요한 오버헤드가 될까요?
- 현재 사용 사례에서 이것 없이도 정상 작동할까요?
Android 공식 문서에서 Data Binding과 RecyclerView의 베스트 프랙티스를 찾아보고, 현재 구현이 적절한지 판단해보세요.
20-30: 팩토리 메서드의 매개변수 설계를 검토해보세요
from메서드의 매개변수 순서와 구조를 살펴보세요:
- 매개변수들 간의 의존성이 있나요?
- 더 직관적인 순서가 있을까요?
- 선택적 매개변수가 있다면 어떻게 처리할 수 있을까요?
또한 이 팩토리 메서드가 다른 ViewHolder들과 일관성 있는 인터페이스를 제공하는지 생각해보세요. 프로젝트 내 다른 ViewHolder들은 어떤 패턴을 사용하고 있나요?
app/src/main/java/woowacourse/shopping/data/util/mapper/CartEntityMapper.kt (1)
7-14: 기본값 설정의 의도를 명확히 해보세요
toEntity함수에서quantity의 기본값을 0으로 설정했습니다. 이 설계 결정에 대해 생각해보세요:
- 수량이 0인 장바구니 아이템이 의미가 있나요?
- 이 함수가 어떤 상황에서 호출될 것 같나요?
- 기본값 대신 필수 매개변수로 만드는 것이 더 안전하지 않을까요?
함수명과 매개변수 설계를 통해 개발자의 의도를 더 명확하게 전달할 수 있는 방법을 고민해보세요.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonAdapter.kt (1)
16-20: 빈 함수 블록 검토가 필요합니다
onBindViewHolder메서드가 비어있는데, 이것이 의도된 것인지 생각해보세요. 더 보기 버튼에 특별한 바인딩 로직이 필요하지 않다면, 이 빈 구현에 대한 주석을 추가하는 것이 좋을까요?🧰 Tools
🪛 detekt (1.23.8)
[warning] 19-20: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
app/src/main/res/layout/fragment_recommend.xml (1)
41-41: 문자열 리소스명 확인이 필요합니다
@string/recomment_item_description에서 "recomment"가 의도된 것인지 확인해보세요. "recommend"의 오타일 가능성이 있습니다.app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSourceImpl.kt (1)
12-19: 비동기 처리 방식에 대해 고민해보세요현재
thread와Handler를 사용한 방식이 동작하지만, 다음 질문들을 생각해보세요:
- 최근 Android 개발에서 권장되는 비동기 처리 방식은 무엇일까요?
- 코루틴을 사용한다면 이 코드가 어떻게 단순해질 수 있을까요?
- 데이터베이스 작업 중 예외가 발생하면 어떻게 처리해야 할까요?
이런 관점에서 현재 구현을 개선할 수 있는 방법을 찾아보세요.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/RecentlyViewedGoodsViewHolder.kt (1)
18-29: LifecycleOwner 설정 방식에 대해 생각해보세요companion object의
from메서드에서만lifecycleOwner를 설정하고 있는데, 이 패턴에 대해 어떻게 생각하시나요?다음 질문들을 고려해보세요:
- ViewHolder의 생성자에서 lifecycleOwner를 받지 않는 이유가 있을까요?
- 현재 방식과 생성자에서 받는 방식의 장단점은 무엇일까요?
- 데이터 바인딩에서 lifecycleOwner의 역할은 무엇인가요?
app/src/main/java/woowacourse/shopping/domain/model/Authorization.kt (1)
19-25: Basic Authentication 키 생성 로직을 개선해보세요
getBasicKey메서드의 구현을 살펴보세요:
- 메서드명이
getBasicKey인데 실제로는 새로 생성하고 있습니다. 이름이 적절한가요?setBasicKey와getBasicKey의 관계는 무엇인가요?- 매번 Base64 인코딩을 새로 하는 것이 효율적일까요?
더 명확하고 효율적인 구조를 생각해보세요.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsViewHolder.kt (1)
10-22: ViewHolder 구현 패턴을 다른 ViewHolder와 비교해보세요이
GoodsViewHolder와RecentlyViewedGoodsViewHolder를 비교해보면 어떤 차이점을 발견할 수 있나요?
- LifecycleOwner: 왜 한 ViewHolder에서는 lifecycleOwner를 설정하고 다른 곳에서는 설정하지 않을까요?
- 바인딩 방식:
bind메서드 사용의 장점은 무엇인가요?- 리스너 설정: init 블록에서 리스너를 설정하는 패턴이 적절한가요?
일관된 패턴을 유지하는 것이 코드베이스에 어떤 도움이 될지 생각해보세요.
app/src/main/java/woowacourse/shopping/feature/login/LoginViewModel.kt (1)
13-18: 이벤트 처리를 위한 SingleLiveData 사용을 검토해보세요SingleLiveData를 사용한 이벤트 처리는 좋은 접근입니다. 하지만 다음 사항들을 고려해보세요:
- 로그인 성공 시 Authorization key를 그대로 전달하는 것이 적절한지 검토해보세요
- 더 명확한 성공/실패 모델을 정의하는 것은 어떨까요?
app/src/main/java/woowacourse/shopping/feature/goods/GoodsGridItemDecoration.kt (1)
28-41: 그리드 레이아웃 로직의 유연성을 높여보세요현재 구현은 2열 그리드에 최적화되어 있습니다. 다음 사항들을 고려해보세요:
- spanCount가 2가 아닌 경우에는 어떻게 동작할까요?
- 어댑터 타입 체크를 더 확장 가능한 방식으로 개선할 수 있을까요?
더 유연한 접근 방법을 생각해보세요:
when (column) { - 0 -> outRect.left = horizontalPaddingPx - 1 -> outRect.right = horizontalPaddingPx + 0 -> outRect.left = horizontalPaddingPx + spanCount - 1 -> outRect.right = horizontalPaddingPx }app/src/main/java/woowacourse/shopping/data/ShoppingDatabase.kt (1)
40-55: 싱글톤 패턴 구현을 개선할 수 있습니다스레드 안전한 싱글톤 구현이지만, 더 안전한 방법을 고려해보세요:
- 현재 구현에서 메모리 누수 가능성은 없을까요?
- Context를 applicationContext로 사용하는 것은 좋은 선택입니다
더 안전한 싱글톤 패턴을 생각해보세요:
fun getDatabase(context: Context): ShoppingDatabase = - instance ?: synchronized(this) { + instance ?: synchronized(this) { + instance ?: Room .databaseBuilder( context.applicationContext, ShoppingDatabase::class.java, BuildConfig.DB_NAME, ).addMigrations(MIGRATION_1_2, MIGRATION_2_3) .build() .also { instance = it } }app/src/main/java/woowacourse/shopping/feature/cart/CartActivity.kt (2)
21-29: 의존성 주입 패턴 고려해보기현재 ViewModelFactory에서 모든 의존성을 직접 생성하고 있습니다. 이 방식의 장단점은 무엇일까요? 더 나은 아키텍처를 위해 어떤 패턴을 적용할 수 있을지 생각해보세요.
힌트: Hilt나 Koin 같은 DI 프레임워크를 사용하면 어떤 이점이 있을까요?
59-65: 메서드 가시성과 책임 분리 고려하기
navigateToRecommend()메서드가 public으로 되어 있는데, 이 메서드를 호출하는 주체가 누구인지 생각해보세요. Activity의 책임 범위를 명확히 하기 위해 어떤 방식으로 개선할 수 있을까요?또한 Fragment 트랜잭션 로직을 별도 메서드로 추출하면 어떤 장점이 있을지 고민해보세요.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/HorizontalSectionAdapter.kt (2)
32-32: 고정된 아이템 개수에 대한 설계 검토
getItemCount()가 항상 1을 반환하는 이유가 무엇인지 생각해보세요. 이런 패턴이 적절한 상황은 언제일까요?Single-item adapter 패턴의 장단점과 대안적인 접근 방법을 고려해보세요.
9-13: 생성자 매개변수 개수 검토생성자에 3개의 매개변수가 있습니다. 이들 간의 관계와 응집도를 생각해보세요.
객체 생성의 복잡도를 줄이기 위해 어떤 패턴을 적용할 수 있을까요? Builder 패턴이나 Factory 패턴을 고려해볼 만한 상황인지 판단해보세요.
app/src/main/java/woowacourse/shopping/util/SingleLiveData.kt (3)
6-38: 일회성 이벤트 패턴 구현 이해하기이 클래스가 해결하려는 문제가 무엇인지 생각해보세요. 일반적인 LiveData와 비교했을 때 어떤 차이점이 있을까요?
observe와observePeek의 차이점을 이해하고, 각각 어떤 상황에서 사용해야 할지 고민해보세요. 또한 Event 클래스의getContentIfNotHandled()와peekContent()메서드의 역할도 함께 살펴보세요.
23-23: Null Safety 고려사항
getValue()메서드에서 null 처리를 어떻게 하고 있는지 확인해보세요.Kotlin의 null safety 특성을 활용하여 더 안전한 API를 제공할 수 있는 방법은 없을까요?
6-14: 클래스 문서화 개선 고려이 클래스가 언제, 왜 사용되어야 하는지에 대한 문서가 있으면 좋겠습니다.
특히 일회성 이벤트 패턴을 처음 접하는 팀원을 위해 주석이나 KDoc으로 사용법과 주의사항을 설명해보는 것은 어떨까요?
app/src/androidTest/java/woowacourse/shopping/data/carts/repository/CartRepositoryTest.kt (2)
44-146: 테스트 시나리오 커버리지 우수함다양한 CRUD 시나리오와 페이징, 빈 상태까지 잘 테스트하고 있습니다. Given-When-Then 구조로 테스트 의도가 명확하게 드러나네요.
추가로 고려해볼 만한 엣지 케이스는 무엇이 있을까요? 예를 들어, 동시성 문제나 네트워크 오류 상황은 어떻게 테스트할 수 있을까요?
148-226: 비동기 테스트 헬퍼 메서드 잘 구현됨CountDownLatch를 사용한 비동기 테스트 처리가 적절합니다. 헬퍼 메서드로 중복을 제거한 것도 좋은 접근입니다.
timeout 값(3초)이 여러 곳에 하드코딩되어 있는데, 이를 상수로 추출하면 어떨까요? 또한 테스트 실패 시 더 명확한 오류 메시지를 제공하는 방법은 없을까요?
app/build.gradle.kts (2)
19-21: 환경별 설정 분리를 잘 구현했습니다.BuildConfig를 활용한 환경별 설정이 좋습니다. 다만 실제 운영 환경에서는 어떤 보안 고려사항이 필요할지 생각해보세요.
64-101: 의존성 관리가 포괄적입니다.필요한 라이브러리들이 잘 정리되어 있습니다. 다만 중복된 의존성이나 버전 관리 측면에서 개선할 점이 있는지 확인해보세요. 예를 들어,
okhttp가 두 번 선언되어 있습니다.app/src/main/java/woowacourse/shopping/data/carts/CartDao.kt (1)
40-52: 트랜잭션 로직을 개선해보세요.
addOrIncreaseQuantity메서드가 잘 구현되었습니다. 다만 더 간결하게 작성할 방법이 있을지 고민해보세요.insert메서드의 반환값을 활용하면 어떨까요?Room의
@Insert(onConflict = OnConflictStrategy.IGNORE)가 이미 설정되어 있으니, 이를 활용한 더 효율적인 로직을 생각해보세요.README.md (1)
1-57: 문서 구조가 매우 체계적입니다.단계별 요구사항과 체크리스트가 잘 정리되어 있어 프로젝트 진행 상황을 명확히 파악할 수 있습니다. 이런 방식의 문서화가 왜 중요한지 생각해보세요.
다만 마크다운 린터에서 지적한 들여쓰기 문제가 있습니다. 일관된 들여쓰기(2칸)를 사용하면 가독성이 더 좋아집니다.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
6-6: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
app/src/main/res/layout/activity_goods_details.xml (1)
86-97: 접근성을 개선해보세요.버튼의 기능은 잘 구현되었지만, 접근성 측면에서 개선할 점이 있습니다:
contentDescription추가 고려- 포커스 처리 방법
사용자 경험 관점에서 어떤 개선이 필요한지 생각해보세요.
app/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModelTest.kt (3)
278-280: 테스트 헬퍼 메서드의 일관성을 검토해보세요다른 테스트 파일들에서도 유사한
waitForCartUpdate메서드가 서로 다른 지연 시간(500ms, 600ms)을 사용하고 있습니다.이런 중복된 패턴을 어떻게 통합하고 더 신뢰할 수 있는 방식으로 개선할 수 있을까요?
283-300: 재사용 가능한 유틸리티로 발전시켜보세요
waitUntilValueEquals메서드가 매우 유용해 보입니다.이런 패턴이 다른 테스트에서도 필요할 것 같은데, 어떻게 하면 프로젝트 전체에서 재사용할 수 있도록 만들 수 있을까요? 테스트 유틸리티 클래스나 확장 함수를 고려해보세요.
114-125: 경계값 테스트를 더 강화해보세요수량이 1일 때 감소하지 않는 것을 테스트하고 있는데, 다른 경계 조건들도 고려해볼 필요가 있을까요?
예를 들어, 최대 수량 제한이나 음수 값 처리 등은 어떻게 테스트할 수 있을까요?
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepository.kt (1)
7-37: 비동기 패턴의 현대화를 고려해보세요현재 콜백 기반의 비동기 패턴을 사용하고 있습니다.
Kotlin의 코루틴이나 Flow를 사용하면 어떤 장점이 있을까요? 특히 복잡한 비동기 작업 체이닝이나 에러 처리 측면에서 어떤 개선이 가능한지 생각해보세요.
app/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendAdapter.kt (1)
83-87: 화면 크기 기반 레이아웃 계산을 검토해보세요
screenWidth / 3으로 아이템 너비를 설정하고 있습니다.다양한 화면 크기와 밀도에서 이 방식이 적절한지 고려해보세요. 리소스 파일이나 다른 방법으로 더 유연하게 처리할 수 있는 방법은 없을까요?
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSourceImpl.kt (2)
25-25: 사용하지 않는 gson 인스턴스를 제거하세요.이미 GsonConverterFactory를 사용하고 있으므로 별도의 Gson 인스턴스는 필요하지 않습니다.
- private val gson = Gson()
41-45: 에러 처리 시 더 구체적인 정보를 제공하는 것을 고려해보세요.현재는 응답 코드만 포함하고 있지만, 서버에서 제공하는 에러 메시지나 body 정보도 함께 전달하면 디버깅에 도움이 됩니다.
예를 들어:
- response.errorBody()?.string()을 활용하여 서버 에러 메시지 포함
- HTTP 상태 코드에 따른 구체적인 에러 메시지 제공
Also applies to: 75-79
app/src/androidTest/java/woowacourse/shopping/feature/cart/CartViewModelTest.kt (2)
41-41: 테스트의 안정성을 개선해보는 방법을 고려해보세요.Thread.sleep을 사용한 동기화 방식은 테스트를 불안정하게 만들 수 있습니다.
어떤 다른 방법들로 비동기 작업의 완료를 기다릴 수 있을까요? LiveData의 특성을 활용한 더 안정적인 동기화 방법을 생각해보세요.
279-285: 동기화 유틸리티 메서드들의 개선을 고려해보세요.현재 Thread.sleep을 사용하고 있는데, 이는 다음과 같은 문제를 야기할 수 있습니다:
- 테스트 실행 시간이 불필요하게 길어짐
- CI 환경에서 타이밍 이슈로 인한 불안정한 테스트
LiveData의 관찰자 패턴을 활용하거나, Awaitility 같은 라이브러리를 사용해서 조건이 만족될 때까지 기다리는 방식을 고려해보는 것은 어떨까요?
app/src/androidTest/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSourceTest.kt (2)
22-24: 테스트 데이터 관리 방식을 개선해보세요.클래스 레벨에서 테스트 데이터를 정의하는 것보다는 어떤 방식이 더 좋을까요?
각 테스트가 독립적으로 실행될 수 있도록 하려면 테스트 데이터를 어떻게 관리하는 것이 좋을지 생각해보세요.
90-109: CountDownLatch를 활용한 훌륭한 비동기 테스트 패턴입니다.Thread.sleep 대신 CountDownLatch를 사용한 것은 매우 좋은 선택입니다. 이는 더 안정적이고 효율적인 테스트를 만듭니다.
다만 5초 타임아웃이 로컬 데이터베이스 작업에는 다소 길어 보입니다. 어떤 이유로 이렇게 설정했는지, 더 짧은 시간으로도 충분하지 않을까요?
app/src/main/java/woowacourse/shopping/feature/BindingAdapter.kt (2)
34-57: 중복 코드를 줄이는 방법을 생각해보세요.cartItems != null 체크가 여러 번 반복되고 있습니다.
이런 중복을 어떻게 제거할 수 있을까요? 함수 상단에서 early return을 사용하거나, 다른 구조적 개선 방법을 고민해보세요.
59-71: 어댑터 타입 확인 로직의 안전성을 고려해보세요.현재 ConcatAdapter 내부의 childAdapter들을 순회하면서 타입을 확인하고 있는데, 이때 예상하지 못한 어댑터 타입이 포함될 가능성은 없을까요?
더 안전한 타입 체크 방식이나 예외 상황 처리를 고민해보세요.
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepositoryImpl.kt (1)
60-87: 복잡한 비동기 로직을 단순화할 수 있는 방법을 고민해보세요.현재 여러 개의 비동기 호출을 수동으로 카운팅해서 관리하고 있습니다.
이런 패턴의 문제점은 무엇일까요? 코루틴이나 RxJava 같은 비동기 처리 라이브러리를 사용하면 어떻게 개선할 수 있을지 생각해보세요.
app/src/main/java/woowacourse/shopping/data/util/RetrofitService.kt (2)
39-39: Authorization 헤더의 반복 패턴을 관찰해보세요.카트 관련 API들에서
Authorization헤더가 계속 반복되고 있습니다. 이런 패턴을 어떻게 개선할 수 있을까요? 인터셉터를 사용하면 어떤 장점이 있을까요?Also applies to: 45-45, 53-53, 60-60, 67-67
22-24: 기본값 설정 방식에 대해 생각해보세요.API 메서드에 하드코딩된 기본값들이 있습니다. 이런 값들을 상수로 분리하는 것은 어떨까요? 유지보수 관점에서 어떤 이점이 있을까요?
Also applies to: 36-38
app/src/main/java/woowacourse/shopping/data/util/MockInterceptor.kt (3)
67-95: JSON 문자열 생성 방식에 대해 고민해보세요.현재 JSON을 수동으로 문자열로 구성하고 있습니다. 이 방식의 잠재적인 문제점은 무엇일까요? 특히 특수문자가 포함된 제품명이나 URL에서 어떤 문제가 발생할 수 있을까요?
Gson이나 다른 JSON 라이브러리를 사용하면 어떤 장점이 있을까요?
114-131: 테스트 데이터 관리 방식을 검토해보세요.현재 제품 데이터가 하드코딩되어 있습니다. 테스트 시나리오가 다양해지면 이 방식의 한계는 무엇일까요? 더 유연한 테스트 데이터 관리 방법은 어떤 것들이 있을까요?
134-145: 데이터 중복 생성 로직을 살펴보세요.같은 제품을 6번 반복해서 42개의 제품을 만들고 있습니다. 이런 접근 방식이 테스트에 어떤 영향을 줄까요? 페이지네이션 테스트에서 더 다양한 데이터가 필요하다면 어떻게 개선할 수 있을까요?
app/src/main/java/woowacourse/shopping/data/carts/repository/CartRemoteDataSource.kt (1)
8-8: 메서드명의 의미를 명확히 해보세요.
fetchCartItemSize라는 메서드명을 보면 정확히 무엇을 반환하는지 명확하지 않습니다. 카트 아이템의 개수인지, 전체 크기인지 애매합니다. 더 명확한 네이밍은 어떤 것들이 있을까요?app/src/main/java/woowacourse/shopping/feature/cart/CartFragment.kt (1)
132-148: 어댑터 전환 로직을 더 안전하게 개선할 수 있습니다.로딩 상태에 따른 어댑터 교체 로직에서
contains()체크 후 추가/제거하는 방식이 반복되고 있습니다.어댑터 교체 로직을 헬퍼 메서드로 분리하여 가독성을 높여보세요:
private fun updateAdapterForLoadingState(isLoading: Boolean) { if (isLoading) { switchToAdapter(cartSkeletonAdapter, cartAdapter) } else { switchToAdapter(cartAdapter, cartSkeletonAdapter) } } private fun switchToAdapter(targetAdapter: RecyclerView.Adapter<*>, removeAdapter: RecyclerView.Adapter<*>) { if (concatAdapter.adapters.contains(removeAdapter)) { concatAdapter.removeAdapter(removeAdapter) } if (!concatAdapter.adapters.contains(targetAdapter)) { concatAdapter.addAdapter(0, targetAdapter) } }app/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsActivity.kt (1)
40-49: 의존성 주입을 더 체계적으로 관리해보세요.현재 Activity에서 직접 의존성을 생성하고 있는데, 이는 테스트하기 어렵고 결합도가 높습니다.
Dagger/Hilt나 수동 DI 컨테이너를 사용하여 의존성 주입을 중앙화하는 것을 고려해보세요. 또는 최소한 ViewModelFactory 패턴을 사용하여 ViewModel 생성 로직을 분리할 수 있습니다.
app/src/main/java/woowacourse/shopping/feature/cart/adapter/CartAdapter.kt (1)
23-47: setItems 메서드의 복잡성을 줄일 수 있는 방법을 고민해보세요현재 구현은 수동으로 아이템들을 비교하고 있습니다. RecyclerView의
DiffUtil을 사용하면 더 효율적이고 안전한 업데이트가 가능합니다. DiffUtil의 장점과 현재 구현 방식의 차이점을 조사해보시고 어떤 방식이 더 적합한지 생각해보세요.app/src/main/java/woowacourse/shopping/data/carts/repository/CartRepositoryImpl.kt (2)
110-112: TODO 주석으로 남겨진 메서드를 어떻게 처리할지 계획해보세요
getAllItemsSize메서드가 구현되지 않은 채로 남아있습니다. 이 메서드가 실제로 필요한지, 언제 구현할 예정인지 생각해보시고, 만약 당장 필요하지 않다면 인터페이스에서 제거하는 것도 고려해보세요.
71-71: private 메서드의 필요성을 검토해보세요
getCartItemByCartResponse메서드가 한 번만 사용되고 있습니다. 이런 경우 별도 메서드로 분리하는 것의 장단점을 생각해보시고, 코드의 가독성과 유지보수성 측면에서 어떤 것이 더 좋을지 고민해보세요.app/src/main/java/woowacourse/shopping/data/carts/repository/CartRemoteDataSourceImpl.kt (2)
27-29: TODO로 남겨진 메서드의 구현 계획을 세워보세요
fetchCartItemSize메서드가 구현되지 않았습니다. 이 메서드가 언제 필요한지, 어떻게 구현할 예정인지 생각해보시고 우선순위를 정해보세요.
38-38: Authorization 헤더 생성 방식을 개선해보세요여러 곳에서
"Basic " + Authorization.basicKey패턴이 반복되고 있습니다. 이런 중복을 어떻게 제거할 수 있을지 생각해보시고, 인증 헤더를 생성하는 더 안전하고 재사용 가능한 방법을 고민해보세요.Also applies to: 76-76, 106-106, 136-136, 167-167, 198-198
app/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsActivityTest.kt (1)
249-264: 사용하지 않는 메서드 제거
findQuantityTextView메서드가 정의되어 있지만 실제로 사용되지 않고 있습니다.미사용 코드는 유지보수에 혼란을 줄 수 있으니 제거하는 것이 좋겠습니다.
app/src/main/java/woowacourse/shopping/feature/cart/CartViewModel.kt (2)
111-122: Named argument 사용 및 매직 넘버 추출가독성 향상을 위해 named argument를 사용하고, 매직 넘버는 상수로 추출하는 것이 좋겠습니다.
+ companion object { + private const val MINIMUM_PAGE = 1 + private const val PAGE_SIZE = 5 + private const val RECOMMENDATION_SIZE = 10 + private const val DEFAULT_PAGE = 1 + } private fun loadDefaultRecommendedGoods() { goodsRepository.fetchPageGoods( - 10, - 1, - { response -> + size = RECOMMENDATION_SIZE, + page = DEFAULT_PAGE, + onSuccess = { response -> val allGoodsList = response.content.map { CartItem(it.toDomain(), 0) } filterRecommendedGoods(allGoodsList) }, - { + onError = { _recommendedGoods.value = emptyList() }, ) }
141-142: 불필요한 래퍼 함수단순한 변환 함수는 직접 호출하거나 extension function으로 만드는 것이 더 명확합니다.
이 함수는 한 곳(line 178)에서만 사용되므로 직접 호출하는 것이 좋겠습니다:
- private fun getCartItemByCartResponse(cartResponse: CartResponse): List<CartItem> = cartResponse.toCartItems() // line 178에서: - val pageItems = getCartItemByCartResponse(cartResponse) + val pageItems = cartResponse.toCartItems()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
app/src/main/res/drawable/image_placeholder.jpgis excluded by!**/*.jpg
📒 Files selected for processing (107)
README.md(1 hunks)app/build.gradle.kts(6 hunks)app/src/androidTest/java/woowacourse/shopping/data/carts/repository/CartRepositoryTest.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSourceTest.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/feature/cart/CartActivityTest.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/feature/cart/CartViewModelTest.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsActivityTest.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsViewModelTest.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsActivityTest.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModelTest.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/util/Fixture.kt(1 hunks)app/src/androidTest/java/woowacourse/shopping/util/getOrAwaitValue.kt(1 hunks)app/src/main/AndroidManifest.xml(2 hunks)app/src/main/java/woowacourse/shopping/MainActivity.kt(0 hunks)app/src/main/java/woowacourse/shopping/data/ShoppingDatabase.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/CartDao.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/CartEntity.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/CartFetchError.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/dto/CartContent.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/dto/CartItemRequest.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/dto/CartQuantity.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/dto/CartResponse.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/dto/PageableX.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/dto/SortXX.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/repository/CartRemoteDataSource.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/repository/CartRemoteDataSourceImpl.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/repository/CartRepository.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/carts/repository/CartRepositoryImpl.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/GoodsDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/RecentSeenGoodsDao.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/RecentSeenGoodsEntity.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/dto/Content.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/dto/GoodsResponse.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/dto/Pageable.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/dto/SortX.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSource.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSourceImpl.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSource.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSourceImpl.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepository.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepositoryImpl.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/util/ApiEndpoints.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/util/MockInterceptor.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/util/RetrofitService.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/util/mapper/CartEntityMapper.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/util/mapper/CartResponseMapper.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/util/mapper/GoodsMapper.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/util/mapper/toDomain.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/model/Authorization.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/model/CartItem.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/model/Goods.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/BindingAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/CartUiModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/GoodsUiModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/QuantityChangeListener.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/CartActivity.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/CartFragment.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/CartViewModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/CartViewModelFactory.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/adapter/CartAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/adapter/CartSkeletonAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/adapter/CartViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/cart/recommend/RecommendFragment.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/GoodsActivity.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/GoodsGridItemDecoration.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/GoodsViewModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/GoodsViewModelFactory.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/HorizontalSectionAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/HorizontalSelectionViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/RecentlyViewedGoodsAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/RecentlyViewedGoodsViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsClickListener.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsSkeletonAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonClickListener.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsActivity.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsAlertMessage.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/login/LoginActivity.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/login/LoginError.kt(1 hunks)app/src/main/java/woowacourse/shopping/feature/login/LoginViewModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/util/Event.kt(1 hunks)app/src/main/java/woowacourse/shopping/util/GoodsUiModelMapper.kt(1 hunks)app/src/main/java/woowacourse/shopping/util/MutableSingleLiveData.kt(1 hunks)app/src/main/java/woowacourse/shopping/util/SingleLiveData.kt(1 hunks)app/src/main/res/drawable/cart_item_background.xml(1 hunks)app/src/main/res/drawable/ic_close_gray.xml(1 hunks)app/src/main/res/drawable/ic_close_white.xml(1 hunks)app/src/main/res/drawable/ic_shopping_cart.xml(1 hunks)app/src/main/res/drawable/left_page_button_background.xml(1 hunks)app/src/main/res/drawable/right_page_button_background.xml(1 hunks)app/src/main/res/drawable/skeleton_background.xml(1 hunks)app/src/main/res/drawable/skeleton_card_background.xml(1 hunks)app/src/main/res/layout/activity_cart.xml(1 hunks)app/src/main/res/layout/activity_goods.xml(1 hunks)app/src/main/res/layout/activity_goods_details.xml(1 hunks)app/src/main/res/layout/activity_login.xml(1 hunks)app/src/main/res/layout/activity_main.xml(0 hunks)app/src/main/res/layout/cart_bottom_bar.xml(1 hunks)app/src/main/res/layout/dialog_login.xml(1 hunks)app/src/main/res/layout/fragment_cart.xml(1 hunks)app/src/main/res/layout/fragment_recommend.xml(1 hunks)
⛔ Files not processed due to max files limit (20)
- app/src/main/res/layout/item_cart.xml
- app/src/main/res/layout/item_cart_skeleton.xml
- app/src/main/res/layout/item_goods.xml
- app/src/main/res/layout/item_goods_skeleton.xml
- app/src/main/res/layout/item_horizontal_section.xml
- app/src/main/res/layout/item_more_button.xml
- app/src/main/res/layout/item_recently_goods.xml
- app/src/main/res/layout/item_recommend_goods.xml
- app/src/main/res/layout/menu_cart_navbar.xml
- app/src/main/res/layout/quantity_selector.xml
- app/src/main/res/menu/nav_cart.xml
- app/src/main/res/menu/nav_close.xml
- app/src/main/res/values/colors.xml
- app/src/main/res/values/dimens.xml
- app/src/main/res/values/strings.xml
- app/src/main/res/values/themes.xml
- app/src/main/res/xml/network_security_config.xml
- app/src/test/java/woowacourse/shopping/data/goods/repository/CartRemoteDataSourceTest.kt
- app/src/test/java/woowacourse/shopping/data/goods/repository/GoodsRepositoryTest.kt
- gradle/libs.versions.toml
💤 Files with no reviewable changes (2)
- app/src/main/res/layout/activity_main.xml
- app/src/main/java/woowacourse/shopping/MainActivity.kt
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.kt`: You MUST follow these guidelines: - Do not provide complete solutions unless explicitly requested. - Guide the programmer in problem-solving instead of providing direct ...
**/*.kt: You MUST follow these guidelines:
- Do not provide complete solutions unless explicitly requested.
- Guide the programmer in problem-solving instead of providing direct answers.
- When asked about programming concepts (e.g., "What is a hook?"), give direct and clear explanations.
- Break problems into smaller, manageable steps to help think through them logically.
- Ask leading questions and provide hints instead of just telling the answer.
- Encourage independent debugging before offering suggestions.
- Refer to relevant documentation instead of providing ready-made solutions.
- Promote modular thinking—breaking problems into reusable components.
- Remind the programmer to reflect on lessons learned after solving an issue.
- If explicitly asked for code (e.g., "Give me the code"), then provide it.
app/src/main/java/woowacourse/shopping/data/util/ApiEndpoints.ktapp/src/main/java/woowacourse/shopping/data/goods/dto/SortX.ktapp/src/main/java/woowacourse/shopping/data/goods/dto/Content.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonViewHolder.ktapp/src/main/java/woowacourse/shopping/feature/cart/adapter/CartViewHolder.ktapp/src/main/java/woowacourse/shopping/feature/login/LoginError.ktapp/src/main/java/woowacourse/shopping/data/goods/dto/GoodsResponse.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonClickListener.ktapp/src/main/java/woowacourse/shopping/data/carts/dto/CartContent.ktapp/src/main/java/woowacourse/shopping/data/util/mapper/CartResponseMapper.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsClickListener.ktapp/src/main/java/woowacourse/shopping/data/carts/dto/SortXX.ktapp/src/main/java/woowacourse/shopping/domain/model/Goods.ktapp/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsAlertMessage.ktapp/src/main/java/woowacourse/shopping/data/goods/GoodsDto.ktapp/src/main/java/woowacourse/shopping/domain/model/CartItem.ktapp/src/main/java/woowacourse/shopping/data/carts/dto/CartItemRequest.ktapp/src/main/java/woowacourse/shopping/data/carts/CartFetchError.ktapp/src/main/java/woowacourse/shopping/data/goods/RecentSeenGoodsEntity.ktapp/src/main/java/woowacourse/shopping/util/GoodsUiModelMapper.ktapp/src/main/java/woowacourse/shopping/feature/goods/GoodsViewModelFactory.ktapp/src/main/java/woowacourse/shopping/feature/cart/recommend/RecommendFragment.ktapp/src/main/java/woowacourse/shopping/feature/GoodsUiModel.ktapp/src/main/java/woowacourse/shopping/util/MutableSingleLiveData.ktapp/src/main/java/woowacourse/shopping/feature/QuantityChangeListener.ktapp/src/main/java/woowacourse/shopping/data/carts/CartEntity.ktapp/src/main/java/woowacourse/shopping/data/util/mapper/GoodsMapper.ktapp/src/main/java/woowacourse/shopping/feature/login/LoginActivity.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonAdapter.ktapp/src/main/java/woowacourse/shopping/feature/cart/adapter/CartSkeletonAdapter.ktapp/src/androidTest/java/woowacourse/shopping/feature/cart/CartActivityTest.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsSkeletonAdapter.ktapp/src/main/java/woowacourse/shopping/data/goods/dto/Pageable.ktapp/src/main/java/woowacourse/shopping/data/goods/RecentSeenGoodsDao.ktapp/src/main/java/woowacourse/shopping/data/carts/dto/CartQuantity.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/HorizontalSelectionViewHolder.ktapp/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendViewHolder.ktapp/src/main/java/woowacourse/shopping/data/util/mapper/toDomain.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/RecentlyViewedGoodsViewHolder.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsViewHolder.ktapp/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsActivityTest.ktapp/src/main/java/woowacourse/shopping/feature/CartUiModel.ktapp/src/androidTest/java/woowacourse/shopping/util/getOrAwaitValue.ktapp/src/androidTest/java/woowacourse/shopping/data/carts/repository/CartRepositoryTest.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/HorizontalSectionAdapter.ktapp/src/main/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSource.ktapp/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModelTest.ktapp/src/main/java/woowacourse/shopping/feature/cart/CartActivity.ktapp/src/main/java/woowacourse/shopping/data/carts/dto/CartResponse.ktapp/src/main/java/woowacourse/shopping/feature/login/LoginViewModel.ktapp/src/main/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSourceImpl.ktapp/src/main/java/woowacourse/shopping/feature/goods/GoodsGridItemDecoration.ktapp/src/main/java/woowacourse/shopping/feature/cart/CartViewModelFactory.ktapp/src/main/java/woowacourse/shopping/data/carts/dto/PageableX.ktapp/src/main/java/woowacourse/shopping/feature/goods/GoodsViewModel.ktapp/src/main/java/woowacourse/shopping/domain/model/Authorization.ktapp/src/main/java/woowacourse/shopping/util/SingleLiveData.ktapp/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/RecentlyViewedGoodsAdapter.ktapp/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsAdapter.ktapp/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModel.ktapp/src/main/java/woowacourse/shopping/data/util/mapper/CartEntityMapper.ktapp/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendAdapter.ktapp/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSourceImpl.ktapp/src/main/java/woowacourse/shopping/data/util/RetrofitService.ktapp/src/main/java/woowacourse/shopping/feature/BindingAdapter.ktapp/src/main/java/woowacourse/shopping/feature/cart/CartFragment.ktapp/src/main/java/woowacourse/shopping/data/carts/CartDao.ktapp/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepository.ktapp/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsViewModelTest.ktapp/src/androidTest/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSourceTest.ktapp/src/androidTest/java/woowacourse/shopping/util/Fixture.ktapp/src/main/java/woowacourse/shopping/feature/goods/GoodsActivity.ktapp/src/main/java/woowacourse/shopping/feature/cart/adapter/CartAdapter.ktapp/src/main/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsActivity.ktapp/src/main/java/woowacourse/shopping/data/util/MockInterceptor.ktapp/src/main/java/woowacourse/shopping/data/carts/repository/CartRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/data/carts/repository/CartRepository.ktapp/src/androidTest/java/woowacourse/shopping/feature/cart/CartViewModelTest.ktapp/src/main/java/woowacourse/shopping/util/Event.ktapp/src/main/java/woowacourse/shopping/data/carts/repository/CartRemoteDataSourceImpl.ktapp/src/main/java/woowacourse/shopping/data/carts/repository/CartRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/feature/cart/CartViewModel.ktapp/src/main/java/woowacourse/shopping/data/ShoppingDatabase.ktapp/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsActivityTest.kt
🧬 Code Graph Analysis (4)
app/src/main/java/woowacourse/shopping/feature/cart/recommend/RecommendFragment.kt (1)
app/src/main/java/woowacourse/shopping/feature/cart/CartFragment.kt (1)
setupBinding(78-81)
app/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModelTest.kt (2)
app/src/androidTest/java/woowacourse/shopping/feature/cart/CartViewModelTest.kt (1)
waitForCartUpdate(283-285)app/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsViewModelTest.kt (2)
waitForCartUpdate(321-323)getCartItemsFromDB(304-315)
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepositoryImpl.kt (6)
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepository.kt (2)
fetchRecentGoodsIds(28-28)fetchGoodsById(23-26)app/src/androidTest/java/woowacourse/shopping/util/Fixture.kt (2)
fetchRecentGoodsIds(68-70)fetchGoodsById(60-66)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSource.kt (2)
fetchRecentGoodsIds(5-12)fetchRecentGoodsIds(6-6)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsLocalDataSourceImpl.kt (1)
fetchRecentGoodsIds(12-19)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSource.kt (1)
fetchGoodsById(23-26)app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSourceImpl.kt (1)
fetchGoodsById(95-121)
app/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsViewModelTest.kt (2)
app/src/androidTest/java/woowacourse/shopping/feature/cart/CartViewModelTest.kt (2)
waitForUpdate(279-281)waitForCartUpdate(283-285)app/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModelTest.kt (2)
waitForCartUpdate(278-280)getCartItemsFromDB(265-276)
🪛 detekt (1.23.8)
app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonAdapter.kt
[warning] 19-20: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSourceImpl.kt
[warning] 92-93: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
app/src/main/java/woowacourse/shopping/data/carts/repository/CartRemoteDataSourceImpl.kt
[warning] 176-177: This empty block of code can be removed.
(detekt.empty-blocks.EmptyElseBlock)
[warning] 183-184: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🪛 markdownlint-cli2 (0.17.2)
README.md
6-6: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (39)
app/src/main/res/drawable/skeleton_background.xml (1)
1-5: 잘 구현된 스켈레톤 배경 리소스입니다.이 drawable은 Android 모범사례를 잘 따르고 있습니다:
- 컬러 리소스 참조 사용 (
@color/gray_400)- 적절한 XML 포맷팅
- 스켈레톤 로딩 UI에 적합한 둥근 모서리 디자인
스켈레톤 로딩 패턴을 구현할 때 이런 방식으로 재사용 가능한 배경을 만드는 것이 좋은 접근법입니다.
app/src/main/java/woowacourse/shopping/data/util/mapper/toDomain.kt (1)
8-8: 매핑 순서가 도메인 모델 정의와 일치하는지 확인 필요
Goods(this.name, this.price, this.imageUrl, this.id, this.category)인자로 전달할 때, 도메인Goods클래스의 생성자 파라미터 순서와 완전히 일치하는지 한번 검증해 보시는 걸 권장드립니다.app/src/main/java/woowacourse/shopping/data/goods/GoodsDto.kt (1)
3-8: DTO 어노테이션 사용 일관성 확인
SortX와SortXX에서는@SerializedName을 사용하지만GoodsDto는 기본 필드명에 의존하고 있습니다. 전체 DTO에서 어노테이션 사용 기준을 정의해 일관성을 확보하세요.app/src/main/java/woowacourse/shopping/feature/GoodsUiModel.kt (1)
3-5:@Parcelize플러그인 활성화 확인 요청
이 파일에서@Parcelize를 사용하려면kotlin-parcelizeGradle 플러그인이 활성화되어야 합니다.build.gradle(혹은build.gradle.kts)에 해당 설정이 제대로 등록되어 있는지 검토해 보셨나요?app/src/main/java/woowacourse/shopping/data/goods/dto/Content.kt (1)
6-15: JSON 필드명 매핑 검증 요청
@SerializedName("category"),"id","imageUrl"),"name"),"price")어노테이션이 실제 API 스펙과 정확히 일치하는지 다시 한번 확인해 보셨나요? 대소문자나 언더스코어 위치 등 사소한 불일치로 인해 GSON 역직렬화가 실패할 수 있습니다.app/src/main/java/woowacourse/shopping/data/goods/dto/Pageable.kt (1)
14-15: 의존성 확인이 필요합니다.
SortX타입을 사용하고 있는데, 이 클래스가 같은 패키지에 존재하는지 확인해보세요.확인할 점:
SortX클래스가 정의되어 있나요?- 네이밍이 일관성 있나요? (다른 DTO들과 비교해보세요)
- nullable로 처리해야 하는 필드는 아닌가요?
app/src/main/java/woowacourse/shopping/data/util/mapper/GoodsMapper.kt (1)
11-11: 필드 매핑을 다시 검토해보세요.
imageUrl을thumbnailUrl로 매핑하고 있습니다.확인해보세요:
- 이 두 필드가 실제로 같은 데이터를 나타내나요?
- 이미지 URL이 항상 썸네일용인가요?
- 큰 이미지와 썸네일을 구분해야 하는 경우는 없나요?
힌트: API 문서를 확인하고, 실제 이미지 크기나 용도를 검증해보세요.
app/src/main/java/woowacourse/shopping/feature/CartUiModel.kt (1)
7-11: UI 모델 설계가 깔끔하네요!Parcelize 어노테이션을 사용한 UI 모델 구현이 적절합니다. 다만 몇 가지 생각해볼 점이 있어요:
cartId와cartQuantity네이밍이 일관성이 있는지 확인해보세요- quantity 값에 대한 유효성 검증이 필요한지 고민해보세요 (음수 값 등)
도메인 모델과 UI 모델을 분리한 것은 좋은 설계 선택입니다!
app/src/main/res/drawable/right_page_button_background.xml (1)
1-15: 페이지네이션 버튼 스타일링이 잘 구성되어 있네요selector를 사용한 상태별 배경 처리가 적절합니다. enabled/disabled 상태에 따른 색상 변화가 명확하게 구분되어 사용자 경험을 향상시킬 것 같아요.
left_page_button_background.xml과의 일관성도 확인해보셨나요? 좌우 버튼의 corner radius나 색상 사용에 대칭성이 유지되고 있는지 살펴보시면 좋을 것 같습니다.
app/src/main/java/woowacourse/shopping/feature/cart/adapter/CartSkeletonAdapter.kt (1)
33-36: ViewHolder 구현이 간결하네요ViewHolder의 구현이 깔끔합니다. binding을 val로 선언한 것도 적절한 선택이에요.
다만 추후 skeleton UI가 복잡해진다면 ViewHolder 내부에 애니메이션 관리 로직을 캡슐화하는 것도 고려해볼 만합니다.
app/src/main/res/layout/activity_goods.xml (1)
17-17: fitsSystemWindows 속성 사용 목적을 명확히 하세요.
android:fitsSystemWindows="true"속성이 설정되어 있는데, 현재 레이아웃에서 시스템 윈도우와의 관계가 명확하지 않습니다. 이 속성이 필요한 이유는 무엇인가요? 상태바나 네비게이션바와 관련된 레이아웃 요구사항이 있나요?app/src/main/res/layout/fragment_cart.xml (1)
26-26: 커스텀 바인딩 어댑터 사용에 대해 더 알아보세요.
app:cartItems="@{viewModel.cart}"라는 커스텀 바인딩 어댑터를 사용하고 있습니다.이런 커스텀 어댑터의 구현이 어떻게 되어 있는지, 그리고 성능상 고려해야 할 점들이 있는지 생각해보세요. RecyclerView의 데이터 변경 시 전체 리스트를 새로 설정하는 것과 DiffUtil을 사용하는 것의 차이점을 조사해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonViewHolder.kt (1)
8-14: ViewHolder 초기화 패턴을 더 깊이 이해해보세요.현재
init블록에서 리스너를 설정하고 있습니다.ViewHolder의 생명주기를 고려했을 때, 이런 방식이 항상 최선인지 생각해보세요. 특히 리스너가 메모리 누수를 일으킬 가능성은 없는지, 그리고 ViewHolder가 재사용될 때는 어떻게 되는지 조사해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/HorizontalSelectionViewHolder.kt (1)
20-22: 데이터 바인딩 설정에 대해 검토해보세요.binding에 lifecycleOwner와 viewModel을 설정하고 있습니다.
다음을 확인해보세요:
- lifecycleOwner가 ViewHolder의 생명주기와 일치하는가?
- ViewHolder가 재사용될 때 이전 바인딩이 적절히 해제되는가?
- Fragment나 Activity의 생명주기가 ViewHolder보다 짧을 수 있는 상황은 없는가?
RecyclerView에서 ViewHolder 재사용 패턴과 데이터 바인딩의 생명주기 관리에 대해 더 알아보세요.
app/src/main/res/layout/fragment_recommend.xml (1)
60-68: 재사용 가능한 컴포넌트 활용이 잘 되었습니다하단 바를
include로 재사용하고 데이터 바인딩으로 ViewModel을 전달하는 방식이 좋습니다. 이런 패턴이 코드 중복을 줄이고 일관성을 유지하는 데 어떤 도움이 되는지 생각해보세요.app/src/main/java/woowacourse/shopping/data/goods/RecentSeenGoodsDao.kt (2)
22-30: 트랜잭션 활용이 잘 구현되었습니다
@Transaction을 사용해서 여러 데이터베이스 작업을 원자적으로 처리하는 것이 좋습니다. 이 구현을 살펴보면서 다음을 생각해보세요:
- 이 트랜잭션이 보장하는 데이터 일관성은 무엇인가요?
- 만약 중간에 작업이 실패하면 어떻게 될까요?
maxCount가 0이나 음수인 경우에 대한 처리가 필요할까요?
19-20: 복잡한 쿼리 로직을 검토해보세요이 쿼리가 의도한 대로 동작하는지 생각해보세요:
- 서브쿼리가 올바른 개수의 최신 항목을 선택하고 있나요?
- 테이블이 비어있거나
maxCount보다 적은 항목이 있을 때는 어떻게 될까요?실제 데이터로 이 쿼리를 테스트해보는 것이 좋겠습니다.
app/src/main/java/woowacourse/shopping/feature/login/LoginViewModel.kt (1)
10-12: 의존성 주입 패턴이 잘 적용되었습니다생성자를 통한 CartRepository 주입은 테스트 가능성과 관심사 분리 측면에서 좋은 선택입니다. ViewModelFactory에서 이 의존성을 어떻게 제공하고 있는지 확인해보세요.
app/src/main/java/woowacourse/shopping/feature/goods/GoodsGridItemDecoration.kt (1)
11-14: 데코레이션 설계가 잘 구성되었습니다ConcatAdapter와 함께 사용하는 커스텀 데코레이션 설계가 적절합니다. horizontalPaddingDp를 파라미터로 받아 재사용 가능하게 만든 점이 좋습니다.
app/src/main/java/woowacourse/shopping/data/ShoppingDatabase.kt (1)
15-19: Room 데이터베이스 구성이 적절합니다엔티티와 DAO를 명확하게 정의하고 버전 관리를 하고 있어 좋습니다. 추상 함수를 통한 DAO 접근도 올바른 패턴입니다.
app/src/main/java/woowacourse/shopping/feature/cart/CartActivity.kt (1)
47-57: 백스택 처리 로직 잘 구현됨홈 버튼 처리 시 백스택 확인 후 적절히 pop하거나 finish하는 로직이 잘 구현되어 있습니다. 사용자 경험을 고려한 좋은 접근입니다.
app/src/androidTest/java/woowacourse/shopping/data/carts/repository/CartRepositoryTest.kt (1)
26-37: 테스트 데이터베이스 설정 검토in-memory 데이터베이스와
allowMainThreadQueries()설정이 테스트 환경에 적절합니다.실제 운영 환경과 다른 설정을 사용하는 이유와 이로 인한 한계점에 대해 생각해보세요. 통합 테스트와 단위 테스트의 차이점도 함께 고려해보시면 좋겠습니다.
app/build.gradle.kts (2)
5-6: 플러그인 추가가 적절합니다.
kotlin-kapt와kotlin-parcelize플러그인이 Room과 데이터 바인딩을 위해 올바르게 추가되었습니다. 이들이 프로젝트에서 어떤 역할을 하는지 생각해보세요.
34-36: Release 빌드 설정을 검토해보세요.DB 이름에 슬래시가 포함되어 있고, BASE_URL이 플레이스홀더처럼 보입니다. 실제 운영 환경에서는 이 값들이 어떻게 설정되어야 할지 고민해보세요.
app/src/main/java/woowacourse/shopping/data/carts/CartDao.kt (1)
11-35: 기본 DAO 구조가 잘 설계되었습니다.Room의 어노테이션을 적절히 활용하여 CRUD 작업과 페이지네이션을 구현했습니다. 각 메서드가 어떤 상황에서 사용될지 생각해보세요.
app/src/main/res/layout/activity_goods_details.xml (2)
6-17: 데이터 바인딩 구조가 적절합니다.ViewModel과 리스너를 데이터 바인딩으로 연결하는 방식이 좋습니다. 이런 구조의 장점이 무엇인지 생각해보세요.
109-109: 조건부 표시 로직이 잘 구현되었습니다.
View.GONE을 사용한 조건부 표시가 적절합니다. 이런 패턴을 사용할 때의 장단점을 생각해보세요.app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/RecentlyViewedGoodsAdapter.kt (1)
26-30: 변경 감지 로직을 개선해보세요현재
oldItem != newItem으로 비교하고 있는데, 이것이 정확한 변경 감지를 보장할까요?
Goods객체의equals메서드가 제대로 구현되어 있는지 확인하고, 필요하다면 더 구체적인 비교 기준을 사용하는 것을 고려해보세요.app/src/main/AndroidManifest.xml (2)
5-5: INTERNET 권한 추가가 적절합니다.네트워크 통신을 위한 필수 권한이 올바르게 추가되었습니다.
19-28: 새로운 액티비티들이 적절히 선언되었습니다.모든 새 액티비티가 exported="false"로 설정되어 있어 보안상 적절합니다. 또한 CartActivity와 GoodsDetailsActivity에 커스텀 라벨이 지정된 것도 좋습니다.
app/src/androidTest/java/woowacourse/shopping/feature/cart/CartViewModelTest.kt (1)
18-301: 전반적으로 잘 구조화된 테스트 코드입니다.다양한 시나리오를 포괄하는 테스트 케이스들이 잘 작성되어 있고, 테스트 데이터 설정과 정리가 적절하게 이루어져 있습니다. 특히 페이지네이션, 아이템 추가/삭제, 수량 조절 등 핵심 기능들이 모두 테스트되고 있네요.
app/src/main/java/woowacourse/shopping/feature/BindingAdapter.kt (1)
17-32: 이미지 로딩 처리가 잘 구현되어 있습니다.null 안전성을 고려하고 placeholder와 error 처리까지 포함한 완성도 높은 구현입니다. Glide 사용법도 적절합니다.
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepositoryImpl.kt (1)
7-10: 의존성 주입을 통한 좋은 구조입니다.원격과 로컬 데이터 소스를 생성자에서 주입받는 방식으로 관심사가 잘 분리되어 있고, 테스트하기 좋은 구조를 가지고 있습니다.
app/src/androidTest/java/woowacourse/shopping/util/Fixture.kt (1)
29-34: 테스트 데이터 생성 로직이 명확하고 효율적입니다.fixture 데이터를 생성하는 방식이 간결하고 이해하기 쉽습니다. 기본 데이터 3개를 10번 반복하여 총 30개의 상품을 만드는 접근 방식이 좋습니다.
app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/GoodsAdapter.kt (1)
49-63: 페이로드 기반 업데이트가 효율적으로 구현되었습니다.수량 변경 시에만 필요한 부분만 업데이트하는 방식이 성능상 유리합니다. 코드도 명확하고 이해하기 쉽습니다.
app/src/main/java/woowacourse/shopping/feature/cart/adapter/CartAdapter.kt (1)
71-75: 체크박스 리스너 설정 패턴이 잘 구현되었습니다리스너를 null로 설정한 후 상태를 변경하고 다시 리스너를 설정하는 패턴으로 중복 콜백을 방지하고 있네요. 이런 패턴이 왜 필요한지 이해하고 계신가요?
app/src/main/java/woowacourse/shopping/feature/goods/GoodsActivity.kt (1)
104-106:❌ Incorrect review comment
Observer 내에서 데이터 재조회 시 무한 루프 위험
goodsWithCartQuantity변경을 감지해서 다시fetchAndSetCartCache()를 호출하는 구조는 무한 루프를 발생시킬 위험이 있습니다.이 로직의 의도가 무엇인지 검토가 필요합니다. 만약 특정 조건에서만 업데이트가 필요하다면, 조건을 추가하거나 다른 트리거를 사용하는 것이 좋겠습니다.
무한 루프 발생 여부를 확인하기 위해
goodsWithCartQuantity와fetchAndSetCartCache정의를 살펴보겠습니다.
🏁 Script executed:
#!/bin/bash # LiveData 프로퍼티 선언 위치 및 관련 메서드 정의 검색 rg -n -A5 -B5 "goodsWithCartQuantity" . rg -n -A5 -B5 "fun fetchAndSetCartCache" .Length of output: 9211
무한 루프 발생 여부 확인을 위해
fetchAndSetCartCache()구현부와CartItem정의를 살펴보겠습니다.
🏁 Script executed:
#!/bin/bash # fetchAndSetCartCache() 전체 구현 확인 sed -n '90,130p' app/src/main/java/woowacourse/shopping/feature/goods/GoodsViewModel.kt # CartItem 데이터 클래스 정의 위치 확인 rg -n "data class CartItem" .Length of output: 1572
무한 루프 방지 확인 완료
GoodsViewModel.bindCartCache()에서 새로 계산한 리스트(newList)와 기존_goodsWithCartQuantity.value를equals로 비교해 같을 경우postValue를 호출하지 않기 때문에, 값이 변경되지 않으면 Observer가 다시 트리거되지 않아 무한 루프가 발생하지 않습니다.검토 지점:
- 파일: app/src/main/java/woowacourse/shopping/feature/goods/GoodsViewModel.kt
bindCartCache() 내부 (라인 110–115)이후 추가 조치 불필요합니다.
Likely an incorrect or invalid review comment.
app/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsViewModelTest.kt (1)
304-315: CountDownLatch를 활용한 좋은 비동기 처리 구현
getCartItemsFromDB메서드는 비동기 작업을 CountDownLatch로 적절히 동기화하고 있습니다. 타임아웃 설정과 assertion도 잘 되어 있네요.이 패턴을 다른 비동기 작업에도 적용하면 Thread.sleep을 제거할 수 있을 것 같습니다.
app/src/main/java/woowacourse/shopping/feature/cart/CartViewModel.kt (1)
249-252: 페이지네이션 환경에서의 전체 선택 로직 문제현재 페이지의 아이템만으로 전체 선택 여부를 판단하는 것은 정확하지 않습니다. 다른 페이지에 선택되지 않은 아이템이 있을 수 있습니다.
전체 선택 상태를 정확히 판단하려면
selectedCartMap의 크기와 전체 아이템 수를 비교하는 것이 더 정확합니다:private fun updateAllSelected() { - val currentPageItems = _visibleCart.value ?: return - _isAllSelected.value = currentPageItems.all { selectedCartMap.containsKey(it.id) } + _isAllSelected.value = selectedCartMap.size == totalCartSizeData && totalCartSizeData > 0 }
| package woowacourse.shopping.feature.goods.adapter.vertical | ||
|
|
||
| fun interface MoreButtonClickListener { | ||
| fun onCLickMoreButton() |
There was a problem hiding this comment.
메서드 이름을 다시 확인해보세요.
코드를 자세히 살펴보면 무언가 눈에 띄는 것이 있나요?
메서드 이름을 천천히 읽어보고, Android나 Java의 일반적인 클릭 리스너 네이밍 컨벤션과 비교해보세요. 예를 들어, View.OnClickListener의 메서드 이름은 어떻게 되어 있나요?
이런 실수를 방지하기 위해 어떤 도구나 방법을 사용할 수 있을지도 생각해보세요. IDE의 스펠 체크 기능이나 코드 리뷰에서 이런 부분을 놓치지 않으려면 어떻게 해야 할까요?
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/feature/goods/adapter/vertical/MoreButtonClickListener.kt
at line 4, the method name onCLickMoreButton is misspelled and does not follow
the standard naming convention for click listeners in Android or Java, which
typically use onClick. Rename the method to onClickMoreButton to correct the
spelling and align with conventions. To prevent such errors, enable IDE spell
checking and carefully review method names during code reviews.
| <EditText | ||
| android:id="@+id/et_user_id" | ||
| android:autofillHints="" | ||
| android:hint="아이디" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
하드코딩된 문자열 리소스 분리
android:hint="아이디" 등 UI 문자열은 직접 레이아웃에 하드코딩하지 마시고 strings.xml로 추출해 관리하는 것이 좋습니다.
🤖 Prompt for AI Agents
In app/src/main/res/layout/dialog_login.xml at line 9, the android:hint
attribute uses a hardcoded string "아이디". To fix this, move the string "아이디" to
the strings.xml resource file and reference it here using @string resource
notation. This improves localization and maintainability by managing UI strings
centrally.
| data class CartItem( | ||
| val goods: Goods, | ||
| var quantity: Int, | ||
| val id: Int = 0, | ||
| ) { | ||
| val totalPrice: Int get() = goods.price * quantity | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
도메인 모델 설계에 대한 개선 제안
totalPrice 계산 프로퍼티는 좋은 설계입니다. 하지만 도메인 모델의 불변성을 위해 quantity를 불변으로 만들고, 수량 변경 시 새 인스턴스를 반환하는 방법을 고려해보세요.
도메인 모델의 불변성을 높이기 위한 리팩토링을 제안합니다:
data class CartItem(
val goods: Goods,
- var quantity: Int,
+ val quantity: Int,
val id: Int = 0,
) {
val totalPrice: Int get() = goods.price * quantity
+
+ init {
+ require(quantity > 0) { "수량은 0보다 커야 합니다" }
+ }
+
+ fun updateQuantity(newQuantity: Int): CartItem {
+ require(newQuantity > 0) { "수량은 0보다 커야 합니다" }
+ return copy(quantity = newQuantity)
+ }
}이렇게 하면 불변 객체의 장점을 유지하면서 수량 변경도 안전하게 처리할 수 있습니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data class CartItem( | |
| val goods: Goods, | |
| var quantity: Int, | |
| val id: Int = 0, | |
| ) { | |
| val totalPrice: Int get() = goods.price * quantity | |
| } | |
| data class CartItem( | |
| val goods: Goods, | |
| val quantity: Int, | |
| val id: Int = 0, | |
| ) { | |
| val totalPrice: Int get() = goods.price * quantity | |
| init { | |
| require(quantity > 0) { "수량은 0보다 커야 합니다" } | |
| } | |
| fun updateQuantity(newQuantity: Int): CartItem { | |
| require(newQuantity > 0) { "수량은 0보다 커야 합니다" } | |
| return copy(quantity = newQuantity) | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/domain/model/CartItem.kt around lines
3 to 9, the quantity property is currently mutable, which reduces domain model
immutability. Change the quantity property to be immutable (val) and add a
method that returns a new CartItem instance with an updated quantity when
quantity changes are needed. This preserves immutability while allowing safe
quantity updates.
| <?xml version="1.0" encoding="utf-8"?> | ||
| <selector xmlns:android="http://schemas.android.com/apk/res/android"> | ||
| <item android:state_enabled="false"> | ||
| <shape android:shape="rectangle"> | ||
| <solid android:color="@color/gray_500" /> | ||
| <corners android:topLeftRadius="4dp" android:bottomLeftRadius="4dp"/> | ||
| </shape> | ||
| </item> | ||
| <item android:state_enabled="true"> | ||
| <shape android:shape="rectangle"> | ||
| <solid android:color="@color/cyan" /> | ||
| <corners android:topLeftRadius="4dp" android:bottomLeftRadius="4dp"/> | ||
| </shape> | ||
| </item> | ||
| </selector> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
접근성과 디자인 일관성을 고려해보세요.
드로어블 구현이 깔끔하게 잘 되어있습니다! 다음 사항들을 추가로 고려해보면 어떨까요?
- 색상 대비가 접근성 가이드라인을 충족하는지 확인해보셨나요?
- 디자인 시스템에서 정의한 색상과 모서리 반지름 값을 사용하고 있나요?
right_page_button_background.xml와 함께 공통 스타일을 추출할 수 있는 부분이 있을까요?
Android 접근성 가이드라인과 머티리얼 디자인 문서를 참고해보세요.
색상 대비를 확인하려면 다음 스크립트로 현재 사용 중인 색상 값들을 확인해보세요:
🏁 Script executed:
#!/bin/bash
# 색상 리소스 확인
rg -A 2 -B 2 "cyan|gray_500" --type xmlLength of output: 4811
버튼 배경 색상 대비 및 디자인 일관성 개선 필요
- 현재 활성 상태의 배경색인
@color/cyan(#00BCD4)과 흰색 텍스트 조합의 대비비가 약 2.3:1로 WCAG AA(4.5:1) 기준에 미달합니다. 텍스트 가독성 보장을 위해 대비비가 충분한 색상으로 조정하세요. - 색상 리소스는 의미 기반 이름(예:
colorPrimary,colorOnPrimary)으로 관리하고, 머티리얼 디자인에서 권장하는 색상 팔레트를 참고해 일관성을 유지하세요. left_page_button_background.xml와right_page_button_background.xml의 공통<shape>및<corners>속성을 분리된 drawable 혹은 스타일 리소스로 추출해 중복을 제거하세요.
접근성(색상 대비)과 유지보수성(리소스 관리)을 함께 고려해 리팩터링을 진행해주시기 바랍니다.
🤖 Prompt for AI Agents
In app/src/main/res/drawable/left_page_button_background.xml lines 1 to 15, the
active background color @color/cyan (#00BCD4) does not meet WCAG AA contrast
ratio requirements against white text, and the color naming lacks semantic
clarity. Update the active background color to a shade with sufficient contrast
per accessibility guidelines, using semantic color resource names like
colorPrimary and colorOnPrimary consistent with the design system. Additionally,
extract the common <shape> and <corners> attributes shared with
right_page_button_background.xml into a separate drawable or style resource to
eliminate duplication and improve maintainability.
| fun GoodsUiModel.toDomain(): Goods = | ||
| Goods( | ||
| name = name, | ||
| price = price, | ||
| thumbnailUrl = thumbnailUrl, | ||
| id = id, | ||
| category = "", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
데이터 무결성과 설계 결정에 대해 생각해보세요.
매핑 함수가 깔끔하게 구현되어 있습니다! 한 가지 고민해볼 점이 있습니다:
toDomain() 함수에서 category = ""로 설정하는 부분이 있는데, 이는 데이터 손실을 의미할 수 있습니다.
- UI 모델에서 도메인 모델로 변환할 때 카테고리 정보가 없는 것이 의도된 설계인가요?
- 이런 경우 어떤 대안적인 접근 방법들이 있을까요?
- 매퍼 함수의 역할과 책임 범위를 어떻게 정의하는 것이 좋을까요?
클린 아키텍처와 도메인 주도 설계 관점에서 이 문제를 어떻게 해결할 수 있을지 고민해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/util/GoodsUiModelMapper.kt around
lines 14 to 21, the toDomain() function sets category to an empty string, which
risks data loss and may not align with intended design. To fix this, ensure the
UI model includes category data or modify the mapper to accept category as a
parameter or fetch it from a reliable source. Avoid hardcoding empty values;
instead, preserve or explicitly handle missing category information to maintain
data integrity and respect the mapper's responsibility of accurate data
transformation between layers.
| @Transaction | ||
| fun removeOrDecreaseQuantity(cartEntity: CartEntity) { | ||
| val existingItem = findByGoodsId(cartEntity.id) | ||
| if (existingItem != null) { | ||
| val quantityUpdatedItem = | ||
| existingItem.copy( | ||
| quantity = existingItem.quantity - cartEntity.quantity, | ||
| ) | ||
| if (quantityUpdatedItem.quantity > 0) { | ||
| update(quantityUpdatedItem) | ||
| } else { | ||
| delete(existingItem) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
수량 감소 로직에서 안전성을 검토해보세요.
현재 로직은 기본적으로 잘 동작하지만, 다음 상황들을 고려해보세요:
cartEntity.quantity가 음수인 경우는 어떻게 처리해야 할까요?- 존재하지 않는 아이템에 대한 감소 요청은 어떻게 처리해야 할까요?
이러한 예외 상황들에 대한 처리 방법을 생각해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/data/carts/CartDao.kt lines 54 to 68,
the removeOrDecreaseQuantity function lacks checks for invalid input such as
negative quantities and non-existent items. To fix this, add validation to
ensure cartEntity.quantity is positive before proceeding, and handle the case
where findByGoodsId returns null by either logging, throwing an exception, or
safely ignoring the operation. This will make the quantity decrease logic more
robust and prevent unintended behavior.
| android:layout_height="wrap_content" | ||
| android:layout_marginStart="18dp" | ||
| android:layout_marginTop="16dp" | ||
| android:text="@{@string/goods_price(viewModel.cartItem.goods.price * viewModel.cartItem.quantity)}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
비즈니스 로직 위치를 고민해보세요.
가격 계산 로직이 레이아웃 파일에 있습니다. 이런 계산 로직을 어디서 처리하는 것이 더 적절할지 생각해보세요.
- ViewModel에서 계산된 값을 제공하는 방법
- BindingAdapter를 활용하는 방법
어떤 접근이 더 유지보수하기 좋을까요?
🤖 Prompt for AI Agents
In app/src/main/res/layout/activity_goods_details.xml at line 64, the price
calculation logic is currently embedded in the layout file, which is not ideal.
Move this calculation out of the XML and into the ViewModel by adding a computed
property that returns the total price based on goods price and quantity. Then
bind the layout text attribute directly to this ViewModel property.
Alternatively, implement a BindingAdapter to handle the calculation and
formatting. This separation improves maintainability and keeps business logic
out of the UI layer.
| cartRepository = cartRepository, | ||
| goodsRepository = fixtureGoodsRepository, | ||
| ) | ||
| Thread.sleep(100) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
테스트 안정성을 위한 개선 방안을 고민해보세요
하드코딩된 Thread.sleep(100)이 테스트를 불안정하게 만들 수 있습니다.
어떤 방법으로 ViewModel 초기화를 더 안정적으로 기다릴 수 있을까요? CountDownLatch나 다른 동기화 메커니즘을 활용해보세요.
🤖 Prompt for AI Agents
In
app/src/androidTest/java/woowacourse/shopping/feature/goodsdetails/GoodsDetailsViewModelTest.kt
at line 66, replace the hardcoded Thread.sleep(100) with a more reliable
synchronization mechanism like CountDownLatch. This will allow the test to wait
explicitly for the ViewModel initialization or any asynchronous operation to
complete, improving test stability and avoiding arbitrary delays. Implement
CountDownLatch to await the completion signal before proceeding with assertions.
| val items: MutableList<Goods> = | ||
| mutableListOf() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
데이터 캡슐화를 고려해보세요
items를 public val로 노출하는 것이 어떤 문제를 일으킬 수 있을까요?
외부에서 직접 리스트를 수정할 수 있게 되면 어댑터의 상태 일관성에 어떤 영향을 미칠까요? private으로 만들고 필요한 경우에만 읽기 전용으로 접근하도록 개선해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/RecentlyViewedGoodsAdapter.kt
around lines 13 to 14, the items list is declared as a public mutable property,
which allows external modification and can break the adapter's state
consistency. Change the visibility of items to private and provide a public
read-only view (e.g., an unmodifiable list or a getter returning a copy) to
encapsulate the data and prevent external code from modifying the list directly.
| fun setItems(newItems: List<Goods>) { | ||
| val oldItems = items.toList() | ||
| items.clear() | ||
| items.addAll(newItems) | ||
| if (oldItems.isEmpty()) { | ||
| @Suppress("NotifyDataSetChanged") | ||
| notifyDataSetChanged() | ||
| } else if (newItems.size > oldItems.size) { | ||
| notifyItemRangeInserted(0, newItems.size - oldItems.size) | ||
| } else if (newItems.size == oldItems.size) { | ||
| oldItems.zip(newItems).forEachIndexed { index, (oldItem, newItem) -> | ||
| if (oldItem != newItem) { | ||
| notifyItemChanged(index) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
setItems 로직의 완성도를 점검해보세요
newItems.size < oldItems.size인 경우에 대한 처리가 빠져있습니다.
이 경우 어떤 알림을 보내야 할까요? notifyItemRangeRemoved를 사용해야 하는 상황을 고려해보세요. 또한 현재 로직이 모든 시나리오를 올바르게 처리하는지 검토해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/feature/goods/adapter/horizontal/RecentlyViewedGoodsAdapter.kt
between lines 16 and 32, the setItems function lacks handling for the case when
newItems.size is less than oldItems.size. To fix this, add an else-if branch
that calls notifyItemRangeRemoved starting from newItems.size for the count of
oldItems.size minus newItems.size. This will notify the adapter of the removed
items properly. Also, review the existing conditions to ensure all scenarios of
list size changes are covered correctly.
| interface GoodsRepository { | ||
| fun fetchGoodsSize(onComplete: (Int) -> Unit) | ||
|
|
||
| fun fetchPageGoods( | ||
| limit: Int, | ||
| offset: Int, | ||
| onComplete: (GoodsResponse) -> Unit, | ||
| onFail: (Throwable) -> Unit, | ||
| ) | ||
|
|
||
| fun fetchCategoryGoods( | ||
| limit: Int, | ||
| category: String, | ||
| onComplete: (GoodsResponse) -> Unit, | ||
| onFail: (Throwable) -> Unit, | ||
| ) | ||
|
|
||
| fun fetchGoodsById( | ||
| id: Int, | ||
| onComplete: (Goods?) -> Unit, | ||
| ) | ||
|
|
||
| fun fetchRecentGoodsIds(onComplete: (List<String>) -> Unit) | ||
|
|
||
| fun fetchRecentGoods(onComplete: (List<Goods>) -> Unit) | ||
|
|
||
| fun fetchMostRecentGoods(onComplete: (Goods?) -> Unit) | ||
|
|
||
| fun loggingRecentGoods( | ||
| goods: Goods, | ||
| onComplete: () -> Unit, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
에러 처리의 일관성을 검토해보세요
일부 메서드(fetchPageGoods, fetchCategoryGoods)는 onFail 콜백을 가지고 있지만, 다른 메서드들(fetchGoodsSize, fetchGoodsById 등)은 그렇지 않습니다.
모든 네트워크 작업이나 데이터베이스 작업에서 실패할 가능성이 있는데, 에러 처리를 어떻게 일관되게 만들 수 있을까요? 각 메서드의 실패 시나리오를 고려해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRepository.kt
between lines 6 and 38, the interface methods have inconsistent error handling;
some methods include an onFail callback while others do not. To fix this, add an
onFail callback parameter to all methods that perform network or database
operations to handle failure cases consistently. Update the method signatures to
include onFail: (Throwable) -> Unit where missing, ensuring all asynchronous
operations provide a way to report errors.
| private fun incrementItemQuantity(itemId: Int) { | ||
| val pos = items.indexOfFirst { it.goods.id == itemId } | ||
| if (pos != -1) { | ||
| val old = items[pos] | ||
| items[pos] = old.copy(quantity = old.quantity + 1) | ||
| notifyItemChanged(pos) | ||
| } | ||
| } | ||
|
|
||
| private fun decrementItemQuantity(itemId: Int) { | ||
| val pos = items.indexOfFirst { it.goods.id == itemId } | ||
| if (pos != -1) { | ||
| val old = items[pos] | ||
| items[pos] = old.copy(quantity = old.quantity - 1) | ||
| if (items[pos].quantity < 0) items[pos].quantity = 0 | ||
| notifyItemChanged(pos) | ||
| } | ||
| } |
There was a problem hiding this comment.
상태 동기화 문제를 고려해보세요
어댑터 내부에서 수량을 직접 변경하면서 동시에 외부 리스너도 호출하고 있습니다.
이런 패턴이 어떤 상태 불일치를 일으킬 수 있을까요? 외부 시스템(ViewModel)과 어댑터의 상태가 서로 다를 때 어떤 문제가 발생할 수 있는지 생각해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendAdapter.kt
lines 37 to 54, the incrementItemQuantity and decrementItemQuantity methods
directly modify the adapter's internal item list, which can cause state
synchronization issues with external listeners or ViewModel. To fix this, remove
direct state changes inside the adapter and instead notify the external listener
or ViewModel to update the data source. Then update the adapter's items only
when the external state changes, ensuring a single source of truth and
preventing inconsistencies.
| items[pos] = old.copy(quantity = old.quantity - 1) | ||
| if (items[pos].quantity < 0) items[pos].quantity = 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
수량 검증 로직을 개선해보세요
if (items[pos].quantity < 0) items[pos].quantity = 0는 임시방편처럼 보입니다.
수량이 음수가 되는 상황을 어떻게 원천적으로 방지할 수 있을까요? 감소 버튼을 비활성화하거나 최소값 검증을 미리 하는 방법을 고려해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendAdapter.kt
around lines 50 to 51, the current code reduces the item quantity and then
corrects negative values by setting them to zero, which is a temporary fix.
Instead, prevent the quantity from becoming negative by adding a check before
decrementing: only decrease the quantity if it is greater than zero.
Alternatively, disable the decrement button in the UI when the quantity is at
its minimum to avoid invalid operations.
| fun setItems(newItems: List<CartItem>) { | ||
| val old = items.toList() | ||
| items.clear() | ||
| items.addAll(newItems) | ||
| when { | ||
| old.isEmpty() -> { | ||
| @Suppress("NotifyDataSetChanged") | ||
| notifyDataSetChanged() | ||
| } | ||
| newItems.size > old.size -> { | ||
| notifyItemRangeInserted(old.size, newItems.size - old.size) | ||
| } | ||
| else -> { | ||
| newItems.forEachIndexed { idx, newItem -> | ||
| if (idx < old.size && newItem != old[idx]) { | ||
| notifyItemChanged(idx) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
setItems 로직의 완성도를 점검해보세요
newItems.size < oldItems.size인 경우에 대한 명시적 처리가 없습니다.
아이템이 제거되었을 때 어떤 알림을 보내야 할까요? else 절에서 모든 경우를 올바르게 처리하고 있는지 검토해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/feature/cart/adapter/RecommendAdapter.kt
between lines 15 and 35, the setItems function lacks explicit handling for the
case when newItems.size is less than old.size, meaning items have been removed.
To fix this, add a condition to detect when items are removed and call
notifyItemRangeRemoved with the appropriate start index and count. Also, review
the else block to ensure it only handles item changes without insertions or
removals, so the notifications accurately reflect the list updates.
| override fun fetchGoodsById( | ||
| id: Int, | ||
| onComplete: (Content) -> Unit, | ||
| ) { | ||
| retrofitService | ||
| .requestProductDetail( | ||
| id = id.toLong(), | ||
| ).enqueue( | ||
| object : Callback<Content> { | ||
| override fun onResponse( | ||
| call: Call<Content>, | ||
| response: Response<Content>, | ||
| ) { | ||
| if (response.isSuccessful && response.body() != null) { | ||
| onComplete(response.body()!!) | ||
| } | ||
| } | ||
|
|
||
| override fun onFailure( | ||
| call: Call<Content>, | ||
| t: Throwable, | ||
| ) { | ||
| println("error : $t") | ||
| } | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
fetchGoodsById 메서드의 에러 처리를 개선하세요.
onResponse에서 실패한 경우와 onFailure의 경우 모두 적절한 에러 콜백이 필요합니다. 현재는 에러를 조용히 무시하고 있어 호출자가 문제를 인지할 수 없습니다.
다른 메서드들처럼 onFailure 파라미터를 추가하여 일관성 있는 에러 처리를 구현하는 것이 어떨까요?
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/goods/repository/GoodsRemoteDataSourceImpl.kt
between lines 95 and 121, the fetchGoodsById method lacks proper error handling
for both unsuccessful responses and failures. Modify the method signature to
include an onFailure callback parameter, then invoke this callback with
appropriate error information inside both the onFailure override and the
onResponse override when the response is not successful or the body is null,
ensuring the caller is notified of any issues consistently.
| private lateinit var countingIdlingResource: CountingIdlingResource | ||
|
|
||
| @Before | ||
| fun registerIdlingResource() { | ||
| countingIdlingResource = CountingIdlingResource("GoodsActivity") | ||
| IdlingRegistry.getInstance().register(countingIdlingResource) | ||
| } | ||
|
|
||
| @After | ||
| fun unregisterIdlingResource() { | ||
| IdlingRegistry.getInstance().unregister(countingIdlingResource) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
IdlingResource를 등록했지만 활용하지 않고 있습니다
CountingIdlingResource를 등록했지만 실제로는 Thread.sleep을 사용하고 있습니다. IdlingResource의 목적은 비동기 작업 완료를 기다리는 것인데, 제대로 활용되지 않고 있네요.
Activity나 ViewModel에서 비동기 작업 시작/종료 시 IdlingResource의 increment/decrement를 호출하도록 구현하면 Thread.sleep을 제거할 수 있습니다.
🤖 Prompt for AI Agents
In
app/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsActivityTest.kt
around lines 32 to 43, the CountingIdlingResource is registered but not used
properly, as Thread.sleep is still used to wait for asynchronous operations. To
fix this, modify the Activity or ViewModel to call
countingIdlingResource.increment() when starting an async task and
countingIdlingResource.decrement() when it finishes, enabling the test to wait
for async completion without Thread.sleep.
| Thread.sleep(200) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
UI 테스트에서 Thread.sleep 사용 개선 필요
여러 테스트에서 Thread.sleep을 사용하고 있는데, 이는 테스트를 불안정하게 만듭니다.
Espresso는 기본적으로 UI 동기화를 제공하지만, 네트워크나 데이터베이스 작업은 IdlingResource가 필요합니다. 다음을 고려해보세요:
- ViewMatchers.isDisplayed()와 함께 waitFor 확장 함수 구현
- 데이터 로딩 상태를 추적하는 IdlingResource 구현
- Espresso-Intents의 intended() 사용 시 자동 동기화 활용
테스트 실행 시간도 줄이고 안정성도 높일 수 있습니다.
Also applies to: 115-116, 147-148, 183-184
🤖 Prompt for AI Agents
In
app/src/androidTest/java/woowacourse/shopping/feature/goods/GoodsActivityTest.kt
around lines 93-94, 115-116, 147-148, and 183-184, replace Thread.sleep calls
with proper synchronization methods. Implement a waitFor extension function
using ViewMatchers.isDisplayed() to wait for UI elements, and create an
IdlingResource to track network or database operations. Use Espresso's built-in
synchronization features, such as intended() for Espresso-Intents, to ensure
tests wait appropriately without arbitrary delays, improving test stability and
reducing execution time.
| fun increaseQuantity(cartItem: CartItem) { | ||
| cartRepository.updateQuantity(cartItem.id, CartQuantity(cartItem.quantity + 1), { | ||
| updateCartQuantity() | ||
| selectedCartMap[cartItem.id]?.quantity = | ||
| (selectedCartMap[cartItem.id]?.quantity ?: 0) + 1 | ||
| }, {}) | ||
| } |
There was a problem hiding this comment.
에러 처리 누락
수량 업데이트 실패 시 사용자에게 알림이 필요합니다. 빈 에러 콜백 대신 적절한 에러 처리를 추가하세요.
fun increaseQuantity(cartItem: CartItem) {
cartRepository.updateQuantity(cartItem.id, CartQuantity(cartItem.quantity + 1), {
updateCartQuantity()
selectedCartMap[cartItem.id]?.quantity =
(selectedCartMap[cartItem.id]?.quantity ?: 0) + 1
- }, {})
+ }, { error ->
+ // 에러 이벤트 발생 또는 토스트 메시지 표시
+ _loginErrorEvent.setValue(CartFetchError.NETWORK_ERROR)
+ })
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/feature/cart/CartViewModel.kt around
lines 145 to 151, the error callback for updateQuantity is empty, missing error
handling. Modify the error callback to include proper error handling logic, such
as notifying the user of the failure to update the quantity, instead of leaving
it empty.
| fun addCartItemOrIncreaseQuantity(cartItem: CartItem) { | ||
| val existing = selectedCartMap[cartItem.id] | ||
| if (existing != null) { | ||
| existing.quantity += 1 | ||
| } else { | ||
| val toAdd = cartItem.copy(quantity = 1) | ||
| selectedCartMap[toAdd.id] = toAdd | ||
| } | ||
| updateAllSelected() | ||
| updateTotalPriceAndCount() | ||
| } |
There was a problem hiding this comment.
동시성 문제 가능성
selectedCartMap을 직접 수정하는 것은 멀티스레드 환경에서 race condition을 야기할 수 있습니다. ConcurrentHashMap 사용이나 동기화 메커니즘을 고려해보세요.
- private val selectedCartMap = mutableMapOf<Int, CartItem>()
+ private val selectedCartMap = java.util.concurrent.ConcurrentHashMap<Int, CartItem>()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun addCartItemOrIncreaseQuantity(cartItem: CartItem) { | |
| val existing = selectedCartMap[cartItem.id] | |
| if (existing != null) { | |
| existing.quantity += 1 | |
| } else { | |
| val toAdd = cartItem.copy(quantity = 1) | |
| selectedCartMap[toAdd.id] = toAdd | |
| } | |
| updateAllSelected() | |
| updateTotalPriceAndCount() | |
| } | |
| private val selectedCartMap = java.util.concurrent.ConcurrentHashMap<Int, CartItem>() |
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/feature/cart/CartViewModel.kt around
lines 129 to 139, the method addCartItemOrIncreaseQuantity modifies
selectedCartMap directly, which can cause race conditions in a multithreaded
environment. To fix this, replace the current map with a thread-safe
ConcurrentHashMap or synchronize the method/block that modifies selectedCartMap
to ensure thread safety during updates.
| fun delete(cartItem: CartItem) { | ||
| selectedCartMap.remove(cartItem.id) | ||
| cartRepository.delete(cartItem.id) { | ||
| updateCartQuantity() | ||
| } | ||
| } |
There was a problem hiding this comment.
비동기 작업 순서 문제
서버 요청이 실패할 경우를 대비해 서버 삭제가 성공한 후에 로컬 상태를 업데이트하는 것이 안전합니다.
fun delete(cartItem: CartItem) {
- selectedCartMap.remove(cartItem.id)
- cartRepository.delete(cartItem.id) {
+ cartRepository.delete(cartItem.id,
+ onSuccess = {
+ selectedCartMap.remove(cartItem.id)
updateCartQuantity()
- }
+ },
+ onError = { error ->
+ // 에러 처리
+ }
+ )
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/feature/cart/CartViewModel.kt around
lines 165 to 170, the current code removes the item from the local
selectedCartMap before confirming the server deletion succeeded. To fix this,
move the removal of the item from selectedCartMap inside the
cartRepository.delete callback so that the local state updates only after the
server deletion is successful, ensuring correct asynchronous operation order and
error safety.
| } | ||
|
|
||
| const val EXTRA_SOURCE = "extra_source" | ||
| const val SOURCE_RECENTLY_VIEWED = "recently_viewed" |
There was a problem hiding this comment.
SOURCE_RECENTLY_VIEWED요 값은 GoodsDetailActivity에서만 사용되고 있어 남겼는데 다른 부분도 함께 선택되어 헷갈리셨겠네요 😅
| private val concatAdapter: ConcatAdapter by lazy { | ||
| ConcatAdapter(cartSkeletonAdapter) | ||
| } |
There was a problem hiding this comment.
나중에 기회가 된다면 일반 어댑터와 ConcatAdapter 중 어떤 것이 비용이 더 많이 발생하는지 알아보셔도 좋겠어요 😊
그런데 로딩 상태를 보여주기 위해 어댑터를 사용해야만 하는 것인가요?
필요한 만큼 xml에 추가해둔뒤 가시성을 제어하는 것과 어떤 차이가 있나 해서 여쭤봅니다!
| private var currentPage: Int = MINIMUM_PAGE | ||
| set(value) { | ||
| field = value | ||
| _page.postValue(value) | ||
| } | ||
| private val _page = MutableLiveData(currentPage) | ||
| val page: LiveData<Int> get() = _page |
There was a problem hiding this comment.
postValue를 사용해야만 하는지도 확인해보면 좋겠어요!
그리고 제 생각으로는 분리하지 않아도 되었을 것처럼 보이기도 해요.
_page의 value를 가져와서 충분히 연산이 가능하지 않을까 생각합니다..!
| fun selectAllItemsFromServer() { | ||
| cartRepository.fetchAllCartItems({ allItems -> | ||
| selectedCartMap.clear() | ||
| allItems.toCartItems().forEach { selectedCartMap[it.id] = it } | ||
| _isAllSelected.value = true | ||
| updateTotalPriceAndCount() | ||
| }, {}) | ||
| } |
There was a problem hiding this comment.
뷰모델은 Repository를 통해 데이터를 가져오고 있습니다.
API를 사용하는지, Local DB를 사용하는지 뷰모델은 알 길이 없죠.
FromServer라는 네이밍은 더 적절한 것으로 변경해보면 좋을 것 같아요!
| data class CartResponse( | ||
| @SerializedName("content") | ||
| val content: List<CartContent>, | ||
| @SerializedName("empty") | ||
| val empty: Boolean, | ||
| @SerializedName("first") | ||
| val first: Boolean, | ||
| @SerializedName("last") | ||
| val last: Boolean, | ||
| @SerializedName("number") | ||
| val number: Int, | ||
| @SerializedName("numberOfElements") | ||
| val numberOfElements: Int, | ||
| @SerializedName("pageable") | ||
| val pageable: PageableX, | ||
| @SerializedName("size") | ||
| val size: Int, | ||
| @SerializedName("sort") | ||
| val sort: SortXX, | ||
| @SerializedName("totalElements") | ||
| val totalElements: Int, | ||
| @SerializedName("totalPages") | ||
| val totalPages: Int, | ||
| ) |
There was a problem hiding this comment.
필드를 삭제하면 정말 문제가 생길까요?
만약 서버에서 내려주지 않으면 어떻게 될지 고민해봐도 좋겠어요!
| override fun onDestroy() { | ||
| super.onDestroy() | ||
| _binding = null | ||
| } |
| protected fun observeToast(toastLiveData: SingleLiveData<String>) { | ||
| toastLiveData.observe(this) { | ||
| Toast.makeText(this, it, Toast.LENGTH_SHORT).show() | ||
| } | ||
| } | ||
|
|
||
| protected fun observeNavigationToLogin(navigateToLogin: SingleLiveData<Unit>) { | ||
| navigateToLogin.observe(this) { | ||
| startActivity(Intent(this, LoginActivity::class.java)) | ||
| } |
There was a problem hiding this comment.
BaseActivity를 만들어두었지만 효과적으로 활용하지 못하고 있는 것 같아요 😭
| val cartQuantity: Int, | ||
| ) : Parcelable | ||
|
|
||
| fun CartItem.toUiModel() = CartUiModel(this.id, this.quantity) |
There was a problem hiding this comment.
모델의 변환이 어디는 같은 파일에, 어디는 매퍼에 위치해있고, 매퍼역시 파편화되어 있습니다! 이 부분도 공통적으로 만들어주면 좋을 것 같아요
| binding.bottomBar.checkboxAll.setOnCheckedChangeListener(null) | ||
| binding.bottomBar.checkboxAll.setOnCheckedChangeListener { _, isChecked -> | ||
| viewModel.selectAllItems(isChecked) | ||
| cartAdapter.notifyDataSetChanged() |
There was a problem hiding this comment.
어댑터의 데이터를 바꿔주는 것은 어댑터가 스스로 할 수 있도록 만들어보도록 해요
| private val _toastMessage = MutableSingleLiveData<String>() | ||
| val toastMessage: SingleLiveData<String> get() = _toastMessage |
셀프 체크리스트
README.md에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
이번 미션은 정말 구현에만 시간이 너무 부족해서 코드가 많이 더럽습니다... 테스트 코드도 작성하지 못했습니다...
이번 미션에서 직렬화 라이브러리를 선택한 이유를 작성하라는 조건이 있어서, 작성합니다!
GSON은 Android에서 검증된 라이브러리로, 다양한 기기와 OS 버전에서 문제 없이 동작합니다.
또한, @SerializedName 등으로 필드 매핑·제외를 세밀하게 조정하기가 간편합니다.
기존 다양한 사람들이 GSON을 사용하면서 더 많은 정보와 노하우가 인터넷 상에 있던 것도, 선택의 이유 중 하나가 되었습니다.
코드 리뷰 커뮤니케이션
📌 GitHub에서 PR에 댓글 남기는 방법
참고 자료
스크린샷
448757985-7f54a7b7-70c8-4eb3-b160-8742fd87a00b.mp4