Skip to content

Conversation

@usageness
Copy link

@usageness usageness commented Apr 25, 2022

안녕하세요, 노스님! 좋은 주말 보내셨나요 😊

주말 동안 피드백 받은 내용을 생각하며 어떻게 고칠 수 있을지 고민했는데
결론적으로 현재 구조에서는 아무리 잘 고쳐도 좋은 구조가 될 수 없다고 느꼈습니다.

결국 구조 자체를 잘못 나누었구나! 라는 생각이 들어서 컴포넌트를 나누는 기준을 생각해보니
재사용이 가능한 단위로 나누는게 적절하다고 느꼈습니다.

기존의 CalculatorButton.js 은 그저 App.js의 코드를 나눠받는 역할에 불과해서
그 자체로는 아무런 사용성이 없었습니다. 철저하게 의존적인 컴포넌트가 되어있더라구요.
(말씀하신 것처럼 재사용하기 정말 정말 어려운... 단위였습니다.)

(이전 step과 변한건 없지만) 동작은 이 곳 에서 확인 부탁드립니다!
아래는 이번 step2에서의 반영 내용입니다!

Class Component를 Function Component로 전환

기존의 클래스 컴포넌트를 함수형 컴포넌트로 전환하였습니다.
크게 바뀐 부분은 이 정도로 정리할 수 있겠네요!

  • state로 직접 관리하던 변수들을 Hooks으로 관리
/* class component */
state = {
  firstNumber = 0,
  ...
}

/* function component */
const [firstNumber, setFirstNumber] = useState(0);
...
  • 기존의 life cycle을 Hooks로 관리
/* class component */
componentDidMount() {
  ...
}

/* function component */
useEffect(() => {
  ...
}, []);
  • 그 외 각종 Hooks 사용 등등...
const onClickModifier = useCallback(() => {
  ...
}, []);

피드백 반영

calculate를 호출할때마다 res 변수를 할당하는게 불필요한 반복적인 연산같고(굉장히 작은 비용이겠지만ㅎㅎ) 세부적으로 코드를 읽어야 해서, 따함수나 메소드를 따로 빼서 사용하면 좋을것 같아요. 세지님은 어떻게 생각하시나요??

  1. calculate 호출 시점이 연산이 실제로 일어나야 하는 경우로 한정되어 있어서 연산의 결과값을 res 변수에 할당하는 것 자체는 큰 무리가 없는 것 같아요... 혹시 제가 뭔가 놓치고 있는 것일지도 모르겠지만 이 부분은 아무리 생각해도 잘 모르겠네요 😭

  2. 코드를 쭉 읽어야 로직이 보인다는 점은 확실한 단점인 것 같아요.
    말씀하신 것처럼 적당하게 분리해서 사용하는게 좋을 것 같습니다 😊
    refactor: 계산 로직 utils/로 분리


밑에 calculatorbutton 컴포넌트와 같이 얘기할껄 그랬네요. 세지님이 말씀하신것처럼 setState를 자식컴포넌트에게 전달해주는건 저도 반대입니다. 지금의 컴포넌트 props를 유지한다면 해당 메소드들이 필요하겠죠.

제가 말씀드리려고 했던 부분은 크게 두가지였어요.

app.js 내부에서는 setResult, setIsFirstNumber와 같은 메소드를 따로 만들어 사용할 필요가 있을까 생각하였습니다.
calculatorbutton의 props가 바뀐다는 전제하에서는 해당 메소드가 삭제되거나 대체되어 불필요할것이라고 판단했습니다.

말씀해주신 것처럼 현재 구조에서는 해결하기 어려운 단점들이 많았습니다.
그래서 잘못 나누어졌다고 생각하는 컴포넌트를 지워버리고,
App.js로 다시 올려 state 만큼 생겨나는 wrapper와 props로 넘어가는 문제를 해결하였습니다.

이후에 class component의 전환과 리팩토링을 진행하면서 사라진 부분이긴 하지만
이때 구조를 변경해두어서 그 다음 작업들이 수월했습니다 😊
refactor: 잘못 나누어진 컴포넌트 제거


지금 불필요하게 setState를 하는듯 보여요. 계산된 값을 처음 setState({result: res}) -> initState -> setResult('오류') or setResult(res) 이렇게 로직이 진행됩니다. 계산된값의 검증을 하고 최종적으로 set하면 될꺼 같으면 init이 필요한지 잘 모르겠습니다
setFirtNumber(res)의 함수에 localStorage.setItem이 있다보니 localStorage도 업데이트도 불필요하게 두번 이루어지고 있는듯합니다

로직을 다듬으며 중복되는 코드들이나 불필요하게 state와 localStorage의 값을 갱신하는 부분들을 찾아 제거하였습니다.
chore: 코드 리뷰 피드백 반영


아직 리액트를 보고 있으면 알쏭달쏭한 기분입니다.
그래도 노스님의 피드백 덕분에 첫 시작 방향을 잘 잡은것 같아 마음이 놓이네요 👍
감사합니다!! 좋은 하루 보내시길 바랄게요 😊

Copy link

@hyoungnam hyoungnam left a comment

Choose a reason for hiding this comment

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

안녕하세요 세지님:) 저도 리액트 Effect를 볼때마다 알쏭달쏭합니다😇 변경사항 잘 확인하였고 몇가지 코멘트 추가적으로 남겼습니다. 이부분 다음 미션하면서 같이 생각해보면 좋을 것 같아요(한편 컴포넌트부분에 대한 코멘트였으니 구조가 잘못되지는 않았다고 생각합니다 )
머지하도록 할꼐요~!

onCalculate();
};

const onClickModifier = useCallback(() => {

Choose a reason for hiding this comment

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

useCallback을 사용하신 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

react에서는 컴포넌트가 리렌더 될때마다 함수를 새로 만들어 할당한다고 들었습니다!
이때 useCallback을 사용하면 리렌더시에 새로 만들지 않고, 기억해둔 함수를 할당해준다고 해서 적용해보았어요 :)
내용이 바뀔 염려가 없는 함수이니만큼 useCallback을 적용하기 좋다고 생각했습니다!

export default function NumberButton(props) {
return (
<div className="digits flex" onClick={props.onClick}>
{Array.from({ length: numberOfButtons }, (_, i) => (

Choose a reason for hiding this comment

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

import vs props
둘중에 어떤게 변경에 유연한가요? Numberbutton을 사용하는 입장에서 버튼을 다섯개로 만들고 싶다고 가정해보아도 좋을것 같습니다. 한편 변경에 꼭 유연해야 할 필요가 있는 컴포넌트나 코드인지도 생각해보고 세지님이 결정내려도 좋을것 같네요:)

Copy link
Author

Choose a reason for hiding this comment

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

모든 버튼이 동일한 크기를 가져야 한다면 import가 변경에 유연하겠지만,
각각의 경우에 다른 버튼 수를 가질 수 있다면 props가 유리해 보입니다!
키패드 버튼이 변할 수 있다면 당연히 다른 경우가 있을텐데 섣불리 import를 사용한 것 같아요... 😥

모든 컴포넌트가 꼭 변경에 유연해야 할 필요는 없었군요...!
생각해 본 적 없는 관점인데 그럴수도 있을 것 같네요 😊

export default function ModifierButton(props) {
return (
<div className="modifiers subgrid" onClick={props.onClick}>
<button className="modifier">AC</button>

Choose a reason for hiding this comment

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

AC vs props.~ vs children
이 세가지 차이점에 대해서 생각해보면 좋을것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

props를 통한 데이터 전달만 사용해봤는데,
엘리먼트 전달에는 children를 이용하는게 좋겠네요!

이런 경우에는 children을 사용하는게 좋아 보입니다 😊

<button className="modifier">{ children }</button>

Comment on lines +70 to +74
setFirstNumber(0);
setSecondNumber(0);
setResult("오류");
setOperator(null);
setIsFirstNumber(true);

Choose a reason for hiding this comment

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

다음미션부터는 setState가 많다면 한번쯤 묶어서 관리해도 되지 않나 생각해보면 좋을것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

현재 상태 그대로 배열로 묶어 관리하거나, 상태 관리 로직이 늘어난다면 useReducer도 사용해보겠습니다!

setSecondNumber(secondNumber * 10 + Number(inputNumber));
};

const onClickOperator = (e) => {

Choose a reason for hiding this comment

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

보통 실제 onClick 이벤트에 로직을 실행하는 함수들을 handle~~ 이렇게 적어놓는편이에요

root.render(
<React.StrictMode>
<App />
<Calculator />

Choose a reason for hiding this comment

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

저도 App보다는 Calculator가 맞다고 생각합니다🤘

@@ -0,0 +1,36 @@
import { ErrorMessage } from "../constants/constants";

const calculateValue = (firstNumber, secondNumber, operator) => {

Choose a reason for hiding this comment

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

잘 분리된것 같습니다:) 매번 이 함수를 새로 정의하고 할당할 필요도 없고요. 저도 다음부터 이부분에 대해서 코멘트할때 좀 더 잘 전달할수있도록 고민해봐야겠네요!

@hyoungnam hyoungnam merged commit 8faff58 into woowacourse:usageness Apr 26, 2022
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