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

[otter] 2주차 첫번째 PR review-FE #73

Merged
merged 13 commits into from
May 18, 2022

Conversation

otterp012
Copy link
Collaborator

@otterp012 otterp012 commented May 18, 2022

안녕하세요 에드! 오터입니다.
저번주에는 개인적인 일정이 너무 많아서 PR을 보내지 못했었어요 😂
이번주에는, 조금 더 열심히 하고자 마음을 먹고 다음과 같은 구현을 진행했습니다.

새로운 구현 내용

  • reducer 를 사용해서 전체적으로 리팩토링을 진행했습니다.
    • 기존에, state만 사용했을 때보다 중복로직이 많이 줄어들었다는 생각이 들었습니다.
    • 동일한 로직인데, state를 사용했을때에는 wallet에서, 그리고form에서 중복된 로직을 계속 적어주었었는데 해당 로직을 합칠 수 있었습니다.
  • 이를 통해서, 돈을 투입하는 로직은 어느정도 완성되었습니다.
    • form에서 돈을 숫자로 작성하거나, wallet에서 금액을 투입이 가능하고 이 상태를 두 페이지에서 모두 확인할 수 있습니다.
    • 돈을 투입하였을때 log가 출력되는 기능을 추가했습니다.

고민하고 있는 부분

  • context 를 사용할때 context가 사용되는 부분이 불필요하게 렌더링하는 문제를 memo, useCallback 등을 통해 개선하려고 공부했습니다.

  • 그런데, 공부를 하다보니 이 부분에 memo를 사용하는게 적절한가? 라는 의문이 많이 들었어요.

    • memo를 잘 사용하려면, props 변화가 잘 일어나지 않고 상태가 잘 변하지 않는 정적인 컴포넌트에 사용하는게 좋다 라고 배웠는데
    • 제가 짠 컴포넌트들을 보아하니 여기저기 다 상태들이 변하고 props가 변하고 있더라구요.
  • 이런 부분에 대해서, memo를 사용하는 조금 더 좋은 기준이 있을까요? 궁금합니다!

  • 또한, context 자체를 두가지로 나눌 수 있지 않을까? 라는 부분도 고민하고 있는데요.

    • 이렇게 context를 나누는 부분은 어떻게 생각하시는지 궁금합니다!
    • 개인적으로 생각했을 때는, context가 하나라면 한 공간에서 모든 상태를 관리한다는 점에서 응집도가 조금 좋다고 느껴졌지만,
    • 이를 통해 불필요한 렌더링이 생긴다면 나누는 것도 좋다라는 생각이 들었습니다.

@otterp012 otterp012 added the review-FE New feature or request label May 18, 2022
@sungik-choi sungik-choi assigned otterp012 and unassigned sungik-choi May 18, 2022
@sungik-choi sungik-choi self-requested a review May 18, 2022 16:35
Copy link

@sungik-choi sungik-choi left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 리듀서를 나누셔서 사용하신 게 관심사의 분리가 잘 되어서 좋아보여요 👍
질문 주신 부분 답변 드리자면,

그런데, 공부를 하다보니 이 부분에 memo를 사용하는게 적절한가? 라는 의문이 많이 들었어요.

memo를 잘 사용하려면, props 변화가 잘 일어나지 않고 상태가 잘 변하지 않는 정적인 컴포넌트에 사용하는게 좋다 라고 배웠는데
제가 짠 컴포넌트들을 보아하니 여기저기 다 상태들이 변하고 props가 변하고 있더라구요.
이런 부분에 대해서, memo를 사용하는 조금 더 좋은 기준이 있을까요? 궁금합니다!

저라면 첫번째로 변화가 자주 일어나는 부분과 자주 일어나지 않은 부분을 분리할 수 있는지(분리하기에 적당한 사이즈인지) 먼저 확인해볼 거 같아요. prop이 변하더라도, 자주 변하지 않는다면 memo의 사용을 고려해볼거 같아요. 사실 프로파일링을 통한 측정이 먼저긴합니다.

또한, context 자체를 두가지로 나눌 수 있지 않을까? 라는 부분도 고민하고 있는데요.

이렇게 context를 나누는 부분은 어떻게 생각하시는지 궁금합니다!
개인적으로 생각했을 때는, context가 하나라면 한 공간에서 모든 상태를 관리한다는 점에서 응집도가 조금 좋다고 느껴졌지만,
이를 통해 불필요한 렌더링이 생긴다면 나누는 것도 좋다라는 생각이 들었습니다.

나누는 건 정말 좋습니다 👍 리듀서를 나누셨듯이, 두 상태의 컨텍스트도 별도로 관리되는 게 좋아보여요.

<Button width="100%">VENDING</Button>
<Button width="100%">WALLET</Button>
{/* 라우팅 추가할 부분 */}
{/* <Button width="100%">STOCK</Button> */}

Choose a reason for hiding this comment

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

주석 제거해주세요!

setIsInputValid(true);
dispatchMoney({
type: 'INSERT',
newState: newInsertedMoneyObj,

Choose a reason for hiding this comment

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

Suggested change
newState: newInsertedMoneyObj,
payload: newInsertedMoneyObj,

보통 payload라는 이름을 사용하는 편이에요.

);

if (isValid) {
setIsInputValid(true);

Choose a reason for hiding this comment

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

아래도 마찬가지로, setIsInputValid(isValid) 와 동일하기에 로직을 더 단순화시킬 수 있지 않을까요?

Comment on lines +16 to +25
for (let i = walletObj.length - 1; i >= 0; i--) {
const [unit, count] = walletObj[i];
if (Number(unit) > money) continue;
if (Number(unit) * Number(count) < money) continue;
else {
let use = Math.floor(money / Number(unit));
money -= Number(unit) * use;
newInsertedMoney[unit] = use;
}
}

Choose a reason for hiding this comment

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

for 반복문을 사용하지 않는 방식으로 변경해보시면 더 좋을 거 같아요!

changed.INSERTED[unit] = INSERTED[unit] + action.newState[unit];
}

const newWALLET = { ...WALLET, ...changed.WALLET };

Choose a reason for hiding this comment

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

newWALLET 같은 형식의 변수명은 사용하지 않으시는 게 좋아요. newWallet 이라고 하시거나, 변하지 않는 값일 경우 NEW_WALLET 이라고 표기할 수 있겠네요.

@sungik-choi sungik-choi merged commit 808f942 into codesquad-members-2022:otter May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-FE New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants