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

[벼리] 장바구니 미션 제출합니다. #3

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

gaeun5744
Copy link

3단계까지 구현하고 제출합니다~

Screen_recording_20241101_165031.webm

질문

  • 다른 activity로 이동하는 구현을 컴포저블 내부에서 구현할지, 이벤트를 위로 끌어올려서 activity 내부에서 로직을 구현할지 고민됩니다..!
    • activity에서 구현하면, 로직을 일관되게 통일할 수 있지만 이벤트에 대한 파라미터가 증가해서 고민되고 있습니다!

Copy link
Contributor

@woowahan-leah woowahan-leah left a comment

Choose a reason for hiding this comment

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

안녕하세요 벼리!

장바구니 3단계까지 구현을 잘 해주셨습니다 💯
전반적으로(특히 네비게이션 관련) 상태 호이스팅을 적용해보시면 더욱 도움이 많이 될 것 같아요~
고민해보시면 좋을만한 코멘트를 달았으니 확인해주시면 감사하겠습니다.

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 🙏 💪

# android-shopping-cart
Copy link
Contributor

Choose a reason for hiding this comment

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

왜 다들 3단계까지만 하시나요 😭
4단계가 찐인데!

CartProductRepository.addItem(
Product(
id = 0,
name = "레아 사랑해요",
Copy link
Contributor

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 🙄


@Before
fun setUp() {
if (CartProductRepository.cartItems.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

에디, 채드에게도 남긴 코멘트지만, 다음 두 가지를 고민해보시면 좋겠습니다.

  1. Cart 내부 상태가 매 테스트마다 독립적임이 보장되지 않는다. (예를 들어 다른 테스트에서 이미 상품을 담은 상태로 이 테스트를 돌리게 되면 실패) Before 구문을 활용하고는 있지만, 근본적인 원인을 해결할 수 없다.
  2. ShoppingCartScreen이 테스트하기 어렵다(내부에서 상태를 가지고 있다)

핵심은 이번 미션을 통해 상태를 갖는 컴포넌트, 저장소에 의존하는 컴포넌트는 테스트하기 어렵다라는 점을 얻어가셨으면 좋겠습니다 🙂

import org.junit.Rule
import org.junit.Test

class CartScreenTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

장바구니 화면에서 발생할 수 있는 시나리오를 잘 조합하여 테스트 코드로 만들어주셨네요! (혹시 TDD도 해보셨을까요?)

지금의 테스트는 Android View(XML) 환경에서 작성하는 Activity/Fragment단위 테스트와 크게 다르지 않은데요,

이번 기회에 컴포즈의 함수 단위 테스트 작성 이점을 충분히 누려보시면 좋겠어요.
대표적으로 4단계에서도 재활용될 수 있는 수량 조절 컴포넌트는 별도의 독립된 테스트 시나리오를 적용할 수 있습니다.


// then
composeTestRule.onNodeWithText(
(cartItem.quantity.currentValue + cartItem.countInterval).toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

다른 개발자들도 이 assert문을 한 눈에 보고 이해할 수 있을까요?
다음 글이 도움되길 바랍니다!

http://jojoldu.tistory.com/615?category=1036934

import nextstep.shoppingcart.ui.theme.Typography

@Composable
fun SubmitButton(
Copy link
Contributor

Choose a reason for hiding this comment

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

공통된 컴포넌트로 분리한 만큼, 네이밍도 특정 도메인과 결합하지 않는 컴포넌트로 설계하면 좋을 것 같아요.

만약 SubmitButton의 역할/책임만 갖는다면 공통된 컴포넌트로 분리하는 것이 불필요해지고, 확장 가능성이 떨어집니다.


@Composable
fun ProductDetailScreen() {
val activity = LocalContext.current.findActivity()
Copy link
Contributor

Choose a reason for hiding this comment

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

이러한 코드는 나중에 컴포즈에서 Navigation을 도입하게 되면 다 걷어내게 되거나, 버그로 이어지게 됩니다

Comment on lines +53 to +56
HorizontalDivider(
thickness = 1.dp,
color = Gray10,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Divider 활용 👍
Box를 Modifier로 조작하는 것과 다르지 않아 보이지만
명시적으로 레이아웃을 나누는 데에 활용하는 컴포넌트를 활용하는 것을 더 선호합니다.

A divider is a thin line that groups content in lists and layouts.

Comment on lines +19 to +32
class ProductListActivity : ComponentActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
enableEdgeToEdge()
setContent {
ShoppingCartTheme {
ProductListScreen()
}
}
}
}

@Composable
fun ProductListScreen() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Activity와 Screen을 같은 파일로 두기보다, 분리하는 편이 각각의 역할을 더 잘 드러낸다고 생각합니다.
Screen이라고 해서 Actvitiy에 종속될 필요는 없어요!

Comment on lines +28 to +33
modifier =
modifier
.fillMaxSize()
.clickable {
onClick(product)
},
Copy link
Contributor

Choose a reason for hiding this comment

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

modifier를 이미 주입받고 있는데, 해당 modifier의 외부에서 clickable을 달아주는 것과 어떻게 다를까요?

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