[쇼핑 주문 1, 2 단계] 포르 미션 제출합니다 #109
Conversation
| size: Int?, | ||
| onResult: (Result<PagedResult<CartProduct>>) -> Unit, | ||
| ) { | ||
| cartProductService.getPagedProducts(page = page, size = size).enqueue( |
There was a problem hiding this comment.
execute와 enqueue 중 왜 enqueue를 사용하셨고,
어떤 차이가 있을까요?
There was a problem hiding this comment.
메인 스레드에서 네트워크 요청을 비동기적으로 처리하기 위해서 enqueue를 사용했습니다!
enqueue는 백그라운드 스레드에서 실행되어 메인 스레드를 차단하지 않고,
요청이 완료되면 onResponse() 또는 onFailure() 콜백이 호출됩니다.
반면 execute는 동기적으로 실행되어 요청이 끝날 때까지 호출한 스레드를 블로킹합니다.
| buildConfigField("String", "NAME", gradleLocalProperties(rootDir, providers).getProperty("name")) | ||
| buildConfigField("String", "PASSWORD", gradleLocalProperties(rootDir, providers).getProperty("password")) |
| @@ -59,6 +66,11 @@ dependencies { | |||
| implementation(libs.okhttp) | |||
| implementation(libs.mockwebserver) | |||
| implementation(libs.converter.gson) | |||
| implementation(libs.retrofit) | |||
| implementation(libs.kotlinx.serialization.json) | |||
There was a problem hiding this comment.
이전에 Gson을 사용할 때, default value가 무시되거나 non-nullable 타입에 null이 들어가는 문제가 발생해 오류를 파악하기 어려웠습니다.
kotlinx.serialization은 Kotlin 언어와 호환성이 좋고,
컴파일 타임에 타입 안정성을 보장해 이러한 문제를 방지할 수 있다는 점에서 선택했습니다!
어떠한 이유로 Default value가 무시되었을까요?
컴파일 단계에서 어떤 차이점이 있는지도 리플렉션 키워드 관련해서 찾아보시고
Gson, Moshi와 어떤 차이가 있는지 공유해주세요!
There was a problem hiding this comment.
현재 상품상세 진입 후 다시 되돌아오면 스크롤 포지션이 초기화 되고 있습니다.
어떤 이유이고, 어떻게 개선할 수 있을까요?
There was a problem hiding this comment.
로그를 찍어보니, 상품 목록 화면으로 돌아올 때마다 Activity가 매번 재생성되고 있었고,
이로 인해 스크롤 포지션이 초기화되고 있었습니다.
뒤로가기 버튼을 클릭하면 해당 Activity를 finish하도록 처리하여 스크롤 위치가 유지되도록 개선했습니다! (f908614)
|
|
||
| override fun onStart() { | ||
| super.onStart() | ||
| viewModel.loadCatalog() |
There was a problem hiding this comment.
configuration change와 같이 실제로 데이터를 load할 필요가 없을 때에도 호출하게 되겠네요..!
이 부분은 Activity Result를 활용하여 개선할 수 있을 것 같은데, 아직 반영하지 못했습니다.
다음 요청에서 이 부분도 같이 반영하겠습니다!
| class ProductAdapter( | ||
| items: List<ProductCatalogItem> = emptyList(), | ||
| private val eventHandler: ProductCatalogEventHandler, | ||
| ) : RecyclerView.Adapter<RecyclerView.ViewHolder>() { |
There was a problem hiding this comment.
ListAdapter를 적용해볼 수 있을까요?
어떤 차이점이 있는지도 공유해주세요!
There was a problem hiding this comment.
기존의 RecyclerView.Adapter를 모두 ListAdapter로 변경했습니다!
ListAdapter는 DiffUtil을 활용하여 데이터 전체를 다시 계산하지 않고 변경된 데이터만 업데이트할 수 있습니다.
덕분에 변경된 데이터의 position을 계산할 필요가 없어 코드도 훨씬 간결해진 것 같습니다!
refactor: 상품 목록 어댑터에 ListAdapter 적용
refactor: 최근 본 상품 목록 어댑터에 ListAdapter 적용
refactor: 장바구니 상품 목록 어댑터에 ListAdapter 적용
refactor: 추천 상품 목록 어댑터에 ListAdapter 적용
| mannodermaus-junit5-core = { group = "de.mannodermaus.junit5", name = "android-test-core", version.ref = "mannodermaus-junit5" } | ||
| mannodermaus-junit5-runner = { group = "de.mannodermaus.junit5", name = "android-test-runner", version.ref = "mannodermaus-junit5" } | ||
| mockwebserver = { module = "com.squareup.okhttp3:mockwebserver", version.ref = "okhttp" } | ||
| okhttp = { module = "com.squareup.okhttp3:okhttp", version.ref = "okhttp" } | ||
| retrofit = { module = "com.squareup.retrofit2:retrofit", version.ref = "converter-gson" } | ||
| retrofit2-kotlinx-serialization-converter = { module = "com.jakewharton.retrofit:retrofit2-kotlinx-serialization-converter", version.ref = "retrofit2-kotlinx-serialization-converter" } |
There was a problem hiding this comment.
kotlin serialization
first party converter 사용이 가능합니다
| cartProducts.map { it.product.id }.toSet() | ||
| val recommended = | ||
| products.items | ||
| .asSequence() |
There was a problem hiding this comment.
Sequence 를 활용해주셨네요 👍
기존 컬렉션 방식과 어떤 차이점이 있을까요?
There was a problem hiding this comment.
Sequence를 사용하면 임시 컬렉션을 사용하지 않고 연산을 연쇄적으로 할 수 있어 효율적으로 계산할 수 있습니다!
기존의 컬렉션 연산은 각 중간 연산마다 컬렉션을 생성하기 때문에 체인이 길어질수록 불필요한 객체 생성이 발생할 수 있습니다.
추천 상품을 구하는 로직에서 필터나 매핑을 하는 연산이 많아 적용해보았습니다!
| class CartProductRecommendationFragment( | ||
| private val productRepository: ProductRepository, | ||
| private val cartProductRepository: CartProductRepository, | ||
| private val recentProductRepository: RecentProductRepository, | ||
| ) : Fragment() { |
There was a problem hiding this comment.
Fragment의 생성자로 repository를 받아야만 할까요?
의존성을 Application에 접근하여 객체를 가져올 수 있게끔 설계해주신 것 같은데
생성자로 받지 않을 수 있지 않을까요?
There was a problem hiding this comment.
그렇네요..! 😅
말씀하신 방식처럼 Fragment의 생성자로 repository를 받지 않고, application에 접근하여 가져오도록 변경했습니다! (e21c28d)
| private val viewModel by lazy { | ||
| ViewModelProvider( | ||
| this, | ||
| CartProductRecommendationViewModelFactory( | ||
| productRepository, | ||
| cartProductRepository, | ||
| recentProductRepository, | ||
| ), | ||
| )[CartProductRecommendationViewModel::class.java] | ||
| } |
There was a problem hiding this comment.
ShoppingCartActivity
- CartProductSelectionFragment
- CartProductRecommendationFragment
방식으로 설계를 잘 해주셨어요.
그렇다면 ShoppingCartViewModel을 두고 해당 뷰모델을 하위 Fragment 에서 참조한다면
각각 ViewModel 생성대신 부모의 ViewModel을 사용할 수 있지 않을까요?
이렇게 설계된거라면 자식들의 관심사는 같다는 이야기가 될테니까요!
There was a problem hiding this comment.
처음에는 두 하위 Fragment에서 공통으로 사용하는 데이터가 있어,
ShoppingCartViewModel을 두고 하위 Fragment에서 부모의 ViewModel을 공유하는 방식으로 구현하려고 했습니다.
하지만 실제로 구현해보니 두 Fragment의 로직이 모두 ViewModel 하나에서 구현하게 되면서 너무 복잡해지고,
수량 +&- 버튼 처리와 같이 UI 로직이 중복되는 상황이 있었습니다..!
그래서 ViewModel을 Fragment 별로 분리하여, 책임을 나누는 방식으로 구현하게 되었습니다!
|
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 Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
Walkthrough이번 변경 사항은 안드로이드 쇼핑 주문 애플리케이션의 전체적인 구조와 기능을 대폭 확장하는 대규모 업데이트입니다. 기존의 MainActivity 및 관련 레이아웃이 삭제되고, 상품 카탈로그, 장바구니, 상품 상세, 추천 상품 등 다양한 화면과 기능이 새롭게 추가되었습니다. 데이터 계층에서는 Room 기반의 최근 본 상품 로컬 DB, Retrofit 기반의 네트워크 통신, 다양한 DTO/Entity/Repository/데이터소스 클래스가 도입되어 서버 연동 및 로컬 캐싱이 구현되었습니다. UI 계층은 데이터 바인딩, RecyclerView, ViewModel, Fragment, 커스텀 뷰홀더, 어댑터, 레이아웃 리소스 등으로 구성되어 있으며, 장바구니 상품 선택, 추천, 주문, 수량 조절, 최근 본 상품 등 복합적인 사용자 흐름을 지원합니다. 테스트를 위한 페이크 레포지토리와 LiveData 동기화 유틸도 추가되었습니다. 리소스 측면에서는 아이콘, 컬러, 스타일, 문자열 등 다양한 자원이 추가되어 UI/UX가 강화되었습니다. 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: 40
♻️ Duplicate comments (8)
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogEventHandler.kt (1)
7-10: 뷰홀더별 EventHandler 분리 검토
뷰홀더마다 EventHandler를 따로 갖는 구조가 중복을 유발할 수 있습니다. 각 핸들러를 통합할 수 있을지 고민해 보셨나요?app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogActivity.kt (2)
47-48: onStart에서 데이터를 로드하는 이유를 생각해보세요.
onStart()에서loadCatalog()을 호출하는 현재 구조를 보면, 이것이 사용자 경험에 어떤 영향을 미칠지 생각해보세요.
- 다른 액티비티에서 돌아올 때마다 어떤 일이 발생할까요?
- 데이터 로딩과 스크롤 포지션 복원 사이에는 어떤 관계가 있을까요?
onCreate,onResume대신onStart를 선택한 이유가 있나요?
109-112: 데이터 바인딩 초기화 위치에 대해 검토해보세요.현재
initBindings()에서 뷰모델과 라이프사이클 오너를 설정하고 있습니다.이 초기화가
initRecyclerView()이후에 호출되는 현재 순서가 적절한지 생각해보세요:
- 어댑터가 뷰모델에 의존하는 경우는 없나요?
- 초기화 순서를 바꾸면 어떤 영향이 있을까요?
app/build.gradle.kts (2)
26-27: 로컬 프로퍼티를 활용한 민감 정보 관리가 좋습니다!BuildConfig를 통해 민감한 인증 정보를 로컬 프로퍼티에서 가져오는 방식이 보안상 안전합니다.
70-70: kotlinx.serialization 선택 이유에 대한 추가 학습을 해보세요.PR 설명에서 언급하신 Gson 대신 kotlinx.serialization을 선택한 이유가 합리적입니다.
이전 리뷰어의 질문에 대해 좀 더 구체적으로 알아보시면 좋을 것 같아요:
- Gson의 default value 무시 문제: 리플렉션 기반 vs 컴파일 타임 코드 생성의 차이
- Moshi와의 비교점도 함께 학습해보세요!
어떤 구체적인 상황에서 default value가 무시되었는지, 그리고 컴파일 타임 안정성이 어떻게 보장되는지 공유해주시겠어요?
app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/ProductAdapter.kt (1)
11-13: ListAdapter 적용이 잘 되었습니다!이전 리뷰 의견을 반영하여 ListAdapter를 사용하신 점이 좋습니다. ListAdapter는 RecyclerView.Adapter보다 다음과 같은 장점이 있습니다:
- DiffUtil을 내장하여 자동으로 효율적인 업데이트 처리
- 백그라운드 스레드에서 diff 계산 수행
- 애니메이션 자동 처리
기본 RecyclerView.Adapter 대비 성능과 사용성이 개선되었네요!
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModel.kt (1)
40-50: 🛠️ Refactor suggestion비즈니스 로직의 재사용성을 고려해보세요.
수량 증가/감소 로직이 다른 ViewModel에서도 반복될 가능성이 높습니다.
이전 리뷰어가 제안한 대로, Domain Layer를 활용해 이런 공통 비즈니스 로직을 캡슐화하는 방법을 고민해보세요. UseCase 패턴이나 별도의 비즈니스 로직 클래스를 만들어서 여러 ViewModel에서 재사용할 수 있도록 구조를 개선해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModel.kt (1)
60-77: 🛠️ Refactor suggestion공통 비즈니스 로직을 추출해보세요.
장바구니 수량 증가/감소 로직이
ProductDetailViewModel과 중복됩니다.이전 리뷰에서 언급된 대로, 이런 공통 비즈니스 로직을 Domain Layer로 분리하는 것을 고려해보세요.
CartOperationUseCase나QuantityManagementUseCase같은 클래스를 만들어서 여러 ViewModel에서 재사용할 수 있도록 구조를 개선해보시기 바랍니다.
🧹 Nitpick comments (106)
app/src/main/res/drawable/ic_add.xml (1)
2-2: 불필요한 공백 줄 제거 고려
2행과 4행의 빈 줄은 리소스 파일 가독성을 위해 제거하는 것이 좋습니다.Also applies to: 4-4
app/src/main/res/values/colors.xml (1)
11-15: 컬러 그룹화 및 정렬 고려 새로 추가한 커스텀 색상을 관련된 기존 색상과 묶거나 이름순으로 정렬하면 유지보수성이 향상됩니다.app/src/main/res/values/themes.xml (1)
5-5: 테마의 colorPrimaryVariant 업데이트 검토 필요 colorPrimary을 dark_gray로 변경하셨다면, primaryVariant도 동일 팔레트 내에서 적절한 회색 계열로 조정하는 것이 일관성에 도움이 됩니다.app/src/main/res/drawable/style_product_edge.xml (1)
1-11: 앱 색상 리소스 활용 제안
현재android:color="@android:color/darker_gray"로 플랫폼 색상을 참조하고 있는데, 프로젝트 전역 색상 일관성을 위해@color/dark_gray(colors.xml 정의) 사용을 권장합니다.app/src/main/res/menu/menu_shopping_cart.xml (1)
1-12: 호환성 개선 제안: 네임스페이스 일관화
android:actionLayout대신app:actionLayout속성을 사용하면 AppCompat 라이브러리와의 호환성을 확보할 수 있습니다.app/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductItem.kt (1)
1-4: KDoc 추가 제안
데이터 클래스의 역할과 각 프로퍼티 의미를 문서화하기 위해 KDoc을 추가하면, IDE에서 바로 참고할 수 있어 유지보수성이 향상됩니다.app/src/main/res/drawable/style_right_button.xml (1)
2-9: 단일 아이템 셀렉터 최적화 제안
현재<selector>에 상태 구분 없이 하나의<item>만 정의되어 있습니다.
상태별 스타일이 필요 없다면<shape>리소스로 간단히 대체하여 파일 복잡도를 줄여볼 수 있겠습니다.app/src/main/res/layout/activity_shopping_cart.xml (2)
2-4: 데이터 바인딩<data>블록 고려
이 레이아웃에서 아직 ViewModel 바인딩 변수를 사용하지 않지만, 향후 Fragment나 ViewModel 연결을 대비해 미리<data>태그를 추가해두면 확장 시 편리합니다.
7-14: FragmentContainerView ID 네이밍 개선
@+id/fragment는 일반적인 명칭이어서 다른 컨테이너와 혼동될 수 있습니다.
예:@+id/fragment_shopping_cart처럼 기능이 드러나는 네이밍을 사용하면 가독성과 유지보수성이 올라갑니다.app/src/main/res/drawable/style_left_button.xml (1)
2-9: 단일 아이템 셀렉터 최적화 제안
<selector>내에<item>이 하나만 존재합니다.
추가 상태가 필요 없다면<shape>로 단순화하여 리소스 관리를 용이하게 할 수 있습니다.app/src/main/java/woowacourse/shopping/data/dto/request/CartProductQuantityRequestDto.kt (1)
1-4: 패키지 구조 및 네이밍 통일성 확인
다른 Request DTO들과 패키지·네이밍 컨벤션이 일관된지,@SerialName설정이 전반적으로 동일한 방식을 따르는지 검토해보세요.app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendEventHandler.kt (1)
5-7: 메서드 파라미터 및 문서화 고려
onOrderClick()만으로는 어떤 상품을 주문하는지 파악하기 어렵습니다. 예를 들어productId: Int같은 식별자를 파라미터로 추가하고, 인터페이스·메서드에 KDoc을 작성하면 명확성이 높아집니다.app/src/main/java/woowacourse/shopping/data/dto/response/CartProductQuantityResponseDto.kt (1)
6-10: 이 DTO에 대한 KDoc 추가 제안
CartProductQuantityResponseDto가 어떤 API 응답을 나타내는지 간단한 KDoc을 추가하면 가독성과 유지보수성 향상에 도움이 됩니다.app/src/main/java/woowacourse/shopping/data/util/DataMapper.kt (1)
7-7: ZoneId 캐싱으로 성능 최적화 검토
현재 호출할 때마다ZoneId.systemDefault()를 조회합니다. 자주 호출되는 경우 오버헤드가 발생할 수 있으니, 상수로 캐싱하는 방안을 고려해 보세요.app/src/main/res/layout/item_product_skeleton.xml (2)
8-16: 스켈레톤 이미지뷰의 접근성을 고려해보세요이미지뷰에
contentDescription이 설정되어 있지 않네요. 스켈레톤 상태임을 나타내는 설명을 추가하면 접근성이 향상될 것입니다. 또한 스켈레톤 애니메이션 효과를 위해 어떤 방식을 고려하고 계신지요?
18-36: 텍스트뷰의 높이와 스타일링을 검토해보세요TextView들이
wrap_content높이로 설정되어 있는데, 스켈레톤 UI에서는 실제 텍스트 높이와 유사한 고정 높이를 주는 것이 더 자연스러울 수 있습니다. 현재 구현에서 실제 상품명과 가격 텍스트의 평균적인 높이는 어느 정도인지 확인해보셨나요?app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectEventHandler.kt (1)
5-11: 인터페이스 설계의 의도를 더 명확히 할 수 있을지 생각해보세요현재 인터페이스가
CartProductViewHolder.EventHandler를 확장하면서 페이지네이션과 전체 선택 기능을 추가하고 있습니다. 이러한 책임들이 하나의 인터페이스에 모두 포함되는 것이 적절한지 고민해보세요. 각 기능의 관심사를 분리하면 어떤 이점이 있을까요?app/src/main/res/layout/item_load_more.xml (1)
22-22: 사용자 피드백을 위한 상태 관리를 고려해보세요현재 onClick 이벤트만 처리하고 있는데, 사용자가 버튼을 클릭한 후 로딩 중임을 알려주는 방법을 생각해보셨나요? 중복 클릭 방지나 로딩 인디케이터 표시 등을 고려하면 더 나은 사용자 경험을 제공할 수 있을 것입니다.
app/src/test/java/woowacourse/shopping/domain/model/ProductTest.kt (3)
10-15: 테스트 케이스명을 통해 의도를 더 명확히 표현해보세요현재 테스트명이 한국어로 잘 작성되어 있지만, "0보다 작으면 안된다"는 표현이 다소 애매할 수 있습니다. "음수 가격일 때 예외가 발생한다" 같이 더 구체적으로 표현하면 어떨까요? 테스트가 검증하는 정확한 조건을 생각해보세요.
17-23: 경계값 테스트의 완성도를 높여보세요0과 100만 테스트하고 있는데, 다른 중요한 경계값들도 고려해보셨나요? 예를 들어 매우 큰 값이나 정수 최대값 근처의 값들은 어떻게 처리되어야 할까요? Product 모델에서 가격의 상한선이 있는지도 확인해보시면 좋을 것 같습니다.
9-24: 테스트 커버리지 확장을 고려해보세요현재는 가격 검증만 테스트하고 있는데, Product 모델의 다른 속성들(id, name 등)에 대한 검증 로직이 있다면 그것들도 테스트가 필요할 수 있습니다. 또한 Product 객체의 동등성 비교나 문자열 표현 등의 기본 동작들도 테스트해볼 가치가 있을까요?
app/src/main/java/woowacourse/shopping/view/util/product/QuantityControlEventHandler.kt (2)
1-7: KDoc 추가 고려
인터페이스와 메서드에 간단한 KDoc을 추가하면 사용 용도와 파라미터 역할을 명확히 전달할 수 있습니다.
1-7: 제네릭 타입 설명
제네릭 T가 어떤 도메인 객체를 처리하는지 예시나 설명을 추가하면 다른 개발자가 구현 시 혼동을 줄일 수 있을 것 같습니다.app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailEventHandler.kt (2)
6-10: 파라미터 일관성 검토
onAddToCartClick()와onLastViewedProductClick()에 대상Product를 파라미터로 전달하는 방식이QuantityControlEventHandler의 메서드 시그니처와 달라 구현 시 혼동이 있을 수 있습니다. 의도된 설계인지 확인해 보세요.
6-10: KDoc 추가 권장
각 메서드의 역할과 호출 시점을 문서화하는 KDoc을 추가하면 코드 이해도가 향상됩니다.app/src/main/java/woowacourse/shopping/data/dto/request/CartProductRequestDto.kt (2)
8-11: nullable 프로퍼티 검토
productId와quantity를 nullable로 정의한 이유가 무엇인가요? 요청 필수값이라면 non-null 로 선언하고 기본값 혹은 유효성 체크를 추가하는 것이 안전할 수 있습니다.
6-12: 데이터 클래스 문서화
DTO 클래스에 간단한 KDoc을 추가하여 API 요청 시 JSON 포맷이나 필드 의미를 설명하면 유지보수가 수월해집니다.app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogEventHandler.kt (1)
1-11: KDoc 및 인터페이스 설명 추가
이 인터페이스가 담당하는 이벤트 흐름을 문서화하는 KDoc을 작성하면 기능을 빠르게 파악할 수 있습니다.app/src/main/java/woowacourse/shopping/domain/model/CartProduct.kt (2)
6-8: 가격 계산 로직 개선 제안
totalPrice를val로 한 번만 계산하는 대신get()이 포함된 커스텀 프로퍼티로 선언하면 더 직관적이고 변경 사항에도 안전할 수 있습니다.
6-12: quantity 유효성 확인 추가 고려
quantity가MINIMUM_PRODUCT_QUANTITY이상인지init { require(quantity >= MINIMUM_PRODUCT_QUANTITY) }로 검증해두면 예기치 않은 사용을 방지할 수 있습니다.app/src/main/java/woowacourse/shopping/view/util/SerializableExt.kt (1)
7-13: 타입 안전성과 예외 처리 고려사항현재 구현은 API 호환성을 잘 처리하고 있습니다. 다음 사항들을 한번 더 검토해보세요:
getSerializableExtra(key) as? T에서 안전 캐스팅을 사용했는데, 만약 잘못된 타입이 저장되어 있다면 어떤 일이 발생할까요?- 이 함수를 사용하는 곳에서 null 반환값에 대한 처리는 충분히 고려되어 있나요?
- 함수명에서 "Compat"의 의미가 사용자에게 명확하게 전달될까요?
이런 부분들을 고려하여 더 안전하고 명확한 API 설계를 생각해보시기 바랍니다.
app/src/main/res/drawable/style_right_arrow.xml (1)
3-22: UI 일관성과 접근성 고려사항상태별 드로어블 구현이 잘 되어 있습니다. 다음 사항들을 추가로 검토해보세요:
primary_color와white_gray사이의 색상 대비가 사용자에게 충분한 시각적 피드백을 제공하나요?- 오른쪽 모서리만 라운드 처리한 이유가 있나요? 왼쪽 화살표와의 일관성은 어떤가요?
- 비활성 상태에서도 사용자가 이것이 버튼임을 인지할 수 있을까요?
접근성 가이드라인을 참고하여 시각적 피드백이 충분한지 확인해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/domain/model/Product.kt (2)
12-14: 도메인 모델의 유효성 검증 고려사항가격에 대한 유효성 검증이 잘 구현되어 있습니다. 다음 사항들을 추가로 고려해보세요:
- 현재는 가격만 검증하고 있는데, 다른 필드들(name, imageUrl 등)에 대한 검증은 필요하지 않을까요?
require()를 사용한 것은 좋지만, 비즈니스 로직에서 이런 예외가 발생했을 때 어떻게 처리할 계획인가요?- 도메인 모델에서 이런 검증을 하는 것과 별도의 유효성 검증 계층을 두는 것 중 어떤 접근이 더 적합할까요?
도메인 주도 설계(DDD) 관점에서 모델의 불변성과 책임을 다시 한번 검토해보시기 바랍니다.
5-11: Serializable 구현에 대한 고려사항Intent를 통한 데이터 전달을 위해 Serializable을 구현하신 것을 확인했습니다. 다음 사항들을 검토해보세요:
- Serializable 대신 Parcelable을 고려해보셨나요? 안드로이드에서는 어떤 것이 더 효율적일까요?
- 향후 Product 클래스에 필드가 추가되거나 변경될 때 직렬화 호환성 문제는 없을까요?
- 이 클래스가 네트워크를 통해 전달되는 경우도 있다면 어떤 직렬화 방식이 더 적합할까요?
성능과 유지보수성을 고려하여 최적의 직렬화 방식을 선택해보시기 바랍니다.
app/src/main/res/layout/item_cart_product_skeleton.xml (1)
10-28: 스켈레톤 UI 설계 고려사항로딩 상태를 위한 스켈레톤 UI가 잘 구현되어 있습니다. 다음 사항들을 추가로 검토해보세요:
- 이 스켈레톤 레이아웃이 실제 데이터가 로드된 후 보여질 레이아웃과 크기와 위치가 정확히 일치하나요?
- 단순한 회색 배경 대신 shimmer 효과나 애니메이션을 추가하면 사용자 경험이 더 좋아질까요?
- TextView에 텍스트가 없는데, 높이가 적절하게 보장되고 있나요?
- 접근성 측면에서 스크린 리더 사용자들을 위한 고려사항은 있나요?
실제 콘텐츠 레이아웃과 비교하여 일관성을 확인하고, 로딩 상태임을 명확히 전달할 수 있는 방법을 고민해보시기 바랍니다.
app/src/test/java/woowacourse/shopping/viewmodel/LiveDataExtension.kt (1)
1-1: 패키지 위치를 검토해보세요.현재
viewmodel패키지에 위치하고 있지만, 이 확장 함수는 LiveData 전반에서 사용할 수 있는 범용 유틸리티입니다.다음을 고려해보세요:
- 이 함수가 ViewModel에만 특화된 기능인가?
- 다른 테스트에서도 재사용할 가능성은 없는가?
- 더 적절한 패키지 위치는 어디일까?
코드의 재사용성과 구조적 일관성을 고려하여 적절한 위치를 찾아보시기 바랍니다.
app/src/main/java/woowacourse/shopping/view/cart/ShoppingCartFragmentFactory.kt (1)
16-20: Factory 확장성을 고려해보세요.현재는 두 개의 Fragment만 처리하고 있지만, 향후 추가될 Fragment들을 어떻게 관리할지 생각해보세요.
다음을 고려해보시기 바랍니다:
- Fragment가 늘어날 때마다 이 Factory를 수정해야 하는가?
- 더 유연한 확장 방식은 없을까?
- 각 Fragment별 생성 로직이 복잡해지면 어떻게 관리할 것인가?
확장 가능한 설계 방향에 대해 고민해보시기 바랍니다.
app/src/main/res/layout/custom_toolbar_shopping_cart.xml (3)
18-18: 배경색을 하드코딩하지 말고 테마 속성 사용을 고려해보세요.
@color/dark_gray대신?attr/colorSurface같은 테마 속성을 사용하면 다크 모드 지원 시 더 유연해집니다.
25-25: 아이콘 뒤집기 의도를 명확히 해주세요.
android:scaleX="-1"로 아이콘을 뒤집는 이유가 무엇인지 코멘트로 설명하거나, 필요하다면 올바른 방향의 아이콘 리소스를 사용하는 것이 더 명확할 수 있습니다.
28-29: 불필요한 ConstraintLayout 제약조건을 제거하세요.FrameLayout 내부에서 ConstraintLayout 제약조건들(
app:layout_constraint*)은 효과가 없습니다. 이런 제약조건들을 제거하여 코드를 정리해보세요.- app:layout_constraintStart_toStartOf="parent" - app:layout_constraintTop_toTopOf="parent"- app:layout_constraintBottom_toBottomOf="parent" - app:layout_constraintStart_toEndOf="@id/iv_shopping_cart"Also applies to: 45-46
app/src/main/res/layout/activity_product_catalog.xml (2)
42-48: 스켈레톤 아이템 개수를 동적으로 처리하는 것을 고려해보세요.현재 각 컬럼에 4개씩 하드코딩되어 있는데, 화면 크기에 따라 필요한 스켈레톤 아이템 개수가 달라질 수 있습니다. 이를 동적으로 계산하거나 충분한 개수로 설정하는 것이 좋겠습니다.
Also applies to: 60-66
37-38: 마진 값을 리소스로 분리하는 것을 고려해보세요.
android:layout_marginHorizontal="18dp"와android:layout_marginVertical="10dp"같은 값들을 dimens 리소스로 분리하면 일관성 있는 디자인 시스템을 구축할 수 있습니다.Also applies to: 55-56
app/src/main/java/woowacourse/shopping/data/ShoppingCartDatabase.kt (2)
14-14: 클래스명과 데이터베이스명의 일관성을 고려해보세요.
ShoppingCartDatabase라는 이름이지만 현재는RecentProduct만 관리하고 있습니다. 향후 장바구니 관련 엔티티들이 추가될 예정이라면 현재 네이밍이 적절하지만, 그렇지 않다면ShoppingDatabase같은 더 일반적인 이름을 고려해볼 수 있습니다.Also applies to: 18-18
10-13: 향후 데이터베이스 마이그레이션을 고려해보세요.현재는 버전 1이라 문제없지만, 향후 스키마 변경 시 마이그레이션 전략을 미리 계획해두는 것이 좋겠습니다. 특히 장바구니 관련 엔티티들이 추가될 때를 대비해보세요.
app/src/main/res/layout/item_recent_product_list.xml (2)
9-21: TextView에 접근성 개선을 고려해보세요현재 TextView가 단순히 "최근 상품" 텍스트만 표시하고 있는데, 스크린 리더 사용자를 위한 접근성을 개선할 수 있습니다.
android:contentDescription속성을 추가하거나, 의미론적으로 더 적합한 헤딩 역할을 부여하는 방법을 생각해보세요.또한 하드코딩된 마진값(20dp, 6dp) 대신 dimension 리소스를 사용하는 것이 유지보수성 측면에서 어떤 장점이 있을지 고민해보세요.
30-35: MaterialDivider의 높이값을 재검토해보세요8dp 높이의 divider는 일반적인 구분선보다 상당히 두꺼운 편입니다. Material Design 가이드라인에서 권장하는 divider 높이는 얼마인지 확인해보고, 현재 설정이 의도된 디자인인지 검토해보세요.
UI 컴포넌트 간 시각적 구분의 목적을 달성하기 위한 더 적절한 높이값은 무엇일까요?
app/src/main/java/woowacourse/shopping/data/dto/response/CartProductDto.kt (1)
7-15: 데이터 검증에 대해 생각해보세요현재 DTO는 서버로부터 받은 데이터를 그대로 수용하고 있습니다. 실제 운영 환경에서는 어떤 상황들이 발생할 수 있을까요?
예를 들어:
quantity가 음수일 수 있을까요?id가 0 이하의 값일 수 있을까요?데이터 무결성을 보장하기 위한 검증 로직을 어디에 추가하는 것이 좋을지 고민해보세요.
app/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductAdapter.kt (2)
8-21: 어댑터 구현이 잘 되어있습니다ListAdapter를 사용하고 이벤트 핸들러 패턴을 적용한 점이 좋습니다. ViewHolder의 정적 팩토리 메서드 사용도 메모리 효율성 측면에서 좋은 선택입니다.
현재 구현에서 대용량 데이터를 처리할 때 고려해야 할 점들은 무엇일까요? 예를 들어, 이미지 로딩이나 메모리 관리 측면에서 추가로 고려할 부분이 있는지 생각해보세요.
23-36: DiffUtil 구현을 검토해보세요현재
areItemsTheSame에서 ID만 비교하고areContentsTheSame에서 전체 객체를 비교하고 있습니다.이 방식이 적절한지 생각해보세요:
CartProductItem에 많은 필드가 있다면 성능에 어떤 영향이 있을까요?- 특정 필드만 변경되었을 때도 전체 아이템이 갱신되는 것이 적절할까요?
더 세밀한 비교 로직을 구현하는 방법에 대해 고민해보세요.
app/src/main/java/woowacourse/shopping/view/util/MutableSingleLiveData.kt (2)
8-14: 메서드 오버라이드의 필요성을 검토해보세요현재
postValue와setValue메서드는 단순히 부모 클래스의 메서드를 호출하기만 합니다.이런 패턴이 필요한 이유는 무엇일까요?
- 단순히 접근성을 변경하기 위함이라면 다른 설계 방법은 없을까요?
- 향후 이 메서드들에 추가 로직이 필요할 예정인가요?
더 명확한 의도를 드러내는 설계 방법에 대해 고민해보세요.
1-15: 문서화를 추가해보세요이 유틸리티 클래스의 사용 목적과 주의사항을 KDoc으로 문서화하는 것이 좋겠습니다.
특히 다음 사항들을 문서화해보세요:
- 언제 이 클래스를 사용해야 하는가?
- SingleLiveData와의 차이점은 무엇인가?
- 올바른 사용 패턴은 무엇인가?
팀원들이 이 클래스를 올바르게 사용할 수 있도록 가이드를 제공해보세요.
app/src/main/res/layout/item_quantity_control.xml (1)
31-44: 수량 표시의 사용자 경험을 개선해보세요현재 수량 텍스트가 단순히 숫자만 표시됩니다. 사용자가 이 숫자의 의미를 더 잘 이해할 수 있도록 어떤 방법들을 사용할 수 있을까요?
예를 들어, 수량이 0일 때와 그렇지 않을 때의 다른 처리나, 최소/최대 수량에 대한 시각적 피드백을 생각해보세요.
app/src/main/java/woowacourse/shopping/data/service/ProductApiService.kt (2)
12-17: suspend 함수 사용을 고려해보세요.현재
Call<T>반환 타입을 사용하고 있는데, 코루틴을 활용한suspend함수로 변경하면 더 간결하고 안전한 비동기 처리가 가능합니다. 어떤 방식이 프로젝트에 더 적합할지 고민해보세요.
14-14: accept 헤더 기본값을 구체화해보세요.
"*/*"보다는"application/json"과 같이 구체적인 미디어 타입을 명시하는 것이 API 통신에서 더 명확합니다. 서버가 어떤 형태의 응답을 기대하는지 고려해보세요.Also applies to: 21-21
app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModelFactory.kt (1)
11-16: 예외 메시지를 더 구체적으로 작성해보세요.
IllegalArgumentException()에 구체적인 에러 메시지를 추가하면 디버깅 시 어떤 ViewModel 클래스가 지원되지 않는지 쉽게 파악할 수 있습니다. 개발자가 문제를 빠르게 해결할 수 있도록 도와주는 메시지를 고민해보세요.app/src/main/java/woowacourse/shopping/ShoppingApplication.kt (1)
18-32: 테스트 환경에서의 의존성 주입을 고려해보세요.현재 구현은 프로덕션 환경에서는 잘 동작하지만, 테스트 시 가짜 구현체로 교체하기 어려울 수 있습니다. 향후 테스트 코드 작성 시 이 부분을 어떻게 해결할지 미리 고민해보세요.
app/src/main/java/woowacourse/shopping/data/dao/RecentProductDao.kt (1)
26-30: 트랜잭션 메서드의 동시성 처리를 고려해보세요.
replaceRecentProduct메서드가 delete 후 insert를 수행하는 구조가 좋습니다. 하지만 동시에 같은 상품에 대해 이 메서드가 호출될 경우를 생각해보셨나요? Room의@Transaction어노테이션이 기본적인 원자성을 보장하지만, 더 안전한 처리 방법이 있을지 고민해보시기 바랍니다.예를 들어, UPSERT 패턴이나
@Insert(onConflict = OnConflictStrategy.REPLACE)같은 대안은 어떨까요?app/src/main/java/woowacourse/shopping/data/TokenProvider.kt (1)
17-19: 초기화 시점의 토큰 설정에 대해 고민해보세요.생성자에서 자동으로 토큰을 설정하는 것이 항상 적절할까요? 만약 사용자가 다른 자격증명을 사용하거나, 토큰을 나중에 설정하고 싶다면 어떻게 될까요?
이런 상황들을 고려해서 초기화 전략을 다시 검토해보시면 어떨까요? 예를 들어 필요시에만 토큰을 설정하는 방법이나, 초기화를 명시적으로 호출하는 방법 등을 생각해볼 수 있습니다.
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailActivity.kt (1)
28-36: ViewModel 팩토리 패턴 사용이 적절합니다.
ShoppingApplication에서 Repository들을 가져와서 ViewModel 팩토리에 주입하는 방식이 좋습니다. 의존성 주입의 기본적인 패턴을 잘 따르고 있네요.하지만 이런 패턴이 여러 액티비티에서 반복된다면, 더 효율적인 DI 방법을 고려해볼 수도 있겠습니다. 예를 들어 Hilt나 Koin 같은 DI 프레임워크 도입을 미래에 고려해보시면 어떨까요?
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogActivity.kt (1)
23-34: 뷰모델 초기화 시점과 의존성 주입 패턴에 대해 생각해보세요.현재
by lazy를 사용해 뷰모델을 초기화하고 있습니다. 이 방식의 장점과 단점은 무엇일까요? 특히 액티비티 생명주기와 관련해서 언제 뷰모델이 실제로 생성되는지 고려해보세요.또한
ShoppingApplication에서 직접 레포지토리들을 가져오는 현재 방식 외에 의존성 주입을 개선할 수 있는 다른 방법들은 없을까요?app/src/main/res/layout/item_recent_product.xml (2)
18-21: 고정된 크기와 접근성을 고려해보세요.현재
android:layout_width="100dp"로 고정 크기를 사용하고 있습니다.다양한 화면 크기와 밀도에서 이 방식이 어떻게 동작할지 생각해보세요:
- 태블릿이나 폴드폰에서는 어떻게 보일까요?
- 더 유연한 크기 지정 방법은 없을까요?
또한 클릭 가능한 영역에 대한 접근성은 어떨까요? 시각적 피드백이나 접근성 설명이 필요하지 않을까요?
39-46: 텍스트 처리 방식을 개선할 방법을 생각해보세요.
maxLines="1"과ellipsize="end"를 사용해 텍스트를 처리하고 있습니다.사용자 경험 관점에서 고려해볼 점들:
- 긴 상품명이 잘릴 때 사용자가 전체 이름을 알 수 있는 방법이 있을까요?
- 12sp 폰트 크기가 모든 사용자에게 적절할까요?
- 다국어 지원 시 텍스트 길이 차이는 어떻게 처리할까요?
더 나은 UX를 제공할 수 있는 대안을 생각해보세요.
app/src/main/java/woowacourse/shopping/domain/repository/ProductRepository.kt (2)
7-21: 비동기 처리 패턴의 일관성을 검토해보세요.현재 모든 메소드가 콜백 기반으로 설계되어 있습니다.
이 설계 결정에 대해 생각해보세요:
- 코루틴과 suspend 함수를 사용하는 것과 비교했을 때의 장단점은 무엇일까요?
Result<List<Product>?>에서 nullable List를 사용하는 이유가 있나요?- 세 메소드 모두 동일한 패턴을 사용하는 것이 일관성 측면에서 좋은가요?
특히 안드로이드에서 일반적으로 사용되는 비동기 패턴들과 비교해서 현재 선택의 근거를 생각해보세요.
17-21: 페이지네이션 메소드의 매개변수 설계를 살펴보세요.
getPagedProducts메소드에서page와size모두 nullable로 설정되어 있습니다.이 설계가 실제 사용에서 어떤 의미를 가지는지 생각해보세요:
- 두 매개변수가 모두 null일 때의 동작은 명확한가요?
- 한쪽만 null일 때는 어떻게 처리해야 할까요?
- API 사용자 입장에서 이 인터페이스가 직관적인가요?
더 명확한 의도를 표현할 수 있는 방법들을 고민해보세요.
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModelFactory.kt (2)
23-23: 예외 메시지를 추가하여 디버깅을 개선해보세요.현재
IllegalArgumentException()을 매개변수 없이 호출하고 있습니다.예외 상황에서 더 나은 개발자 경험을 제공하려면:
- 어떤 클래스가 지원되지 않는지 알 수 있는 메시지를 추가하면 어떨까요?
- 지원되는 클래스 목록을 포함하면 도움이 될까요?
예외 메시지가 향후 디버깅에 어떤 도움을 줄 수 있는지 생각해보세요.
다음과 같은 형태로 개선할 수 있습니다:
throw IllegalArgumentException("Unknown ViewModel class: ${modelClass.name}")🧰 Tools
🪛 detekt (1.23.8)
[warning] 23-23: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
14-22: 팩토리 패턴의 확장성을 고려해보세요.현재 하나의 ViewModel만 처리하는 팩토리입니다.
이 설계에 대해 생각해보세요:
- 만약 ProductDetailViewModel에 서브클래스가 생긴다면 어떻게 될까요?
- 팩토리에서 여러 ViewModel 타입을 지원해야 한다면 어떻게 구조를 개선할 수 있을까요?
@Suppress("UNCHECKED_CAST")를 피할 수 있는 더 타입 안전한 방법은 없을까요?팩토리 패턴의 유연성과 타입 안전성을 동시에 고려한 대안을 생각해보세요.
app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductListViewHolder.kt (1)
10-13: 생성자 매개변수의 접근 제어자를 고려해보세요.
eventHandler매개변수가init블록에서만 사용되는데private로 표시하지 않으셨네요. 이 매개변수가 클래스의 다른 부분에서 사용될 계획이 있으신가요? 그렇지 않다면 어떤 접근 제어자를 사용하는 것이 좋을까요?app/src/main/res/layout/activity_product_detail.xml (1)
69-69: 문자열 포맷팅 방식을 고려해보세요.
String.format을 사용한 가격 표시가 다국화를 고려할 때 적절한 방법인지 생각해보셨나요? 통화 형식이나 지역별 숫자 표시 방식에 대해 어떻게 처리할 계획이신가요?app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModelFactory.kt (1)
23-23: 예외 메시지를 더 구체적으로 작성해주세요.정적 분석 도구에서 지적한 대로, 예외에 의미 있는 메시지를 추가하는 것이 좋겠습니다. 어떤 ViewModel 클래스가 지원되지 않는지 알 수 있도록 개선해보실 수 있나요?
- throw IllegalArgumentException() + throw IllegalArgumentException("Unknown ViewModel class: ${modelClass.name}")🧰 Tools
🪛 detekt (1.23.8)
[warning] 23-23: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModelFactory.kt (1)
14-24: 예외 메시지를 더 구체적으로 만들어보세요현재
IllegalArgumentException()에 메시지가 없어서 디버깅 시 어떤 문제인지 알기 어려울 수 있습니다. 어떤 ViewModel 클래스가 지원되지 않는지 명시하는 메시지를 추가해보는 것은 어떨까요?- throw IllegalArgumentException() + throw IllegalArgumentException("Unknown ViewModel class: ${modelClass.name}")app/src/main/java/woowacourse/shopping/view/util/product/ProductViewHolder.kt (2)
17-22: 클릭 리스너 설정의 중복을 개선할 수 있습니다현재 수량 증가/감소 버튼에 대한 클릭 리스너를 개별적으로 설정하고 있습니다. 코드의 가독성과 유지보수성을 높이기 위해 어떤 방법을 사용할 수 있을까요?
힌트:
- 데이터 바인딩에서 직접 메서드를 바인딩하는 방법
- 람다 표현식을 더 간결하게 작성하는 방법
이러한 방법들을 고려해보세요.
25-32: bind 메서드의 데이터 설정 순서를 고려해보세요
currentItem설정과 binding 데이터 설정의 순서가 중요할 수 있습니다. 데이터 바인딩의 실행 시점과 클릭 이벤트가 발생하는 시점을 고려했을 때, 어떤 순서가 더 안전할까요?app/src/main/java/woowacourse/shopping/domain/repository/RecentProductRepository.kt (1)
5-18: Repository 패턴이 잘 구현되어 있습니다인터페이스가 명확하고 각 메서드의 책임이 잘 분리되어 있습니다.
Result<T>타입을 사용한 에러 처리도 좋은 접근입니다.다만 비동기 처리 방식에 대해 고민해볼 점이 있습니다. 현재 콜백 기반 방식을 사용하고 있는데, Kotlin Coroutines의
suspend함수를 고려해본 적이 있나요?suspend 함수의 장점과 현재 콜백 방식의 장점을 비교해보고, 프로젝트의 아키텍처에 어떤 방식이 더 적합한지 생각해보세요.
app/src/test/java/woowacourse/shopping/fixture/FakeRecentProductRepository.kt (2)
6-8: 테스트 픽스처의 스레드 안전성을 고려해보세요현재
mutableListOf를 사용하고 있는데, 멀티스레드 환경에서 테스트를 실행할 경우 동시성 문제가 발생할 수 있습니다. 단순한 단위 테스트에서는 문제없지만, 통합 테스트나 병렬 테스트 실행 시 고려해볼 필요가 있습니다.
20-24: 페이징 로직의 정확성을 확인해보세요
sortedByDescending으로 최신 순 정렬 후drop(offset).take(limit)으로 페이징을 구현했는데, 이는 올바른 접근입니다. 하지만 실제 사용 시나리오에서offset이 리스트 크기를 초과할 경우의 동작을 테스트해보는 것이 좋겠습니다.app/src/main/java/woowacourse/shopping/data/service/CartProductApiService.kt (1)
20-22: accept 헤더의 중복 설정을 개선해보세요모든 메소드에서 동일한
accept헤더를 설정하고 있습니다. 인터셉터나 기본 헤더 설정을 통해 중복을 제거하는 것이 좋겠습니다.Retrofit 인스턴스 설정에서 공통 헤더를 추가하는 방법을 고려해보세요:
// RetrofitInstance에서 .addInterceptor { chain -> val request = chain.request().newBuilder() .addHeader("accept", "*/*") .build() chain.proceed(request) }Also applies to: 27-28, 33-34, 39-40, 44-46
app/src/main/java/woowacourse/shopping/data/repository/ProductRepositoryImpl.kt (1)
8-10: 의존성 주입 구조를 개선해보세요현재는 RemoteDataSource만 사용하고 있지만, 향후 로컬 캐싱을 추가할 때를 대비해 인터페이스 기반 의존성 주입을 고려해볼 수 있습니다.
class ProductRepositoryImpl( private val remoteDataSource: ProductDataSource, // 향후 추가: private val localDataSource: ProductDataSource? = null ) : ProductRepositoryapp/src/main/res/layout/fragment_cart_product_recommend.xml (3)
24-34: 추천 라벨의 접근성 개선을 고려해보세요텍스트 크기가 24sp로 큰 편이고 굵은 글씨체를 사용하고 있습니다. 시각적 계층 구조는 좋지만, 접근성을 위해
contentDescription이나android:importantForAccessibility속성 추가를 고려해보세요.
46-55: RecyclerView의 수직 중앙 정렬 방식을 개선해보세요.현재 RecyclerView가 화면 중앙에 위치하도록 top과 bottom을 모두 연결하고 있는데, 이는 추천 상품 목록의 높이가 고정적이지 않을 때 문제가 될 수 있습니다.
추천 타이틀과 설명 아래에 고정된 여백을 두고 RecyclerView를 배치하는 것은 어떨까요? 현재 구조에서 어떤 문제점이 발생할 수 있는지 생각해보세요.
64-64: String.format을 data binding에서 사용할 때의 고려사항은 무엇일까요?Data binding에서 String.format을 직접 사용하고 있는데, 이 방식이 가지는 장단점을 한 번 생각해보세요. ViewModel에서 포맷팅된 문자열을 제공하는 방식과 비교했을 때 어떤 차이가 있을까요?
app/src/main/java/woowacourse/shopping/view/util/SingleLiveData.kt (4)
15-21: 스레드 안전성 고려사항
setValue와postValue메서드가MutableLiveData의 해당 메서드들을 직접 호출하고 있습니다. LiveData의 스레드 안전성을 활용하고 있다는 점은 좋지만,Event객체 생성 과정에서의 메모리 할당을 고려해보세요.빈번한 이벤트 발생 시 GC 압박이 될 수 있으니, 이벤트 풀링이나 재사용 패턴을 고려해볼 수 있을지 생각해보시기 바랍니다.
32-37: observePeek 메서드의 용도 명확화
observePeek메서드는 이벤트 소비 여부와 관계없이 항상 콜백을 호출합니다. 이는 SingleLiveData의 "single consumption" 개념과 상충할 수 있습니다.이 메서드의 사용 시나리오가 무엇인지, 그리고 일반적인
observe메서드와의 차이점을 명확히 하는 주석을 추가하는 것을 고려해보세요.
23-23: getValue() 메서드의 null 반환 처리를 개선해보세요.현재 getValue()가 null을 반환할 수 있는데, 이는 Event가 이미 소비되었거나 값이 설정되지 않았을 때입니다. 호출하는 쪽에서 이 두 상황을 구분할 필요가 있는지 생각해보세요. API 사용자가 더 명확하게 이해할 수 있는 방법은 없을까요?
25-37: observe와 observePeek의 차이점을 문서화해보세요.두 observe 메서드의 동작 차이가 중요한데, 메서드명만으로는 그 차이를 명확히 알기 어렵습니다. 각각 언제 사용해야 하는지에 대한 가이드라인을 제공하는 것이 좋겠습니다. 특히 observePeek 사용 시 주의사항은 무엇일까요?
app/src/test/java/woowacourse/shopping/fixture/FakeProductRepository.kt (2)
8-16: 테스트 데이터의 다양성 개선현재 모든 가짜 상품이 동일한 패턴(
imageUrl은 빈 문자열,name은 "Product $index",price는 순차적)으로 생성됩니다. 실제 테스트 시나리오를 더 잘 반영하기 위해 데이터의 다양성을 고려해보세요.예를 들어, 일부 상품의 이름을 다르게 하거나, 가격에 변동을 주는 것을 생각해볼 수 있습니다.
8-16: 테스트 데이터의 다양성을 고려해보세요.현재 모든 상품이 동일한 패턴(빈 이미지 URL, "Product $index" 이름, 순차적 가격)을 가지고 있습니다. 실제 시나리오를 더 잘 반영하려면 어떤 다양성이 필요할까요? 예를 들어, 가격 범위나 이름 패턴을 다양화하면 어떤 테스트 케이스들을 더 잘 검증할 수 있을까요?
app/src/main/java/woowacourse/shopping/view/cart/ShoppingCartActivity.kt (1)
60-60: 액션바 제목의 다국어화 고려해보기현재 액션바 제목이 영어로 하드코딩되어 있습니다. 애플리케이션의 다국어 지원 계획은 어떤가요? string 리소스 파일을 활용하는 방안을 검토해보시면 어떨까요?
app/src/main/java/woowacourse/shopping/data/dto/response/ProductResponseDto.kt (1)
6-30: DTO 속성 순서와 가독성 개선 방안현재 DTO의 속성들이 어떤 기준으로 정렬되어 있는지 생각해보세요. 일반적으로 DTO에서 속성을 배치할 때 고려할 수 있는 기준들은 무엇이 있을까요? (예: 중요도, 알파벳 순서, 논리적 그룹핑)
더 읽기 쉬운 구조를 만들기 위해 관련된 속성들을 함께 그룹화하는 방법을 고민해보시면 어떨까요?
app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/ProductCatalogItem.kt (2)
6-8: Sealed Class와 ViewType Enum의 설계 검토Sealed class의 각 하위 클래스가 이미 타입 안전성을 제공하는데, 별도의 ViewType enum이 필요한 이유를 생각해보세요.
RecyclerView Adapter에서
getItemViewType()을 구현할 때 어떤 방식이 더 효율적일지 고민해보시면 어떨까요? Sealed class의 장점을 최대한 활용하는 방법을 찾아보세요.
13-16: 확장 가능성 고려하기현재 ProductItem에 quantity 정보가 포함되어 있습니다. 향후 이 카탈로그 아이템에 추가될 수 있는 정보들(예: 할인율, 재고 상태, 즐겨찾기 여부 등)을 미리 생각해보시고, 이런 확장을 어떻게 대응할 것인지 구조를 설계해보시면 어떨까요?
app/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductViewHolder.kt (1)
32-36: 인터페이스 설계의 일관성EventHandler 인터페이스가
QuantityControlEventHandler<CartProduct>를 확장하면서 추가 메서드들을 정의하고 있습니다.이런 인터페이스 상속 구조에서 일관성을 유지하기 위해 고려해야 할 점들은 무엇일까요? 메서드 네이밍 컨벤션이나 매개변수 패턴을 비교해보시면 어떨까요?
app/src/main/res/layout/item_product.xml (3)
24-28: 하드코딩된 크기 값을 리소스로 관리하는 것을 고려해보세요.
176dp,18dp,10dp등의 하드코딩된 값들을dimens.xml에 정의하면 일관성과 유지보수성이 향상됩니다. 다양한 화면 크기에서의 반응형 디자인도 고려해볼 수 있습니다.
30-39: 이미지 로딩 실패 시 대체 이미지를 추가해보세요.현재
tools:src만 설정되어 있는데, 실제 이미지 로딩 실패 시를 대비한android:src또는 커스텀 바인딩 어댑터에서 에러 처리를 고려해보세요.
80-91: 접근성을 위한 contentDescription을 추가해보세요.플러스 버튼에
android:contentDescription을 추가하여 스크린 리더 사용자도 기능을 이해할 수 있도록 해보세요.app/src/main/java/woowacourse/shopping/data/datasource/remote/CartProductRemoteDataSource.kt (1)
62-64: Location 헤더 파싱에서 예외 처리를 개선해보세요.현재
IllegalArgumentException을 던지고 있는데, 더 구체적인 에러 메시지나 다른 예외 타입을 고려해보세요. 디버깅에 도움이 될 것입니다.- ?: throw IllegalArgumentException() + ?: throw IllegalStateException("Location 헤더에서 cart product ID를 추출할 수 없습니다")app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendFragment.kt (2)
20-22: 생성자에서 ShoppingApplication 직접 주입 방식을 개선해보세요.Fragment 생성자에서 Application을 직접 주입받는 것보다는 Fragment Factory 패턴이나 Hilt/Dagger 같은 DI 프레임워크를 활용하는 것이 테스트하기 용이하고 표준적인 안드로이드 아키텍처 패턴입니다.
Fragment Factory를 통해 의존성을 주입받는 방법을 고려해보시겠어요?
119-123: Bundle에서 원시 타입에 putSerializable 사용을 최적화하세요.Int 타입에 대해
putSerializable을 사용하는 것보다putInt를 사용하는 것이 더 효율적입니다.- putSerializable(KEY_TOTAL_PRICE, totalPrice) - putSerializable(KEY_TOTAL_COUNT, totalCount) + totalPrice?.let { putInt(KEY_TOTAL_PRICE, it) } + totalCount?.let { putInt(KEY_TOTAL_COUNT, it) }읽을 때도
getInt를 사용하면 됩니다.app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/ProductAdapter.kt (1)
30-38: 타입 캐스팅 시 안전성을 고려해보세요.현재
when문에서 타입 체크 후 강제 캐스팅을 하고 있는데, 혹시라도 타입이 맞지 않을 경우 ClassCastException이 발생할 수 있습니다.더 안전한 방법으로 스마트 캐스팅을 활용하거나, sealed class의 특성을 이용해보시겠어요?
when (val item = getItem(position)) { is ProductCatalogItem.RecentProductsItem -> { require(holder is RecentProductListViewHolder) holder.bind(item) } // ... }이런 방식으로 타입 안전성을 더 높일 수 있습니다.
app/src/main/java/woowacourse/shopping/data/datasource/remote/ProductRemoteDataSource.kt (1)
58-59: 에러 처리 로직을 개선할 수 있습니다.하나의 상품 조회가 실패하면 전체 결과가 실패로 처리됩니다. 이것이 의도된 동작인지 검토해보세요.
일부 상품 조회가 실패해도 성공한 상품들은 반환하는 것이 사용자 경험상 더 나을 수 있습니다. 어떤 정책이 앱의 요구사항에 더 적합한지 생각해보세요.
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModel.kt (1)
66-66: 에러 처리 방식을 개선해보세요.현재 모든 에러가
Log.e로만 처리되고 있습니다.사용자에게 에러 상황을 알리는 방법을 고려해보세요. Toast, Snackbar, 또는 에러 상태를 나타내는 LiveData를 추가하는 등의 방법이 있습니다. 각 에러 타입별로 어떤 처리가 적절한지 생각해보시기 바랍니다.
Also applies to: 80-80, 87-87
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModel.kt (2)
111-118: 초기 로딩 로직을 개선할 수 있습니다.초기 로딩 시
PRODUCT_SIZE_LIMIT * (page + 1)로 여러 페이지를 한 번에 로드하고 있습니다.이 방식이 성능상 적절한지 검토해보세요. 네트워크 비용과 메모리 사용량을 고려했을 때, 실제로 필요한 만큼만 로드하는 것이 더 나을 수 있습니다. 현재 요구사항에서 이런 일괄 로딩이 꼭 필요한지 생각해보시기 바랍니다.
145-158: 장바구니 상태 관리 로직이 복잡합니다.로컬
cartProducts컬렉션과 Repository 간의 동기화가 복잡합니다.Repository에서 최신 상태를 가져와서 UI를 업데이트하는 방식이나, 단일 진실 공급원(Single Source of Truth) 패턴을 고려해보세요. 현재 방식에서 상태 불일치가 발생할 가능성은 없는지 검토해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModel.kt (1)
63-92: 중첩된 콜백을 코루틴으로 개선하면 어떨까요?현재 콜백 지옥(callback hell) 패턴이 보입니다. 코루틴을 사용하면 더 읽기 쉽고 에러 처리가 용이한 코드를 작성할 수 있습니다.
고려해볼 점:
viewModelScope.launch를 사용한 구조화된 동시성suspend함수로 리포지토리 메서드 변경- try-catch로 통합된 에러 처리
app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModel.kt (2)
77-91: 마지막 아이템 삭제 시 페이지 처리 로직이 훌륭합니다페이지의 마지막 아이템을 삭제할 때 이전 페이지로 이동하는 로직이 잘 구현되어 있습니다. 다만 삭제 실패 시 처리가 필요합니다.
삭제 실패 시 사용자에게 알림을 제공하는 것을 고려해보세요.
119-134: 전체 선택 로직이 복잡하지만 정확합니다현재 페이지의 아이템만 선택/해제하는 로직이 올바르게 구현되어 있습니다. 복잡도를 줄이기 위해 헬퍼 메서드로 분리하는 것을 고려해보세요.
app/src/main/res/layout/item_cart_product.xml (1)
65-65: 커스텀 바인딩 어댑터 사용에 대한 이해도를 확인해보세요.
app:imageUrl속성을 사용하고 있는데, 이것이 어떻게 동작하는지 이해하고 계신가요? BindingAdapter가 이미지 로딩 실패나 네트워크 오류 상황을 어떻게 처리하는지 확인해보는 것이 좋겠습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (107)
README.md(1 hunks)app/build.gradle.kts(5 hunks)app/src/main/AndroidManifest.xml(2 hunks)app/src/main/java/woowacourse/shopping/MainActivity.kt(0 hunks)app/src/main/java/woowacourse/shopping/ShoppingApplication.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/ShoppingCartDatabase.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/TokenProvider.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dao/RecentProductDao.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/datasource/local/RecentProductLocalDataSource.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/datasource/remote/CartProductRemoteDataSource.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/datasource/remote/ProductRemoteDataSource.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/request/CartProductQuantityRequestDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/request/CartProductRequestDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/response/CartProductDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/response/CartProductQuantityResponseDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/response/CartProductResponseDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/response/PageableDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/response/ProductDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/response/ProductResponseDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/dto/response/SortDto.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/entity/RecentProductEntity.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/model/PagedResult.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/network/RequestInterceptor.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/network/RetrofitInstance.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/repository/CartProductRepositoryImpl.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/repository/ProductRepositoryImpl.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/repository/RecentProductRepositoryImpl.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/service/CartProductApiService.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/service/ProductApiService.kt(1 hunks)app/src/main/java/woowacourse/shopping/data/util/DataMapper.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/model/CartProduct.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/model/Product.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/model/RecentProduct.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/repository/CartProductRepository.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/repository/ProductRepository.kt(1 hunks)app/src/main/java/woowacourse/shopping/domain/repository/RecentProductRepository.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/ShoppingCartActivity.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/ShoppingCartFragmentFactory.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendEventHandler.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendFragment.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModelFactory.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/recommend/adapter/RecommendedProductAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/recommend/adapter/RecommendedProductItem.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectEventHandler.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectFragment.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModelFactory.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductItem.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogActivity.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogEventHandler.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModelFactory.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/LoadMoreViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/ProductAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/ProductCatalogItem.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductListViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailActivity.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailEventHandler.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModel.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModelFactory.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/util/BindingAdapter.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/util/Event.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/util/MutableSingleLiveData.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/util/SerializableExt.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/util/SingleLiveData.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/util/product/ProductViewHolder.kt(1 hunks)app/src/main/java/woowacourse/shopping/view/util/product/QuantityControlEventHandler.kt(1 hunks)app/src/main/res/drawable/ic_add.xml(1 hunks)app/src/main/res/drawable/ic_cancel.xml(1 hunks)app/src/main/res/drawable/ic_shopping_cart.xml(1 hunks)app/src/main/res/drawable/style_left_arrow.xml(1 hunks)app/src/main/res/drawable/style_left_button.xml(1 hunks)app/src/main/res/drawable/style_product_edge.xml(1 hunks)app/src/main/res/drawable/style_right_arrow.xml(1 hunks)app/src/main/res/drawable/style_right_button.xml(1 hunks)app/src/main/res/drawable/style_round_primary.xml(1 hunks)app/src/main/res/drawable/style_round_white.xml(1 hunks)app/src/main/res/layout/activity_main.xml(0 hunks)app/src/main/res/layout/activity_product_catalog.xml(1 hunks)app/src/main/res/layout/activity_product_detail.xml(1 hunks)app/src/main/res/layout/activity_shopping_cart.xml(1 hunks)app/src/main/res/layout/custom_toolbar_shopping_cart.xml(1 hunks)app/src/main/res/layout/fragment_cart_product_recommend.xml(1 hunks)app/src/main/res/layout/fragment_cart_product_select.xml(1 hunks)app/src/main/res/layout/item_cart_product.xml(1 hunks)app/src/main/res/layout/item_cart_product_skeleton.xml(1 hunks)app/src/main/res/layout/item_load_more.xml(1 hunks)app/src/main/res/layout/item_product.xml(1 hunks)app/src/main/res/layout/item_product_skeleton.xml(1 hunks)app/src/main/res/layout/item_quantity_control.xml(1 hunks)app/src/main/res/layout/item_recent_product.xml(1 hunks)app/src/main/res/layout/item_recent_product_list.xml(1 hunks)app/src/main/res/menu/menu_shopping_cart.xml(1 hunks)app/src/main/res/values/colors.xml(1 hunks)app/src/main/res/values/strings.xml(1 hunks)app/src/main/res/values/themes.xml(1 hunks)app/src/test/java/woowacourse/shopping/domain/model/ProductTest.kt(1 hunks)app/src/test/java/woowacourse/shopping/fixture/FakeCartProductRepository.kt(1 hunks)app/src/test/java/woowacourse/shopping/fixture/FakeProductRepository.kt(1 hunks)app/src/test/java/woowacourse/shopping/fixture/FakeRecentProductRepository.kt(1 hunks)app/src/test/java/woowacourse/shopping/viewmodel/InstantTaskExecutorExtension.kt(1 hunks)app/src/test/java/woowacourse/shopping/viewmodel/LiveDataExtension.kt(1 hunks)
⛔ Files not processed due to max files limit (6)
- app/src/test/java/woowacourse/shopping/viewmodel/cart/CartProductSelectViewModelTest.kt
- app/src/test/java/woowacourse/shopping/viewmodel/product/ProductCatalogViewModelTest.kt
- app/src/test/java/woowacourse/shopping/viewmodel/product/ProductDetailViewModelTest.kt
- build.gradle.kts
- gradle/libs.versions.toml
- settings.gradle.kts
💤 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/model/PagedResult.ktapp/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductItem.ktapp/src/main/java/woowacourse/shopping/view/cart/ShoppingCartFragmentFactory.ktapp/src/main/java/woowacourse/shopping/data/dto/response/CartProductQuantityResponseDto.ktapp/src/main/java/woowacourse/shopping/data/dto/response/PageableDto.ktapp/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendEventHandler.ktapp/src/main/java/woowacourse/shopping/data/dto/response/SortDto.ktapp/src/main/java/woowacourse/shopping/data/dto/request/CartProductRequestDto.ktapp/src/main/java/woowacourse/shopping/data/dto/request/CartProductQuantityRequestDto.ktapp/src/main/java/woowacourse/shopping/domain/model/Product.ktapp/src/main/java/woowacourse/shopping/data/entity/RecentProductEntity.ktapp/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailEventHandler.ktapp/src/main/java/woowacourse/shopping/domain/model/CartProduct.ktapp/src/main/java/woowacourse/shopping/view/util/SerializableExt.ktapp/src/main/java/woowacourse/shopping/data/dao/RecentProductDao.ktapp/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectEventHandler.ktapp/src/main/java/woowacourse/shopping/data/ShoppingCartDatabase.ktapp/src/main/java/woowacourse/shopping/view/util/product/QuantityControlEventHandler.ktapp/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModelFactory.ktapp/src/main/java/woowacourse/shopping/view/util/BindingAdapter.ktapp/src/main/java/woowacourse/shopping/view/cart/recommend/adapter/RecommendedProductItem.ktapp/src/main/java/woowacourse/shopping/view/util/MutableSingleLiveData.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogActivity.ktapp/src/main/java/woowacourse/shopping/data/TokenProvider.ktapp/src/main/java/woowacourse/shopping/data/network/RequestInterceptor.ktapp/src/main/java/woowacourse/shopping/domain/model/RecentProduct.ktapp/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModelFactory.ktapp/src/main/java/woowacourse/shopping/data/dto/response/CartProductDto.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogEventHandler.ktapp/src/main/java/woowacourse/shopping/domain/repository/ProductRepository.ktapp/src/main/java/woowacourse/shopping/data/network/RetrofitInstance.ktapp/src/main/java/woowacourse/shopping/data/util/DataMapper.ktapp/src/main/java/woowacourse/shopping/domain/repository/RecentProductRepository.ktapp/src/test/java/woowacourse/shopping/viewmodel/LiveDataExtension.ktapp/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectFragment.ktapp/src/main/java/woowacourse/shopping/data/datasource/local/RecentProductLocalDataSource.ktapp/src/test/java/woowacourse/shopping/domain/model/ProductTest.ktapp/src/main/java/woowacourse/shopping/data/service/CartProductApiService.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductListViewHolder.ktapp/src/main/java/woowacourse/shopping/ShoppingApplication.ktapp/src/main/java/woowacourse/shopping/view/cart/recommend/adapter/RecommendedProductAdapter.ktapp/src/main/java/woowacourse/shopping/data/datasource/remote/CartProductRemoteDataSource.ktapp/src/main/java/woowacourse/shopping/view/util/Event.ktapp/src/test/java/woowacourse/shopping/viewmodel/InstantTaskExecutorExtension.ktapp/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModelFactory.ktapp/src/main/java/woowacourse/shopping/data/repository/ProductRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/data/dto/response/ProductDto.ktapp/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductViewHolder.ktapp/src/main/java/woowacourse/shopping/data/repository/CartProductRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductAdapter.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/adapter/LoadMoreViewHolder.ktapp/src/main/java/woowacourse/shopping/domain/repository/CartProductRepository.ktapp/src/test/java/woowacourse/shopping/fixture/FakeRecentProductRepository.ktapp/src/main/java/woowacourse/shopping/view/util/SingleLiveData.ktapp/src/main/java/woowacourse/shopping/data/dto/response/ProductResponseDto.ktapp/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailActivity.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductAdapter.ktapp/src/main/java/woowacourse/shopping/data/service/ProductApiService.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductViewHolder.ktapp/src/main/java/woowacourse/shopping/view/cart/ShoppingCartActivity.ktapp/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendFragment.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModelFactory.ktapp/src/main/java/woowacourse/shopping/view/util/product/ProductViewHolder.ktapp/src/main/java/woowacourse/shopping/data/datasource/remote/ProductRemoteDataSource.ktapp/src/test/java/woowacourse/shopping/fixture/FakeCartProductRepository.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/adapter/ProductAdapter.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/adapter/ProductCatalogItem.ktapp/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModel.ktapp/src/main/java/woowacourse/shopping/data/dto/response/CartProductResponseDto.ktapp/src/test/java/woowacourse/shopping/fixture/FakeProductRepository.ktapp/src/main/java/woowacourse/shopping/data/repository/RecentProductRepositoryImpl.ktapp/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModel.ktapp/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModel.ktapp/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModel.kt
🧬 Code Graph Analysis (6)
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogActivity.kt (2)
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailActivity.kt (4)
setUpView(52-60)initBindings(62-72)initObservers(74-84)onOptionsItemSelected(43-50)app/src/main/java/woowacourse/shopping/view/cart/ShoppingCartActivity.kt (2)
setUpView(47-55)onOptionsItemSelected(38-45)
app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectFragment.kt (2)
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogActivity.kt (2)
initBindings(109-112)initObservers(114-123)app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendFragment.kt (2)
initBindings(81-86)initObservers(88-102)
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailActivity.kt (2)
app/src/main/java/woowacourse/shopping/view/cart/ShoppingCartActivity.kt (2)
setUpView(47-55)newIntent(58-58)app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogActivity.kt (3)
setUpView(76-84)initBindings(109-112)initObservers(114-123)
app/src/main/java/woowacourse/shopping/view/cart/ShoppingCartActivity.kt (2)
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailActivity.kt (1)
setUpView(52-60)app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogActivity.kt (1)
setUpView(76-84)
app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendFragment.kt (1)
app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectFragment.kt (2)
initBindings(56-78)initObservers(80-84)
app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModel.kt (1)
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModel.kt (1)
updateCartProduct(145-158)
🪛 detekt (1.23.8)
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModelFactory.kt
[warning] 23-23: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModelFactory.kt
[warning] 23-23: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
🔇 Additional comments (84)
app/src/main/res/drawable/ic_add.xml (3)
1-1: 벡터 드로어블 기본 설정 확인
android:width,android:height,android:viewportWidth/Height,android:tint속성이 24dp 기본 Material 아이콘 가이드라인에 맞게 잘 정의되었습니다.
3-3: 플러스(+) 모양 경로 정의 적절
android:pathData가 의도한 플러스 기호 형태를 정확히 표현하고 있습니다.
5-5: 닫는 태그 올바름
</vector>태그가 문제없이 닫혀 있어 리소스 유효성에 이상이 없습니다.app/src/main/res/drawable/ic_cancel.xml (1)
1-9: 적절한 벡터 드로어블 정의입니다. 아이콘 크기, 뷰포트, 경로 데이터와 채움 색상이 모두 올바르게 설정되어 있습니다.app/src/main/res/drawable/ic_shopping_cart.xml (1)
1-9: 쇼핑 카트 아이콘 벡터 드로어블이 정상적으로 추가되었습니다. 크기, 뷰포트, 경로 데이터 및 흰색 채움이 의도대로 설정되어 있습니다.app/src/main/res/values/colors.xml (1)
11-15: 새로운 컬러 리소스가 잘 정의되었습니다. 앱 전반의 일관된 테마 구축에 기여하며 네이밍 컨벤션에도 부합합니다.app/src/main/res/drawable/style_round_primary.xml (1)
1-5: 원형 기본 스타일 드로어블이 올바르게 정의되었습니다. @color/primary_color 참조를 통해 일관된 테마 색상을 적용했습니다.app/src/main/res/drawable/style_round_white.xml (1)
1-5: 화이트 타원 드로어블 정의 승인
style_round_white.xml은 흰색 배경의 타원형 쉐이프를 올바르게 정의하고 있어, UI 컴포넌트에 일관된 스타일로 적용 가능하므로 문제 없습니다.app/src/main/java/woowacourse/shopping/data/model/PagedResult.kt (1)
1-6: 제네릭 페이징 결과 데이터 클래스 승인
PagedResult<T>는 항목 목록과 추가 페이지 여부를 명확히 표현하며 Kotlin 관례에 부합하는 간결한 구현입니다.app/src/main/java/woowacourse/shopping/view/cart/recommend/adapter/RecommendedProductItem.kt (1)
1-8: 추천 상품 아이템 데이터 클래스 승인
RecommendedProductItem은 불변 속성으로 필요한 정보만 깔끔하게 담고 있어, RecyclerView 어댑터에서 안전하게 사용 가능합니다.app/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductItem.kt (1)
5-8: isSelected 상태 관리 전략 검토
isSelected를val로 선언하셨는데, 선택 상태를 변경할 때copy()를 통해 새로운 인스턴스를 생성하실 계획인가요?
만약 빈번한 토글이 필요하다면var로 선언하거나 별도의 상태 관리 로직을 도입하는 것을 고려해보시면 어떨까요?app/src/main/java/woowacourse/shopping/data/dto/request/CartProductQuantityRequestDto.kt (1)
6-10: DTO 유효성 검증 방향 점검
quantity필드에 0 혹은 음수 값이 전달되는 시나리오가 있는지 API 스펙을 재확인해보셨나요?
필요하다면 DTO 레벨에서 간단한 검증 로직을 추가하거나 문서화해 두시면 안전합니다.app/src/main/java/woowacourse/shopping/data/dto/response/SortDto.kt (1)
6-14: 정렬 DTO 정의 적절
@Serializable과@SerialName이 올바르게 사용되었습니다. 추가 필드는 필요 없어 보입니다.app/src/main/java/woowacourse/shopping/data/entity/RecentProductEntity.kt (1)
9-10: DAO 충돌 전략 검증 요청
productId를 PK로 사용하는데, 중복 저장 시viewedAt갱신을 위해 DAO에서OnConflictStrategy.REPLACE같은 전략을 적용하셨는지 확인해 주세요.app/src/main/res/drawable/style_left_arrow.xml (1)
1-24:✅ Verification successful
상태 기반 drawable 리소스가 올바르게 구현되었습니다.
왼쪽 화살표의 enabled/disabled 상태를 적절히 처리하고 있습니다. 다만 사용된 색상 리소스들이 colors.xml에 정의되어 있는지 확인해보세요.
다음 스크립트로 색상 리소스 정의를 확인할 수 있습니다:
🏁 Script executed:
#!/bin/bash # 색상 리소스가 정의되어 있는지 확인 rg -A 2 -B 2 "primary_color|white_gray" app/src/main/res/values/Length of output: 509
drawable 리소스 및 색상 정의 검증 완료
- app/src/main/res/drawable/style_left_arrow.xml
• enabled/disabled 상태별 셀렉터와 왼쪽 모서리 라운드 설정이 올바르게 구현되어 있습니다.- app/src/main/res/values/colors.xml
•<color name="primary_color">#FF04C09E</color>
•<color name="white_gray">#FFAAAAAA</color>
두 색상 리소스가 정상적으로 정의되어 있어 추가 조치가 필요 없습니다.README.md (1)
1-45: 문서화가 체계적이고 포괄적으로 개선되었습니다.기능 요구사항과 구현 상태가 명확하게 정리되어 있어 프로젝트의 진행 상황을 쉽게 파악할 수 있습니다. 특히 단계별 개발 계획과 상세한 체크리스트가 잘 구성되어 있습니다.
app/src/test/java/woowacourse/shopping/viewmodel/LiveDataExtension.kt (1)
9-10: 라이선스 정보 표기를 확인하세요.Google의 코드를 사용할 때 라이선스 정보를 명시한 것은 좋습니다.
프로젝트의 라이선스 정책을 확인해보세요:
- 프로젝트 전체의 라이선스 정책과 일치하는가?
- 이런 외부 코드 사용에 대한 가이드라인이 있는가?
적절한 라이선스 관리 방법에 대해 팀과 논의해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/data/dto/response/ProductDto.kt (2)
7-19: 깔끔한 DTO 구현입니다!kotlinx.serialization 사용과 명시적인 @SerialName 지정이 잘 되어 있습니다. PR 설명에서 언급한 대로 Gson 대신 kotlinx.serialization을 선택한 것이 좋은 결정인 것 같습니다.
21-28: 확장 함수로 변환 로직을 분리한 것이 좋습니다.DTO에서 도메인 모델로의 변환을 확장 함수로 처리한 것이 깔끔합니다. 도메인 계층의 순수성을 유지하면서도 변환 로직을 명확하게 분리했네요.
app/src/main/res/layout/activity_product_catalog.xml (1)
22-71: Shimmer 로딩 효과 구현이 좋습니다!스켈레톤 UI를 통한 로딩 상태 표시가 잘 구현되어 있습니다. 사용자 경험 향상에 도움이 될 것 같습니다.
app/src/main/res/values/strings.xml (1)
3-20: 한국어 문자열 리소스가 잘 정리되어 있습니다!쇼핑 기능에 필요한 문자열들이 체계적으로 추가되어 있고, 포맷 지정자와 특수문자 처리도 적절하게 되어 있습니다. 다국어 지원을 고려한 좋은 구조입니다.
app/src/main/java/woowacourse/shopping/data/ShoppingCartDatabase.kt (1)
23-34: Thread-safe한 싱글톤 패턴이 잘 구현되어 있습니다!
@Volatile과synchronized블록을 사용한 double-checked locking 패턴과applicationContext사용으로 메모리 누수를 방지한 것이 좋습니다.app/src/main/java/woowacourse/shopping/data/dto/response/CartProductDto.kt (1)
17-22: 확장 함수의 에러 처리를 고려해보세요
product.toProduct()호출이 실패할 가능성은 없을까요? 현재 코드에서는 어떤 예외 상황이 발생할 수 있는지, 그리고 이를 어떻게 처리할지 생각해보세요.도메인 모델로의 변환 과정에서 발생할 수 있는 문제들을 어떻게 사전에 방지하거나 적절히 처리할 수 있을까요?
app/src/test/java/woowacourse/shopping/viewmodel/InstantTaskExecutorExtension.kt (1)
9-31: 테스트에서 비동기 처리를 동기화하는 이유를 이해하고 계신가요?이 확장(Extension)이
ArchTaskExecutor의 동작을 어떻게 변경하는지, 그리고 왜 테스트에서 이런 변경이 필요한지 생각해보세요.실제 앱에서 LiveData가 백그라운드 스레드에서 작동할 때와 테스트 환경에서의 차이점을 이해하면, 이런 유틸리티의 필요성을 더 잘 파악할 수 있습니다.
추가 학습: Android Architecture Components의 테스트 가이드를 참고해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/view/util/Event.kt (2)
6-21: 스레드 안전성에 대해 고민해보셨나요?현재 구현에서
hasBeenHandled필드의 읽기와 쓰기가 원자적이지 않습니다. 멀티스레드 환경에서 어떤 문제가 발생할 수 있을까요?Android의 LiveData가 메인 스레드에서 관찰되는 특성을 고려하여, 이 구현이 안전한지 생각해보세요. 만약 동시성 문제가 있다면 어떤 방법으로 해결할 수 있을까요?
힌트:
@Volatile어노테이션이나 동기화 메커니즘을 조사해보세요.
23-26: 이벤트 패턴의 사용 시나리오를 이해하고 계신가요?
peekContent()와getContentIfNotHandled()의 차이점을 명확히 이해하고 계신가요? 각각 어떤 상황에서 사용해야 하는지 생각해보세요.실제 앱에서 이벤트가 여러 번 처리되면 안 되는 경우들(예: 결제 처리, 화면 전환 등)을 고려해보시고, 이 패턴이 그런 문제들을 어떻게 해결하는지 이해해보세요.
app/src/main/java/woowacourse/shopping/ShoppingApplication.kt (1)
14-33: 잘 구조화된 의존성 주입 설정입니다!
lazy초기화를 활용한 의존성 주입이 깔끔하게 구현되었습니다. 각 repository의 의존성이 명확하게 정의되어 있고 순환 의존성도 없어 보입니다.app/src/main/java/woowacourse/shopping/view/cart/recommend/adapter/RecommendedProductAdapter.kt (2)
8-22: RecyclerView 어댑터 모범 사례를 잘 따르고 있습니다!
ListAdapter와DiffUtil을 적절히 활용하여 효율적인 리스트 업데이트가 가능하도록 구현되었습니다.ProductViewHolder재사용과EventHandler패턴으로 코드 재사용성도 높였네요.
25-36: DiffUtil 구현이 정확합니다.
areItemsTheSame에서 ID 비교,areContentsTheSame에서 전체 객체 비교를 하는 것이 올바른 패턴입니다. 이를 통해 RecyclerView가 변경사항을 정확히 감지하고 효율적으로 업데이트할 수 있습니다.app/src/main/java/woowacourse/shopping/data/dao/RecentProductDao.kt (1)
14-18: 페이지네이션 쿼리가 잘 구현되었습니다.LIMIT과 OFFSET을 사용한 페이지네이션 구현이 적절합니다.
viewed_at DESC정렬로 최신 상품이 먼저 나오도록 한 것도 좋은 설계입니다.app/src/main/java/woowacourse/shopping/data/TokenProvider.kt (1)
30-33: Base64 인코딩 구현이 올바릅니다.Basic Authentication 표준에 맞게
name:password형태로 인코딩하고 있네요. 보안 관점에서 이 방식이 적절한지, 그리고 HTTPS를 통해서만 사용되는지 확인해보시기 바랍니다.app/src/main/java/woowacourse/shopping/data/dto/response/CartProductResponseDto.kt (1)
6-30: 페이지네이션 DTO 구조가 잘 설계되었습니다.Spring Boot의 표준 페이지네이션 응답 구조를 정확히 반영하고 있고,
kotlinx.serialization라이브러리 선택도 PR 설명에서 언급한 대로 좋은 판단입니다. 모든 필수 페이지네이션 필드들이 포함되어 있어 완전한 구조를 갖추고 있습니다.
@SerialName어노테이션으로 JSON 필드명을 명시적으로 지정한 것도 좋은 방식입니다.app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailActivity.kt (2)
52-60: 다른 액티비티와 일관된 UI 설정 패턴입니다.
setUpView()메서드의 edge-to-edge 설정이 다른 액티비티들(ShoppingCartActivity,ProductCatalogActivity)과 동일한 패턴을 따르고 있어 일관성이 좋습니다. 이런 패턴 통일성이 코드 유지보수에 도움이 됩니다.
79-83: Intent 플래그 사용을 다시 검토해보세요.
FLAG_ACTIVITY_CLEAR_TOP플래그를 사용하고 있는데, 이것이 정말 원하는 동작인지 확인해보시기 바랍니다. 이 플래그는 현재 액티비티 위의 모든 액티비티를 제거합니다.최근 본 상품으로 이동할 때 기존의 상품 상세 화면은 어떻게 처리되어야 할지 UX 관점에서 생각해보시고, 적절한 플래그를 선택하시기 바랍니다.
app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductListViewHolder.kt (2)
20-20: 아이템 애니메이션을 비활성화한 이유가 궁금합니다.
itemAnimator = null로 설정하신 특별한 이유가 있으신가요? 이런 설정이 사용자 경험에 어떤 영향을 미칠 수 있을지 생각해보셨나요?
27-36: 팩토리 메서드 패턴이 잘 구현되었습니다.ViewHolder 생성을 위한 팩토리 메서드가 표준 패턴을 잘 따르고 있습니다. 뷰 바인딩과 의존성 주입이 깔끔하게 처리되었네요.
app/src/main/res/layout/activity_product_detail.xml (2)
96-96: 잠재적인 NullPointerException을 확인해주세요.visibility 조건에서
viewmodel.lastViewedProduct.product.id에 접근하고 있는데,lastViewedProduct가 null이 아님을 먼저 확인한 후에product.id에 접근하고 있나요? 현재 조건식에서 단락 평가(short-circuit evaluation)가 올바르게 동작하는지 확인이 필요합니다.이런 복잡한 조건을 ViewModel에서 처리하는 computed property를 만드는 것은 어떨까요?
76-85: 수량 조절 컴포넌트 통합이 잘 되었습니다.include로 재사용 가능한 수량 조절 레이아웃을 깔끔하게 통합하시고, 데이터 바인딩으로 ViewModel의 quantity와 연결하신 점이 좋습니다.
app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModelFactory.kt (1)
9-13: 의존성 주입 패턴이 잘 구현되었습니다.3개의 repository를 명확하게 주입받는 구조가 좋습니다. 의존성이 명시적으로 드러나 테스트하기 쉬운 코드가 되었네요.
app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectFragment.kt (3)
62-77: Fragment 간 데이터 전달 방식을 검토해보세요.
parentFragmentManager.commit으로 fragment를 교체할 때 Bundle로 데이터를 전달하고 있습니다.현재 방식에서 사용자가 뒤로 가기를 눌렀을 때 어떤 일이 일어날까요? 데이터의 일관성은 어떻게 보장할 수 있을까요?
86-89: ViewBinding 라이프사이클 관리가 적절합니다.
onDestroyView에서 binding을 null로 설정하여 메모리 누수를 방지하는 것이 잘 구현되었습니다.
56-61: 바인딩 설정이 체계적으로 구성되었습니다.ViewModel, lifecycle owner, event handler를 적절히 연결하고 RecyclerView adapter도 깔끔하게 설정되었네요.
app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductViewHolder.kt (3)
9-15: ViewHolder 구현이 잘 되어 있습니다데이터 바인딩을 활용한 ViewHolder 구현이 깔끔합니다. init 블록에서 eventHandler를 바인딩에 설정하는 방식으로 이벤트 처리를 UI와 분리한 것이 좋은 설계입니다.
21-23: 이벤트 처리 인터페이스가 명확합니다EventHandler 인터페이스를 통해 클릭 이벤트 처리를 추상화한 것이 좋습니다. 이는 ViewHolder의 책임을 명확히 하고 테스트 가능성을 높입니다.
25-34: Factory 메서드 패턴이 적절히 적용되었습니다companion object의 from 메서드를 통해 ViewHolder 생성을 캡슐화한 것이 좋습니다. 이는 ViewHolder 생성 로직을 한 곳에 모으고 재사용성을 높입니다.
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModelFactory.kt (1)
9-13: 의존성 주입이 깔끔하게 구현되었습니다ProductCatalogViewModel에 필요한 3개의 repository를 생성자로 주입받는 구조가 좋습니다. 이는 의존성 역전 원칙을 잘 따르고 있습니다.
app/src/main/java/woowacourse/shopping/data/datasource/local/RecentProductLocalDataSource.kt (1)
6-17: 데이터 소스 계층이 명확하게 구현되었습니다DAO를 감싸는 데이터 소스 클래스로서 역할이 명확합니다. 현재는 단순한 위임 패턴이지만, 향후 로컬 데이터 처리 로직(캐싱, 변환 등)을 추가할 때 확장하기 좋은 구조입니다.
이 계층에서 추가로 고려해볼 점이 있다면:
- 에러 처리나 로깅 로직
- 데이터 변환이나 검증 로직
- 캐싱 전략
현재 단계에서는 깔끔한 구현이라고 생각합니다.
app/src/main/AndroidManifest.xml (4)
5-5: 인터넷 권한이 적절히 추가되었습니다네트워크 통신을 위한 INTERNET 권한이 올바르게 추가되었습니다.
8-8: Application 클래스 설정이 좋습니다ShoppingApplication 클래스를 통해 전역 설정과 의존성 초기화를 관리하는 것이 좋은 접근입니다.
17-17: cleartext traffic 설정을 재검토해보세요
android:usesCleartextTraffic="true"설정이 개발 목적인지 확인해보세요. 프로덕션 환경에서는 보안상 HTTPS를 사용하는 것이 권장됩니다.개발 단계에서 필요한 설정이라면:
- 주석으로 그 이유를 명시하거나
- 개발/프로덕션 빌드를 분리하여 프로덕션에서는 제거하는 것을 고려해보세요
20-35: Activity 계층 구조가 잘 설계되었습니다ProductCatalogActivity를 메인 런처로 하고, 다른 Activity들이 이를 부모로 설정한 구조가 자연스러운 네비게이션 흐름을 제공합니다.
android:exported="false"로 내부 Activity들을 적절히 보호한 것도 좋습니다.app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/recent/RecentProductAdapter.kt (2)
24-35: DiffUtil 구현이 잘 되어 있습니다
areItemsTheSame에서 product ID로 아이템을 비교하고,areContentsTheSame에서 전체 객체를 비교하는 것이 적절합니다. 이는 RecyclerView의 성능 최적화에 도움이 됩니다.ListAdapter와 DiffUtil을 올바르게 활용한 좋은 예시입니다.
8-21: 표준적인 어댑터 패턴을 잘 구현했습니다ViewHolder 생성과 바인딩 로직이 명확하고 간결합니다. 이벤트 핸들러를 생성자에서 받아 ViewHolder에 전달하는 방식도 관심사 분리 원칙을 잘 따르고 있습니다.
app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/LoadMoreViewHolder.kt (1)
8-29: 간결하고 명확한 ViewHolder 구현입니다단일 책임을 가진 ViewHolder로서 매우 잘 설계되었습니다. 데이터 바인딩을 통한 이벤트 처리와 팩토리 메서드 패턴도 일관성 있게 적용되어 있습니다.
페이징 UI 패턴에서 자주 사용되는 "더 보기" 기능을 재사용 가능한 컴포넌트로 잘 분리했습니다.
app/src/test/java/woowacourse/shopping/fixture/FakeRecentProductRepository.kt (2)
9-11: null 처리 로직이 올바르게 구현되었습니다
lastOrNull()을 사용하여 빈 리스트 상황을 적절히 처리하고 있고,Result.success로 감싸서 일관된 응답 형태를 유지하고 있습니다.
32-34: 중복 제거 로직이 올바르게 구현되었습니다같은 상품 ID를 가진 기존 항목을 제거한 후 새 항목을 추가하는 로직이 정확합니다. 이는 실제
replaceRecentProduct의 의도에 부합합니다.app/src/main/res/layout/fragment_cart_product_select.xml (1)
24-46: 로딩 상태 UI가 잘 구현되었습니다Shimmer 효과를 사용한 로딩 상태와 skeleton 레이아웃을 반복 포함하여 사용자 경험을 개선했습니다.
android:visibility바인딩도 적절합니다.app/src/main/java/woowacourse/shopping/data/service/CartProductApiService.kt (2)
18-23: API 엔드포인트 설계가 REST 원칙을 잘 따르고 있습니다GET 메소드로 장바구니 아이템을 조회하고, 페이징 파라미터를 쿼리로 받는 것이 적절합니다.
21-22: nullable 페이징 파라미터의 서버 동작을 확인해보세요
page와size파라미터가 nullable로 설정되어 있는데, 서버가 null 값을 어떻게 처리하는지 확인이 필요합니다. 기본값 적용 방식을 문서화하거나 명시적으로 처리하는 것이 좋겠습니다.app/src/main/java/woowacourse/shopping/data/repository/ProductRepositoryImpl.kt (1)
8-32: Repository 패턴이 올바르게 구현되었습니다단순한 위임 패턴으로 Repository 인터페이스를 구현하고 있어 관심사 분리가 잘 되어 있습니다. 향후 캐싱이나 로컬 데이터소스 추가 시 이 계층에서 로직을 확장할 수 있어 좋은 구조입니다.
app/src/main/res/layout/fragment_cart_product_recommend.xml (3)
46-55: RecyclerView의 수평 스크롤 설정 확인
android:orientation="horizontal"이 설정되어 있지만, 이는 RecyclerView 자체가 아닌 LayoutManager의 설정입니다. 현재app:layoutManager로 LinearLayoutManager를 지정했는데, 수평 스크롤을 위해서는 LayoutManager에서 orientation을 설정해야 합니다.이 설정이 Fragment나 Adapter에서 올바르게 처리되고 있는지 확인해보세요.
64-64: 데이터 바인딩 표현식의 null 안전성String.format과 문자열 리소스를 사용한 데이터 바인딩에서 ViewModel의 속성들이 null일 가능성을 고려해보세요.
viewmodel.totalPrice와viewmodel.totalCount가 초기화되지 않았을 때의 동작을 검토해보시기 바랍니다.Also applies to: 78-78
78-81: 주문 버튼의 접근성과 사용성을 검토해보세요.주문 버튼에서 totalCount를 표시하고 있는데, 만약 선택된 상품이 없을 때는 어떻게 처리되어야 할까요? 버튼의 활성화/비활성화 상태 관리에 대해 고민해보세요.
app/src/main/res/layout/item_cart_product.xml (4)
65-65: 커스텀 바인딩 어댑터 의존성 확인
app:imageUrl커스텀 바인딩 어댑터를 사용하고 있습니다. 이 어댑터가BindingAdapter.kt에서 올바르게 구현되어 있는지, 그리고 이미지 로딩 실패 시의 fallback 처리가 되어 있는지 확인해보세요.특히 네트워크 오류나 잘못된 URL에 대한 처리 방식을 검토해보시기 바랍니다.
71-79: 포함된 레이아웃의 데이터 전달 방식
item_quantity_control레이아웃을 포함하면서app:quantity속성으로 데이터를 전달하고 있습니다. 이 방식이 작동하려면 포함된 레이아웃에서도 해당 속성을 받을 수 있는 데이터 바인딩 변수가 정의되어 있어야 합니다.포함된 레이아웃에서 수량 변경 이벤트를 어떻게 부모로 전달하는지 확인해보세요.
71-79: Include된 수량 컨트롤과의 데이터 바인딩 연동을 확인해보세요.
item_quantity_control레이아웃을 include하고 있는데, quantity 속성만 전달하고 있습니다. 수량 변경 이벤트는 어떻게 처리되는지 확인해보세요. 포함된 레이아웃에서 발생하는 이벤트가 현재 EventHandler로 제대로 전달되는 구조인지 점검이 필요합니다.
38-45: 체크박스 상태 관리의 단방향성을 고려해보세요.체크박스가
isSelected변수에 바인딩되어 있고 클릭 이벤트는 별도 핸들러로 처리하고 있습니다. 이러한 단방향 데이터 플로우가 올바르게 구현되었는지 확인해보세요. 사용자가 체크박스를 직접 클릭했을 때와 프로그래매틱하게 상태가 변경될 때 모두 일관되게 동작하는지 검토가 필요합니다.app/src/main/java/woowacourse/shopping/view/util/SingleLiveData.kt (1)
6-13: 생성자 설계의 일관성 검토기본 생성자와 초기값을 받는 생성자가 모두
protected로 선언되어 있습니다. 이는 abstract 클래스의 특성상 적절하지만, 초기값 생성자에서 즉시Event로 래핑하는 것이 의도된 동작인지 확인해보세요.초기값이 설정된 경우 첫 번째 관찰자가 해당 이벤트를 소비할 수 있다는 의미인데, 이것이 예상된 동작인지 고려해보시기 바랍니다.
app/src/test/java/woowacourse/shopping/fixture/FakeProductRepository.kt (3)
38-45: 페이징 로직의 엣지 케이스 처리페이징 구현에서
page나size가 null일 때 빈 결과를 반환하고 있습니다. 하지만 실제 운영 환경에서는 이런 경우 기본값을 사용하거나 오류를 반환할 수 있습니다.실제
ProductRepositoryImpl의 동작과 일치하는지 확인하고, 일관된 동작을 보장하는 방법을 고려해보세요. 또한 음수 페이지나 사이즈 값에 대한 처리도 검토해보시기 바랍니다.
42-44: 페이징 계산 로직 검증
hasNext계산에서page * size + pagedItems.size < fakeProducts.size조건을 사용하고 있습니다. 이 로직이 올바른지 확인해보세요.특히 마지막 페이지에서
pagedItems.size가size보다 작을 때의 동작과, 페이지 인덱스가 0부터 시작하는지 1부터 시작하는지에 따른 계산 차이를 검토해보시기 바랍니다.
42-44:❓ Verification inconclusive
페이징 계산 로직의 정확성을 검증해보세요.
현재 페이징 로직이 올바른지 한 번 더 확인해보세요.
page * size + pagedItems.size < fakeProducts.size조건이 모든 경우에 정확히 hasNext를 계산하는지 검토가 필요합니다. 특히 마지막 페이지에서 정확히 size만큼의 아이템이 있을 때는 어떻게 동작할까요?다음 스크립트로 페이징 로직을 검증해보세요:
🏁 Script executed:
#!/bin/bash # 페이징 로직 검증을 위한 단위 테스트 확인 rg -A 10 -B 5 "FakeProductRepository.*test|test.*FakeProductRepository" --type kotlinLength of output: 87
페이징 로직 단위 테스트 추가 및 검증 요청
File:
app/src/test/java/woowacourse/shopping/fixture/FakeProductRepository.kt(Lines 42–44)현재 코드
val pagedItems = fakeProducts.drop(page * size).take(size) val hasNext = page * size + pagedItems.size < fakeProducts.size onResult(Result.success(PagedResult(pagedItems, hasNext)))에 대한 단위 테스트가 확인되지 않습니다. 특히 마지막 페이지가 정확히
size만큼의 아이템을 반환하는 경우와 남은 아이템 개수가size보다 작을 때hasNext가 올바르게 동작하는지 살펴볼 필요가 있습니다.아래와 같은 시나리오를 포함한 테스트 케이스를 작성해 주세요:
- page = 0, fakeProducts 수가 size보다 큰 경우
- 중간 페이지
- 마지막 페이지(정확히 size개의 아이템)
- 마지막 페이지(남은 아이템 수 < size)
- page가 음수거나 가능한 범위를 벗어나는 경우
위 테스트를 통해 페이징 계산 로직의 정확성을 반드시 검증해 주세요.
app/src/main/java/woowacourse/shopping/view/cart/ShoppingCartActivity.kt (1)
21-23: FragmentFactory 설정 순서에 대한 이해FragmentFactory를
super.onCreate()전에 설정하는 이유를 설명해보시겠어요? 이 순서가 중요한 이유와 Fragment 생명주기와의 관계를 생각해보시면 도움이 될 것 같습니다.app/src/main/java/woowacourse/shopping/data/dto/response/ProductResponseDto.kt (1)
9-9: Long 타입 사용의 적절성 검토totalElements에 Long 타입을 사용한 이유를 설명해보시겠어요? API 스펙에서 정말로 Long 범위의 값이 필요한지, 아니면 Int로도 충분한지 검토해보시면 좋겠습니다.
app/src/main/java/woowacourse/shopping/view/cart/select/adapter/CartProductViewHolder.kt (2)
14-14: lateinit var 사용 시 안전성 검토
currentItem을 lateinit var로 선언하셨네요.bind()메서드가 호출되기 전에 이 변수에 접근할 가능성은 없는지 검토해보세요.ViewHolder의 생명주기를 고려했을 때, 이 변수를 더 안전하게 처리할 수 있는 방법들을 생각해보시면 어떨까요? (힌트: nullable 타입이나 다른 초기화 패턴)
18-24: 이벤트 핸들링 타이밍 문제 분석init 블록에서 클릭 리스너를 설정할 때
currentItem을 참조하고 있습니다. 이때 발생할 수 있는 시나리오들을 생각해보세요:
- 사용자가
bind()호출 전에 버튼을 클릭한다면?- ViewHolder가 재사용될 때의 상황은?
이런 문제들을 어떻게 방지할 수 있을지 방법을 찾아보시면 좋겠습니다.
app/src/main/java/woowacourse/shopping/domain/repository/CartProductRepository.kt (1)
6-36: 잘 설계된 레포지토리 인터페이스입니다!콜백 패턴과
Result타입을 일관성 있게 사용하여 비동기 작업을 안전하게 처리하도록 설계되었습니다. 페이징, CRUD 작업이 체계적으로 정의되어 있어 유지보수성이 좋습니다.app/src/main/java/woowacourse/shopping/data/datasource/remote/CartProductRemoteDataSource.kt (1)
164-168: HTTP 상태 코드 상수들이 잘 정의되어 있습니다!성공 응답 코드를 상수로 정의하여 코드의 가독성과 유지보수성을 높였습니다.
app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendFragment.kt (1)
88-102: Observer 패턴 구현이 깔끔합니다!ViewModel과의 데이터 바인딩 및 LiveData 관찰 패턴이 잘 구현되어 있고, 각 이벤트별로 적절한 액션을 취하고 있습니다. Fragment 생명주기를 고려한 관찰자 등록도 올바르게 되어 있네요.
app/build.gradle.kts (1)
52-55: 빌드 기능 설정이 적절합니다.데이터 바인딩과 빌드 설정 활성화가 새로 추가된 UI 아키텍처와 잘 맞아떨어집니다.
app/src/main/java/woowacourse/shopping/data/repository/CartProductRepositoryImpl.kt (1)
8-17: Repository 패턴 구현이 깔끔합니다.Repository 인터페이스를 구현하여 데이터 소스를 추상화하고, 일관된 콜백 패턴을 사용하는 점이 좋습니다. 도메인 계층과 데이터 계층의 분리가 잘 되어 있네요.
app/src/main/java/woowacourse/shopping/view/product/catalog/adapter/ProductAdapter.kt (1)
44-69: DiffUtil 구현이 효율적입니다.아이템 비교 로직이 각 타입별로 적절하게 구현되어 있고, 특히 ProductItem에서는 ID 기반 비교를 통해 성능 최적화가 잘 되어 있습니다. RecentProductsItem과 LoadMoreItem은 싱글톤 성격이라 타입 체크만으로 충분한 점도 잘 고려되었네요.
app/src/main/java/woowacourse/shopping/view/product/detail/ProductDetailViewModel.kt (1)
52-69: 장바구니 추가 후 수량 초기화 로직을 검토해보세요.성공적인 장바구니 추가 후 수량이 항상
MINIMUM_QUANTITY로 초기화됩니다.이것이 의도된 UX인지 확인해보세요. 사용자가 수량을 조정한 후 장바구니에 추가했을 때, 수량이 초기화되는 것이 자연스러운지 생각해보시고, 다른 쇼핑 앱들의 동작을 참고해보는 것도 좋겠습니다.
app/src/main/java/woowacourse/shopping/view/product/catalog/ProductCatalogViewModel.kt (1)
32-37: MediatorLiveData 사용이 좋습니다.이전 리뷰어가 제안한 LiveData 변환을 잘 활용하고 있습니다.
MediatorLiveData를 사용해서 여러 데이터 소스를 조합하는 패턴이 적절합니다. 다른 복잡한 데이터 변환이 필요한 부분에서도 이런 패턴을 적용할 수 있는지 검토해보세요.app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModel.kt (1)
37-50: LiveData 변환을 활용한 반응형 프로퍼티 구현이 훌륭합니다
map을 사용하여 선택된 상품들의 총 가격과 수량을 자동으로 계산하는 방식이 매우 깔끔합니다. 반응형 프로그래밍의 좋은 예시입니다.
| @BindingAdapter("imageUrl") | ||
| fun ImageView.setImage(image: String?) { | ||
| image ?: return | ||
| Glide | ||
| .with(this.context) | ||
| .load(image) | ||
| .placeholder(R.drawable.ic_launcher_foreground) | ||
| .centerCrop() | ||
| .into(this) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
이미지 로딩 로직을 개선할 수 있는 부분들을 고려해보세요.
현재 구현은 기본적인 기능은 잘 수행하지만, 몇 가지 질문을 해보겠습니다:
- 이미지 로딩이 실패했을 때는 어떻게 처리하면 좋을까요?
- 네트워크가 불안정한 상황에서 사용자 경험을 어떻게 개선할 수 있을까요?
- 플레이스홀더로
ic_launcher_foreground를 사용하는 것이 적절할까요?
이런 상황들을 처리하기 위해 Glide에서 제공하는 추가 메서드들을 살펴보시기 바랍니다.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/view/util/BindingAdapter.kt lines 8 to
17, improve the image loading logic by adding error handling and better user
experience features. Use Glide's error() method to specify a fallback image when
loading fails, and consider using a more appropriate placeholder image that fits
the app's design instead of ic_launcher_foreground. Additionally, enable caching
strategies or use Glide's thumbnail() method to improve loading performance and
user experience in unstable network conditions.
| class RequestInterceptor(private val tokenProvider: TokenProvider) : Interceptor { | ||
| override fun intercept(chain: Interceptor.Chain): Response { | ||
| val builder = chain.request().newBuilder() | ||
| val authorizationValue = HEADER_VALUE_AUTHORIZATION.format(tokenProvider.getToken()) | ||
|
|
||
| builder.addHeader(HEADER_NAME_AUTHORIZATION, authorizationValue) | ||
|
|
||
| return chain.proceed(builder.build()) | ||
| } | ||
|
|
||
| companion object { | ||
| private const val HEADER_NAME_AUTHORIZATION = "Authorization" | ||
| private const val HEADER_VALUE_AUTHORIZATION = "Basic %s" | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
인터셉터 구현이 기본적으로 올바르게 작성되었습니다.
하지만 몇 가지 고려해볼 점들이 있습니다:
tokenProvider.getToken()이 null을 반환하거나 예외를 던질 가능성은 없나요?- 모든 요청에 Authorization 헤더를 추가하는 것이 적절한가요?
- 토큰이 만료되었을 때는 어떻게 처리할 계획인가요?
이런 예외 상황들을 어떻게 처리하면 좋을지 생각해보세요. TokenProvider의 구현도 함께 살펴보시면 도움이 될 것입니다.
TokenProvider의 구현을 확인해보세요:
🏁 Script executed:
#!/bin/bash
# TokenProvider 구현 확인
rg -A 10 "class TokenProvider" app/src/main/java/Length of output: 971
🏁 Script executed:
#!/bin/bash
rg -A 20 "fun getToken" app/src/main/java/woowacourse/shopping/data/TokenProvider.ktLength of output: 603
Authorization 헤더 추가 시 토큰 null 처리 및 보안 강화 필요
RequestInterceptor와 TokenProvider 구현을 검토한 결과, 다음 사항을 반영하여 수정이 필요합니다:
- getToken()이 nullable(String?)을 반환하므로 null일 때 헤더를 추가하지 않도록 처리
- SharedPreferences에 평문(Base64)으로 민감정보를 저장 중이므로 EncryptedSharedPreferences 또는 Android Keystore 활용 권장
- 실제 API 호출에 앞서 토큰 만료 여부나 유효성 검증 로직(재발급)도 고려할 것
수정 예시 (app/src/main/java/woowacourse/shopping/data/network/RequestInterceptor.kt):
override fun intercept(chain: Interceptor.Chain): Response {
val builder = chain.request().newBuilder()
- val authorizationValue = HEADER_VALUE_AUTHORIZATION.format(tokenProvider.getToken())
- builder.addHeader(HEADER_NAME_AUTHORIZATION, authorizationValue)
+ tokenProvider.getToken()?.let { token ->
+ val authorizationValue = HEADER_VALUE_AUTHORIZATION.format(token)
+ builder.addHeader(HEADER_NAME_AUTHORIZATION, authorizationValue)
+ }
return chain.proceed(builder.build())
}또한 TokenProvider.kt:
- SharedPreferences → EncryptedSharedPreferences 변경
- 토큰 만료 시 갱신 로직 추가 검토
📝 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.
| class RequestInterceptor(private val tokenProvider: TokenProvider) : Interceptor { | |
| override fun intercept(chain: Interceptor.Chain): Response { | |
| val builder = chain.request().newBuilder() | |
| val authorizationValue = HEADER_VALUE_AUTHORIZATION.format(tokenProvider.getToken()) | |
| builder.addHeader(HEADER_NAME_AUTHORIZATION, authorizationValue) | |
| return chain.proceed(builder.build()) | |
| } | |
| companion object { | |
| private const val HEADER_NAME_AUTHORIZATION = "Authorization" | |
| private const val HEADER_VALUE_AUTHORIZATION = "Basic %s" | |
| } | |
| } | |
| class RequestInterceptor(private val tokenProvider: TokenProvider) : Interceptor { | |
| override fun intercept(chain: Interceptor.Chain): Response { | |
| val builder = chain.request().newBuilder() | |
| tokenProvider.getToken()?.let { token -> | |
| val authorizationValue = HEADER_VALUE_AUTHORIZATION.format(token) | |
| builder.addHeader(HEADER_NAME_AUTHORIZATION, authorizationValue) | |
| } | |
| return chain.proceed(builder.build()) | |
| } | |
| companion object { | |
| private const val HEADER_NAME_AUTHORIZATION = "Authorization" | |
| private const val HEADER_VALUE_AUTHORIZATION = "Basic %s" | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/data/network/RequestInterceptor.kt
around lines 7 to 21, the current implementation adds an Authorization header
without checking if the token is null, which can cause issues. Modify the
intercept method to first retrieve the token from tokenProvider.getToken(),
check if it is null, and only add the Authorization header if the token is
non-null. Additionally, review TokenProvider implementation to replace plain
SharedPreferences storage with EncryptedSharedPreferences or Android Keystore
for better security, and consider adding token expiration checks and refresh
logic before making API calls.
| @SerialName("offset") | ||
| val offset: Long, | ||
| @SerialName("sort") | ||
| val sort: SortDto, | ||
| @SerialName("paged") | ||
| val paged: Boolean, | ||
| @SerialName("pageNumber") | ||
| val pageNumber: Int, | ||
| @SerialName("pageSize") | ||
| val pageSize: Int, | ||
| @SerialName("unpaged") | ||
| val unpaged: Boolean, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
데이터 타입 일관성과 중복성을 고려해보세요.
페이지네이션 관련 필드들의 타입과 구조에서 몇 가지 검토할 점이 있습니다:
-
offset은Long타입이지만pageNumber와pageSize는Int타입입니다. 페이지네이션에서 일관된 타입 사용을 고려해볼 필요가 있습니다. -
paged와unpaged필드가 모두 존재하는데, 이는 논리적으로 서로 반대 관계입니다. 하나의 필드로 충분할 수 있으니 API 스펙을 다시 확인해보세요.
어떤 이유로 이러한 구조를 선택하셨는지 생각해보시고, API 응답 구조와 일치하는지 검증해보시기 바랍니다.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/data/dto/response/PageableDto.kt
between lines 8 and 19, ensure consistent data types for pagination fields by
using the same numeric type for offset, pageNumber, and pageSize, preferably Int
or Long based on API spec. Also, remove either the paged or unpaged boolean
field since they are logically opposite; keep only one to avoid redundancy and
align with the API response structure.
| override fun onOrderClick() { | ||
| selectedCartIds.forEach { id -> | ||
| cartProductRepository.delete(id) {} | ||
| } | ||
| _finishOrderEvent.postValue(Unit) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
주문 처리 시 에러 처리 및 사용자 피드백 부족
주문 처리 중 실패할 경우를 대비한 처리가 없습니다. 또한 여러 개의 삭제 요청이 부분적으로만 성공할 수 있습니다.
개선 제안:
- 삭제 실패 시 에러 처리
- 로딩 상태 표시
- 모든 삭제가 완료된 후에만 완료 이벤트 발생
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/view/cart/recommend/CartProductRecommendViewModel.kt
around lines 152 to 157, the onOrderClick method lacks error handling and user
feedback during the deletion of cart items. Modify the method to handle errors
for each delete operation, track the loading state to indicate progress, and
ensure that the _finishOrderEvent is only posted after all deletions succeed.
Implement callbacks or coroutines to manage asynchronous deletions, aggregate
results, and provide appropriate user feedback on failure.
| private fun updateCartProduct( | ||
| item: CartProduct, | ||
| quantityDelta: Int, | ||
| ) { | ||
| val existing = | ||
| cartProductItems.value.orEmpty().firstOrNull { it.cartProduct.id == item.id } ?: return | ||
| val newQuantity = existing.cartProduct.quantity + quantityDelta | ||
| if (newQuantity < MINIMUM_QUANTITY) return | ||
| repository.updateQuantity(existing.cartProduct, quantityDelta) { | ||
| loadPage(page.value ?: FIRST_PAGE_NUMBER) | ||
| } | ||
| val currentSelected = selectedCartProducts.value.orEmpty() | ||
| if (item in currentSelected) { | ||
| val newItem = item.copy(quantity = newQuantity) | ||
| _selectedCartProducts.postValue(currentSelected - item + newItem) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
수량 업데이트 후 전체 페이지 새로고침은 비효율적입니다
수량 변경 시마다 전체 페이지를 다시 로드하는 것은 성능상 비효율적입니다. 로컬 상태만 업데이트하고 필요시에만 서버와 동기화하는 방법을 고려해보세요.
대안:
- 로컬 상태를 먼저 업데이트하고 UI 반영
- 서버 요청 실패 시에만 페이지 새로고침
- 또는 변경된 아이템만 부분 업데이트
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModel.kt
between lines 136 and 152, avoid reloading the entire page after updating the
quantity as it is inefficient. Instead, update the local state immediately by
modifying the relevant cart product's quantity and updating the selected items
accordingly. Perform the repository update call asynchronously, and only trigger
a full page reload or error handling if the server update fails. This approach
improves performance by minimizing unnecessary UI refreshes.
| fun loadPage(page: Int = FIRST_PAGE_NUMBER) { | ||
| _isLoading.value = true | ||
| repository.getPagedProducts(page - 1, PAGE_SIZE) { result -> | ||
| result | ||
| .onSuccess { pagedResult -> | ||
| _isLoading.value = false | ||
| _cartProductItems.value = | ||
| pagedResult.items.map { | ||
| CartProductItem(it, it in selectedCartProducts.value.orEmpty()) | ||
| } | ||
| updatePageState(page, pagedResult.hasNext) | ||
| }.onFailure { Log.e("error", it.message.toString()) } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
에러 처리 개선 및 로딩 상태 관리는 잘하셨습니다
로딩 상태 관리는 잘 구현되어 있으나, 에러 발생 시 사용자 피드백이 없습니다.
}.onFailure {
+ _isLoading.value = false
Log.e("error", it.message.toString())
+ // 에러 상태를 사용자에게 알림
}📝 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 loadPage(page: Int = FIRST_PAGE_NUMBER) { | |
| _isLoading.value = true | |
| repository.getPagedProducts(page - 1, PAGE_SIZE) { result -> | |
| result | |
| .onSuccess { pagedResult -> | |
| _isLoading.value = false | |
| _cartProductItems.value = | |
| pagedResult.items.map { | |
| CartProductItem(it, it in selectedCartProducts.value.orEmpty()) | |
| } | |
| updatePageState(page, pagedResult.hasNext) | |
| }.onFailure { Log.e("error", it.message.toString()) } | |
| } | |
| } | |
| fun loadPage(page: Int = FIRST_PAGE_NUMBER) { | |
| _isLoading.value = true | |
| repository.getPagedProducts(page - 1, PAGE_SIZE) { result -> | |
| result | |
| .onSuccess { pagedResult -> | |
| _isLoading.value = false | |
| _cartProductItems.value = | |
| pagedResult.items.map { | |
| CartProductItem(it, it in selectedCartProducts.value.orEmpty()) | |
| } | |
| updatePageState(page, pagedResult.hasNext) | |
| } | |
| .onFailure { | |
| _isLoading.value = false | |
| Log.e("error", it.message.toString()) | |
| // 에러 상태를 사용자에게 알림 | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/view/cart/select/CartProductSelectViewModel.kt
around lines 52 to 65, the error handling logs the error but does not provide
any user feedback. Modify the onFailure block to update a user-visible error
state or message so the UI can inform the user about the failure. Ensure that
_isLoading.value is also set to false in the failure case to properly reflect
the loading state.
| abstract class SingleLiveData<T> { | ||
| private val liveData = MutableLiveData<Event<T>>() | ||
|
|
||
| protected constructor() | ||
|
|
||
| protected constructor(value: T) { | ||
| liveData.value = Event(value) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Abstract 클래스 설계의 목적을 명확히 해보세요.
SingleLiveData를 abstract class로 설계한 이유가 무엇인지 생각해보세요. 직접 인스턴스화하지 못하게 하려는 의도라면, 이를 상속받는 구체 클래스에서는 어떤 추가 기능을 제공해야 할까요? MutableSingleLiveData와 같은 구현체의 역할 분담에 대해 고민해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/view/util/SingleLiveData.kt around
lines 6 to 13, clarify the purpose of making SingleLiveData an abstract class.
If the intent is to prevent direct instantiation, ensure that subclasses like
MutableSingleLiveData provide concrete implementations or additional
functionality that justify the abstraction. Refactor the design to clearly
separate responsibilities between the abstract base and its subclasses,
documenting the intended usage and extension points.
| if (page == null || size == null) { | ||
| onResult(Result.success(PagedResult(emptyList(), false))) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
페이징 파라미터 검증 로직을 강화해보세요.
page나 size가 null일 때 빈 결과를 반환하고 있는데, 실제 API에서는 이런 경우 어떻게 동작해야 할까요? 음수 값이나 0 같은 유효하지 않은 값들에 대한 처리도 고려해보세요. 테스트에서 이런 엣지 케이스들을 검증할 수 있도록 로직을 개선하면 어떨까요?
🤖 Prompt for AI Agents
In app/src/test/java/woowacourse/shopping/fixture/FakeProductRepository.kt
around lines 38 to 41, the current paging parameter validation only checks for
null values and returns an empty result. Improve this by also validating that
page and size are positive integers greater than zero. If either is invalid
(null, zero, or negative), return an empty PagedResult as before. Update the
test cases to cover these edge cases to ensure robust validation.
✅ Actions performedReviews paused. |
| size: Int?, | ||
| onResult: (Result<PagedResult<CartProduct>>) -> Unit, | ||
| ) { | ||
| cartProductService.getPagedProducts(page = page, size = size).enqueue( |
There was a problem hiding this comment.
메인 스레드에서 네트워크 요청을 비동기적으로 처리하기 위해서 enqueue를 사용했습니다!
enqueue는 백그라운드 스레드에서 실행되어 메인 스레드를 차단하지 않고,
요청이 완료되면 onResponse() 또는 onFailure() 콜백이 호출됩니다.
반면 execute는 동기적으로 실행되어 요청이 끝날 때까지 호출한 스레드를 블로킹합니다.
There was a problem hiding this comment.
로그를 찍어보니, 상품 목록 화면으로 돌아올 때마다 Activity가 매번 재생성되고 있었고,
이로 인해 스크롤 포지션이 초기화되고 있었습니다.
뒤로가기 버튼을 클릭하면 해당 Activity를 finish하도록 처리하여 스크롤 위치가 유지되도록 개선했습니다! (f908614)
| val binding = CustomToolbarShoppingCartBinding.inflate(layoutInflater) | ||
| binding.viewmodel = viewModel | ||
| binding.lifecycleOwner = this |
There was a problem hiding this comment.
CustomToolbarShoppingCartBinding의 인스턴스를 클래스 프로퍼티로 선언하는 방식으로 변경했습니다! (9161851)
남겨주신 코멘트의 의도가 맞을까요..?!
| interface ProductCatalogEventHandler : | ||
| RecentProductViewHolder.EventHandler, | ||
| ProductViewHolder.EventHandler, | ||
| LoadMoreViewHolder.EventHandler |
There was a problem hiding this comment.
말씀하신 것처럼 현재는 RecentProductViewHolder와 ProductViewHolder 모두 상품 상세로 이동하는 로직이 동일합니다.
다만, ViewHolder마다 이벤트를 다르게 처리할 가능성을 고려해 각각의 EventHandler를 정의했습니다..!
| private val _productCatalogItems = MutableLiveData<List<ProductCatalogItem>>() | ||
| val productCatalogItems: LiveData<List<ProductCatalogItem>> get() = _productCatalogItems | ||
|
|
||
| private val _selectedProduct = MutableSingleLiveData<Product>() | ||
| val selectedProduct: SingleLiveData<Product> get() = _selectedProduct | ||
|
|
||
| private val _totalQuantity = MutableLiveData(MINIMUM_QUANTITY) | ||
| val totalQuantity: LiveData<Int> get() = _totalQuantity | ||
|
|
||
| private val _onFinishLoading = MutableLiveData(false) | ||
| val onFinishLoading: LiveData<Boolean> get() = _onFinishLoading |
There was a problem hiding this comment.
매번 데이터를 업데이트하는 작업이 번거로웠는데, MediatorLiveData를 활용하니 데이터 처리 로직이 훨씬 간결해졌습니다! 😮
refactor: 상품 목록 LiveData 구조 개선 및 MediatorLiveData 적용
refactor: 상품 총 금액을 LiveData의 map을 통해 계산
refactor: LiveData map을 사용하여 장바구니 상품 선택 ViewModel 로직 개선
| class ProductAdapter( | ||
| items: List<ProductCatalogItem> = emptyList(), | ||
| private val eventHandler: ProductCatalogEventHandler, | ||
| ) : RecyclerView.Adapter<RecyclerView.ViewHolder>() { |
There was a problem hiding this comment.
기존의 RecyclerView.Adapter를 모두 ListAdapter로 변경했습니다!
ListAdapter는 DiffUtil을 활용하여 데이터 전체를 다시 계산하지 않고 변경된 데이터만 업데이트할 수 있습니다.
덕분에 변경된 데이터의 position을 계산할 필요가 없어 코드도 훨씬 간결해진 것 같습니다!
refactor: 상품 목록 어댑터에 ListAdapter 적용
refactor: 최근 본 상품 목록 어댑터에 ListAdapter 적용
refactor: 장바구니 상품 목록 어댑터에 ListAdapter 적용
refactor: 추천 상품 목록 어댑터에 ListAdapter 적용
| cartProducts.map { it.product.id }.toSet() | ||
| val recommended = | ||
| products.items | ||
| .asSequence() |
There was a problem hiding this comment.
Sequence를 사용하면 임시 컬렉션을 사용하지 않고 연산을 연쇄적으로 할 수 있어 효율적으로 계산할 수 있습니다!
기존의 컬렉션 연산은 각 중간 연산마다 컬렉션을 생성하기 때문에 체인이 길어질수록 불필요한 객체 생성이 발생할 수 있습니다.
추천 상품을 구하는 로직에서 필터나 매핑을 하는 연산이 많아 적용해보았습니다!
| class CartProductRecommendationFragment( | ||
| private val productRepository: ProductRepository, | ||
| private val cartProductRepository: CartProductRepository, | ||
| private val recentProductRepository: RecentProductRepository, | ||
| ) : Fragment() { |
There was a problem hiding this comment.
그렇네요..! 😅
말씀하신 방식처럼 Fragment의 생성자로 repository를 받지 않고, application에 접근하여 가져오도록 변경했습니다! (e21c28d)
| private val viewModel by lazy { | ||
| ViewModelProvider( | ||
| this, | ||
| CartProductRecommendationViewModelFactory( | ||
| productRepository, | ||
| cartProductRepository, | ||
| recentProductRepository, | ||
| ), | ||
| )[CartProductRecommendationViewModel::class.java] | ||
| } |
There was a problem hiding this comment.
처음에는 두 하위 Fragment에서 공통으로 사용하는 데이터가 있어,
ShoppingCartViewModel을 두고 하위 Fragment에서 부모의 ViewModel을 공유하는 방식으로 구현하려고 했습니다.
하지만 실제로 구현해보니 두 Fragment의 로직이 모두 ViewModel 하나에서 구현하게 되면서 너무 복잡해지고,
수량 +&- 버튼 처리와 같이 UI 로직이 중복되는 상황이 있었습니다..!
그래서 ViewModel을 Fragment 별로 분리하여, 책임을 나누는 방식으로 구현하게 되었습니다!
| override fun onAddClick(item: Product) { | ||
| cartProductRepository.insert(item.id, QUANTITY_TO_ADD) { result -> | ||
| result | ||
| .onSuccess { cartProductId -> | ||
| cartProducts.add(CartProduct(cartProductId, item, QUANTITY_TO_ADD)) | ||
| updateQuantity(item, QUANTITY_TO_ADD) | ||
| }.onFailure { | ||
| Log.e("error", it.message.toString()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override fun onQuantityIncreaseClick(item: Product) { | ||
| val cartProduct = cartProducts.first { it.product.id == item.id } | ||
| cartProductRepository.updateQuantity(cartProduct, QUANTITY_TO_ADD) { | ||
| cartProducts.removeIf { it.product.id == item.id } | ||
| cartProducts.add(cartProduct.copy(quantity = cartProduct.quantity + QUANTITY_TO_ADD)) | ||
| updateQuantity(item, QUANTITY_TO_ADD) | ||
| } | ||
| } | ||
|
|
||
| override fun onQuantityDecreaseClick(item: Product) { | ||
| val cartProduct = cartProducts.first { it.product.id == item.id } | ||
| cartProductRepository.updateQuantity(cartProduct, -QUANTITY_TO_ADD) { | ||
| cartProducts.removeIf { it.product.id == item.id } | ||
| val newQuantity = cartProduct.quantity - QUANTITY_TO_ADD | ||
| if (newQuantity > MINIMUM_QUANTITY) cartProducts.add(cartProduct.copy(quantity = newQuantity)) | ||
| updateQuantity(item, -QUANTITY_TO_ADD) | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분은 아직 반영하지 못했습니다 😓
다음 단계를 진행하면서 같이 반영하겠습니다!
| @@ -22,6 +21,8 @@ import woowacourse.shopping.view.product.detail.ProductDetailActivity | |||
|
|
|||
| class ProductCatalogActivity : AppCompatActivity() { | |||
| MediatorLiveData<List<ProductCatalogItem>>().apply { | ||
| addSource(recentProducts) { value = buildCatalogItems() } | ||
| addSource(productItems) { value = buildCatalogItems() } | ||
| } |
안녕하세요, 제이든!
레벨2에서 다시 만나 뵙게 되어서 반갑습니다 😃
미션 요구사항을 우선적으로 구현하느라 리팩터링과 테스트 코드 작성이 미흡한 상태입니다..
또한, 막판에 API 콜백에
Result클래스를 적용하면서 코드의 뎁스가 깊어진 부분이 생겼습니다.이후 리팩터링을 진행하면서 점차 개선하겠습니다!
쇼핑 주문 미션 동안 잘 부탁드립니다! 🙇🏻♀️
셀프 체크리스트
README.md에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
이번 미션의 코드
이번 미션의 커밋만 모은 링크입니다
인증 정보 설정
사용자 인증을 위해
local.properties파일에 아래 정보를 추가해야 합니다:JSON 직렬화 라이브러리 선택 이유
서버 통신을 위한 JSON 직렬화 라이브러리로 kotlinx.serialization을 선택했습니다.
이전에 Gson을 사용할 때, default value가 무시되거나 non-nullable 타입에 null이 들어가는 문제가 발생해 오류를 파악하기 어려웠습니다.
kotlinx.serialization은 Kotlin 언어와 호환성이 좋고,컴파일 타임에 타입 안정성을 보장해 이러한 문제를 방지할 수 있다는 점에서 선택했습니다!
스크린샷
Screen_recording_20250529_173503.mp4