Skip to content
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

[채드] 4단계 장바구니 미션 제출합니다. #5

Open
wants to merge 11 commits into
base: dpcks0509
Choose a base branch
from

Conversation

dpcks0509
Copy link

@dpcks0509 dpcks0509 commented Nov 4, 2024

3단계 피드백 리팩토링 및 4단계 구현 완료하였습니다!
SnapshotStateList를 사용해 보았는데 알맞게 사용한 것인지는 모르겠습니다! (피드백 받고 싶어요!!)

시연 영상

default.mp4

@dpcks0509 dpcks0509 self-assigned this Nov 4, 2024
Copy link

@murjune murjune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 채드! 3단계에 이어서 4단계 미션까지 잘 수행해주셨네요 💯
제가 남겨드린 피드백은 제 개인적인 의견이고 정답이 아닐 수 있습니다!
채드가 피드백에 의문을 갖으신다면 언제든지 코멘트나 DM 보내주세요! 💪

그리고 현재 머지 충돌이 발생하고 있는데 해결해주시면 좋을 것 같습니다~

.editorconfig Outdated
@@ -0,0 +1,2 @@
[*.{kt,kts}]
ktlint_function_naming_ignore_when_annotated_with = Composable, Test
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 맞다 ktlint ㅋㅋㅋ 잘 적용해주셨군요!

@@ -8,6 +8,12 @@ espressoCore = "3.6.1"
lifecycleRuntimeKtx = "2.8.4"
activityCompose = "1.9.1"
composeBom = "2024.06.00"
ktlint = "12.1.0"
coil = "3.0.0-rc02"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coil 3.0.0 이 릴리즈 되었습니다. 😉

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그사이에 3.0.1까지 나왔네요! 수정하였습니다!!

Comment on lines 34 to 36
androidx-appcompat = { group = "androidx.appcompat", name = "appcompat", version.ref = "appcompat" }
material = { group = "com.google.android.material", name = "material", version.ref = "material" }
androidx-activity = { group = "androidx.activity", name = "activity", version.ref = "activity" }
Copy link

@murjune murjune Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분이 없어도 동작할 것 같습니다 👀

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇네요 불필요한 libs 삭제하였습니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string resource를 야무지게 잘 적용해주셨군요! 👍

Comment on lines +231 to +234
imageUrl =
"https://search.pstatic.net/common/?src=http%3A%2F%2Fblogfiles.naver.net" +
"%2FMjAyNDAyMjNfMjkg%2FMDAxNzA4NjE1NTg1ODg5.ZFPHZ3Q2HzH7GcYA1_Jl0lsIdvAnzUF2h6Qd6bgDLHkg." +
"_7ffkgE45HXRVgX2Bywc3B320_tuatBww5y1hS4xjWQg.JPEG%2FIMG_5278.jpg&type=sc960_832",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 잘 몰라서 그런데 Coil 이 Preview 에서도 반영이 되나요??
네트워크 통신을 자동으로 해주는건가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coil은 preview가 적용이 안되는 것 같아요,, 추후에 지원이 됐으면 좋겠네요!

Comment on lines +5 to +9
sealed interface ProductListAction {
data class AddProduct(val product: Product) : ProductListAction

data class DecreaseProductQuantity(val product: Product) : ProductListAction
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 액션을 인터페이스로 잘 추상화해주셨군요!

아마 컴포저블 함수의 parameter 의 개수를 줄이기 위해 적용하신 것이겠죠??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 맞습니다!

val snackbarHostState = remember { SnackbarHostState() }
val snackbarMessage = stringResource(R.string.shopping_cart_order_completed)

val shoppingCartProducts = DatabaseShoppingCartRepository.shoppingCartProducts
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Composable 은 UI 요소 입니다. UI 가 Repository 을 모르도록 데이터를 받아오는 로직을 상위로 올려볼까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

activity에서 screen의 parameter로 데이터를 넘기도록 수정했습니다!

Comment on lines 42 to 47
DatabaseShoppingCartRepository.removeProduct(product = productA)
DatabaseShoppingCartRepository.removeProduct(product = productB)

DatabaseShoppingCartRepository.addProduct(product = productA)
DatabaseShoppingCartRepository.addProduct(product = productA)
DatabaseShoppingCartRepository.addProduct(product = productB)
Copy link

@murjune murjune Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 ShoppingCartScreen 이 DatabaseShoppingCartRepository 와 강결합되어 있기 때문에 테스트할 때 DatabaseShoppingCartRepository를 설정해주고 있습니다.

아래 남겨드린 피드백을 적용하시면 자연스럽게 해결될 것 같네요 💪

import nextstep.shoppingcart.domain.model.ShoppingCartProduct
import nextstep.shoppingcart.domain.repository.ShoppingCartRepository

object DatabaseShoppingCartRepository : ShoppingCartRepository {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일반적으로 Repository 는 remote에서 오는지 local에서 오는지 등 데이터의 출처를 추상화하여 데이터를 받아옵니다. 그러나, Database 라는 데이터 출처를 class 명으로 나타내고 있네요.

채드는 Repository 패턴을 사용하시는 다른 이유가 있으신가요?? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음 그렇네요,, Repository 는 remote에서 오는지 local에서 오는지 등 데이터의 출처에 관심이 없겠군요. class 명 수정하겠습니다!

import nextstep.shoppingcart.domain.repository.ShoppingCartRepository

object DatabaseShoppingCartRepository : ShoppingCartRepository {
private val _shoppingCartProducts: SnapshotStateList<ShoppingCartProduct> = mutableStateListOf()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SnapshotStateList 를 잘 사용해주셨군요!

mutableStateListOfmutableStateOf(list) 는 어떤 차이가 있을까요? 언제 상태 변경을 감지하는지 위주로 설명해주세요 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutableStateListOf는 리스트 자체의 변경과 아이템의 추가/삭제를 자동으로 감지하여 UI에 반영해줍니다.
반면 mutableStateOf(list)는 리스트 전체를 교체할 때만 상태 변경을 감지하므로, 아이템을 추가하거나 삭제할 때는 새 리스트로 할당해야 UI 업데이트가 이루어집니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants