[클린코드 리액트 3기 김지영] 계산기 구현 #67
Conversation
wonsss
left a comment
wonsss
left a comment
There was a problem hiding this comment.
안녕하세요! 지영님 😄 반갑습니다.
늦게 합류하게 되셔서 과제하실 시간이 많이 부족하셨을텐데, 열심히 하시어 과제를 제출해주셨군요.
수고 많으셨고 감사합니다! 👍
리뷰 남겨드린 내용 참고하시어 도움이 되셨으면 좋겠습니다.
그리고 첫 번째 질문은 리뷰 코멘트 내에 달아두었습니다.
두 번째 질문은 다음과 같았는데요.
지금은 clickHandler 안에서 모든 클릭의 target 값에 따라, display(계산기 창) state에 저장하거나 display 값을 계산하거나 reset을 하고있있는데요. target의 값이 숫자인지, 연산자인지, 지우기인지 등 확인하는 부분은 다 if 조건 안에 &&으로 구별되어있어서 이 부분은 조금 더 가독성있고 안정적이게 작성하고싶은데 방법이 있을까요?
피연산자(숫자), 사칙연산자, 등호, 초기화(AC) 각각 할일(책임)이 다르다고 생각됩니다.
"함수는 한 가지 책임"만 가지게 만들어 유지보수성을 높이는 것이 좋습니다. 그리고 그러려려면 함수를 되도록 작게 만들고, 한 가지만 하도록 하는 방향이 좋습니다.
그래서 함수를 4개로 분리할 수도 있을 것 같네요! 그렇다면, 기존에 clickHandler 함수 내에 있던 입력값이 숫자인지 피연산자인지 등호인지 등을 파악하던 조건문은 새 함수들에서는 사라지게 됩니다.
함수의 이름도 어떤 기능을 수행하는 것인지 명확하게 알 수 있도록 지어야 합니다.
이렇게 하게 되면, 가독성과 유지보수성이 높아집니다!
There was a problem hiding this comment.
root에 .vscode 폴더와 react-calculator 폴더가 있네요.
현재 react-calculator 폴더 안에 파일들을 넣어 분리할 필요가 있는 상황이 아니어서, root로 다 꺼내는 게 나을 것 같아요 😄
아마 vite 프로젝트 만들기 명령어를 사용하면서 한번에 프로젝트가 생성되면서 root 하위에 새로운 프로젝트 이름의 폴더가 생기신 것 같아요. ㅎㅎ 저는 말씀드린 대로 밖으로 다 꺼내서 쓸 것 같습니다.
| {numbers.map((num) => { | ||
| return ( | ||
| <Button | ||
| clickHandler={clickHandler} |
There was a problem hiding this comment.
컴포넌트 props와 핸들러 함수에 이름을 짓는 것도 일반적인 컨벤션이 있는데, 참고하시면 좋을 것 같아요. 이름을 잘 지어두면, 나중에 가독성과 유지보수에 도움이 됩니다 😄
- prop의 이름에 접두사 on 를 사용합니다. (e.g. onClick)
- 핸들러 함수의 이름에 handle 접두사를 사용합니다. (e.g. handleClickNumber)
- 내장된 이벤트 핸들러의 prop 이름을 커스텀 이벤트의 prop 이름에서 중복 사용하는 것을 피합니다. 예를 들어, 만약 네이티브 focus/click 이벤트 사용이 관심사가 아니라면 prop 이름으로 onFocus나 onClick 대신에 onSelect 같은 이름을 사용합니다.
| const numbers = [1, 2, 3, 4, 5, 6, 7, 8, 9, 0]; | ||
| const operators = ["/", "%", "-", "+", "="]; | ||
|
|
||
| const clickHandler = (e: React.MouseEvent<HTMLButtonElement, MouseEvent>) => { |
There was a problem hiding this comment.
이 핸들러 함수가 '숫자'와 '연산자' 입력에 대한 모든 케이스를 담당하고 있네요. 숫자 클릭에 대한 핸들러 함수와 연산자 클릭에 대한 핸들러 함수로 나누면 어떨까요?
| {operators.map((operator) => { | ||
| return <Button clickHandler={clickHandler} value={operator} />; | ||
| })} |
There was a problem hiding this comment.
이렇게 하면 더 심플해집니다!
| {operators.map((operator) => { | |
| return <Button clickHandler={clickHandler} value={operator} />; | |
| })} | |
| {operators.map(operator => ( | |
| <Button clickHandler={clickHandler} value={operator} /> | |
| ))} |
| {numbers.map((num) => { | ||
| return ( |
There was a problem hiding this comment.
array의 map 메서드가 컴포넌트를 바로 반환할 때 return을 사용하지 않고 바로 컴포넌트를 소괄호로 감싸서 반환하시는 스타일이 더 간단하고 가독성에 좋아요
| const value = e.currentTarget.value.toString(); | ||
|
|
||
| if (!operators.includes(value)) { | ||
| console.log(value); |
| if (value === "=") { | ||
| setDisplay(eval(display)); | ||
| } else if (value === "AC") { |
There was a problem hiding this comment.
"=", "AC" 이런 매직 리터럴은 상수로 공통 관리하여 사용하시면 좋아요.
react-calculator/src/App.tsx
Outdated
| const numbers = [1, 2, 3, 4, 5, 6, 7, 8, 9, 0]; | ||
| const operators = ["/", "%", "-", "+", "="]; |
There was a problem hiding this comment.
별도의 상수 파일로 분리하면 좋겠습니다!
operators를 이와 같이 배열로 관리하면, 인덱스를 통해서 요소에 접근해야 하는데, 첫번째는 "/"고 다섯번째는 "="라는 규칙은 기억하기 어려울 것 같아요. 따라서 key, value인 객체로 관리하면 좋겠습니다!
| function Button({ clickHandler, value, className = undefined }: Props) { | ||
| return ( | ||
| <button | ||
| onClick={(e) => clickHandler(e)} |
There was a problem hiding this comment.
아래와 같이 해도 동일합니다 😄
| onClick={(e) => clickHandler(e)} | |
| onClick={clickHandler} |
| setDisplay((currentFormula) => `${currentFormula}${value}`); | ||
| } else { | ||
| if (value === "=") { | ||
| setDisplay(eval(display)); |
There was a problem hiding this comment.
계산 로직을 eval 함수로 대체하셨군요
그런데 자바스크립트에서 eval 함수는 사용하지 않는 것이 좋습니다.
자세한 내용은 https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/eval 참고해보세요~!
사칙연산이 요구사항이니까, 좌측피연산자, 연산자, 우측피연산자 이렇게 3가지 인수를 갖고 사칙연산하여 계산결과를 반환하는 함수를 직접 구현해보시는 것도 좋을 것 같습니다 😄
|
@ji0kim |
2 participants
안녕하세요,
이번에 라스트미닛에 합류하게돼서 정리 못한 일정으로 시간 투자를 많이 못했습니다. ㅠ 🥲
아직 task구현을 완료하지 못했지만 일단 PR 보냅니다.
일단 작동하도록 만드는 과정중에 있는데요, 지금까지 하는 중에 어려웠던 점이나 궁금했던 점 리스트하면,
처음에 프로젝트 셋업할 때 fork > clone 한 프로젝트에 vite 을 설치하고 eslint setting.json 파일을 추가하면서 프로젝트 구조가 2중으로 생성된 것 같더라고요. 보통 어떤 과정으로 셋업 하시는지 궁금합니다.
지금은 clickHandler 안에서 모든 클릭의 target 값에 따라, display(계산기 창) state에 저장하거나 display 값을 계산하거나 reset을 하고있있는데요. target의 값이 숫자인지, 연산자인지, 지우기인지 등 확인하는 부분은 다 if 조건 안에 &&으로 구별되어있어서 이 부분은 조금 더 가독성있고 안정적이게 작성하고싶은데 방법이 있을까요?
아직 구현이 완료되지 않아서 진행 해나가면서 변경이 있을 것 같은데 피드백 해주시면 반영해서 진행해보겠습니다. 감사합니다.🙂