-
Notifications
You must be signed in to change notification settings - Fork 71
[쇼핑 주문 1, 2 단계] 뭉치 미션 제출합니다. #97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
commit ee14a860ec9a0714f214fcd95f654211afad7d53 Author: m6z1 <[email protected]> Date: Mon May 26 17:25:13 2025 +0900 refactor: mock server 클래스 네이밍 수정 commit a4a475aa8a1513138ac0d1219e006db5e6cc59f1 Author: m6z1 <[email protected]> Date: Mon May 26 17:24:56 2025 +0900 test: mock 상품 서버 테스트 추가 commit 47fdb3bea0ee013cb424c87c8bfb8d30108956bc Author: m6z1 <[email protected]> Date: Mon May 26 16:38:17 2025 +0900 chore: 사용하지 않는 코드 제거 commit 612f085807c885699c164d20f6e86d17f7ed56c4 Author: m6z1 <[email protected]> Date: Mon May 26 16:35:51 2025 +0900 test: 변경된 프로덕션 코드에 테스트 성공하도록 수정 commit dca0b22d72d29978bc665ac0194982d1904bb39b Author: m6z1 <[email protected]> Date: Mon May 26 16:11:59 2025 +0900 refactor: 함수 분리 commit 01fd74e94b7c12be72e13f5ff8daa461208ede41 Author: m6z1 <[email protected]> Date: Mon May 26 16:03:38 2025 +0900 refactor: 상품 목록 뷰 수량 조절 컴포넌트 사용 commit 31531c70376089106aca19669d623c75212e3362 Author: m6z1 <[email protected]> Date: Mon May 26 15:22:53 2025 +0900 refactor: 불필요한 함수 제거 commit 6e28f6050542ccc04302c19cdac02c8bb59ffe98 Author: m6z1 <[email protected]> Date: Mon May 26 15:18:17 2025 +0900 feat: 상품 목록 값 서버 통신으로 호출 commit 42855097a4721063665b068c823ce32639837821 Author: m6z1 <[email protected]> Date: Mon May 26 15:17:41 2025 +0900 chore: 패키지 이동 commit 78818df44b0758dc9c5e82cabce440403f304780 Author: m6z1 <[email protected]> Date: Mon May 26 14:57:46 2025 +0900 build: 네트워크용 라이브러리 추가 commit caf311ff815614eb919d8cebcae31702122f7eee Author: m6z1 <[email protected]> Date: Mon May 26 13:51:22 2025 +0900 chore: 패키지 분리 commit ae13808677f08f12f8cbb510aeef10af0354e05e Author: m6z1 <[email protected]> Date: Mon May 26 13:40:08 2025 +0900 fix: 최근 본 상품 이전의 상품 개수 업데이트 안 되는 이슈 해결 commit f45a1c61953dd5883735ba1a56a92977ff20ed1b Author: m6z1 <[email protected]> Date: Mon May 26 13:21:57 2025 +0900 feat: 마지막으로 본 상품 페이지 뒤로가기 시 상품 목록 이동 구현 commit 5bf546fdf96a39ef84bfe6a64db5da86496aaa0f Author: m6z1 <[email protected]> Date: Mon May 26 04:27:51 2025 +0900 fix: 마지막에 본 상품 클릭 시 마지막에 본 상품 ui 출력되는 이슈 해결 commit e8fd2f41bb2f2397fcb25a4378815852ff5b7182 Author: m6z1 <[email protected]> Date: Mon May 26 04:04:52 2025 +0900 feat: 마지막으로 본 상품일 경우 마지막으로 본 상품 ui 가시성 처리 commit 8c46f764a2567fadf19dd33cafee52497a4ec9a6 Author: m6z1 <[email protected]> Date: Mon May 26 04:03:08 2025 +0900 feat: 상품 상세화면에서 마지막으로 본 상품 출력 로직 구현 commit b16d1ab30766be7ac144df73b7824837a69fcea7 Author: m6z1 <[email protected]> Date: Mon May 26 03:47:49 2025 +0900 refactor: string 리소스 추출 commit 66930ef84673dcd35a2e04e49f53c060d6a1052d Author: m6z1 <[email protected]> Date: Mon May 26 03:45:11 2025 +0900 feat: 앱 재시작돼도 최근 본 상품 목록 유지 commit ba7864091b772e7ee917f3b0e4152c12c844ffba Author: m6z1 <[email protected]> Date: Mon May 26 03:07:33 2025 +0900 refactor: RecyclerView 아이템 데코레이터 구현 commit 694ed96ee0250a214b1773d225731cde11e2c083 Author: m6z1 <[email protected]> Date: Mon May 26 03:02:10 2025 +0900 feat: 최근 본 상품 로직 구현 commit 10b2c41995678e2cd75cca6c5bf179da198c2731 Author: m6z1 <[email protected]> Date: Mon May 26 01:39:17 2025 +0900 chore: 불필요한 파일 제거 commit d83505db73294dbaba35f2501108bd217ace370e Author: m6z1 <[email protected]> Date: Mon May 26 01:39:12 2025 +0900 feat: 앱 재시작에도 상품 목록 장바구니 개수 유지 commit 28f983d287be72890bd3d2f99071ffb22b302cca Author: m6z1 <[email protected]> Date: Sun May 25 23:56:56 2025 +0900 feat: 앱 재시작에도 장바구니 데이터 유지 commit f9212ff04a70429d6cd5bfe9fe4296e8f94fea6e Author: m6z1 <[email protected]> Date: Sun May 25 23:56:41 2025 +0900 build: Room 라이브러리 버전 2.7.1 업데이트 commit 3884639a89d30be1804a6571705bb2782c4bcabf Author: m6z1 <[email protected]> Date: Sun May 25 23:56:15 2025 +0900 refactor: application class 생성 commit 534abd57fad0a7b502e4827aa31ba35648cab837 Author: m6z1 <[email protected]> Date: Sun May 25 19:02:09 2025 +0900 docs: 기능 요구 사항 정리 commit fea92920ee32b6cf5e39bb5e31ab3fc22ed81726 Author: m6z1 <[email protected]> Date: Sun May 25 18:56:01 2025 +0900 build: room 라이브러리 추가 commit 2d4262ce2b9a64af8433115c8c950051b66dec3c Author: m6z1 <[email protected]> Date: Sun May 25 15:12:43 2025 +0900 feat: 장바구니 화면에서 뒤로 갔을 때 상태 일치 로직 구현 commit 2c3f80de3ec329a009faf842b6eb436ac6d34c4c Author: m6z1 <[email protected]> Date: Sun May 25 14:16:38 2025 +0900 feat: 상품 상세 화면에서 뒤로 갔을 때 상태 일치 로직 구현 commit 57b84e929376b0f2dc80df52252a816ace74b2b4 Author: m6z1 <[email protected]> Date: Thu May 22 17:11:32 2025 +0900 feat: 장바구니에 담은 수량 조절 로직 구현 commit 609dffb3ffbc68b3c5dd0f461369c8793fba60c2 Author: m6z1 <[email protected]> Date: Thu May 22 16:58:09 2025 +0900 feat: 장바구니 상품 제거 / 장바구니 수량 빼는 로직 분리 commit 6753a2cca3abc0fb406b5c1721f0e2f1827f0915 Author: m6z1 <[email protected]> Date: Thu May 22 16:23:04 2025 +0900 feat: 장바구니 화면 수량 별 가격 출력 구현 commit f402d7cd9223d0c08d6433c8e25089b7e5e20181 Author: m6z1 <[email protected]> Date: Thu May 22 16:22:45 2025 +0900 refactor: 장바구니 상품 조회 반환 타입 수정 commit 4a0f8bb5ae5b3e600a2032e446f96e440248c60e Author: m6z1 <[email protected]> Date: Thu May 22 15:46:49 2025 +0900 feat: 상품 상세 화면 수량만큼 선택한 총 가격 출력 구현 commit eeb7c0fb2991195183e81e750b079993a5dcae2a Author: m6z1 <[email protected]> Date: Thu May 22 15:40:18 2025 +0900 feat: 상품 상세 화면 수량 조절 버튼 구현 commit a6cc3d7bee32fe9fbd1e17aff9d19b65133b8251 Author: m6z1 <[email protected]> Date: Thu May 22 15:40:08 2025 +0900 feat: 상품 수량 공용 컴포넌트 Ui 구현 commit d4d75847633f6508b3246ef589c1088300fa2f11 Author: m6z1 <[email protected]> Date: Thu May 22 13:34:51 2025 +0900 feat: 상품 목록 뷰 장바구니 로직 추가 - 각 상품에서 + - 버튼을 통해 수량을 조절할 수 있다. - 장바구니에 담근 총 수량이 보인다. commit 967db9894b60d73b7ba3513e87862d85abccab9f Author: m6z1 <[email protected]> Date: Wed May 21 17:17:17 2025 +0900 feat: 각 상품에 + 버튼 ui 구현 commit 90a01e7aeb3560e84c5237fbdaf0dd5464f1b7f6 Author: m6z1 <[email protected]> Date: Wed May 21 13:41:47 2025 +0900 refactor: 함수 네이밍 명시적으로 수정 commit 85b96fc36d86051debe34415217b798fe4a60f04 Author: m6z1 <[email protected]> Date: Wed May 21 13:37:05 2025 +0900 refactor: 중복되는 값 변수에 할당 - ProductsViewModel에서 중복되는 라이브 데이터 값 할당 후 사용 commit 83f3c2baaa82e81d79343f8d24a4a13fd5435793 Author: m6z1 <[email protected]> Date: Wed May 21 13:22:53 2025 +0900 refactor: 상품 목록 아이템 추가 함수 네이밍 수정 commit 93c65d1e824bc4b8aa22cd8236a6bdff7cada002 Author: m6z1 <[email protected]> Date: Wed May 21 13:21:09 2025 +0900 chore: activity exported 속성 수정 commit 5834af9e3b78e5857db83d2b8b6be782278dc8f1 Author: m6z1 <[email protected]> Date: Wed May 21 13:18:21 2025 +0900 refactor: 상품 목록 조회 호출 로직 수정 commit ad2e4ca0075a301fee2cea0a6d01f46020a4022e Author: m6z1 <[email protected]> Date: Wed May 21 11:26:17 2025 +0900 refactor: result 콜백 매개변수 네이밍 수정 commit a6fbdb0403a5cf770a713656f27c4681cf918724 Author: m6z1 <[email protected]> Date: Wed May 21 11:23:13 2025 +0900 docs: 기능 요구 사항 정리 commit 519909ad15a249eea2d31c129b39a9bf8398fdb1 Author: Son Myeongji <[email protected]> Date: Tue May 20 20:13:07 2025 +0900 [쇼핑 장바구니 1, 2단계] 뭉치 미션 제출합니다. (woowacourse#101) * docs: 기능 요구 사항 작성 * build: 데이터 바인딩 추가 * feat: Product 생성 * feat: 상품 목록 화면에서 행당 2개의 상품을 출력 * chore: 패키지 분리 * build: Glide 라이브러리 추가 * chore: 더미 데이터 수정 * feat: ProductUrls 구현 * build: kapt 추가 * feat: BindingAdapter setImageUrl 구현 * feat: 인터넷 권한 추가 * feat: 이미지 바인딩 어댑터 사용 * chore: BindingAdapter 패키지 이동 * feat: 아이템 간 마진 조정 * chore: 메뉴 아이콘들 추가 * feat: 앱 테마 NoActionBar 수정 * feat: 상품 목록 페이지 toolbar 구현 * feat: color 리소스 이름 변경 * feat: 장바구니 화면 구현 * feat: 장바구니 버튼 클릭 이벤트 구현 * feat: 뒤로 가기 버튼 클릭 이벤트 구현 * feat: 상품 상세 화면 구현 * chore: 패키지 이동 * feat: Product 직렬화 * feat: 상품 클릭 시 상품 상세로 이동 * refactor: 에러 메시지 String 리소스 추출 * feat: 상품 상세 화면 X 버튼 클릭 이벤트 구현 * chore: 패키지 분리 * feat: 장바구니 저장소 구현 * feat: 장바구니 추가 기능 구현 * feat: 상품 목록 저장소 구현 * feat: 상품 목록 뷰모델 구현 * feat: 장바구니 뷰모델 구현 * feat: 장바구니 상품 삭제 구현 * docs: 기능 요구 사항 작성 * feat: 상품 목록 더보기 구현 * refactor: 함수 이름 변경 * feat: 장바구니 페이지네이션 뷰 구현 * refactor: 팩토리 메서드 네이밍 수정 * feat: 장바구니 페이지네이션 뷰홀더 추가 * feat: 페이지네이션 구현 * feat: 페이지에 5개씩 가져오도록 구현 * feat: 장바구니 페이지네이션 구현 * docs: 기능 요구 사항 체크리스트 업데이트 * feat: 페이지네이션 아이템 가시성 처리 * refactor: 상품 목록 뷰 멀티 뷰 타입으로 개선 * test: ProductsViewModel 테스트 추가 등록된 상품들을 가져올 수 있다 * test: ShoppingCartViewModel 테스트 추가 - 장바구니에 담은 상품들을 보여준다 - 제거하고 싶은 상품을 장바구니에서 삭제할 수 있다 - 장바구니의 다음 페이지 정보를 확인할 수 있다 - 장바구니의 이전 페이지 정보를 확인할 수 있다 * refactor: 매퍼 함수 위치 수정 * refactor: 중복되는 layoutManager 설정 제거 * refactor: 클릭 리스너 interface 네이밍 수정 * refactor: 상품 상세 뷰 스크롤 되도록 수정 * refactor: repository 비동기 콜백 구조로 수정 * refactor: 불필요한 함수 호출 로직 개선 * refactor: 사용하지 않는 데이터바인딩 변수 제거 * refactor: 함수 분리 * feat: 장바구니 마지막 페이지 데이터 없을 경우 이전 페이지로 이동하도록 수정 * fix: 장바구니 무한 호출 이슈 해결 * refactor: 더보기 버튼 가시성 로직 수정 * refactor: 페이지네이션 여부 판단 책임 뷰모델로 이동 * refactor: 무한스크롤 여부 판단 책임 뷰모델로 이동 * refactor: Product 객체 생성자에 imageUrl 추가 * chore: view/common 패키지 분리 * refactor: 휘발성 이벤트 SingleLiveData 적용 * refactor: 화면 회전 시 뷰모델 호출 횟수 개선 * chore: 코드 포맷팅 * refactor: 상품 목록 RecyclerView 데이터 수정 로직 개선 * fix: 더보기 버튼 클릭 시 상품 목록 데이터 호출되지 않는 이슈 해결 * build: corecore-testing 라이브러리 추가 * test: 뷰모델 테스트 위한 확장함수 구현 * test: ProductsViewModel 테스트 추가 * test: TestFixture 매개변수의 값에 따라 생성될 수 있도록 수정 * test: ShoppingCartViewModel 테스트 추가 * test: ProductDetailViewModel 테스트 추가 * test: ProductDetail UI 테스트 추가 --------- Co-authored-by: Giovanni Junseo Kim <[email protected]> commit e31149b Author: woowahan-dino <[email protected]> Date: Mon Apr 7 14:31:36 2025 +0900 build: upgrade to kotlin 2.1.0 and android gradle plugin 8.5.2 commit 281c081 Author: woowahan-pjs <[email protected]> Date: Fri Apr 12 12:00:25 2024 +0900 build: upgrade to android gradle plugin 8.3.1 and add junit 5 commit 186fb7f Author: woowahan-pjs <[email protected]> Date: Mon May 8 14:05:24 2023 +0900 feat: set up the project commit c0c96e0 Author: 박재성 <[email protected]> Date: Mon Apr 10 20:30:26 2023 +0900 Initial commit
잘못된 뷰모델 메서드 호출 minus -> add
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이번 변경에서는 Android 쇼핑 카트 애플리케이션의 전체 기능 명세와 아키텍처가 대규모로 추가 및 업데이트되었습니다. README.md는 앱의 기능 요구사항을 상세히 설명하는 명세서로 확장되었습니다. 앱의 빌드 스크립트와 의존성 설정이 대폭 보강되어, 네트워크, 이미지 로딩, 데이터 직렬화, Room 데이터베이스, 테스트 도구 등이 추가되었습니다. 앱의 주요 기능별로 도메인 모델, 데이터 레이어(로컬/원격/레포지토리), ViewModel, 액티비티, 어댑터, 뷰홀더, 바인딩 어댑터, 공통 유틸리티, 이벤트 관리 구조가 새롭게 구현되었습니다. 또한, 각 기능별 UI 레이아웃 XML과 다양한 상태/버튼/아이콘 드로어블 리소스가 추가되었으며, 메인 액티비티와 관련 파일은 삭제되고, 제품 목록, 상세, 장바구니, 추천 등 각 화면별 액티비티 및 테스트 코드가 신설되었습니다. 전반적으로 앱의 전체 구조와 기능이 처음부터 완성형으로 구축된 대규모 신규 반영입니다. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뭉치 안녕하세요!! 미션 구현하느라 고생하셨습니다.
Gson과 Kotlin Serialization의 차이점이 더 있을텐데 어떤 것이 있을까요? 🤔
고민해보면 좋을 부분들과, 궁금한 부분들에 리뷰드렸습니다!
반영 후 재요청 부탁드려요! 💪
private const val USER_PASSWORD_KEY = "password" | ||
|
||
@Suppress("ktlint:standard:property-naming") | ||
private var INSTANCE: AuthenticationRepository? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
대문자로 써준 이유가 있으신가요?
올대문자로 쓰는 것은 const val
같은 컴파일 타임 상수에 사용하는 네이밍으로 알고 있어서요~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
싱글톤처럼 사용되는 값이라 올 대문자로 썼는데, 오해의 소지가 있을 것 같아 카멜케이스로 수정했습니다!
e76194b
private const val BASE_URL = | ||
"http://techcourse-lv2-alb-974870821.ap-northeast-2.elb.amazonaws.com/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local.properties, BuildConfig로 이 URL을 외부에서 관리해볼 수 있어요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseUrl 을 숨기도록 수정했습니다!
77f803b
import woowacourse.shopping.data.authentication.repository.AuthenticationRepository | ||
import java.io.IOException | ||
|
||
class AppInterceptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
더 명확한 이름은 없을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용자 인증 정보를 가진 인터셉터여서 AuthenticationInterceptor
로 수정했습니다!
a7af392
private val productService: ProductService by lazy { | ||
ApiClient | ||
.getApiClient(DefaultAuthenticationRepository.get()) | ||
.create(ProductService::class.java) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Products API에서도 Basic 인증이 필요할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분을 놓쳤네요😓
상품을 가져올 땐 사용자 인증이 필요없다는 것을 확인해, retrofit 객체 함수를 유저 인증이 필요한 것과 필요없는 것으로 분리해서 사용했습니다!
a81c33c
id = "oungsi2000", | ||
password = "password", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 정보도 local.properties와 BuildConfig로 관리해볼 수 있습니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 내용 반영했습니다!
71f3ffd
productsRepository.getRecentWatchingProducts(10) { result -> | ||
result | ||
.onSuccess { recentWatchingProducts -> | ||
var updatedProducts = productsWithoutLoadItem | ||
if (hasRecentWatching) { | ||
updatedProducts = productsWithoutLoadItem.drop(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만약 최근에 본 상품 위치가 바뀌면 이 코드는 어떻게 될까요? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위치가 바뀌면 문제가 생길 수 있을 것 같습니다 !
따라서 filterNot 을 이용해 해당 타입을 제거한 값만 가질 수 있도록 수정했습니다.
72b513f
} | ||
} | ||
|
||
viewModel.shoppingCartProductsToOrder.observe(this) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
없어도 되는 코드일까요? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoppingCartProductsToOrder
값을 어디서도 옵저빙 하고 있지 않을 경우에 저 값을 직접적으로 사용하게 되면 null 값이 뜨는 것을 확인했습니다.
따라서, LiveData의 값을 사용하려면 반드시 옵저빙이 선행되어야 한다는 점을 인지하고, 해당 코드를 추가하게 되었습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 LiveData라서 어쩔 수 없겠군요 😢
보다보니 역으로 shoppingCart
가 MediatorLiveData를 사용해야하는건 아닐까 싶기도 하구요!
원본 데이터들이 바뀌었을 때 뷰를 위한 데이터 리스트인 shoppingCart
가 따라 바뀌는 형태가 된다면 어떨까요?
private val _isLoading: MutableLiveData<Boolean> = MutableLiveData(true) | ||
val isLoading: LiveData<Boolean> get() = _isLoading | ||
|
||
private val _totalPrice: MediatorLiveData<Int> = MediatorLiveData<Int>().apply { value = 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MediatorLiveData
는 무엇인가요?
사용하게 된 이유가 궁금해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MediatorLiveData는 여러 개의 LiveData를 관찰하고, 그 값들을 조합하거나 가공해서 하나의 데이터로 만들 수 있는 LiveData 로 알고 있습니다!
totalPrice / totalQuantity 등의 경우 다른 LiveData의 값이 변경될 때마다 해당 값이 변경되어야 했습니다. LiveData 로 선언이 됐을 경우 이 값을 업데이트 하기 위해서 항상 새로 계산해야 하는 경우가 있었는데, MediatorLiveData 를 통해서 계산된 값을 자동으로 업데이트할 수 있어 사용했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MediatorLiveData
대신 LiveData.map
을 사용하는 것을 고려해볼 수도 있을 것 같아요~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 이런 것도 있군요 ! 감사합니다 : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ListAdapter를 활용해볼 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 내용 반영했습니다!
private fun handleEmptySelectedQuantityVisibility() { | ||
binding.productQuantityComponentLayout.visibility = View.GONE | ||
binding.productPlusQuantityButtonDefault.visibility = View.VISIBLE | ||
} | ||
|
||
private fun handleSelectedQuantityVisibility(quantity: Int) { | ||
binding.productQuantityComponentLayout.visibility = View.VISIBLE | ||
binding.productPlusQuantityButtonDefault.visibility = View.GONE | ||
binding.productQuantityComponent.quantity = quantity | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
visibility를 binding한 productItem값을 보고, xml에서 설정해줄 수 있지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추천 상품 및 메인 화면 상품 목록 수량 조절 버튼 가시성을 데이터 바인딩으로 수정했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 67
♻️ Duplicate comments (16)
app/src/main/java/woowacourse/shopping/data/product/remote/dto/ProductResponseDto.kt (1)
20-27
: 🛠️ Refactor suggestion도메인 변환 책임에 대해 다시 한번 생각해보세요.
이전 리뷰 코멘트에서 언급된 내용과 관련하여: DTO에서 도메인 객체로의 변환이 과연 DTO의 책임일까요?
다음 관점에서 고민해보세요:
- 단일 책임 원칙(SRP) 측면에서 DTO의 역할은 무엇인가요?
- 도메인 변환 로직이 DTO에 있을 때와 별도 Mapper에 있을 때의 장단점은?
- 향후 도메인 모델이 변경될 때 영향범위는 어떻게 달라질까요?
리팩토링 계획에 이런 아키텍처 개선도 포함해보시면 좋겠습니다.
app/src/main/java/woowacourse/shopping/view/common/QuantityObservable.kt (1)
3-3
: 🛠️ Refactor suggestion마커 인터페이스의 목적과 네이밍에 대해 고민해보세요.
이전 리뷰 코멘트에서 언급된 "Observable"의 의미에 대해 다시 생각해보면:
- 현재 빈 인터페이스인데 "Observable"이라는 이름이 적절한가요?
- 실제 Observer 패턴이나 Reactive Programming의 Observable과 혼동될 수 있지 않을까요?
- 이 인터페이스를 구현하는 클래스들의 공통점은 무엇인가요?
마커 인터페이스를 사용하는 이유와 더 명확한 네이밍에 대해 고민해보시고, 리팩토링 시점에 개선해보세요.
app/src/main/java/woowacourse/shopping/domain/authentication/UserAuthentication.kt (1)
1-6
: 도메인 레이어에서의 인증 정보 모델링에 대해 다시 한번 생각해보세요.기존 리뷰어의 질문과 동일한 관점에서,
UserAuthentication
이 도메인 계층에 적절한지 고민해볼 필요가 있습니다.다음 질문들을 통해 스스로 답을 찾아보세요:
- 이 클래스가 순수한 비즈니스 로직을 표현하는가요?
- 인증 정보는 도메인 개념일까요, 아니면 인프라스트럭처 관심사일까요?
- Clean Architecture에서 도메인 계층의 역할은 무엇인가요?
힌트: 인증 방식이 변경되거나 다른 시스템과 통합할 때 이 모델이 어떻게 영향받을지 생각해보세요.
app/src/main/java/woowacourse/shopping/data/product/remote/dto/PageableResponseDto.kt (1)
8-19
: @SerialName 사용에 대한 이전 리뷰 의견과 관련해서이전 리뷰에서
@SerialName
이 왜 필요한지에 대한 질문이 있었는데, 이에 대해 생각해보셨나요?
@SerialName
이 언제 필요하고 언제 생략할 수 있는지 고민해보시면 좋겠습니다:
- JSON 필드명과 Kotlin 프로퍼티명이 다를 때
- Kotlin 네이밍 컨벤션과 API 응답의 네이밍 컨벤션이 다를 때
- 명시적으로 직렬화 필드명을 지정하고 싶을 때
현재 코드에서는 모든 프로퍼티에
@SerialName
을 사용하고 있는데, 각각이 정말 필요한지 검토해보시고, 팀의 코딩 컨벤션과 일치하는지 확인해보시면 좋겠습니다.app/src/main/java/woowacourse/shopping/ShoppingApplication.kt (1)
54-59
: 하드코딩된 인증 정보에 대한 보안 고려사항이전 리뷰에서 언급된 것처럼, 하드코딩된 사용자 인증 정보가 포함되어 있습니다. 이런 민감한 정보를 어떻게 더 안전하게 관리할 수 있을지 생각해보세요.
힌트: BuildConfig나 local.properties를 활용하는 방법을 고려해보세요. 또한 개발/테스트 환경과 프로덕션 환경을 어떻게 구분할 수 있을까요?
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductAdapter.kt (2)
8-31
: ListAdapter 활용을 고려해보세요현재 구현에서
notifyDataSetChanged()
를 사용하고 있는데, 이는 성능상 효율적이지 않을 수 있습니다. ListAdapter를 사용하면 어떤 이점들이 있을까요? DiffUtil과 함께 어떻게 더 효율적인 업데이트를 구현할 수 있는지 생각해보세요.
8-31
: 🛠️ Refactor suggestionListAdapter 사용을 고려해보는 것은 어떨까요?
이전 리뷰에서 제안된 ListAdapter 사용을 다시 한번 고려해보시길 권합니다. 현재
notifyDataSetChanged()
를 사용하고 있는데, 이는 전체 리스트를 다시 그리는 비효율적인 방법입니다.ListAdapter를 사용하면 어떤 장점들이 있을지 한번 생각해보세요:
- DiffUtil을 내부적으로 사용하여 어떤 부분이 변경되었는지 자동으로 계산
- 더 부드러운 애니메이션 효과
- 성능 최적화
ListAdapter로 변경하려면 어떤 요소들이 필요할지 먼저 조사해보시겠어요?
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/service/ShoppingCartService.kt (1)
20-23
: API 메서드 네이밍 컨벤션을 고려해보세요메서드명
postCartItem
에 대해 생각해보세요:
- HTTP 메서드(POST)를 메서드명에 포함하는 것이 좋은 관례일까요?
- 메서드명만 보고 어떤 동작을 하는지 명확하게 알 수 있나요?
- 도메인 관점에서 더 의미있는 이름은 무엇일까요?
비슷한 기능을 하는 다른 메서드들(
updateCartItemQuantity
,deleteCartItem
)과 네이밍 일관성도 함께 고려해보세요.app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductViewHolder.kt (1)
28-37
: ****과거 리뷰 코멘트에서 언급된 것처럼, visibility 설정을 XML 바인딩을 통해 처리하는 방법을 고려해보세요.
현재 코드에서 visibility 로직이 ViewHolder에 있는 이유를 생각해보고, 이를 데이터 바인딩으로 옮길 수 있는지 검토해보세요. 어떤 장단점이 있을까요?
app/src/main/java/woowacourse/shopping/data/product/repository/DefaultProductsRepository.kt (1)
1-1
: Repository 구조에 대한 고민이전에 hyemdooly님이 DataSource 분리에 대해 좋은 질문을 하셨는데요, 이에 대한 뭉치님의 생각이 궁금합니다!
현재 구조의 장단점:
- 장점: 간단하고 직관적
- 단점: 테스트 어려움, 책임이 혼재
DataSource를 분리한다면:
- RemoteDataSource: 네트워크 관련 로직
- LocalDataSource: 데이터베이스 관련 로직
- Repository: 두 DataSource를 조율
어떤 접근이 프로젝트에 더 적합하다고 생각하시나요?
app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/DefaultShoppingCartRepository.kt (2)
105-110
:⚠️ Potential issue일관된 응답 검증 패턴 적용 필요
다른 메서드들과 동일하게 HTTP 응답 상태를 확인해야 합니다.
override fun onResponse( call: Call<Unit>, response: Response<Unit>, ) { - onResult(Result.success(Unit)) + if (response.isSuccessful) { + onResult(Result.success(Unit)) + } else { + onResult(Result.failure(Exception("Failed to update quantity: ${response.code()}"))) + } }
150-155
:⚠️ Potential issue응답 검증 및 기본값 처리 개선 필요
HTTP 응답 상태를 확인하지 않고, null일 때 0을 반환하는 것은 실제 에러를 숨길 수 있습니다.
override fun onResponse( call: Call<CartCountsResponseDto?>, response: Response<CartCountsResponseDto?>, ) { - onResult(Result.success(response.body()?.quantity ?: 0)) + if (response.isSuccessful) { + onResult(Result.success(response.body()?.quantity ?: 0)) + } else { + onResult(Result.failure(Exception("Failed to fetch quantity: ${response.code()}"))) + } }app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartActivity.kt (1)
122-122
:⚠️ Potential issue빈 옵저버 제거
아무 동작도 하지 않는 옵저버는 불필요합니다. 향후 사용 예정이라면 TODO 주석을 추가하고, 그렇지 않다면 제거하는 것이 좋습니다.
app/src/main/java/woowacourse/shopping/view/product/ProductsActivity.kt (1)
48-48
: Boolean 변수명 개선 필요Boolean 타입 변수에는 is, has, should 등의 접두사를 사용하는 것이 좋습니다.
app/src/main/java/woowacourse/shopping/view/product/ProductsViewModel.kt (1)
127-129
: 최근 본 상품 위치 변경 시 안전성 문제하드코딩된 인덱스를 사용하는 것은 위험합니다. 최근 본 상품의 위치가 변경되면 예상치 못한 동작이 발생할 수 있습니다.
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModel.kt (1)
31-48
: MediatorLiveData 사용 이유 설명이 필요합니다
MediatorLiveData
는 여러 LiveData 소스를 관찰하고, 그 소스들이 변경될 때마다 자동으로 값을 업데이트하는 LiveData입니다. 여기서는_shoppingCart
의 변경을 관찰하여 총 가격, 수량, 전체 선택 상태 등의 파생된 값들을 자동으로 계산하는 데 사용하고 있네요.이 접근 방식은 좋지만, 왜 이 방법을 선택했는지 간단한 주석을 추가하면 다른 개발자들이 코드를 이해하는 데 도움이 될 것 같습니다.
🧹 Nitpick comments (70)
app/src/main/res/drawable/background_order_button_disabled.xml (1)
2-5
: 명시적 shape 속성 및 코너 반경 추가 권장
현재shape
요소에 기본 타입(사각형)이 사용되지만,android:shape="rectangle"
을 명시하면 가독성이 향상됩니다. 또한 버튼의 모서리 일관성을 위해corners
속성으로 반경을 지정하는 것을 고려해보세요.app/src/main/res/drawable/background_order_button_enabled.xml (1)
2-4
: 통일된 모서리 스타일 적용 제안
background_order_button_enabled.xml
에서도android:shape="rectangle"
을 명시하고,background_order_button_disabled.xml
과 동일한corners
반경을 적용하여 디자인 일관성을 유지하면 좋겠습니다.app/src/main/res/drawable/background_shopping_cart_pagination_plus_button_un_enabled.xml (1)
1-9
: disabled 상태 드로어블 네이밍 컨벤션 검토 권장
현재un_enabled
네이밍을 사용 중이나, Android 가이드라인 상disabled
용어 사용이 더 명확할 수 있습니다. 프로젝트 전체 네이밍 컨벤션을 고려해 일관성 있는 용어로 수정하는 것을 제안합니다.app/src/main/res/drawable/ic_back.xml (1)
1-9
: 경로 데이터의-0
좌표를0
으로 정리해주세요.
벡터 경로에서-0
은 0 과 동일하지만 가독성을 위해 음수 부호를 제거하는 편이 좋습니다.- android:pathData="M15.84,6.92V8.92H3.84L9.34,14.42L7.92,15.84L-0,7.92L7.92,-0L9.34,1.42L3.84,6.92H15.84Z" + android:pathData="M15.84,6.92V8.92H3.84L9.34,14.42L7.92,15.84L0,7.92L7.92,0L9.34,1.42L3.84,6.92H15.84Z"app/src/main/java/woowacourse/shopping/view/common/Messages.kt (1)
6-8
: 유용한 확장 함수입니다. 더 유연하게 만들 방법을 고민해보세요.공통 기능을 확장 함수로 추출한 접근이 좋습니다. 다만 다음 사항들을 고려해보면 어떨까요?
- 스낵바의 지속 시간이 항상
LENGTH_SHORT
이어야 할까요?- 액션 버튼이 필요한 경우는 어떻게 처리할까요?
- 이 함수를 어떻게 테스트할 수 있을까요?
현재 구현도 충분하지만, 미래의 요구사항 변화에 대비해 어떻게 확장 가능하게 만들 수 있는지 생각해보세요.
app/src/androidTest/java/woowacourse/shopping/fixture/TestFixture.kt (1)
1-6
: 테스트 픽스처의 구조와 네이밍을 개선해보세요.현재 구현은 기능적으로 문제없지만 몇 가지 개선을 고려해볼 수 있습니다:
- 파일명이
TestFixture
인데 내부에 여러 픽스처를 추가할 가능성을 고려하면TestFixtures
가 더 적절할 수 있습니다.fakeContext
라는 네이밍이 실제 용도를 명확히 드러내는지 생각해보세요.- 이 Context가 어떤 용도로 사용되는지 KDoc 주석을 추가하는 것이 어떨까요?
테스트 픽스처의 일관성과 확장성을 위해 어떤 네이밍 컨벤션을 사용할 계획인지 고민해보세요.
app/src/main/res/drawable/ic_shopping_cart.xml (1)
1-9
: 벡터 드로어블의 색상 관리와 접근성을 개선해보세요.현재 구현에서 개선할 수 있는 부분들을 생각해보세요:
android:fillColor="#ffffff"
처럼 하드코딩된 색상을 사용하고 있습니다. 다크 모드나 테마 변경을 고려하여@color/
리소스를 사용하는 것이 좋을까요?이 아이콘이 버튼에서 사용될 때 접근성(content description)은 어떻게 처리하고 계신가요?
pathData가 매우 복잡한데, 이런 복잡한 경로를 가진 아이콘의 경우 디자이너와 협업하여 최적화할 수 있는 방법이 있는지 확인해보세요.
벡터 드로어블의 재사용성과 테마 대응을 위해 어떤 전략을 사용할 계획인지 생각해보세요.
app/src/main/res/drawable/background_shopping_cart_pagination_minus_button_un_enabled.xml (1)
1-9
: 드로어블 파일의 네이밍 컨벤션을 검토해보세요.파일명과 구조에 대해 몇 가지 개선점을 고려해보세요:
un_enabled
라는 표현보다는disabled
가 더 명확하고 일반적입니다. Android의 상태 네이밍 컨벤션을 따르는 것이 어떨까요?파일명이 상당히 길어서 가독성이 떨어집니다. 더 간결하면서도 의미를 전달할 수 있는 네이밍 구조를 고민해보세요.
이런 상태별 드로어블들을 selector로 관리하고 있는지 확인해보세요. enabled/disabled 상태를 하나의 selector에서 관리하면 유지보수가 더 쉬워집니다.
프로젝트 전체에서 사용하는 드로어블 네이밍 컨벤션이 일관성 있게 적용되고 있는지 점검해보세요.
app/src/main/java/woowacourse/shopping/domain/shoppingCart/ShoppingCarts.kt (1)
3-6
: 도메인 모델의 명확성과 확장성을 고려해보세요.현재 구현에서 개선을 고려할 수 있는 부분들을 생각해보세요:
클래스명이
ShoppingCarts
(복수형)인데 실제로는 하나의 페이지를 나타내는 것 같습니다.ShoppingCartPage
또는ShoppingCartItems
가 더 명확할 수 있을까요?페이지네이션을 위해서는
last
외에도 현재 페이지 번호나 전체 페이지 수 같은 정보가 필요할 수 있습니다. 향후 확장 가능성을 고려해보세요.빈 리스트나 null 값에 대한 처리는 어떻게 하고 계신가요? 도메인 규칙을 반영한 검증 로직이 필요한지 고민해보세요.
도메인 모델이 비즈니스 요구사항을 명확히 표현하고 있는지, 그리고 향후 기능 확장에 유연하게 대응할 수 있는지 검토해보세요.
app/src/main/java/woowacourse/shopping/domain/product/Product.kt (2)
3-3
: 직렬화 방식에 대한 일관성을 고려해보세요.PR 설명에서 Kotlin Serialization을 선택했다고 하셨는데, 여기서는 Java의
Serializable
을 사용하고 있습니다. 이 선택의 이유가 무엇인지 생각해보시고, 앱 전반에서 일관된 직렬화 방식을 사용하는 것이 어떨지 고민해보세요.질문: 도메인 모델에서 Java Serializable을 사용하는 이유가 특별히 있나요? 다른 DTO 클래스들과의 일관성은 어떻게 유지할 계획인가요?
10-10
: 카테고리 타입 안전성을 고려해보세요.
category
를String
으로 정의하셨는데, 이렇게 하면 어떤 문제점들이 생길 수 있을지 생각해보세요.힌트:
- 오타나 잘못된 카테고리 이름이 들어갈 가능성은?
- 카테고리 목록이 정해져 있다면 어떤 방식이 더 안전할까요?
다른 방법들을 고민해보시고, 현재 설계의 장단점을 분석해보세요.
app/src/androidTest/java/woowacourse/shopping/fixture/ProductFixtures.kt (1)
5-12
: 테스트 픽스처 설계에 대해 생각해보세요.현재 하나의 제품 픽스처만 정의되어 있는데, 더 포괄적인 테스트를 위해 어떤 것들이 필요할지 고민해보세요.
고려사항들:
- 경계값 테스트를 위한 픽스처들 (최소/최대 가격, 긴 이름 등)
- 다양한 카테고리의 제품들
- 특수 문자나 이모지가 포함된 이름들
외부 의존성 질문: 현재 이미지 URL이 외부 사이트를 가리키고 있는데, 이것이 테스트 환경에서 어떤 문제를 일으킬 수 있을지 생각해보세요.
app/src/main/java/woowacourse/shopping/data/product/remote/dto/SortResponseDto.kt (1)
7-14
: 데이터 모델의 의미적 설계에 대해 고민해보세요.현재 구현된
SortResponseDto
를 살펴보면sorted
와unsorted
두 개의 Boolean 속성이 있습니다. 이 두 속성이 서로 상반된 의미를 가지는 것 같은데, 이런 경우 데이터 모델링 관점에서 어떤 방식이 더 명확하고 안전할지 생각해보세요.또한
empty
속성이 정렬 상태와 어떤 관계인지 명확하지 않습니다. 이 속성의 의미를 코드에서 더 명확하게 표현할 수 있는 방법은 없을까요?다음과 같은 질문들을 고려해보세요:
- 두 개의 Boolean 값 대신 상태를 표현하는 더 안전한 방법이 있을까요?
- API 응답의 의미를 코드에서 더 명확하게 드러낼 수 있는 방법은 무엇일까요?
app/src/main/res/drawable/btn_product_plus.xml (1)
1-21
: 벡터 드로어블 구현이 깔끔합니다.아이콘의 크기와 스타일링이 일관성 있게 구현되어 있습니다.
디자인 시스템 관점에서 고려해볼 점들:
- 다른 버튼 아이콘들(마이너스 버튼 등)과 스타일이 일치하는지 확인해보세요
- 접근성을 위한 충분한 터치 영역(최소 48dp)이 확보되어 있는지 검토해보세요
- 색상 값
#555555
가 앱의 디자인 시스템에서 정의된 색상인지 확인해보세요app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartCountsResponseDto.kt (1)
7-10
: 데이터 유효성과 의미 명확성에 대해 생각해보세요.현재
quantity
속성이Int
타입으로 정의되어 있습니다. 장바구니의 개수 정보를 다룰 때 고려해야 할 몇 가지 질문이 있습니다:
- 음수 값이 의미가 있을까요? 만약 그렇지 않다면 어떤 방식으로 이를 보장할 수 있을까요?
quantity
가 정확히 무엇을 의미하는지 명확한가요? (전체 아이템 개수 vs 고유 상품 종류 수)다음을 고려해보세요:
- 데이터 유효성을 보장하는 방법
- 속성명이나 주석을 통해 의미를 더 명확하게 전달하는 방법
- 도메인 모델로 변환할 때 필요한 검증 로직
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductListener.kt (1)
5-9
: 인터페이스 설계가 깔끔합니다!
ProductQuantityClickListener
를 확장하여 기능을 추가하는 접근 방식이 좋습니다. 하지만 인터페이스 설계를 더 개선할 수 있는 부분들이 있어 보입니다.다음 질문들을 고민해보세요:
onBackButtonClick()
과onOrderButtonClick()
메서드가 정말 이 인터페이스의 책임에 맞나요?- 이 두 메서드들이 수량 조절 리스너와 함께 있는 것이 응집력 측면에서 적절한가요?
- 향후 더 많은 버튼이나 액션이 추가될 가능성을 고려했나요?
인터페이스 분리 원칙(ISP)을 생각해보시고, 필요하다면 역할별로 인터페이스를 분리하는 것도 고려해보세요.
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/SortResponseDto.kt (1)
6-14
: DTO 구조는 표준을 잘 따르고 있습니다.Kotlin Serialization을 사용한 DTO 구현이 깔끔합니다. 다만 비즈니스 로직 관점에서 몇 가지 궁금한 점이 있습니다:
empty
,sorted
,unsorted
세 프로퍼티의 관계를 생각해보세요:
- 이 세 값이 동시에 true/false일 수 있는 경우는 어떤 의미인가요?
sorted
와unsorted
가 동시에 true인 경우가 있을까요?- API 명세에서 이 값들의 의미와 관계가 명확히 정의되어 있나요?
DTO에 간단한 KDoc 주석을 추가하여 각 프로퍼티의 의미를 명확히 하면 코드 가독성이 향상될 것 같습니다.
app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailClickListener.kt (1)
5-9
: 인터페이스 설계가 깔끔하지만 개선점을 고려해보세요.현재
ProductQuantityClickListener
를 확장하는 방식은 좋은 접근입니다. 하지만 몇 가지 고려해볼 점들이 있습니다:
- 메서드명
onAddingToShoppingCart()
에서 "-ing" 형태보다는onAddToShoppingCart()
같은 명령형이 더 일반적입니다.- 이 인터페이스가 정말
ProductQuantityClickListener
를 확장해야 하는지 생각해보세요. 상속 관계가 "is-a" 관계를 나타내는 것이 맞나요?인터페이스 분리 원칙(ISP)을 고려했을 때, 어떤 방식이 더 나은 설계일까요?
app/src/main/java/woowacourse/shopping/view/product/ProductsEvent.kt (1)
3-8
: 이벤트 네이밍과 구조화 방식을 개선해보세요.현재 enum 구조에서 개선할 점들을 생각해보세요:
- 이벤트명이 일관성 있나요?
UPDATE_PRODUCT_FAILURE
와NOT_ADD_TO_SHOPPING_CART
의 네이밍 패턴이 다른 이유는 무엇일까요?- 모든 실패 이벤트를 하나의 enum에 모으는 것이 확장성 측면에서 좋을까요?
- 각 이벤트가 추가 데이터(예: 에러 메시지, 실패 원인)를 필요로 한다면 어떻게 처리할 건가요?
sealed class나 다른 방식을 사용했을 때의 장단점을 비교해보고, 어떤 상황에서 어떤 방식이 더 적합한지 고민해보세요.
app/src/main/java/woowacourse/shopping/view/common/ResultFrom.kt (1)
8-8
: 세미콜론 사용에 대해 고민해보세요Kotlin에서 enum 값 뒤의 세미콜론은 문법적으로는 정확하지만, 추가 멤버가 없다면 일반적으로 생략합니다.
현재 코드에서 세미콜론이 필요한 이유가 있나요? 코드의 일관성과 가독성 측면에서 어떤 선택이 더 좋을지 생각해보시면 좋겠습니다.
app/src/main/java/woowacourse/shopping/view/common/BindingAdapter.kt (1)
7-16
: 이미지 로딩 시 사용자 경험 개선을 고려해보세요.현재 구현은 기본적인 기능은 수행하지만, 실제 사용자가 경험할 수 있는 상황들을 생각해보면 어떨까요?
몇 가지 고려해볼 점들:
- 이미지 로딩 중일 때 사용자에게 어떤 피드백을 줄 수 있을까요?
- 네트워크 오류나 잘못된 URL로 인해 로딩이 실패했을 때는 어떻게 처리하면 좋을까요?
- Glide의 어떤 메서드들을 활용하면 이런 상황들을 개선할 수 있을까요?
app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailEvent.kt (1)
3-9
: 이벤트 구조와 상태 관리 패턴을 고민해보세요.현재 이벤트들이 평면적으로 나열되어 있는데, 상태 관리 관점에서 어떻게 개선할 수 있을까요?
생각해볼 점들:
- 유사한 이벤트들(예: FAILURE 관련)을 어떻게 그룹화할 수 있을까요?
- 이벤트에 추가 데이터가 필요한 경우는 어떻게 처리하면 좋을까요?
- Sealed class와 enum의 차이점을 고려했을 때, 어떤 것이 더 적합할까요?
ViewModel에서 이런 이벤트들을 어떻게 효율적으로 관리하고 있는지도 함께 살펴보시면 도움이 될 것 같습니다.
app/src/main/java/woowacourse/shopping/view/common/Event.kt (1)
3-18
: 이벤트 래퍼 클래스 구현이 깔끔합니다단일 소비 이벤트 패턴을 잘 구현하셨네요. UI 이벤트 관리에 효과적인 패턴입니다.
다음 사항들을 한번 고려해보세요:
hasBeenHandled
프로퍼티의 스레드 안전성은 어떨까요? UI 스레드에서만 사용된다면 문제없지만, 다른 스레드에서도 접근할 가능성은 없을까요?- 클래스를
open
으로 선언한 이유가 있나요? 상속이 필요한 특별한 케이스가 있을까요?이런 설계 결정들에 대해 생각해보고, 필요시 문서화해보세요.
app/src/androidTest/java/woowacourse/shopping/productDetail/ProductDetailActivityTest.kt (1)
34-42
: 테스트 커버리지 확장 고려기본적인 텍스트 표시 테스트가 잘 구현되어 있습니다.
추가로 고려해볼 수 있는 테스트 케이스들이 있을까요?
- 잘못된 상품 ID가 전달되었을 때는 어떻게 될까요?
- 수량 변경 버튼들의 동작은 어떻게 테스트할 수 있을까요?
- 장바구니 추가 기능은 어떻게 검증할 수 있을까요?
단계별로 하나씩 추가해나가는 것을 고려해보세요.
app/src/main/java/woowacourse/shopping/view/common/MutableSingleLiveData.kt (1)
3-15
: 상태 관리 패턴이 잘 구현되었습니다SingleLiveData를 확장하여 mutable 접근을 제공하는 깔끔한 구현입니다.
상태 관리 아키텍처 관점에서 생각해보세요:
- 이 패턴이 ViewModel에서 UI 이벤트를 관리하는 데 어떤 장점을 제공하나요?
- 일반적인 MutableLiveData 대신 이 클래스를 사용하는 이유는 무엇일까요?
- 다른 ViewModel들에서 이 패턴을 일관성 있게 적용하고 있나요?
전체 앱의 상태 관리 전략에서 이 클래스의 역할을 문서화해보는 것도 좋겠습니다.
app/src/main/java/woowacourse/shopping/view/product/ProductMoreViewHolder.kt (3)
8-14
: ViewHolder 생성자와 초기화 구조를 개선해보세요.현재 구조는 적절하지만, 더 나은 관심사 분리를 위해 고려해볼 점이 있습니다. 바인딩에 직접 리스너를 할당하는 것과 ViewHolder에서 처리하는 것 중 어떤 접근이 더 적절할지 생각해보세요.
데이터 바인딩을 통해 클릭 이벤트를 처리할 때의 장단점은 무엇인가요?
17-25
: 팩토리 메서드의 매개변수 순서를 일관성 있게 정리해보세요.매개변수 순서를 살펴보면, 일반적으로 Android에서는
parent
가 먼저 오는 것이 관례입니다. 프로젝트 전체에서 일관된 패턴을 유지하는 것이 중요합니다.다른 ViewHolder 클래스들과 비교해서 매개변수 순서가 일관성이 있는지 확인해보세요.
27-30
: 함수형 인터페이스 네이밍과 위치를 검토해보세요.
ProductMoreClickListener
라는 네이밍이 적절한지 고려해보세요. "More"라는 용어가 명확한 의미를 전달하는지, 그리고 이 인터페이스가 ViewHolder 내부에 있는 것이 최적의 구조인지 생각해보세요.인터페이스를 별도 파일로 분리하는 것과 내부에 두는 것의 장단점은 무엇일까요?
app/src/main/java/woowacourse/shopping/data/product/remote/service/ProductService.kt (1)
10-21
: 현대적인 비동기 처리 방식을 고려해보세요.현재
Call<>
타입을 사용하고 있지만, 코루틴 기반의suspend
함수를 사용하는 것을 고려해보세요. 이렇게 하면 더 간결하고 현대적인 비동기 처리가 가능합니다.코루틴을 사용할 때의 장점과
Call<>
타입을 사용할 때의 차이점은 무엇인가요?app/src/main/res/layout/activity_shopping_cart_recommend.xml (2)
25-25
: tools:context 속성값이 실제 액티비티와 일치하지 않습니다.
tools:context=".view.shoppingCart.ShoppingCartActivity"
로 되어 있지만, 실제로는ShoppingCartRecommendActivity
를 가리켜야 할 것 같습니다. 이런 불일치는 개발 도구에서 혼란을 야기할 수 있습니다.실제 액티비티 클래스명을 확인하여 수정해보세요.
67-67
: 하드코딩된 마진값을 리소스로 분리해보세요.
android:layout_marginTop="104dp"
와 같은 하드코딩된 값들이 여러 곳에 있습니다. 디자인 시스템의 일관성을 위해 이러한 값들을dimens.xml
로 분리하는 것을 고려해보세요.어떤 마진값들을 공통 리소스로 만들면 좋을지 생각해보세요.
app/src/main/java/woowacourse/shopping/data/product/dataSource/VolatileProductDataSource.kt (2)
1-1
: ktlint 억제 어노테이션의 필요성을 재검토해보세요.파일 전체에 대해 ktlint를 억제하고 있습니다. 이것이 정말 필요한지, 그리고 특정 규칙만 억제할 수 있는지 검토해보세요.
코드 스타일 검사를 우회하는 것보다는 스타일을 맞추는 것이 좋지 않을까요?
162-177
: 매직 넘버와 하드코딩된 메시지를 상수로 분리해보세요.
-1
, 에러 메시지 등이 하드코딩되어 있습니다. 이러한 값들을 의미있는 상수로 추출하면 코드의 가독성과 유지보수성이 향상됩니다.또한 에러 메시지를 다국어 지원을 고려해서 리소스 파일로 분리하는 것도 고려해보세요.
app/src/main/java/woowacourse/shopping/view/product/ProductAdapter.kt (1)
48-56
:onBindViewHolder
에서 타입 안전성을 개선할 수 있습니다.현재
as
캐스팅을 사용하고 있는데, 만약 잘못된 타입이 전달되면 런타임 에러가 발생할 수 있습니다.어떤 방법으로 이런 위험을 줄일 수 있을까요? ViewHolder의 타입과 Item의 타입 간의 관계를 더 안전하게 만드는 방법을 생각해보세요.
app/src/main/java/woowacourse/shopping/data/network/ApiClient.kt (1)
23-26
: 로깅 레벨을 빌드 타입에 따라 동적으로 설정하는 것을 고려해보세요.현재
HttpLoggingInterceptor.Level.BODY
로 고정되어 있는데, 이는 프로덕션 빌드에서도 모든 HTTP 요청/응답 본문을 로그에 출력하게 됩니다.어떤 방법으로 Debug 빌드에서만 상세 로깅을 활성화하고, Release 빌드에서는 로깅을 비활성화하거나 최소화할 수 있을까요?
app/src/main/java/woowacourse/shopping/data/product/local/dao/RecentWatchingDao.kt (1)
14-26
: 복잡한 서브쿼리의 성능을 검토해보세요.가장 최근에 본 상품의 카테고리를 기준으로 추천 상품을 가져오는 로직은 좋지만, 서브쿼리가 포함된 복잡한 쿼리입니다.
데이터량이 많아지면 이 쿼리의 성능이 어떨지 생각해보세요. 인덱스가 필요한 컬럼들이 있는지, 또는 쿼리를 더 효율적으로 작성할 수 있는 방법이 있는지 고민해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/view/common/SingleLiveData.kt (2)
6-13
: 생성자 설계의 의도를 명확히 해보세요두 개의 protected 생성자를 제공하고 있는데, 어떤 상황에서 각각을 사용하도록 설계하셨나요? 서브클래스에서 이를 어떻게 활용할지 생각해보시고, 각 생성자의 용도를 주석으로 명시하는 것은 어떨까요?
6-31
: 이벤트 처리 구현에서 고려해볼 점들이 있습니다SingleLiveData 구현이 잘 되어 있습니다! 다만 몇 가지 추가로 고려해볼 점들이 있는데요:
메모리 관리:
getValue()
메소드에서 이벤트가 소비되지 않은 채로 남아있을 수 있습니다. 이것이 메모리에 어떤 영향을 줄 수 있을까요?스레드 안전성: 여러 스레드에서 동시에 접근할 때 어떤 일이 발생할 수 있을지 생각해보세요.
사용성: 현재 구현에서 Event 클래스의 존재를 외부에서 알아야 하는데, 이를 더 캡슐화할 수 있는 방법은 없을까요?
Android의 SingleLiveEvent 패턴에 대한 다른 구현 방식들도 조사해보시면 도움이 될 것 같습니다. 어떤 방식이 더 나은지 비교해보는 것은 어떨까요?
app/src/main/res/layout/activity_shopping_cart.xml (1)
42-48
: 하드코딩된 값들을 리소스로 분리하는 것을 고려해보세요
layout_marginStart="4dp"
,padding="16dp"
등 하드코딩된 값들이 있네요. 이런 값들을 dimens.xml에 정의하면 어떤 장점이 있을까요? 일관성 있는 디자인 시스템을 만들기 위해 어떤 방법들이 있는지 생각해보세요.app/src/main/java/woowacourse/shopping/view/product/RecentProductAdapter.kt (1)
7-10
: 어댑터의 데이터 관리 방식에 대해 생각해보세요현재 어댑터가
RecentWatchingItem
을 생성자에서 받아 저장하고 있습니다. 이런 설계에서 고려해볼 점들이 있을까요?
- 데이터가 변경될 때 어댑터는 어떻게 알 수 있을까요?
- 현재 구조에서 새로운 최근 본 상품이 추가되면 어떤 일이 발생할까요?
- 어댑터의 책임과 데이터 소스의 책임을 어떻게 분리할 수 있을까요?
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/service/ShoppingCartService.kt (1)
17-18
: API 응답 타입에 대해 생각해보세요
getCartCounts()
메서드가CartCountsResponseDto
를 반환하고 있습니다.
- 카운트 정보만 필요한 경우에 전체 DTO가 필요할까요?
- 더 간단한 타입이나 원시 타입을 사용할 수는 없을까요?
- 클라이언트 코드에서 이 응답을 어떻게 사용하게 될지 예상해보세요.
app/src/main/res/layout/item_product.xml (2)
22-30
: 이미지뷰의 접근성과 성능을 고려해보세요상품 이미지 표시 방식에 대해 생각해보세요:
contentDescription
에 상품명을 바인딩하고 있는데, 이것이 접근성 측면에서 충분할까요?- 1:1 비율 제약과
match_parent
너비가 다양한 화면 크기에서 어떻게 동작할까요?- 이미지 로딩 실패 시 사용자 경험은 어떨까요?
접근성 가이드라인과 이미지 로딩 라이브러리의 best practice를 확인해보세요.
43-58
: 레이아웃 계층 구조를 검토해보세요수량 컴포넌트를 위한 ConstraintLayout 래퍼에 대해 생각해보세요:
- 추가 ConstraintLayout이 꼭 필요한가요?
- include된 컴포넌트를 직접 parent ConstraintLayout에 배치할 수는 없을까요?
- 레이아웃 depth를 줄이면 어떤 장점이 있을까요?
성능 최적화 관점에서 레이아웃 계층을 분석해보세요.
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartProductAdapter.kt (1)
61-63
: 인터페이스 설계에 대해 재고해보세요두 개의 서로 다른 리스너 인터페이스를 상속받는 설계입니다.
질문해보세요:
- 이런 설계가 단일 책임 원칙(SRP)에 부합할까요?
- 각 ViewHolder가 필요로 하는 리스너만 받도록 설계를 개선할 수 있을까요?
- 인터페이스 분리 원칙(ISP)을 적용하면 어떻게 개선할 수 있을까요?
app/src/main/java/woowacourse/shopping/data/product/remote/dto/ProductsResponseDto.kt (1)
7-31
: 데이터 클래스 설계에서 고민해볼 점들페이징 응답을 위한 많은 필드들이 정의되어 있습니다.
자문해보세요:
- 모든 필드가 실제로 사용되고 있나요?
- 일부 필드들을 별도의 클래스로 그룹화하면 더 명확해질까요?
- 페이징 관련 로직이 DTO에 있는 것이 적절할까요?
- 도메인 모델과 DTO의 책임이 명확히 분리되어 있나요?
다른 페이징 DTO들과 중복되는 구조는 없는지 확인해보세요.
app/src/main/java/woowacourse/shopping/view/product/RecentProductViewHolder.kt (3)
31-33
: 인터페이스 위치와 네이밍에 대해 생각해보세요인터페이스가 ViewHolder 클래스 내부에 정의되어 있습니다.
고려해볼 점들:
- 이 인터페이스가 다른 곳에서도 사용될 가능성은 없을까요?
- 인터페이스 이름이 역할을 명확히 표현하고 있나요?
- ViewHolder와 리스너의 결합도가 너무 높지는 않을까요?
- 패키지 구조상 어디에 위치하는 것이 더 적절할까요?
다른 ViewHolder들의 리스너 인터페이스와 일관성 있는 설계를 하고 있는지 확인해보세요.
12-14
: 초기화 블록 사용에 대해 검토해보세요
init
블록에서 리스너를 설정하고 있습니다.생각해볼 점들:
- 이런 방식이 데이터 바인딩의 생명주기와 잘 맞을까요?
- 리스너 설정을 다른 방법으로 할 수는 없을까요?
- ViewHolder의 재사용 시 문제가 될 수 있는 부분은 없을까요?
- 바인딩과 리스너 설정의 책임을 어떻게 분리할 수 있을까요?
16-18
: 데이터 바인딩의 생명주기를 고려해보세요단순히 데이터를 바인딩에 할당하고 있습니다.
질문해보세요:
executePendingBindings()
나 다른 바인딩 관련 메소드 호출이 필요할까요?- 이전 데이터의 정리가 필요한 경우는 없을까요?
- 바인딩 과정에서 예외가 발생할 가능성은 없을까요?
- ViewHolder 재사용 시 고려해야 할 점들은 무엇일까요?
app/src/main/java/woowacourse/shopping/data/product/repository/ProductsRepository.kt (1)
5-31
: 비동기 처리 패턴에 대한 고려사항현재 콜백 기반의 비동기 처리를 사용하고 있는데, 몇 가지 질문을 드려보겠습니다:
- 여러 개의 비동기 작업을 연속으로 수행해야 할 때 콜백 패턴의 문제점은 무엇일까요?
- Android에서 권장하는 현대적인 비동기 처리 방식은 무엇인지 찾아보셨나요?
Result
타입을 사용한 에러 처리는 좋은 접근이지만, suspend 함수와 함께 사용하면 어떤 장점이 있을까요?콜백 패턴 대신 코루틴의 suspend 함수를 고려해보는 것은 어떨까요? 이에 대해 조사해보시고 어떤 차이점이 있는지 생각해보세요.
app/src/main/java/woowacourse/shopping/view/product/ProductViewHolder.kt (1)
18-25
: 메서드 구조와 가독성 개선 고려
bind
메서드의 구조를 살펴보면 early return을 사용하고 있습니다. 이 패턴에 대해 다음을 고려해보세요:
- early return이 코드 가독성에 미치는 영향은 무엇일까요?
- 두 가지 상태(수량이 0인 경우와 그렇지 않은 경우)를 더 명확하게 구분할 수 있는 방법은 없을까요?
- 상태에 따른 UI 변경 로직을 더 일관성 있게 처리할 수 있는 방법을 생각해보셨나요?
조건문을 if-else 구조로 변경하거나, 상태 관리를 위한 별도의 메서드를 만드는 것을 고려해보세요.
app/src/main/java/woowacourse/shopping/view/common/GridItemDecoration.kt (1)
44-50
: 로직 단순화 가능성spacing 계산 로직을 보면 조건이 복잡해 보입니다:
- 첫 번째 컬럼과 나머지 컬럼의 spacing 규칙을 더 직관적으로 표현할 수 있는 방법이 있을까요?
- 매직 넘버(예:
/2
)의 의미를 명확하게 하려면 어떻게 해야 할까요?- 이 로직을 별도 메서드로 분리하면 어떤 장점이 있을까요?
코드의 의도를 더 명확하게 표현하는 방법들을 고민해보세요.
app/src/main/java/woowacourse/shopping/view/product/ProductsItem.kt (1)
16-23
: 데이터 모델 설계 검토
ProductItem
의 설계에 대해 몇 가지 질문드립니다:
shoppingCartId
가 nullable인 이유는 무엇인가요? 이것이 UI 상태를 나타내는 건가요?selectedQuantity
의 기본값이 0인 것이 모든 상황에서 적절한가요?- UI 모델과 도메인 모델을 분리하는 것에 대해 어떻게 생각하시나요?
도메인 주도 설계(DDD)에서 말하는 계층 분리에 대해 조사해보시고, 현재 구조의 장단점을 분석해보세요.
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartPaginationViewHolder.kt (1)
38-42
: 인터페이스 설계에 대해 생각해보세요현재
ShoppingCartPaginationClickListener
가 단순히onMinusPage()
와onPlusPage()
메소드만 제공하는데, 이것이 충분한지 고려해보세요:
- 현재 페이지 정보가 필요한 경우는 없을까요?
- 페이지 이동 시 추가적인 컨텍스트가 필요할 수도 있지 않을까요?
다른 ViewHolder들과의 일관성도 함께 검토해보시기 바랍니다.
README.md (1)
9-27
: 마크다운 리스트 들여쓰기를 정리해보세요정적 분석 도구에서 지적한 대로 리스트의 들여쓰기가 일관되지 않습니다. 마크다운 표준에 따르면:
- 주 리스트 항목: 2칸 들여쓰기
- 하위 리스트 항목: 4칸 들여쓰기
현재 일부 항목들이 4칸이나 8칸으로 들여쓰기되어 있어 구조가 명확하지 않습니다.
리스트 계층 구조를 다시 정리하여 가독성을 높여보시기 바랍니다.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
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: 4; Actual: 8(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
app/src/main/res/layout/activity_products.xml (3)
37-44
: 하드코딩된 여백값을 리소스로 분리해보세요
android:layout_marginStart="26dp"
처럼 하드코딩된 여백값이 있습니다.이런 값들을
dimens.xml
에서 관리하면 어떤 장점이 있을까요? 디자인 일관성과 유지보수 관점에서 생각해보시기 바랍니다.
66-78
: 장바구니 수량 표시의 접근성을 고려해보세요장바구니 수량을 표시하는 TextView에 몇 가지 개선점을 생각해볼 수 있습니다:
- 스크린 리더 사용자를 위한
contentDescription
이 필요하지 않을까요?- 수량이 0일 때의 처리는 어떻게 하고 있나요?
- 수량이 99를 초과하는 경우의 UI는 고려되었나요?
이런 엣지 케이스들에 대한 처리 방안을 검토해보시기 바랍니다.
107-128
: 스켈레톤 UI 구현이 흥미롭네요GridLayout을 사용해서 2열 구조로 스켈레톤을 구성한 점이 좋습니다.
하지만 현재 8개의 스켈레톤 아이템이 고정되어 있는데, 실제 화면 크기나 밀도에 따라 보여줄 스켈레톤 개수를 동적으로 조정하는 방법도 고려해볼 수 있을 것 같습니다. 어떻게 생각하시나요?
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartProductViewHolder.kt (1)
34-41
: 인터페이스 설계의 일관성을 검토해보세요
ShoppingCartProductClickListener
가ProductQuantityClickListener
를 확장하면서 추가로 두 개의 메소드를 정의하고 있습니다.다음을 고려해보세요:
onRemoveButton
과onProductSelectedButton
의 매개변수 일관성은 어떤가요?- 다른 ViewHolder들의 리스너 인터페이스와 비교했을 때 설계가 일관적인가요?
- 이 인터페이스를 구현하는 클래스에서 모든 메소드가 의미있게 사용되고 있나요?
인터페이스 분리 원칙(ISP) 관점에서도 생각해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductViewHolder.kt (1)
19-26
: bind 메서드의 조건 분기 로직을 개선해보세요.현재
selectedQuantity
가 0인 경우 early return을 사용하고 있는데, 이는 가독성을 떨어뜨릴 수 있습니다. 조건문을 더 명확하게 구조화하는 방법을 고려해보세요.힌트: if-else 구조로 변경하거나, when 표현식을 사용해보는 것은 어떨까요?
app/src/main/java/woowacourse/shopping/data/authentication/repository/DefaultAuthenticationRepository.kt (1)
35-35
: 에러 메시지 현지화 전략을 검토해보세요.에러 메시지가 한국어로 하드코딩되어 있습니다. 프로젝트의 다국어 지원 계획이나 에러 메시지 관리 전략을 고려해보세요.
문자열 리소스를 사용하거나, 개발자용 메시지는 영어로 유지하는 방법도 고려해볼 수 있습니다.
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/ShoppingCartRecommendActivity.kt (2)
41-46
: 데이터 전달 방식 개선 고려현재
Serializable
을 사용한 데이터 전달 방식이 구현되어 있는데요. 이 방식에 대해 어떤 장단점이 있을까요?고려해볼 수 있는 대안들:
- Parcelable은 어떤 이점이 있을까요?
- 데이터가 많은 경우 다른 방식(예: ID만 전달하고 Repository에서 조회)은 어떨까요?
79-81
: TODO 주석 오타 수정작은 오타가 있네요!
- // TOOD: 주문하기 api 연동 + // TODO: 주문하기 api 연동주문하기 API 연동 구현이 필요하신가요? 필요하시다면 이슈를 생성해드릴까요?
app/src/main/java/woowacourse/shopping/data/product/repository/DefaultProductsRepository.kt (1)
117-134
: 의존성 관리 방식 개선싱글톤 패턴을 직접 구현하셨는데, 이 방식의 장단점에 대해 생각해보셨나요?
현재 방식의 특징:
- 전역 상태 관리
- 초기화 순서 의존성
- 테스트 시 모킹 어려움
대안으로 고려해볼 수 있는 것들:
- Hilt/Dagger 같은 DI 프레임워크
- 수동 DI (Factory 패턴)
- Application 클래스에서 관리
어떤 방식이 프로젝트의 규모와 요구사항에 더 적합할까요?
app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/ShoppingCartRepository.kt (1)
6-37
: 인터페이스 문서화 추가 권장각 메서드의 역할과 파라미터를 명확히 하기 위해 KDoc 주석을 추가하면 좋겠습니다. 특히
increaseQuantity
와decreaseQuantity
에서quantity
파라미터가 증가/감소할 양인지 최종 수량인지 명확하지 않습니다.예시:
/** * 장바구니에 제품을 추가합니다. * @param product 추가할 제품 * @param quantity 추가할 수량 * @param onResult 결과 콜백 */ fun add( product: Product, quantity: Int, onResult: (Result<Unit>) -> Unit, )app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailViewModel.kt (1)
123-131
: 중복 코드 제거가격 계산 로직이 중복되어 있습니다. 별도 메서드로 추출하면 유지보수가 쉬워집니다.
private fun updatePrice() { _price.value = quantity.value?.times(product.value?.product?.price ?: 0) } fun plusQuantity() { _quantity.value = (_quantity.value ?: 0) + 1 updatePrice() } fun minusQuantity() { _quantity.value = (_quantity.value)?.minus(1)?.coerceAtLeast(1) updatePrice() }app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailActivity.kt (1)
37-41
: early return 시 가독성 개선 고려
return finish()
패턴은 작동하지만, 가독성을 위해 분리하는 것을 고려해보세요.- binding.root.showSnackBar(getString(R.string.product_not_provided_error_message)) - return finish() + binding.root.showSnackBar(getString(R.string.product_not_provided_error_message)) + finish() + returnapp/src/main/java/woowacourse/shopping/view/product/ProductsViewModel.kt (2)
251-272
: LiveData 상태 관리 개선 필요현재 LiveData의 값을 직접 수정하는 방식은 상태 관리를 복잡하게 만들 수 있습니다. 불변성을 유지하면서 상태를 업데이트하는 방식을 고려해보세요.
더 나은 상태 관리를 위해:
- data class와 copy()를 활용한 불변 업데이트
- sealed class를 사용한 상태 표현
- StateFlow 사용 고려
18-344
: ViewModel의 책임 분리 고려현재 ViewModel이 너무 많은 비즈니스 로직을 포함하고 있습니다. 다음을 고려해보세요:
- UseCase 패턴 도입: 복잡한 비즈니스 로직을 별도의 UseCase 클래스로 분리
- 상태 관리 개선: sealed class와 StateFlow를 활용한 더 명확한 상태 관리
- 의존성 주입: Repository를 직접 가져오는 대신 생성자 주입 활용
이렇게 하면 테스트가 용이해지고 코드의 재사용성이 높아집니다.
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModel.kt (2)
74-80
: 불필요한 Boolean 비교를 제거하세요
isChecked
가 이미 Boolean 타입이므로 명시적인== true
비교는 불필요합니다.- .filter { it.isChecked == true } + .filter { it.isChecked }
232-232
: 불필요한 Boolean 비교를 제거하세요- if (loadable == false) return + if (!loadable) return
📜 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
(4 hunks)app/src/androidTest/java/woowacourse/shopping/fixture/ProductFixtures.kt
(1 hunks)app/src/androidTest/java/woowacourse/shopping/fixture/TestFixture.kt
(1 hunks)app/src/androidTest/java/woowacourse/shopping/productDetail/ProductDetailActivityTest.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/ShoppingApplication.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/authentication/repository/AuthenticationRepository.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/authentication/repository/DefaultAuthenticationRepository.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/common/converter.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/network/ApiClient.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/network/AuthenticationInterceptor.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/dataSource/ProductDataSource.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/dataSource/ProductRemoteDataSource.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/dataSource/VolatileProductDataSource.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/local/dao/RecentWatchingDao.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/local/database/ProductDatabase.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/local/entity/ProductEntity.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/local/entity/RecentWatchingEntity.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/remote/MockProductServer.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/remote/dto/PageableResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/remote/dto/ProductResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/remote/dto/ProductsResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/remote/dto/SortResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/remote/service/ProductService.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/repository/DefaultProductsRepository.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/product/repository/ProductsRepository.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartCountsResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartItemQuantityRequestDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartItemRequestDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/PageableResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ProductResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemsResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/SortResponseDto.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/service/ShoppingCartService.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/DefaultShoppingCartRepository.kt
(1 hunks)app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/ShoppingCartRepository.kt
(1 hunks)app/src/main/java/woowacourse/shopping/domain/authentication/UserAuthentication.kt
(1 hunks)app/src/main/java/woowacourse/shopping/domain/product/Product.kt
(1 hunks)app/src/main/java/woowacourse/shopping/domain/shoppingCart/ShoppingCartProduct.kt
(1 hunks)app/src/main/java/woowacourse/shopping/domain/shoppingCart/ShoppingCarts.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/BindingAdapter.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/Event.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/GridItemDecoration.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/IntentExtensions.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/Messages.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/MutableSingleLiveData.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/ProductQuantityClickListener.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/QuantityObservable.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/ResultFrom.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/common/SingleLiveData.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/ProductAdapter.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/ProductMoreViewHolder.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/ProductRecentWatchingViewHolder.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/ProductViewHolder.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/ProductsActivity.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/ProductsEvent.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/ProductsItem.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/ProductsViewModel.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/RecentProductAdapter.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/product/RecentProductViewHolder.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailActivity.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailClickListener.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailEvent.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailViewModel.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartActivity.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartClickListener.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartEvent.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartItem.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartPaginationViewHolder.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartProductAdapter.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartProductViewHolder.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModel.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductAdapter.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductListener.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductViewHolder.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/ShoppingCartRecommendActivity.kt
(1 hunks)app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/ShoppingCartRecommendViewModel.kt
(1 hunks)app/src/main/res/drawable/background_order_button.xml
(1 hunks)app/src/main/res/drawable/background_order_button_disabled.xml
(1 hunks)app/src/main/res/drawable/background_order_button_enabled.xml
(1 hunks)app/src/main/res/drawable/background_product_detail_recent_watching.xml
(1 hunks)app/src/main/res/drawable/background_product_minus_quantity_button.xml
(1 hunks)app/src/main/res/drawable/background_product_plus_quantity_button.xml
(1 hunks)app/src/main/res/drawable/background_product_quantity.xml
(1 hunks)app/src/main/res/drawable/background_shopping_cart_pagination_minus_button.xml
(1 hunks)app/src/main/res/drawable/background_shopping_cart_pagination_minus_button_enabled.xml
(1 hunks)app/src/main/res/drawable/background_shopping_cart_pagination_minus_button_un_enabled.xml
(1 hunks)app/src/main/res/drawable/background_shopping_cart_pagination_plus_button.xml
(1 hunks)app/src/main/res/drawable/background_shopping_cart_pagination_plus_button_enabled.xml
(1 hunks)app/src/main/res/drawable/background_shopping_cart_pagination_plus_button_un_enabled.xml
(1 hunks)app/src/main/res/drawable/background_shopping_cart_product.xml
(1 hunks)app/src/main/res/drawable/background_shopping_cart_quantity.xml
(1 hunks)app/src/main/res/drawable/btn_product_plus.xml
(1 hunks)app/src/main/res/drawable/ic_back.xml
(1 hunks)app/src/main/res/drawable/ic_close.xml
(1 hunks)app/src/main/res/drawable/ic_shopping_cart.xml
(1 hunks)app/src/main/res/layout/activity_main.xml
(0 hunks)app/src/main/res/layout/activity_product_detail.xml
(1 hunks)app/src/main/res/layout/activity_products.xml
(1 hunks)app/src/main/res/layout/activity_shopping_cart.xml
(1 hunks)app/src/main/res/layout/activity_shopping_cart_recommend.xml
(1 hunks)app/src/main/res/layout/activity_shopping_cart_skeleton.xml
(1 hunks)app/src/main/res/layout/component_product_quantity.xml
(1 hunks)app/src/main/res/layout/item_product.xml
(1 hunks)
⛔ Files not processed due to max files limit (16)
- app/src/main/res/layout/item_product_more.xml
- app/src/main/res/layout/item_product_recent_watching.xml
- app/src/main/res/layout/item_product_skeleton.xml
- app/src/main/res/layout/item_recent_watching.xml
- app/src/main/res/layout/item_recommend_product.xml
- app/src/main/res/layout/item_shopping_cart_pagination.xml
- app/src/main/res/layout/item_shopping_cart_product.xml
- app/src/main/res/values/colors.xml
- app/src/main/res/values/strings.xml
- app/src/main/res/values/themes.xml
- app/src/test/java/woowacourse/shopping/data/MockProductServerTest.kt
- app/src/test/java/woowacourse/shopping/view/common/InstantTaskExecutorExtension.kt
- app/src/test/java/woowacourse/shopping/view/common/TestExtensions.kt
- app/src/test/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModelTest.kt
- build.gradle.kts
- 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/domain/authentication/UserAuthentication.kt
app/src/main/java/woowacourse/shopping/view/common/Messages.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartItemRequestDto.kt
app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailEvent.kt
app/src/main/java/woowacourse/shopping/data/authentication/repository/AuthenticationRepository.kt
app/src/main/java/woowacourse/shopping/data/common/converter.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartCountsResponseDto.kt
app/src/main/java/woowacourse/shopping/data/network/ApiClient.kt
app/src/main/java/woowacourse/shopping/view/common/QuantityObservable.kt
app/src/main/java/woowacourse/shopping/data/product/dataSource/ProductRemoteDataSource.kt
app/src/main/java/woowacourse/shopping/data/product/remote/MockProductServer.kt
app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailClickListener.kt
app/src/main/java/woowacourse/shopping/domain/product/Product.kt
app/src/main/java/woowacourse/shopping/view/product/ProductRecentWatchingViewHolder.kt
app/src/main/java/woowacourse/shopping/view/product/ProductMoreViewHolder.kt
app/src/main/java/woowacourse/shopping/data/product/local/dao/RecentWatchingDao.kt
app/src/main/java/woowacourse/shopping/data/product/remote/dto/ProductResponseDto.kt
app/src/androidTest/java/woowacourse/shopping/fixture/TestFixture.kt
app/src/main/java/woowacourse/shopping/view/common/ProductQuantityClickListener.kt
app/src/main/java/woowacourse/shopping/view/product/ProductViewHolder.kt
app/src/main/java/woowacourse/shopping/view/product/ProductsEvent.kt
app/src/main/java/woowacourse/shopping/view/common/GridItemDecoration.kt
app/src/androidTest/java/woowacourse/shopping/fixture/ProductFixtures.kt
app/src/main/java/woowacourse/shopping/domain/shoppingCart/ShoppingCarts.kt
app/src/main/java/woowacourse/shopping/data/product/remote/dto/ProductsResponseDto.kt
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartPaginationViewHolder.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemResponseDto.kt
app/src/main/java/woowacourse/shopping/data/product/local/entity/ProductEntity.kt
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductListener.kt
app/src/main/java/woowacourse/shopping/view/common/ResultFrom.kt
app/src/main/java/woowacourse/shopping/view/common/IntentExtensions.kt
app/src/androidTest/java/woowacourse/shopping/productDetail/ProductDetailActivityTest.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemsResponseDto.kt
app/src/main/java/woowacourse/shopping/view/common/Event.kt
app/src/main/java/woowacourse/shopping/data/product/local/database/ProductDatabase.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/SortResponseDto.kt
app/src/main/java/woowacourse/shopping/data/product/local/entity/RecentWatchingEntity.kt
app/src/main/java/woowacourse/shopping/ShoppingApplication.kt
app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailViewModel.kt
app/src/main/java/woowacourse/shopping/data/product/remote/dto/SortResponseDto.kt
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/ShoppingCartRecommendViewModel.kt
app/src/main/java/woowacourse/shopping/data/product/dataSource/ProductDataSource.kt
app/src/main/java/woowacourse/shopping/view/common/BindingAdapter.kt
app/src/main/java/woowacourse/shopping/data/product/remote/dto/PageableResponseDto.kt
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartEvent.kt
app/src/main/java/woowacourse/shopping/view/product/RecentProductViewHolder.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartItemQuantityRequestDto.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/ShoppingCartRepository.kt
app/src/main/java/woowacourse/shopping/data/product/remote/service/ProductService.kt
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartActivity.kt
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartClickListener.kt
app/src/main/java/woowacourse/shopping/view/common/SingleLiveData.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/PageableResponseDto.kt
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartProductViewHolder.kt
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductViewHolder.kt
app/src/main/java/woowacourse/shopping/view/common/MutableSingleLiveData.kt
app/src/main/java/woowacourse/shopping/view/product/ProductAdapter.kt
app/src/main/java/woowacourse/shopping/data/authentication/repository/DefaultAuthenticationRepository.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ProductResponseDto.kt
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/ShoppingCartRecommendActivity.kt
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartProductAdapter.kt
app/src/main/java/woowacourse/shopping/view/product/RecentProductAdapter.kt
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductAdapter.kt
app/src/main/java/woowacourse/shopping/data/product/repository/ProductsRepository.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/service/ShoppingCartService.kt
app/src/main/java/woowacourse/shopping/data/product/repository/DefaultProductsRepository.kt
app/src/main/java/woowacourse/shopping/data/product/dataSource/VolatileProductDataSource.kt
app/src/main/java/woowacourse/shopping/data/network/AuthenticationInterceptor.kt
app/src/main/java/woowacourse/shopping/view/product/ProductsItem.kt
app/src/main/java/woowacourse/shopping/domain/shoppingCart/ShoppingCartProduct.kt
app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailActivity.kt
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartItem.kt
app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/DefaultShoppingCartRepository.kt
app/src/main/java/woowacourse/shopping/view/product/ProductsActivity.kt
app/src/main/java/woowacourse/shopping/view/product/ProductsViewModel.kt
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModel.kt
🧬 Code Graph Analysis (5)
app/src/main/java/woowacourse/shopping/data/product/dataSource/ProductRemoteDataSource.kt (1)
app/src/main/java/woowacourse/shopping/data/common/converter.kt (1)
convertJsonToList
(6-13)
app/src/main/java/woowacourse/shopping/view/productDetail/ProductDetailViewModel.kt (3)
app/src/main/java/woowacourse/shopping/data/product/repository/DefaultProductsRepository.kt (1)
updateRecentWatchingProduct
(74-92)app/src/main/java/woowacourse/shopping/data/product/repository/ProductsRepository.kt (1)
updateRecentWatchingProduct
(22-25)app/src/main/java/woowacourse/shopping/view/product/ProductsViewModel.kt (1)
updateRecentWatching
(213-241)
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartActivity.kt (1)
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartPaginationViewHolder.kt (1)
onPlusPage
(41-41)
app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/DefaultShoppingCartRepository.kt (1)
app/src/main/java/woowacourse/shopping/view/product/ProductsViewModel.kt (1)
updateShoppingCartQuantity
(205-211)
app/src/main/java/woowacourse/shopping/view/product/ProductsViewModel.kt (2)
app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/DefaultShoppingCartRepository.kt (2)
updateShoppingCartQuantity
(93-120)add
(47-75)app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/ShoppingCartRepository.kt (1)
add
(13-17)
🪛 markdownlint-cli2 (0.17.2)
README.md
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: 4; Actual: 8
(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: 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)
47-47: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (51)
app/src/main/res/drawable/background_order_button.xml (1)
2-7
: 상태별 Drawable 분기 적절 구현
enabled
/disabled
상태에 따라 올바르게 배경 리소스를 분기하고 있어 의도한 UI 동작과 부합합니다. 특별한 문제 없이 잘 작성되었습니다.app/src/main/res/drawable/background_shopping_cart_quantity.xml (1)
1-6
: 쉐이프 드로어블이 적절하게 정의되었습니다.
타원(oval) 형태와 mint 색상(@color/mint
)이 의도대로 설정되어 있습니다. 리소스 네이밍과 구조가 명확하여 가독성이 좋습니다.app/src/main/res/drawable/background_shopping_cart_pagination_minus_button_enabled.xml (1)
1-9
: 왼쪽 페이지네이션 버튼 enabled 상태 드로어블 검토 완료
모서리(bottomLeftRadius
,topLeftRadius
) 및 색상(@color/mint
) 설정이 일관되고 의도에 부합합니다. 파일 구조와 네이밍도 명확합니다.app/src/main/res/drawable/background_shopping_cart_pagination_plus_button_enabled.xml (1)
1-9
: 오른쪽 페이지네이션 버튼 enabled 상태 드로어블 검토 완료
bottomRightRadius
와topRightRadius
값이 잘 적용되었고, mint 색상이 적절합니다. 구조가 명확하여 유지보수하기 좋습니다.app/src/main/res/drawable/background_shopping_cart_pagination_plus_button.xml (1)
1-6
: 페이지네이션 plus 버튼 상태 선택자(selector) 정의 적합
state_enabled
에 따른 올바른 드로어블 참조가 이루어졌습니다. 선택자 순서 및 속성 설정이 명확합니다.app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartClickListener.kt (1)
3-9
: 인터페이스 설계가 깔끔합니다!각 메서드가 명확한 역할을 가지고 있고 네이밍도 일관성 있게 잘 작성되었습니다.
한 가지 고려해볼 점:
onAllSelectedButtonClick(isChecked: Boolean)
메서드만 매개변수를 받는데, 다른 메서드들과의 일관성을 위해 이런 설계를 선택한 이유가 있는지 생각해보세요.app/src/main/java/woowacourse/shopping/view/common/IntentExtensions.kt (1)
7-12
: 안드로이드 API 버전 차이를 잘 처리한 확장함수입니다!reified 제네릭과 안전한 캐스팅을 적절히 활용했네요. TIRAMISU 이후 변경된 API도 올바르게 대응했습니다.
생각해볼 점: 이런 유틸리티 함수를 확장함수로 구현하는 것과 별도의 유틸리티 클래스로 만드는 것 중 어느 쪽이 더 나은 접근인지 고민해보세요. 각각의 장단점은 무엇일까요?
app/src/main/java/woowacourse/shopping/data/product/remote/dto/ProductResponseDto.kt (1)
7-19
: Kotlin Serialization 어노테이션이 잘 적용되어 있습니다!모든 필드에
@SerialName
이 명시되어 있어 JSON 매핑이 명확하네요.app/src/main/res/drawable/background_product_detail_recent_watching.xml (1)
1-8
: 변경된 테두리 스타일이 명확하고 일관성 있습니다.
둥근 모서리(4dp)와 1dp 테두리로 “최근 본 상품” 영역을 깔끔하게 감싸고 있어 UI 가독성이 좋습니다.app/src/main/res/drawable/background_product_quantity.xml (1)
1-5
: 심플한 흰색 배경 설정이 적절합니다.
수량 조절 컴포넌트에 일관된 흰색 배경을 제공하며 추가 속성 없이도 의도한 디자인을 잘 표현하고 있습니다.app/src/main/res/drawable/background_shopping_cart_product.xml (1)
1-8
: 장바구니 상품 아이템 테두리 스타일이 일관적입니다.
4dp 둥근 모서리와 1dp 회색 테두리로 상품 항목이 잘 구분되며 다른 컴포넌트들(최근 본 상품 등)과 디자인 통일성도 유지됩니다.app/src/main/res/drawable/background_product_minus_quantity_button.xml (1)
1-7
: 왼쪽 엣지 둥근 모서리 설정이 의도에 부합합니다.
감소 버튼에만 좌측 모서리(4dp)만 둥글게 처리해 수량 조절 버튼 전체가 한 덩어리로 보이도록 잘 설계되었습니다.app/src/main/res/drawable/background_product_plus_quantity_button.xml (1)
1-7
: 잘 구현된 드로어블 리소스입니다.수량 증가 버튼을 위한 배경 드로어블이 적절하게 구현되었습니다. 우측 모서리만 둥글게 처리하여 수량 컨트롤 UI의 디자인 일관성을 유지하고 있습니다.
app/src/main/res/drawable/ic_close.xml (1)
1-9
: 깔끔하게 구현된 닫기 아이콘입니다.벡터 드로어블로 구현된 닫기 아이콘이 올바르게 작성되었습니다. 16dp 크기와 흰색 색상으로 일관된 디자인을 유지하고 있어 좋습니다.
app/src/main/java/woowacourse/shopping/domain/product/Product.kt (1)
8-8
: 가격 데이터 타입에 대해 생각해보세요.
price
를Int
로 정의하셨는데, 실제 쇼핑 앱에서 가격을 다룰 때 고려해야 할 점들이 있습니다.생각해볼 점들:
- 소수점이 있는 가격은 어떻게 처리할 건가요?
- 통화 단위는 어떻게 표현할 건가요?
- 가격의 범위나 유효성 검증은 필요하지 않을까요?
이런 요구사항들을 고려해서 현재 설계가 적절한지 검토해보세요.
app/src/main/java/woowacourse/shopping/data/product/local/entity/ProductEntity.kt (1)
6-12
: 도메인 모델과의 일관성을 확인해보세요.
ProductEntity
와Product
도메인 모델을 비교해보세요. 뭔가 빠진 것이 있지 않나요?스스로 확인해보세요:
- 두 클래스의 필드들을 나란히 놓고 비교해보세요
- 엔티티와 도메인 모델 간 변환 시 어떤 문제가 생길까요?
- 이런 불일치가 있을 때 어떻게 처리할 계획인가요?
추가 고민: Room 엔티티에서 데이터베이스 제약조건(nullable, unique 등)이 필요한 필드들은 없을까요?
app/src/main/res/drawable/background_shopping_cart_pagination_minus_button.xml (1)
1-6
: 표준 Android 셀렉터 패턴을 올바르게 구현했습니다.enabled/disabled 상태에 따른 drawable 리소스 관리가 적절하게 구현되어 있습니다. 이런 방식으로 UI 상태를 관리하는 것은 Android의 모범 사례입니다.
app/src/main/java/woowacourse/shopping/view/common/ProductQuantityClickListener.kt (1)
3-7
: 인터페이스 설계가 잘 되어 있습니다!이 인터페이스는 단일 책임 원칙을 잘 따르고 있고,
QuantityObservable
이라는 추상화를 사용해서 구체적인 타입에 의존하지 않게 설계되었네요.이런 추상화가 코드의 유연성과 테스트 가능성에 어떤 장점을 주는지 생각해보셨나요? 특히 다양한 화면(상품 목록, 상세, 장바구니 등)에서 이 인터페이스를 재사용할 때 어떤 이점이 있을지 고민해보시면 좋을 것 같습니다.
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartEvent.kt (1)
3-8
: 이벤트 기반 에러 처리 설계가 좋습니다!장바구니 관련 실패 이벤트들을 하나의 enum으로 명확하게 정의했네요. 각 이벤트 이름도 어떤 동작에서 실패했는지 쉽게 알 수 있도록 잘 명명되었습니다.
이렇게 실패 이벤트를 별도로 관리하는 것이 ViewModel과 View 간의 커뮤니케이션에서 어떤 장점을 제공하는지 생각해보셨나요? 특히 사용자에게 적절한 피드백을 제공할 때 이런 구조가 어떻게 도움이 될지 고민해보시면 좋겠습니다.
app/src/main/java/woowacourse/shopping/data/product/dataSource/ProductRemoteDataSource.kt (1)
17-36
: 네트워크 호출의 안전성과 예외 처리HTTP 통신을 수행할 때 발생할 수 있는 다양한 상황들을 고려해보세요:
- 네트워크 연결이 실패하는 경우는 어떻게 처리할까요?
- 서버에서 예상과 다른 응답을 반환하는 경우는?
- JSON 파싱이 실패하는 경우는 어떻게 될까요?
- 이 메서드가 메인 스레드에서 호출된다면 어떤 문제가 있을까요?
네트워크 프로그래밍의 베스트 프랙티스와 Android의 백그라운드 작업 가이드라인을 참고해보시기 바랍니다.
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/PageableResponseDto.kt (1)
18-19
: 프로퍼티 이름과 SerialName 간의 불일치를 확인해보세요.
unPaged
프로퍼티의@SerialName("unpaged")
에서 대소문자가 다릅니다. 이것이 의도된 것인지 확인해보세요.어떤 상황에서 이런 차이가 필요할까요? API 응답의 실제 필드명을 확인하고, 일관성을 위해 다른 프로퍼티들과 동일한 네이밍 패턴을 따르는 것이 좋을 것 같습니다.
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ProductResponseDto.kt (1)
8-28
: DTO 구조가 깔끔하네요! 하지만 데이터 검증에 대해 생각해보세요.
toDomain()
메서드의 구현이 깔끔하고, SerialName 어노테이션도 일관성 있게 사용되었습니다.한 가지 고려해볼 점은 모든 프로퍼티가 non-null로 선언되어 있는데, API 응답에서 일부 데이터가 누락될 가능성은 없을까요? 특히
imageUrl
이나category
같은 필드는 어떨까요?실제 API 명세를 확인하고, 필요하다면 nullable 타입 사용을 고려해보세요. 그리고
toDomain()
메서드에서 데이터 유효성 검증이 필요한지도 생각해보세요.app/src/main/java/woowacourse/shopping/data/network/AuthenticationInterceptor.kt (1)
20-28
: intercept 메서드 구현이 깔끔합니다.
with
스코프 함수를 사용해서 코드 가독성을 높였고, OkHttp 인터셉터 패턴을 올바르게 따르고 있습니다.app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemResponseDto.kt (1)
8-22
: DTO 구성이 잘 되어 있습니다. 하지만 데이터 유효성을 고려해보세요.중첩된
product.toDomain()
호출로 구성(composition)을 잘 처리했고, 전체적인 구조가 일관성 있습니다.다음 사항들을 고려해볼까요?
quantity
가 음수이거나 0인 경우는 어떻게 처리해야 할까요?- 장바구니 아이템에서 수량의 최대값 제한이 필요할까요?
도메인 모델로 변환하기 전에 이런 비즈니스 규칙들을 어디서 검증하는 것이 좋을까요? DTO 레벨에서 할지, 도메인 레벨에서 할지 생각해보세요.
app/src/main/java/woowacourse/shopping/ShoppingApplication.kt (1)
22-22
: 데이터베이스 마이그레이션 전략 고려
fallbackToDestructiveMigration()
을 사용하고 있습니다. 이것이 프로덕션 환경에서 어떤 영향을 미칠지 생각해보세요.사용자 데이터 손실을 방지하기 위해 어떤 대안적인 마이그레이션 전략을 고려할 수 있을까요? Room의 마이그레이션 옵션들을 조사해보세요.
app/src/main/java/woowacourse/shopping/data/product/remote/service/ProductService.kt (1)
17-21
: API 엔드포인트 경로의 일관성을 확인해보세요.
/products
와/products/{id}
패턴이 RESTful API 설계 원칙에 부합하는지 확인해보세요. 또한 프로젝트 전체의 API 엔드포인트 네이밍 규칙과 일관성이 있는지 검토해보세요.REST API 설계에서 단수형과 복수형을 언제 사용해야 하는지 알고 계신가요?
app/src/main/java/woowacourse/shopping/data/product/dataSource/VolatileProductDataSource.kt (1)
170-177
:✅ Verification successful
페이지네이션 로직의 경계 조건을 검토해보세요.
slice
메서드를 사용할 때와subList
메서드를 사용할 때의 차이점을 이해하고 계신가요? 또한startPosition + size > products.size
조건에서 정확히 경계값일 때의 동작을 확인해보세요.각 분기에서 반환되는 결과가 일관된 방식으로 처리되는지 생각해보세요.
다음 스크립트로 경계 조건을 테스트해보세요:
🏁 Script executed:
#!/bin/bash # Description: 페이지네이션 로직의 경계 조건 테스트를 위한 코드 검토 # ProductDataSource 인터페이스와 사용처 확인 ast-grep --pattern 'interface ProductDataSource { $$$ }' # load 메서드 사용처 확인 rg -A 3 "\.load\(" --type ktLength of output: 757
페이지네이션 경계 조건이 올바르게 처리되고 있습니다
startPosition + size > products.size
인 경우slice(startPosition..products.size - 1)
을, 그렇지 않은 경우subList(startPosition, startPosition + size)
을 사용하여 동일한 결과(마지막 인덱스까지의 상품 목록)를 반환하므로 경계값(==
,>
) 모두 문제가 없습니다. 추가 수정 불필요합니다.app/src/main/java/woowacourse/shopping/view/product/ProductAdapter.kt (1)
11-30
: DiffUtil 구현이 잘 되어 있습니다!
areItemsTheSame
에서 타입별로 적절한 비교 로직을 구현했고,areContentsTheSame
에서는 data class의equals
메서드를 활용하는 것이 좋은 접근입니다.app/src/main/java/woowacourse/shopping/data/network/ApiClient.kt (1)
12-21
: Retrofit 클라이언트 설정이 잘 구성되어 있습니다.의존성 주입을 통한
AuthenticationRepository
전달과 Kotlin Serialization 사용이 좋은 선택입니다. BuildConfig를 통한 BASE_URL 관리도 적절합니다.app/src/main/AndroidManifest.xml (3)
17-17
: 개발용 설정이 프로덕션에 노출되지 않도록 주의하세요.
android:usesCleartextTraffic="true"
설정은 HTTP 통신을 허용하는 개발용 설정입니다.프로덕션 배포 시에는 어떤 보안 고려사항이 있을까요? 이 설정을 빌드 타입에 따라 다르게 적용할 수 있는 방법을 생각해보세요.
20-27
: 새로 추가된 액티비티들의 exported 설정이 적절합니다.내부적으로만 사용되는 액티비티들을
android:exported="false"
로 설정한 것은 보안상 좋은 선택입니다. 외부에서 직접 접근할 수 없도록 보호됩니다.
5-5
: INTERNET 권한 추가가 적절합니다.API 통신을 위해 필요한 권한이 올바르게 추가되었습니다.
app/src/main/java/woowacourse/shopping/view/common/SingleLiveData.kt (1)
25-30
: 이벤트 처리 패턴이 잘 구현되어 있습니다Event 클래스와 함께 일회성 이벤트 처리를 위한 패턴이 적절히 구현되어 있네요.
getContentIfNotHandled()
를 사용하여 중복 이벤트 처리를 방지하는 것이 좋습니다.app/src/main/res/layout/activity_product_detail.xml (3)
59-66
: 이미지 로딩 실패 시의 처리를 고려해보세요현재 이미지 로딩을 위해 커스텀 바인딩 어댑터를 사용하고 있는 것 같습니다. 네트워크 오류나 이미지 로딩 실패 시 사용자에게 어떤 피드백을 제공할 수 있을까요? placeholder나 error 이미지 처리 방법을 고민해보세요.
122-160
: 조건부 표시 로직이 잘 구현되어 있습니다
android:visibility
속성을 사용하여 최근 본 상품 섹션을 조건부로 표시하는 로직이 적절히 구현되어 있네요. 데이터 바인딩을 통한 UI 상태 관리가 잘 되어 있습니다.
130-130
: 조건부 가시성 로직을 검토해보세요
viewModel.recentProductBoxVisible ? View.VISIBLE : View.GONE
표현식이 사용되고 있는데, 이런 조건부 가시성 처리에서 고려해야 할 점들이 있습니다.생각해볼 점:
recentWatchingProduct
가 null일 때는 어떻게 처리되나요?- 데이터 바인딩에서 null-safe한 처리가 되고 있는지 확인해보세요
더 안전한 조건 처리 방법에 대해 생각해보시겠어요?
app/src/main/java/woowacourse/shopping/view/product/RecentProductAdapter.kt (1)
16-16
: 데이터 접근 패턴을 검토해보세요
recentWatchingItems.products.size
로 직접 접근하고 있습니다.다음과 같은 시나리오를 고려해보세요:
- products 리스트가 null이거나 비어있다면?
- 데이터 일관성을 보장하는 더 안전한 방법이 있을까요?
app/src/main/java/woowacourse/shopping/view/product/ProductRecentWatchingViewHolder.kt (1)
16-18
: RecyclerView 설정 패턴을 검토해보세요RecyclerView에 어댑터만 설정하고 있습니다.
다음 설정들이 필요한지 고려해보세요:
- LayoutManager는 어디서 설정되나요?
- 수평 스크롤을 위한 orientation은 설정되어 있나요?
- ItemDecoration이나 다른 설정들이 필요할까요?
XML 레이아웃 파일과 함께 확인해보는 것을 권장합니다.
app/src/main/res/layout/item_product.xml (1)
75-85
: 텍스트 뷰의 제약 조건을 확인해보세요가격 표시 TextView의 제약 조건을 살펴보세요:
android:layout_width="0dp"
와 start/end 제약이 적절히 설정되어 있나요?- 상품명과 가격 사이의 간격이 일정하게 유지될까요?
- 다양한 텍스트 길이에서 레이아웃이 깨지지 않을까요?
다양한 데이터 케이스에서 레이아웃을 테스트해보는 것을 권장합니다.
app/build.gradle.kts (1)
29-33
: 보안과 설정 관리에 대해 생각해보세요
local.properties
에서 BASE_URL을 읽어와 BuildConfig에 추가하고 있습니다.고려해볼 점들:
local.properties
파일이 없을 때는 어떻게 될까요?- 이 설정이 버전 관리에서 제외되어야 하는 이유는 무엇일까요?
- 다른 환경(개발, 스테이징, 프로덕션)에서는 어떻게 관리할 수 있을까요?
- 에러 처리는 어떻게 해야 할까요?
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartPaginationViewHolder.kt (1)
24-36
: 팩토리 메소드 패턴이 잘 적용되었네요companion object를 통한 ViewHolder 생성 패턴이 깔끔하게 구현되어 있습니다. 이는 RecyclerView.Adapter에서 ViewHolder를 생성할 때 일관성을 유지하는 좋은 방법입니다.
README.md (1)
5-71
: 기능 명세가 매우 상세하고 체계적입니다각 기능별로 요구사항이 구체적으로 정의되어 있고, 완료 상태도 체크박스로 명확하게 표시되어 있습니다. 특히 다음 부분들이 잘 정리되어 있습니다:
- 상품 목록의 UI/UX 세부사항
- 서버 연동 및 데이터 지속성 요구사항
- 페이지네이션 규칙
- 추천 알고리즘 로직
이런 상세한 명세는 개발 과정에서 매우 유용할 것 같습니다.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
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: 4; Actual: 8(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
37-37: 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)
47-47: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
app/src/main/res/layout/activity_products.xml (1)
84-95
: RecyclerView 설정이 잘 되어있습니다
android:clipToPadding="false"
와 적절한 패딩 설정으로 스크롤 영역이 잘 구성되어 있고, 데이터 바인딩을 통한 어댑터 연결도 깔끔합니다.app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartProductViewHolder.kt (2)
18-21
: 바인딩 로직에서 중복 설정을 확인해보세요
bind
메소드에서binding.shoppingCartProductItem = item
으로 전체 아이템을 설정한 후, 다시binding.shoppingCartQuantityComponent.quantity = item.shoppingCartProduct.quantity
로 수량을 별도로 설정하고 있습니다.이렇게 중복으로 설정하는 이유가 있나요? 데이터 바인딩에서 quantity가 자동으로 설정되지 않는 특별한 이유가 있는지 확인해보시기 바랍니다.
23-32
: 팩토리 메소드가 일관성 있게 구현되었습니다다른 ViewHolder와 동일한 패턴으로
of
메소드를 구현하여 일관성을 유지한 점이 좋습니다. 이런 패턴은 코드 가독성과 유지보수성에 도움이 됩니다.app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/RecommendProductViewHolder.kt (1)
39-48
: Factory 메서드 패턴이 잘 적용되었습니다.companion object를 통한 ViewHolder 생성 패턴이 깔끔하게 구현되어 있네요. 이 패턴의 장점을 이해하고 계신지 확인해보세요.
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartItem.kt (1)
6-15
: sealed interface를 활용한 타입 설계가 우수합니다.
ShoppingCartItem
을 sealed interface로 설계하여 타입 안전성을 확보한 점이 좋습니다.QuantityObservable
인터페이스를 함께 구현한 것도 적절한 설계 결정입니다.app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemsResponseDto.kt (1)
8-32
: DTO 구조와 직렬화 설정이 적절합니다.페이지네이션 응답을 위한 DTO 구조가 완성도 있게 설계되었고,
@SerialName
어노테이션을 통한 필드 매핑도 정확합니다.app/src/main/java/woowacourse/shopping/data/authentication/repository/DefaultAuthenticationRepository.kt (1)
10-21
: 프로퍼티 구현과 업데이트 로직이 깔끔합니다.SharedPreferences를 활용한 getter/setter 구현이 적절하고,
updateUserAuthentication
메서드를 통한 일괄 업데이트도 잘 설계되었습니다.app/src/main/java/woowacourse/shopping/view/product/ProductsActivity.kt (1)
140-164
: GridLayoutManager 설정이 잘 구현되었습니다SpanSizeLookup을 통해 다양한 아이템 타입에 대한 span size를 적절히 처리하고 있습니다.
app/src/main/res/layout/activity_shopping_cart.xml (1)
88-97
: 체크박스 상태 관리 로직을 검토해보세요체크박스의
onClick
이벤트에서!shoppingCartProductAllSelectCheckBox.checked
를 사용하고 있는데, 이 방식에 잠재적인 문제가 있을 수 있습니다.생각해볼 점:
- 사용자가 체크박스를 클릭했을 때 실제 체크 상태 변경 타이밍은 언제일까요?
- 현재 로직이 의도한 대로 동작할까요?
더 안전한 상태 관리 방법에 대해 조사해보시겠어요?
@Serializable | ||
data class CartItemQuantityRequestDto( | ||
@SerializedName("quantity") | ||
val quantity: Int, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Kotlin Serialization 문법을 확인해보세요.
Kotlin Serialization을 사용하기로 했다면, 해당 라이브러리의 올바른 애노테이션 사용법을 학습해보세요.
찾아보실 것들:
- Kotlin Serialization에서 JSON 필드명 매핑은 어떻게 하나요?
@SerializedName
은 어느 라이브러리의 애노테이션인가요?- Kotlin Serialization의 올바른 애노테이션은 무엇인가요?
공식 문서를 참조해서 올바른 방식으로 수정해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartItemQuantityRequestDto.kt
lines 6 to 10, the code incorrectly uses @SerializedName which belongs to Gson,
not Kotlin Serialization. To fix this, replace @SerializedName("quantity") with
@SerialName("quantity") from kotlinx.serialization, and ensure the import for
kotlinx.serialization.SerialName is added. This aligns the annotation with
Kotlin Serialization's correct JSON field mapping syntax.
import com.google.gson.annotations.SerializedName | ||
import kotlinx.serialization.Serializable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
직렬화 라이브러리 혼용 문제를 발견해보세요.
import 문을 자세히 보세요. 뭔가 이상한 점이 있지 않나요?
스스로 확인해보세요:
- 어떤 직렬화 라이브러리들이 import되어 있나요?
- 이 두 라이브러리를 동시에 사용하면 어떤 문제가 생길까요?
- PR 설명에서 언급한 직렬화 방식 선택과 일치하나요?
이런 혼용이 런타임에 어떤 영향을 미칠지 생각해보시고, 일관된 방식으로 수정하는 방법을 고민해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartItemQuantityRequestDto.kt
at lines 3-4, there is a mix of two serialization libraries imported: Gson's
SerializedName and kotlinx.serialization's Serializable. This inconsistency can
cause runtime serialization issues. To fix it, choose one serialization library
consistent with the project's standard (likely kotlinx.serialization), remove
the other import, and update annotations accordingly to use only the selected
library's serialization annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드래빗이 질문폭격기가 되었네요 😱
import문을 한번 확인해보시죵 (다른 파일들도 있는 것 같아요)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어라 그러게욧 SerializedName 을 사용하고 있었네요 😓 해당 내용 반영했습니다.
@Entity(tableName = "recent_watching") | ||
data class RecentWatchingEntity( | ||
@PrimaryKey val productId: Long, | ||
@Embedded val product: Product, | ||
val watchedAt: Long = System.currentTimeMillis(), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Room 엔티티 설계에서 몇 가지 개선점을 고려해보세요.
현재 구조에서 다음 사항들을 검토해보시기 바랍니다:
-
시간 관리 방식:
System.currentTimeMillis()
를 기본값으로 사용하면 테스트가 어려워집니다. 시간 주입을 고려해보세요. -
@Embedded Product: Product 도메인 객체를 직접 임베드하면 테이블 컬럼이 많아질 수 있습니다. Product의 모든 필드가 정말 필요한지 검토해보세요.
-
Primary Key 설계: 같은 상품을 여러 번 본 경우는 어떻게 처리할 계획인가요? 최신 시간으로 업데이트하려면 어떤 전략을 사용할 건가요?
이런 질문들을 통해 엔티티 설계를 다시 한번 검토해보시면 더 견고한 구조를 만들 수 있을 것 같습니다.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/product/local/entity/RecentWatchingEntity.kt
around lines 8 to 13, improve the entity design by removing the default
System.currentTimeMillis() to allow time injection for easier testing,
reconsider embedding the entire Product object to avoid excessive columns by
including only necessary fields or referencing productId, and revise the primary
key strategy to handle multiple views of the same product, such as using a
composite key or updating the watchedAt timestamp on repeated views to maintain
accurate recent watching records.
import com.google.gson.annotations.SerializedName | ||
import kotlinx.serialization.Serializable | ||
|
||
@Serializable | ||
data class CartItemRequestDto( | ||
@SerializedName("productId") | ||
val productId: Long, | ||
@SerializedName("quantity") | ||
val quantity: Int, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
직렬화 라이브러리 혼용 문제가 있습니다.
현재 코드에서 Kotlin Serialization(@Serializable
)과 Gson(@SerializedName
)을 동시에 사용하고 있습니다. 이는 다음과 같은 문제를 야기할 수 있습니다:
문제점들:
- 의존성 충돌 가능성
- 런타임에서 예상치 못한 직렬화 동작
- 코드 일관성 저하
- 번들 크기 증가
해결 방향을 고민해보세요:
- 프로젝트에서 어떤 직렬화 라이브러리를 사용하기로 했나요?
- PR 목표에서 Kotlin Serialization을 선택했다고 했는데, 그렇다면 Gson 어노테이션이 왜 필요한가요?
- 다른 DTO들은 어떤 방식을 사용하고 있나요?
일관된 직렬화 전략을 선택하고 적용하는 것이 중요합니다.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/CartItemRequestDto.kt
around lines 3 to 12, the code mixes Kotlin Serialization (@Serializable) and
Gson (@SerializedName) annotations, causing potential conflicts and
inconsistency. To fix this, remove the Gson @SerializedName annotations and rely
solely on Kotlin Serialization annotations for this DTO, ensuring alignment with
the project's chosen serialization library and consistency with other DTOs.
interface ProductDataSource { | ||
fun load( | ||
lastProductId: Long?, | ||
size: Int, | ||
): List<ProductEntity> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
데이터 소스 인터페이스의 동기 처리 방식을 재고해보세요.
현재 인터페이스가 List<ProductEntity>
를 직접 반환하는 동기 방식인데, 몇 가지 질문을 해보겠습니다:
- 네트워크 요청이나 데이터베이스 접근이 메인 스레드를 차단하지 않을까요?
- 데이터 로딩 실패 시 어떻게 처리할 계획인가요?
- 현대 Android 개발에서 권장하는 비동기 처리 방식은 무엇일까요?
suspend
함수나 Flow
를 사용하는 방식을 고려해보고, 어떤 장단점이 있는지 생각해보세요. Repository 패턴에서 데이터 소스의 역할은 무엇이어야 할까요?
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/product/dataSource/ProductDataSource.kt
around lines 5 to 10, the ProductDataSource interface currently defines a
synchronous load function returning List<ProductEntity>, which can block the
main thread during network or database operations and lacks failure handling.
Refactor the load function to be a suspend function to support asynchronous
execution, enabling non-blocking calls and better error handling with Kotlin
coroutines. This aligns with modern Android development practices and ensures
the data source efficiently supports the repository pattern's role in providing
data asynchronously.
interface ProductClickListener : ProductQuantityClickListener { | ||
fun onProductClick(productItem: ProductsItem.ProductItem) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
인터페이스 설계 원칙 검토
ProductClickListener
가 ProductQuantityClickListener
를 상속받고 있는 구조를 보면서 질문드립니다:
- 단일 책임 원칙(SRP)의 관점에서 이 구조가 적절한가요?
- 제품 클릭과 수량 클릭이 항상 함께 필요한 기능일까요?
- 인터페이스를 더 작고 구체적인 단위로 나누면 어떤 장점이 있을까요?
인터페이스 분리 원칙(ISP)에 대해 조사해보시고, 현재 구조의 장단점을 분석해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/view/product/ProductViewHolder.kt
around lines 49 to 51, the ProductClickListener interface inherits from
ProductQuantityClickListener, which may violate the Interface Segregation
Principle (ISP) and Single Responsibility Principle (SRP). To fix this, separate
the two interfaces so that ProductClickListener and ProductQuantityClickListener
are independent, allowing classes to implement only the specific interface they
need. This improves modularity, reduces unnecessary dependencies, and makes the
code easier to maintain and test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요것도 고민해보면 좋을 것 같아요.
처음에는 한 뷰에 제품 클릭과 수량 클릭이 같이 있으니 나쁘지 않은 구조라고 생각했는데, 코드래빗의 코멘트를 보니 SRP를 해치는 걸까 생각이 드네용 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한 뷰(리사이클러뷰 아이템)에서 제품 클릭과 수량 클릭 이벤트가 함께 발생하며 하나의 상호작용 흐름으로 묶여 사용되기 때문에 SRP 를 위반한다고 생각하지 않습니다!
또한, ProductQuantityClickListener를 상속하는 방식으로 ProductClickListener를 정의함으로써, 수량 버튼 컴포넌트를 여러 곳에서 일관되게 재사용할 수 있었습니다.
var gridIndex = -1 | ||
for (i in 0..position) { | ||
if (adapter.getItemViewType(i) == viewTypeToDecorate) { | ||
gridIndex++ | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
성능 최적화 고려사항
그리드 인덱스를 계산하는 부분에서 성능 이슈가 있을 수 있습니다:
- 현재 구현에서 각 아이템마다 처음부터 해당 위치까지 반복문을 실행하는데, 이의 시간 복잡도는 얼마나 될까요?
- 아이템 개수가 많아질 때 이런 방식의 문제점은 무엇일까요?
- 어댑터나 다른 곳에서 뷰 타입별 인덱스 정보를 미리 계산해두면 어떨까요?
RecyclerView의 성능 최적화 방법들을 조사해보시고, 더 효율적인 인덱스 계산 방법을 생각해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/view/common/GridItemDecoration.kt
around lines 35 to 41, the current approach recalculates the grid index by
iterating from the start to the current position for each item, resulting in
O(n^2) time complexity as the list grows. To fix this, avoid repeated iteration
by caching or precomputing the view type indices outside the loop or maintaining
a mapping of positions to grid indices as items are bound or loaded. This will
reduce redundant computations and improve performance for large item counts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 시간 될 때 고민해보시면 좋을 것 같아요~!
companion object { | ||
fun from(viewType: Int): ItemType = entries[viewType] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입 안전성 개선
from
메서드에서 배열 인덱스 접근 시 안전성을 고려해보세요:
- 유효하지 않은
viewType
값이 전달되면 어떤 일이 발생할까요? - 배열 범위를 벗어나는 인덱스에 대한 예외 처리가 필요하지 않을까요?
- 더 안전한 타입 변환 방법은 무엇이 있을까요?
Kotlin의 getOrNull()
같은 안전한 접근 방법이나 예외 처리에 대해 알아보시고, 더 견고한 코드로 만들어보세요.
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/view/product/ProductsItem.kt around
lines 35 to 37, the from method accesses the entries array by index without
checking if the viewType is within valid bounds, which can cause exceptions for
invalid indices. To fix this, replace direct array access with a safe method
like getOrNull(viewType) to avoid out-of-bounds errors, and handle the null case
appropriately, either by returning a default value or throwing a controlled
exception to ensure type safety and robustness.
fun bind(item: ShoppingCartItem.PaginationItem) { | ||
binding.page = item.page | ||
binding.shoppingCartPageCountPlusButton.isEnabled = item.nextEnabled | ||
binding.shoppingCartPageCountMinusButton.isEnabled = item.previousEnabled | ||
binding.root.isVisible = item.nextEnabled || item.previousEnabled | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
페이지네이션 UI 가시성 로직을 다시 살펴보세요
현재 bind
메소드에서 페이지네이션의 가시성을 item.nextEnabled || item.previousEnabled
로 결정하고 있는데, 이 로직이 의도한 UX와 일치하는지 확인해보세요.
다음과 같은 상황들을 고려해보세요:
- 첫 번째 페이지에서는 어떤 버튼들이 보여야 할까요?
- 마지막 페이지에서는 어떤 버튼들이 보여야 할까요?
- 페이지가 하나뿐일 때는 어떻게 동작해야 할까요?
현재 로직으로 이런 케이스들이 올바르게 처리되는지 검토해보시기 바랍니다.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartPaginationViewHolder.kt
around lines 17 to 22, the visibility logic for the pagination UI is currently
set to show the root view if either nextEnabled or previousEnabled is true.
Review and update this logic to correctly handle edge cases: ensure the first
page disables or hides the previous button, the last page disables or hides the
next button, and if there is only one page, the entire pagination UI should be
hidden. Adjust the visibility condition accordingly to reflect these UX
requirements.
companion object { | ||
fun from(viewType: Int): ItemType = entries[viewType] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
❓ Verification inconclusive
배열 인덱스 접근의 안전성을 검토해보세요.
entries[viewType]
를 사용할 때 잘못된 viewType
값이 전달되면 어떤 일이 발생할까요?
이런 상황을 방지하기 위한 방법들을 생각해보세요. 예외 처리나 유효성 검증을 추가하는 것이 필요할지 고민해보세요.
배열 인덱스 안전성 검증 추가 필요
ShoppingCartItem.kt
의 companion object from(viewType: Int)
메서드는
fun from(viewType: Int): ItemType = entries[viewType]
처럼 배열을 직접 인덱싱해, 유효 범위를 벗어난 viewType
이 들어올 경우
ArrayIndexOutOfBoundsException
이 발생할 수 있습니다. 아래와 같이 유효성 검증이나 예외 처리를 추가해 안전성을 확보하세요.
• 대상 위치
- 파일: app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartItem.kt
- 라인: 29–31
• 제안 사항
• viewType
이 entries.indices
(0..entries.size-1) 범위 내인지 확인
• getOrNull
/getOrElse
활용 또는 require
로 사전 조건 검증
• 잘못된 값일 경우 메시지와 함께 커스텀 예외를 던지거나 기본값 반환
• 예시 수정안
companion object {
fun from(viewType: Int): ItemType {
require(viewType in entries.indices) { "Invalid viewType: $viewType" }
return entries[viewType]
}
}
🤖 Prompt for AI Agents
In app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartItem.kt
around lines 29 to 31, the from(viewType: Int) method directly indexes the
entries array without validating the viewType, risking an
ArrayIndexOutOfBoundsException if an invalid index is passed. To fix this, add a
validation step to check if viewType is within entries.indices using require or
getOrElse/getOrNull, and handle invalid values by throwing an exception with a
clear message or returning a default value to ensure safe access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review continued from previous batch...
fun List<ShoppingCartItemResponseDto>.toDomain(): List<ShoppingCartProduct> = | ||
shoppingCartItems.map(ShoppingCartItemResponseDto::toDomain) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
확장 함수의 로직을 다시 살펴보세요.
39번 줄의 확장 함수에서 shoppingCartItems.map
을 호출하고 있는데, 이는 클래스 프로퍼티를 참조하고 있습니다. 하지만 확장 함수의 수신자는 List<ShoppingCartItemResponseDto>
입니다.
이 로직이 의도한 대로 동작하는지 확인해보세요. 확장 함수에서 this
를 사용해야 하는 것은 아닌지 검토해보세요.
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemsResponseDto.kt
Length of output: 1385
확장 함수에서 수신자 대신 클래스 프로퍼티를 참조하고 있습니다.
현재 ShoppingCartItemsResponseDto
내부에 선언된
fun List<ShoppingCartItemResponseDto>.toDomain(): List<ShoppingCartProduct> =
shoppingCartItems.map(ShoppingCartItemResponseDto::toDomain)
는 리시버(this
)가 아닌 shoppingCartItems
프로퍼티를 사용하고 있어, 의도와 다르게 동작하거나 다른 리스트에는 재사용이 불가능합니다.
- app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemsResponseDto.kt: 39번 줄
다음 중 하나로 수정을 권장합니다:
-fun List<ShoppingCartItemResponseDto>.toDomain(): List<ShoppingCartProduct> =
- shoppingCartItems.map(ShoppingCartItemResponseDto::toDomain)
+fun List<ShoppingCartItemResponseDto>.toDomain(): List<ShoppingCartProduct> =
+ this.map(ShoppingCartItemResponseDto::toDomain)
또는 확장 함수를 클래스 바깥으로 이동해 명확하게 분리할 수 있습니다.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/shoppingCart/remote/dto/ShoppingCartItemsResponseDto.kt
around lines 39 to 41, the extension function on
List<ShoppingCartItemResponseDto> incorrectly uses the class property
shoppingCartItems instead of the receiver list. To fix this, replace
shoppingCartItems with 'this' to refer to the receiver list inside the extension
function, ensuring it operates on the list it is called on. Alternatively,
consider moving the extension function outside the class to clarify its scope
and usage.
private var instance: AuthenticationRepository? = null | ||
|
||
fun initialize(authDataStore: SharedPreferences) { | ||
if (instance == null) { | ||
instance = DefaultAuthenticationRepository(authDataSource = authDataStore) | ||
} | ||
} | ||
|
||
fun get(): AuthenticationRepository = instance ?: throw IllegalStateException("초기화 되지 않았습니다.") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
❓ Verification inconclusive
싱글톤 패턴의 스레드 안전성을 고려해보세요.
현재 구현에서 멀티스레드 환경에서 initialize
메서드가 동시에 호출되면 어떤 문제가 발생할 수 있을까요?
스레드 안전성을 보장하기 위한 방법들을 고민해보세요. 어떤 키워드나 패턴을 사용할 수 있을지 생각해보세요.
싱글톤 초기화 시 스레드 안전성 보장 필요
멀티스레드 환경에서 initialize
가 동시에 호출되면 두 개 이상의 인스턴스가 생성될 수 있습니다.
다음과 같은 방법으로 스레드 안전성을 확보하세요:
instance
필드를@Volatile
로 선언- 이중 확인 잠금(double-checked locking)을 적용하거나,
synchronized
키워드 사용 - 또는 Kotlin
lazy { … }
혹은object
싱글톤으로 전환
예시 (이중 확인 잠금):
--- a/app/src/main/java/woowacourse/shopping/data/authentication/repository/DefaultAuthenticationRepository.kt
+++ b/app/src/main/java/woowacourse/shopping/data/authentication/repository/DefaultAuthenticationRepository.kt
@@ companion object {
- private var instance: AuthenticationRepository? = null
+ @Volatile
+ private var instance: AuthenticationRepository? = null
@@ fun initialize(authDataStore: SharedPreferences) {
- if (instance == null) {
- instance = DefaultAuthenticationRepository(authDataSource = authDataStore)
- }
+ if (instance == null) {
+ synchronized(this) {
+ if (instance == null) {
+ instance = DefaultAuthenticationRepository(authDataSource = authDataStore)
+ }
+ }
+ }
위와 같이 수정하여 멀티스레드에서도 한 번만 초기화되도록 보장하세요.
📝 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.
private var instance: AuthenticationRepository? = null | |
fun initialize(authDataStore: SharedPreferences) { | |
if (instance == null) { | |
instance = DefaultAuthenticationRepository(authDataSource = authDataStore) | |
} | |
} | |
fun get(): AuthenticationRepository = instance ?: throw IllegalStateException("초기화 되지 않았습니다.") | |
} | |
companion object { | |
@Volatile | |
private var instance: AuthenticationRepository? = null | |
fun initialize(authDataStore: SharedPreferences) { | |
if (instance == null) { | |
synchronized(this) { | |
if (instance == null) { | |
instance = DefaultAuthenticationRepository(authDataSource = authDataStore) | |
} | |
} | |
} | |
} | |
fun get(): AuthenticationRepository = | |
instance ?: throw IllegalStateException("초기화 되지 않았습니다.") | |
} |
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/authentication/repository/DefaultAuthenticationRepository.kt
around lines 27 to 36, the singleton initialization is not thread-safe, which
can lead to multiple instances being created in a multithreaded environment. To
fix this, declare the instance variable as @Volatile and apply double-checked
locking inside the initialize method using synchronized to ensure only one
instance is created. Alternatively, consider using Kotlin's lazy initialization
or converting the class to an object singleton for inherent thread safety.
override fun onPlusShoppingCartClick(quantityObservable: QuantityObservable) { | ||
val item = quantityObservable as ProductsItem.ProductItem | ||
viewModel.addProductToShoppingCart(item, item.selectedQuantity) | ||
} | ||
|
||
override fun onMinusShoppingCartClick(quantityObservable: QuantityObservable) { | ||
val item = quantityObservable as ProductsItem.ProductItem | ||
viewModel.minusProductToShoppingCart(item, item.selectedQuantity) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입 캐스팅 안전성 확보 필요
현재 코드에서 as
연산자를 사용한 강제 캐스팅이 있습니다. 만약 다른 타입의 QuantityObservable
이 전달되면 어떤 일이 발생할까요?
안전한 캐스팅을 위해 고려해볼 수 있는 방법:
as?
와 null 체크is
연산자를 사용한 타입 검사- sealed class나 다른 패턴 활용
어떤 방법이 이 상황에 가장 적합할까요?
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/view/shoppingCartRecommend/ShoppingCartRecommendActivity.kt
around lines 64 to 72, the forced cast using 'as' on QuantityObservable can
cause runtime exceptions if the type is unexpected. To fix this, replace the
forced cast with a safe cast using 'as?' and add a null check before proceeding,
or use an 'is' check to verify the type before casting. This ensures type safety
and prevents crashes when the passed object is not of the expected type.
android:textColor="@color/gray_5" | ||
android:textSize="22sp" | ||
android:textStyle="bold" | ||
app:layout_constraintBottom_toBottomOf="@id/productPlusQuantityButton" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConstraintLayout 제약 조건 오류 수정
productPlusQuantityButton
의 제약 조건이 자기 자신을 참조하고 있습니다. 이로 인해 레이아웃이 의도한 대로 표시되지 않을 수 있어요.
- app:layout_constraintBottom_toBottomOf="@id/productPlusQuantityButton"
+ app:layout_constraintBottom_toBottomOf="parent"
- app:layout_constraintTop_toTopOf="@id/productPlusQuantityButton" />
+ app:layout_constraintTop_toTopOf="parent" />
이렇게 수정하면 어떤 변화가 있을지 확인해보세요!
Also applies to: 75-75
🤖 Prompt for AI Agents
In app/src/main/res/layout/component_product_quantity.xml at lines 71 and 75,
the constraint attributes reference the view's own ID, causing a
self-referential constraint error. Update these constraints to reference a
different, appropriate view ID instead of the view itself to fix the layout
issue and ensure proper positioning.
thread { | ||
runCatching { | ||
recentWatchingDao.getRecentWatchingProducts(size) | ||
}.onSuccess { recentWatchingProducts: List<RecentWatchingEntity> -> | ||
onResult(Result.success(recentWatchingProducts.map { it.product })) | ||
}.onFailure { exception -> | ||
onResult(Result.failure(exception)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
비동기 처리 방식 개선 고려
현재 thread {}
블록을 사용하고 있는데, Kotlin에서 비동기 처리를 위한 더 나은 방법이 있을까요?
고려해볼 수 있는 점들:
- 코루틴의 장점은 무엇일까요?
- 스레드 풀 관리는 어떻게 되고 있을까요?
- 취소 가능한 작업이 필요한 경우는 어떻게 처리할까요?
현재 방식과 코루틴 방식의 차이점을 생각해보시면 좋을 것 같아요!
Also applies to: 63-72, 78-91
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/data/product/repository/DefaultProductsRepository.kt
around lines 48 to 56 (and similarly at 63-72, 78-91), the current use of raw
thread blocks for asynchronous processing should be replaced with Kotlin
coroutines. Coroutines provide better thread pool management, support for
structured concurrency, and allow cancellation of ongoing tasks. Refactor the
code to launch these operations within a coroutine scope using appropriate
dispatchers (e.g., Dispatchers.IO for database calls) and handle success and
failure results within coroutine context instead of raw threads.
private fun isEmptyPage(products: List<ShoppingCartProduct>): Boolean = | ||
if (products.isEmpty() && page != MINIMUM_PAGE) { | ||
minusPage() | ||
true | ||
} else { | ||
false | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
부작용이 있는 체크 함수를 개선하세요
isEmptyPage
함수는 단순히 확인만 해야 하는데, minusPage()
를 호출하여 상태를 변경하는 부작용이 있습니다. 이는 함수 이름과 실제 동작이 일치하지 않아 혼란을 줄 수 있습니다.
이 로직을 updateShoppingCart
메서드 내부로 이동하거나, 함수 이름을 더 명확하게 변경하는 것을 고려해보세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModel.kt
around lines 119 to 126, the isEmptyPage function has a side effect by calling
minusPage(), which changes state despite the function name suggesting a pure
check. To fix this, either move the minusPage() call out of isEmptyPage into the
updateShoppingCart method where state changes are expected, or rename
isEmptyPage to a name that clearly indicates it performs a state update,
ensuring the function's behavior matches its name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 리뷰를 참고해주세요~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true 값을 반환할 경우에 minusPage 를 호출하도록 수정했습니다.
private fun updateShoppingCartItems( | ||
products: List<ShoppingCartProduct>, | ||
currentShoppingCartItems: List<ShoppingCartItem>, | ||
) { | ||
if (currentShoppingCartItems.isEmpty()) { | ||
_shoppingCart.value = | ||
buildList { | ||
addAll(currentShoppingCartItems) | ||
addAll(products.map(::ShoppingCartProductItem)) | ||
} | ||
return | ||
} | ||
if (page == 1) { | ||
val productItems = | ||
products.map { product -> | ||
val existing = | ||
currentShoppingCartItems | ||
.filterIsInstance<ShoppingCartProductItem>() | ||
.find { it.shoppingCartProduct.id == product.id } | ||
|
||
ShoppingCartProductItem( | ||
shoppingCartProduct = product, | ||
isChecked = existing?.isChecked == true, | ||
) | ||
} | ||
_shoppingCart.value = productItems | ||
return | ||
} | ||
val updatedItems = | ||
currentShoppingCartItems | ||
.map { item -> | ||
if (item is ShoppingCartProductItem) { | ||
val updatedProduct = | ||
products.find { it.id == item.shoppingCartProduct.id } | ||
if (updatedProduct != null) { | ||
item.copy( | ||
shoppingCartProduct = updatedProduct, | ||
isChecked = item.isChecked, | ||
) | ||
} else { | ||
item | ||
} | ||
} else { | ||
item | ||
} | ||
}.toMutableList() | ||
|
||
val existingIds = | ||
updatedItems | ||
.filterIsInstance<ShoppingCartProductItem>() | ||
.map { it.shoppingCartProduct.id } | ||
|
||
val newItems = | ||
products | ||
.filter { it.id !in existingIds } | ||
.map { ShoppingCartProductItem(shoppingCartProduct = it) } | ||
|
||
updatedItems.addAll(newItems) | ||
|
||
_shoppingCart.value = updatedItems | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
복잡한 updateShoppingCartItems 메서드를 리팩토링하세요
이 메서드는 너무 많은 책임을 가지고 있고 복잡합니다. 다음과 같이 개선할 수 있습니다:
- 각 조건(빈 리스트, 첫 페이지, 기존 페이지)에 대한 처리를 별도 메서드로 분리
- 134번 라인의 불필요한
addAll(currentShoppingCartItems)
제거 (빈 리스트 추가) - 중복 코드 제거
메서드를 더 작은 단위로 나누면 테스트하기도 쉬워지고 가독성도 향상됩니다.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModel.kt
between lines 127 and 187, the updateShoppingCartItems method is overly complex
and handles multiple responsibilities. Refactor by extracting separate private
methods for each condition: handling empty currentShoppingCartItems, handling
the first page update, and handling updates for subsequent pages. Remove the
unnecessary addAll(currentShoppingCartItems) call when the current list is
empty. Consolidate and reuse code where possible to eliminate duplication,
improving readability and testability.
fun decreaseQuantity(shoppingCartProductItem: ShoppingCartProductItem) { | ||
shoppingCartRepository.decreaseQuantity( | ||
shoppingCartProductItem.shoppingCartProduct.id, | ||
shoppingCartProductItem.shoppingCartProduct.quantity - 1, | ||
) { result -> | ||
result | ||
.onSuccess { | ||
updateShoppingCart() | ||
_hasUpdatedProducts.value = true | ||
}.onFailure { | ||
_event.postValue(ShoppingCartEvent.DECREASE_SHOPPING_CART_PRODUCT_FAILURE) | ||
} | ||
} | ||
} | ||
|
||
fun increaseQuantity(shoppingCartProductItem: ShoppingCartProductItem) { | ||
shoppingCartRepository.increaseQuantity( | ||
shoppingCartProductItem.shoppingCartProduct.id, | ||
shoppingCartProductItem.shoppingCartProduct.quantity + 1, | ||
) { result -> | ||
result | ||
.onSuccess { | ||
updateShoppingCart() | ||
_hasUpdatedProducts.value = true | ||
}.onFailure { | ||
_event.postValue(ShoppingCartEvent.ADD_SHOPPING_CART_PRODUCT_FAILURE) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
수량 변경 메서드의 네이밍과 유효성 검사를 개선하세요
-
decreaseQuantity
와increaseQuantity
메서드가 실제로는 절대 수량을 설정하고 있습니다. Repository 메서드 이름을updateQuantity
로 변경하거나, 실제로 증감 연산을 수행하도록 수정하세요. -
수량 변경 시 유효성 검사가 필요합니다:
- 수량이 0 미만이 되지 않도록
- 수량이 재고나 최대 주문 가능 수량을 초과하지 않도록
수량 감소 시 최소값 검증이 필요한지 확인해보세요. 예를 들어 수량이 1일 때 감소를 시도하면 어떻게 되나요?
🏁 Script executed:
#!/bin/bash
# Locate ShoppingCartRepository and its methods across the codebase
echo "1. Finding ShoppingCartRepository interface/abstract class:"
rg -n "interface ShoppingCartRepository" -n app/src || rg -n "abstract class ShoppingCartRepository" -n app/src
echo -e "\n2. Finding decreaseQuantity definitions:"
rg -n "fun decreaseQuantity" -n app/src
echo -e "\n3. Finding increaseQuantity definitions:"
rg -n "fun increaseQuantity" -n app/src
echo -e "\n4. Listing implementations of ShoppingCartRepository:"
rg -n "class .*ShoppingCartRepositoryImpl" -n app/src
Length of output: 1620
🏁 Script executed:
#!/bin/bash
# Inspect ShoppingCartViewModel around relevant lines and search for '== false'
FILE_VM=app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModel.kt
echo "===== ViewModel (lines 180-260) ====="
sed -n '180,260p' $FILE_VM
echo -e "\n===== '== false' occurrences in ViewModel ====="
rg -n "== false" -n $FILE_VM
# Inspect DefaultShoppingCartRepository to see how quantity is handled
FILE_REPO=app/src/main/java/woowacourse/shopping/data/shoppingCart/repository/DefaultShoppingCartRepository.kt
echo -e "\n===== DefaultShoppingCartRepository (lines 70-110) ====="
sed -n '70,110p' $FILE_REPO
Length of output: 5026
수량 변경 로직 네이밍·유효성·LiveData 업데이트 일관성 개선 요청
- ShoppingCartViewModel의
decreaseQuantity
/increaseQuantity
는 내부에서 절대 수량을 설정하고 있으므로,
• 메서드명을updateQuantity
로 통합하거나
• 뷰모델에서currentQuantity ± 1
을 직접 계산하여 전달하도록 변경해주세요. - 수량 변경 전 사전 검증을 추가하세요.
• 감소 시 1보다 작아지지 않도록 (quantity > MIN_QUANTITY
)
• 증가 시 재고 또는 최대 주문 수량(maxQuantity
)을 초과하지 않도록 - 백그라운드 콜백 내 LiveData 업데이트는
postValue
를, UI 스레드에서는setValue
를 일관되게 사용해주세요.
(현재 콜백 내부에서value =
대신postValue
로 수정 권장) if (loadable == false)
구문은if (!loadable)
로 간결하게 바꿔주세요.
필요하다면 MINIMUM_QUANTITY
, MAXIMUM_QUANTITY
같은 상수를 선언해 재사용하세요.
🤖 Prompt for AI Agents
In
app/src/main/java/woowacourse/shopping/view/shoppingCart/ShoppingCartViewModel.kt
around lines 201 to 229, refactor decreaseQuantity and increaseQuantity methods
to either rename them to updateQuantity or ensure they only increment/decrement
the quantity by 1 instead of setting absolute values. Add validation before
changing quantity to prevent it from going below a minimum (e.g., 1) or
exceeding maximum stock or order limits. Use postValue consistently inside
background callbacks for LiveData updates instead of directly setting value.
Also, simplify any 'if (loadable == false)' checks to 'if (!loadable)'. Define
constants like MINIMUM_QUANTITY and MAXIMUM_QUANTITY for reuse if needed.
<com.facebook.shimmer.ShimmerFrameLayout | ||
android:id="@+id/shoppingCartSkeletonLayout" | ||
android:layout_width="match_parent" | ||
android:layout_height="0dp" | ||
android:layout_marginHorizontal="18dp" | ||
android:visibility="gone" | ||
app:layout_constraintBottom_toBottomOf="parent" | ||
app:layout_constraintTop_toBottomOf="@id/shoppingCartToolbar"> | ||
|
||
<LinearLayout | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent" | ||
android:orientation="vertical"> | ||
|
||
<include layout="@layout/activity_shopping_cart_skeleton" /> | ||
|
||
<include layout="@layout/activity_shopping_cart_skeleton" /> | ||
|
||
<include layout="@layout/activity_shopping_cart_skeleton" /> | ||
|
||
<include layout="@layout/activity_shopping_cart_skeleton" /> | ||
|
||
<include layout="@layout/activity_shopping_cart_skeleton" /> | ||
|
||
<include layout="@layout/activity_shopping_cart_skeleton" /> | ||
|
||
<include layout="@layout/activity_shopping_cart_skeleton" /> | ||
</LinearLayout> | ||
|
||
</com.facebook.shimmer.ShimmerFrameLayout> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
스켈레톤 로딩의 접근성을 개선해보세요
스켈레톤 레이아웃이 로딩 중임을 사용자에게 알리는 방법을 고려해보셨나요?
시각 장애인 사용자도 현재 데이터가 로딩 중임을 알 수 있도록 하려면 어떤 속성을 추가해야 할까요?
힌트: android:importantForAccessibility
나 android:contentDescription
속성을 활용해보세요.
🤖 Prompt for AI Agents
In app/src/main/res/layout/activity_shopping_cart.xml between lines 147 and 176,
the ShimmerFrameLayout used for the skeleton loading does not provide
accessibility information to indicate loading status. To fix this, add the
attribute android:importantForAccessibility="yes" and set
android:contentDescription="Loading shopping cart items" on the
ShimmerFrameLayout to inform screen readers that the content is currently
loading.
<ImageView | ||
android:id="@+id/productDetailImage" | ||
android:layout_width="match_parent" | ||
android:layout_height="0dp" | ||
android:adjustViewBounds="true" | ||
app:imageUrl="@{viewModel.product.product.imageUrl}" | ||
app:layout_constraintDimensionRatio="1:1" | ||
app:layout_constraintTop_toTopOf="parent" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
이미지 로딩 실패 시 처리를 고려해보세요
상품 이미지가 로딩에 실패했을 때를 대비한 처리가 있나요?
고려해볼 점들:
- 네트워크 오류로 이미지를 불러올 수 없을 때
- 잘못된 URL이 제공되었을 때
- 로딩 중 사용자 경험
이런 경우들을 어떻게 처리하면 좋을지 생각해보고, 관련 속성들을 조사해보시겠어요?
🤖 Prompt for AI Agents
In app/src/main/res/layout/activity_product_detail.xml around lines 59 to 66,
the ImageView for the product image lacks handling for image loading failures
and loading states. To fix this, add attributes or binding adapters to show a
placeholder image while loading, and an error image if loading fails due to
network issues or invalid URLs. Investigate and use properties or libraries that
support these states to improve user experience during image loading.
@coderabbitai pause |
✅ Actions performedReviews paused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 둘리!
재리뷰요청 드립니다!!!
이번 리뷰도 잘 부탁드립니다
private const val USER_PASSWORD_KEY = "password" | ||
|
||
@Suppress("ktlint:standard:property-naming") | ||
private var INSTANCE: AuthenticationRepository? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
싱글톤처럼 사용되는 값이라 올 대문자로 썼는데, 오해의 소지가 있을 것 같아 카멜케이스로 수정했습니다!
e76194b
private const val BASE_URL = | ||
"http://techcourse-lv2-alb-974870821.ap-northeast-2.elb.amazonaws.com/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseUrl 을 숨기도록 수정했습니다!
77f803b
import woowacourse.shopping.data.authentication.repository.AuthenticationRepository | ||
import java.io.IOException | ||
|
||
class AppInterceptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용자 인증 정보를 가진 인터셉터여서 AuthenticationInterceptor
로 수정했습니다!
a7af392
private val productService: ProductService by lazy { | ||
ApiClient | ||
.getApiClient(DefaultAuthenticationRepository.get()) | ||
.create(ProductService::class.java) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분을 놓쳤네요😓
상품을 가져올 땐 사용자 인증이 필요없다는 것을 확인해, retrofit 객체 함수를 유저 인증이 필요한 것과 필요없는 것으로 분리해서 사용했습니다!
a81c33c
id = "oungsi2000", | ||
password = "password", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 내용 반영했습니다!
71f3ffd
fun getCartCounts(): Call<CartCountsResponseDto> | ||
|
||
@POST("cart-items") | ||
fun postCartItem( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serivce 에서는 직접적으로 서버와 통신할 수 있는 창구라 생각합니다!
따라서 호출하는 쪽에서 어떤 방식의 서버 요청인지 명확히 인지할 수 있도록 하는 것이 더 사용하기 편할 것 같아 해당 내용으로 메서드 명을 지었습니다.
private fun handleEmptySelectedQuantityVisibility() { | ||
binding.productQuantityComponentLayout.visibility = View.GONE | ||
binding.productPlusQuantityButtonDefault.visibility = View.VISIBLE | ||
} | ||
|
||
private fun handleSelectedQuantityVisibility(quantity: Int) { | ||
binding.productQuantityComponentLayout.visibility = View.VISIBLE | ||
binding.productPlusQuantityButtonDefault.visibility = View.GONE | ||
binding.productQuantityComponent.quantity = quantity | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추천 상품 및 메인 화면 상품 목록 수량 조절 버튼 가시성을 데이터 바인딩으로 수정했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
레포지토리에서 어떤 라이브러리를 통해서 데이터를 가져올 것인지 알 필요가 없을 것 같습니다.
이번 미션에서 데이터소스라는 것이 필요할까? 라는 생각이 들었는데, 추후 retrofit > ktor 로 라이브러리가 변경됐을 경우, 혹은 로컬 db 중 여러 개를 사용할 경우 레포지토리가 해당 구현체를 모두 알아 변경사항이 생길 것 같습니다.
따라서, 레포지토리는 변경되지 않고, 데이터소스만 변경되야 할 것 같아 모든 레포지토리에 대해서 데이터 소스를 분리했습니다.
fun toDomain(): Product = | ||
Product( | ||
id = id, | ||
name = name, | ||
price = price, | ||
imageUrl = imageUrl, | ||
category = category, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 내용에 대해 저번 장바구니에서 비슷한 내용으로 리뷰로 받아서 고민해봤습니다!
- 해당 엔티티(혹은 dto) 파일 내부에 매퍼를 둔다.
- 도메인이 비슷한 매퍼들끼리 모아 별도의 파일로 분리한다.
1번 방식은 엔티티나 매핑 대상 객체가 변경될 때, 해당 객체와 가까운 위치에서 매퍼도 함께 수정할 수 있다는 장점이 있습니다.
하지만, 지금 같이 기능이 많지 않은 경우엔 오히려 과도한 분리가 될 수 있겠다는 생각이 들었습니다.
2번 방식은 매퍼들을 한 눈에 파악할 수 있고, xxxMapper.kt처럼 파일 이름을 통해 어떤 매퍼들이 있을 지 한 눈에 알 수 있단 장점이 있을 것 같습니다!
다만, 매퍼 수가 많아질 경우 오히려 특정 매퍼를 찾기 어려워지지 않을까 하는 우려도 있습니다.
따라서 변경사항이 생겼을 때 가장 가까운 곳에서 수정할 수 있도록 현재는 dto 에 매퍼 함수를 두었습니다!
확장함수는 캡슐화를 깨지 않는걸까?
상황에 따라 다를 수 있다고 생각합니다.
확장 함수가 도메인 객체에 대해 너무 많은 지식이나 규칙을 담고 있어, 매핑 과정에 도메인의 비즈니스 로직이 들어간다면 캡슐화를 해친다고 생각하지만, 단순히 생성자 <-> 생성자 수준의 값 매핑이라면, 이는 캡슐화를 해친다고 보긴 어렵다고 생각합니다.
productsRepository.getRecentWatchingProducts(10) { result -> | ||
result | ||
.onSuccess { recentWatchingProducts -> | ||
var updatedProducts = productsWithoutLoadItem | ||
if (hasRecentWatching) { | ||
updatedProducts = productsWithoutLoadItem.drop(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위치가 바뀌면 문제가 생길 수 있을 것 같습니다 !
따라서 filterNot 을 이용해 해당 타입을 제거한 값만 가질 수 있도록 수정했습니다.
72b513f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뭉치 안녕하세요~ 쇼핑 주문 1, 2단계 미션도 고생많으셨습니다!!
다음 미션 리뷰 요청 때는 장바구니 페이지네이션도.. 함께 포함해주세요 👀
추가로 직렬화 라이브러리 선택 이유도 다음 PR에 포함되면 좋겠습니다. 왜 reflection이 문제가 될 수 있을까요? 🤔
이번 미션은 다음 미션 진행을 위해 merge하겠습니다! 리뷰도 다음 미션에 반영해주시면 됩니다.
다음 미션도 화이팅이에용 💪
implementation(libs.google.glide) | ||
implementation(libs.okhttp3.okhttp) | ||
implementation(libs.mockwebserver) | ||
implementation(libs.gson) | ||
implementation(libs.androidx.room.runtime) | ||
implementation(libs.kotlinx.serialization.json) | ||
implementation(libs.retrofit.kotlinx.serialization.converter) | ||
implementation(libs.retrofit) | ||
implementation(libs.shimmer) | ||
implementation(libs.converter.gson) | ||
implementation(libs.logging.interceptor) | ||
kapt(libs.androidx.room.compiler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드래빗 리뷰처럼 Gson과 Kotlinx Serialization 중 하나를 정리하는 것이 좋아보여요!
ActivityScenario.launch<ProductDetailActivity>(intent) | ||
|
||
scenario = ActivityScenario.launch(intent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 리뷰도 참고해주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하지 않는 코드는 정리해주세요!
fun getApiClient(): Retrofit = | ||
Retrofit | ||
.Builder() | ||
.baseUrl(BuildConfig.BASE_URL) | ||
.addConverterFactory( | ||
Json.asConverterFactory(MEDIA_TYPE_JSON.toMediaType()), | ||
).build() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 클라이언트를 사용하는 경우에는 loggingInterceptor를 사용하지 못할 것 같아요 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 내용 반영했습니다.
|
||
@Serializable | ||
data class PageableResponseDto( | ||
@SerialName("offset") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다 👍 난독화면에서도 장점이 있으니 이 부분도 찾아보면 좋을 것 같아요~
} | ||
} | ||
|
||
viewModel.shoppingCartProductsToOrder.observe(this) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 LiveData라서 어쩔 수 없겠군요 😢
보다보니 역으로 shoppingCart
가 MediatorLiveData를 사용해야하는건 아닐까 싶기도 하구요!
원본 데이터들이 바뀌었을 때 뷰를 위한 데이터 리스트인 shoppingCart
가 따라 바뀌는 형태가 된다면 어떨까요?
private val _isLoading: MutableLiveData<Boolean> = MutableLiveData(true) | ||
val isLoading: LiveData<Boolean> get() = _isLoading | ||
|
||
private val _totalPrice: MediatorLiveData<Int> = MediatorLiveData<Int>().apply { value = 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MediatorLiveData
대신 LiveData.map
을 사용하는 것을 고려해볼 수도 있을 것 같아요~!
private fun isEmptyPage(products: List<ShoppingCartProduct>): Boolean = | ||
if (products.isEmpty() && page != MINIMUM_PAGE) { | ||
minusPage() | ||
true | ||
} else { | ||
false | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 리뷰를 참고해주세요~!
private val _hasUpdatedProducts: MutableLiveData<Boolean> = | ||
MutableLiveData(false) | ||
val hasUpdatedProducts: LiveData<Boolean> get() = _hasUpdatedProducts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LiveData가 아니어도 될 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var 로 수정했습니다!
|
||
companion object { | ||
private const val MINIMUM_PAGE = 1 | ||
private const val COUNT_PER_PAGE: Int = Int.MAX_VALUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 리뷰도 참고해주세요~!
셀프 체크리스트
README.md
에 기능 목록을 정리하고 명확히 기술하였는가?어떤 부분에 집중하여 리뷰해야 할까요?
이번 미션 커밋만 모아둔 링크입니다
안녕하세요 둘리 블랙잭 이후로 또 뵙네요 ㅎㅎ!
이번 미션에서 저번 미션 제 코드를 썼는데 리팩토링 되지 않은 코드라 가독성이 많이 떨어집니다.. ㅠ_ㅠ
또한, 이번 미션에서도 구현에 중점을 두어 함수 분리 등 더 가독성이 좋아지지 않은 코드가 됐습니다. . . . .
코드리뷰 받은 후 테스트 추가 및 수정, 리팩토링 진행하겠습니다🥹

지금 이런 상태라 모든 기능에 대해서 정상적으로 작동하진 않을 것 같습니다. . . ㅠ ㅠ 이 부분 또한 다음 리뷰 요청 때 수정해 보내겠습니다.
전반적으로 상태 관리에 대한 리뷰 및 뷰모델 로직에 대해서 리뷰를 받고 싶습니다!
이번 리뷰 잘 부탁드립니다 🙇♀️
Gson 대신 Kotlin Serialization을 선택한 이유는 리플렉션 비용이 없고, 컴파일 시점에 오류를 사전에 확인할 수 있어 더 안전하고 빠르다 판단해서 선택했습니다!
스크린샷
[홈화면 최근 본 상품]
default.mp4
[가장 최근 본 상품 기준 추천 상품]
default.mp4