[1단계 - 계산기] 유세지(김용래) 미션 제출합니다. #17
Conversation
Co-authored-by: Koy <[email protected]>
Co-authored-by: Koy <[email protected]>
Co-authored-by: Koy <[email protected]>
Co-authored-by: Koy <[email protected]>
Co-authored-by: Koy <[email protected]>
Co-authored-by: Koy <[email protected]>
Co-authored-by: Koy <[email protected]>
Co-authored-by: Koy <[email protected]>
Co-authored-by: Koy <[email protected]>
hyoungnam
left a comment
hyoungnam
left a comment
There was a problem hiding this comment.
안녕하세요 세지님! 저도 바닐라만 리뷰하다가 리액트를 리뷰하려고 하다보니 어색하고.. 클래스 컴포넌트도 오랜만이여서 새롭네요:) 기능동작 확인하였고 몇가지 코멘트 남겼으니 확인해주세요~!
| calculate = () => { | ||
| const res = (() => { | ||
| switch (this.state.operator) { | ||
| case "+": | ||
| return this.add(); | ||
| case "-": | ||
| return this.sub(); | ||
| case "X": | ||
| return this.multiple(); | ||
| case "/": | ||
| return this.divide(); | ||
| default: | ||
| throw new Error("존재하지 않는 연산자입니다."); | ||
| } | ||
| })(); | ||
|
|
||
| this.setState({ result: res }, () => { | ||
| this.initState(); | ||
|
|
||
| if (res === Infinity || isNaN(res)) { | ||
| this.setFirstNumber(0); | ||
| this.setResult("오류"); | ||
| localStorage.setItem("prevValue", "오류"); | ||
| return; | ||
| } | ||
|
|
||
| this.setFirstNumber(res); | ||
| this.setResult(res); | ||
| localStorage.setItem("prevValue", res); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
- 이곳에서 즉시실행함수를 사용해야하는 이유나 이점이 있나요??
- 지금 불필요하게 setState를 하는듯 보여요. 계산된 값을 처음 setState({result: res}) -> initState -> setResult('오류') or setResult(res) 이렇게 로직이 진행됩니다. 계산된값의 검증을 하고 최종적으로 set하면 될꺼 같으며 init이 필요한지 잘 모르겠습니다
- setFirtNumber(res)의 함수에 localStorage.setItem이 있다보니 localStorage 업데이트도 불필요하게 두번 이루어지고 있는듯합니다
There was a problem hiding this comment.
이곳에서 즉시실행함수를 사용해야하는 이유나 이점이 있나요??
switch 문 내부에서 break 대신 return을 사용하려고 즉시 실행 함수로 만들어주었습니다!
return 문을 사용하는 것이 라인 수도 줄어들고, 보기에도 편해서 사용하게 되었습니다.
지금 불필요하게 setState를 하는듯 보여요. 계산된 값을 처음 setState({result: res}) -> initState -> setResult('오류') or setResult(res) 이렇게 로직이 진행됩니다. 계산된값의 검증을 하고 최종적으로 set하면 될꺼 같으면 init이 필요한지 잘 모르겠습니다
확실히 검증하기 전에 state를 set하는것은 불필요한 로직처럼 보이네요 😅 수정하겠습니다!
마지막 init은 연속 계산을 감안해 state를 재조정하려 넣었는데 로직을 수정하게 되면 한번에 처리할 수 있을 것 같아요!
setFirtNumber(res)의 함수에 localStorage.setItem이 있다보니 localStorage도 업데이트도 불필요하게 두번 이루어지고 있는듯합니다
급하게 만들다보니 state나 localStorage 업데이트를 중복해서 수행하는 로직이 곳곳에 나타나는 것 같아요 😥
로직을 다시 점검해서 중복 갱신이 일어나지 않도록 수정 하겠습니다!
There was a problem hiding this comment.
switch 문 내부에서 break 대신 return을 사용하려고 즉시 실행 함수로 만들어주었습니다!
return 문을 사용하는 것이 라인 수도 줄어들고, 보기에도 편해서 사용하게 되었습니다.
calculate를 호출할때마다 res 변수를 할당하는게 불필요한 반복적인 연산같고(굉장히 작은 비용이겠지만ㅎㅎ) 세부적으로 코드를 읽어야 해서, 따함수나 메소드를 따로 빼서 사용하면 좋을것 같아요. 세지님은 어떻게 생각하시나요??
| setFirstNumber = (number) => { | ||
| this.setState( | ||
| { | ||
| firstNumber: Number(number), | ||
| }, | ||
| localStorage.setItem("prevValue", number) | ||
| ); | ||
| }; | ||
|
|
||
| setOperator = (operator) => { | ||
| this.setState({ operator }); | ||
| }; | ||
|
|
||
| setSecondNumber = (number) => { | ||
| this.setState( | ||
| { | ||
| secondNumber: Number(number), | ||
| }, | ||
| localStorage.setItem("prevValue", number) | ||
| ); | ||
| }; | ||
|
|
||
| setResult = (result) => { | ||
| this.setState({ result }); | ||
| }; | ||
|
|
||
| setIsFirstNumber = (isFirstNumber) => { | ||
| this.setState({ isFirstNumber }); | ||
| }; |
There was a problem hiding this comment.
setState를 wrapping 함수가 필요한가 했을때 저는 조금 의문이 들었습니다. 지금 규모에서 setState(인자)로도 의미전달은 충분할것 같고, 반복이 심하지 않고요.
There was a problem hiding this comment.
그렇다면 현재 구조에서 부모의 setState를 원형 그대로 자식 컴포넌트로 전달해주는 방법도 괜찮을까요?
자식 컴포넌트에게 너무 큰 권한을 가진 함수를 전달해주는 느낌이라 괜히 걱정이 되네요... 😥
There was a problem hiding this comment.
밑에 calculatorbutton 컴포넌트와 같이 얘기할껄 그랬네요. 세지님이 말씀하신것처럼 setState를 자식컴포넌트에게 전달해주는건 저도 반대입니다. 지금의 컴포넌트 props를 유지한다면 해당 메소드들이 필요하겠죠.
제가 말씀드리려고 했던 부분은 크게 두가지였어요.
- app.js 내부에서는 setResult, setIsFirstNumber와 같은 메소드를 따로 만들어 사용할 필요가 있을까 생각하였습니다.
- calculatorbutton의 props가 바뀐다는 전제하에서는 해당 메소드가 삭제되거나 대체되어 불필요할것이라고 판단했습니다.
|
|
||
| render() { | ||
| return ( | ||
| <> |
| <CalculatorButton | ||
| setFirstNumber={this.setFirstNumber} | ||
| setOperator={this.setOperator} | ||
| setSecondNumber={this.setSecondNumber} | ||
| setIsFirstNumber={this.setIsFirstNumber} | ||
| calculate={this.calculate} | ||
| setResult={this.setResult} | ||
| result={this.state.result} | ||
| initState={this.initState} | ||
| isFirstNumber={this.state.isFirstNumber} | ||
| firstNumber={this.state.firstNumber} | ||
| secondNumber={this.state.secondNumber} | ||
| /> |
There was a problem hiding this comment.
지금 미션의 취지와 규모에서 재사용할일이 없긴하겠지만... 재사용하기가 정말 어려운 컴포넌트인것 같아요. 이 컴포넌트를 재사용한다고 했을때 어떤 이벤트에 로직을 주입하는것인지 파악하기 어렵습니다. 만약 AC를 클릭했을때 alert를 하도록 로직을 추가한다고 하면 어떻게 될까요? 새로 props도 뚫어줘야하고 컴포넌트 내부도 바뀌게 될것 같습니다
There was a problem hiding this comment.
제가 생각하기에도 자식 컴포넌트에 너무 많은 함수를 props로 전달해줘서 로직을 파악하기 어렵고,
기능의 추가나 변경이 있을때 어떻게 수정해줘야 할지 막막해보이는 상황인 것 같아요.
step2 제출 전까지 현재 상태를 개선할 수 있도록 고민해보겠습니다! 😊
|
노스님, 빠른 리뷰 감사드립니다!! |
2 participants
노스님, 안녕하세요! 이번 미션의 리뷰이 유세지입니다 😊
이번 미션에서는 리액트 자체에 익숙해진다는 마음으로 임해보았습니다.
한동안 바닐라 자바스크립트만 사용하다보니 리액트는 어떻게 사용했던건지 기억이 가물가물하네요 😅
오랜만에 공식문서도 보고, 생명주기도 떠올리며 즐겁게 코딩하였습니다!
짧은 미션 기간이지만 잘 부탁드립니다!
데모 페이지
요구 사항