[클린코드 2기 정현수] 계산기 미션 #97
Conversation
- eslint, prettier 설치 및 설정 - index.js을 index.html에 연결
- cypress-watch-and-reload 라이브러리 설치해서 src 파일들이 변경돼도 자동으로 테스트하도록 설정 - cypress.json 여러가지 설정 - video, baseUrl, watch 경로
- cypress를 이용해 테스트 구축
- app.js 생성 후 App 클래스를 작성
- 이벤트 위임을 이용해 클릭 이벤트 구현
- Digits 컴포넌트와 비슷하게 이벤트 위임을 이용해 클릭 이벤트 구현
- Digits, Operations 클릭 시 발동하는 이벤트인 recordTotalValue를 구현
- state를 App으로부터 받고 render하는 방식으로 변경
- sum -> evaluationResult
- AC(all clear) 버튼 클릭 시 0으로 리셋
- 조금 더 명확한 버튼을 클릭하도록 get method를 통해서 접근
- numberCount state를 추가해서 숫자가 몇 글자 입력됐는지 감시
- whatOperatorUseInExpression - calculateExpressionWithOperator - calculateReduceWithOperator 세 가지 함수로 모듈화
- OPERATOR에서 OPERATION으로 변경
- 가독성을 높이기 위해 이벤트들을 함수로 만들어 사용 - 실제 코드에서 사용하는 상수 값들을 사용
- cypress commands를 생성해서 함수를 만듦 - gitignore에서 cypress/support 제거
- pleaseEnterNumberBeforeOperator가 아닌 pleaseEnterNumberBeforeOperation으로 변경
- 연산자 앞에는 숫자가 있어야 한다. - 아무 입력 없이 연산자를 클릭하면 alert를 띄움 - 연산자를 연속으로 누를 수 없다. - '2++' 이런식의 식은 안되도록 테스트 케이스 생성
- lastClickedButton state를 선언해서 마지막 입력이 연산자면 alert로 메세지를 호출
- this.state.currentTotal === INIT_STATE.currentTotal를 isClean() 이라는 함수로 변경
- onClickButtons 함수에서 onClickDigit만 따로 빼서 digit 클래스에 전달
- App 클래스에서 계산하지 않고, executor util obj를 생성해서 계산 로직을 분리
- onClickDigit로 변경됨
- operation을 추출하는 함수를 util 함수로 생각해서 옮김
- "=" 클릭 시 맨 마지막 클릭한 버튼은 숫자여야 한다.
- validateWhenClickOperation 함수를 생성해 검증하는 함수를 따로 생성 - 쓸모없는 함수들 (calculateExpression, recordOperation)들을 삭제하고 변수 명들을 조금 더 명확하게 변경
There was a problem hiding this comment.
정말 상세한 리뷰 감사합니다!
제가 생각지도 못했던 부분들이 많아서 신기하고 재미있었어요!
제가 리뷰해주신 부분들 혼자 생각해보면서 질문사항이 몇 가지가 생겼습니다!
지금은 여러 연산자가 들어갔을 때는 구현하지 못했습니다. 리뷰 해 주신 부분들을 잘 생각하면서 리팩토링 한 것을 또 피드백 받고 싶어서 이렇게 리뷰 요청 드렸습니다!
추가적으로 구현해야 하는 것
- 여러 연산자 들어갔을 때 구현하기
질문사항
1. 자식 컴포넌트 Dom 변수 위치에 대해서
현재는 app.js 코드에서 자식 컴포넌트들을 위와 같이 선언하고 사용하고 있는데요
자식 컴포넌트들에 첫번째 인자로 DOM 객체를 넘겨주는 방식으로 하고 있는데
이 Dom객체를 자식 컴포넌트에서 불러서 쓰는게 나을까요? 아니면 현재처럼 app.js에서 넘겨주는게 깔끔할까요?.. 감이 오질 않습니다! 멘토님은 어떻게 하실 것 같나요?
2. 컴포넌트에 대해서
현재는 모든 state를 app.js에 생성해서 사용하고, state들이 변경시키고 싶을 때 setState를 불러서 자식 컴포넌트들을 해당 state를 기준으로 rendering하는 방식으로 사용하고 있습니다.
지금 컴포넌트에서 state를 사용하는 곳이 total 컴포넌트 밖에 없어서 리렌더링을 위해서 컴포넌트를 나눴다기 보다는 이벤트리스너마다 컴포넌트를 나누어서 진행을 했다?.. 라는 느낌이 더 강한데요 혹시 멘토님은 이런 상황에서는 어떤 방식으로 컴포넌트를 나누는게 낫다고 생각하시나요!?
3. window.Function 사용
const evaluatedExperssion = window.Function(`return ${expression.replace('X', '*')}`)();
console.log(evaluatedExperssion);위와 같이 window.Function을 사용하면, string을 계산하도록 되어있는데 사용하지 않는 이유는 성능상의 문제나 다른 문제가 있을까요?
| "prettier": "^2.5.1" | ||
| }, | ||
| "dependencies": { | ||
| "cypress-watch-and-reload": "^1.7.4" |
There was a problem hiding this comment.
저도 devDenpendencies가 맞다고 생각했는데, devDenpendencies로 옮기니까 위와 같이 dependencies에 있어야 한다고 뜨더라구요! 공식문서에도 npm install --save-dev 가 아닌 그냥 npm install로 설명되어 있습니다!
cypress/support/commands.js
Outdated
| import { DOM } from '../../src/js/constants.js'; | ||
|
|
||
| Cypress.Commands.add('clickDigit', digit => | ||
| cy.get(`${DOM.digits} > ${DOM.digit}`).contains(digit).click(), | ||
| ); | ||
|
|
||
| Cypress.Commands.add('clickOperation', operation => | ||
| cy.get(`${DOM.operations} > ${DOM.operation}`).contains(operation).click(), | ||
| ); | ||
|
|
||
| Cypress.Commands.add('clickModifier', modifier => | ||
| cy.get(`${DOM.modifiers} > ${DOM.modifier}`).contains(modifier).click(), | ||
| ); | ||
|
|
||
| Cypress.Commands.add('checkAlertMessage', message => | ||
| cy.on('window:alert', text => { | ||
| expect(text).to.contains(message); | ||
| }), | ||
| ); | ||
|
|
||
| Cypress.Commands.add('totalShouldBe', result => cy.get(DOM.total).should('have.text', result)); |
There was a problem hiding this comment.
피드백 주신 부분들을 commands로 옮겼습니다!
| return; | ||
| } | ||
|
|
||
| if (this.$total.isClean()) { |
There was a problem hiding this comment.
분기문을 함수로 추출해서 의도를 파악하기 쉽게 했습니다!
| validateWhenClickOperation(operation) { | ||
| if (operation === OPERATION.equal && validator.isOperation(this.state.lastClickedButton)) { | ||
| alert(MESSAGE.lastCharacterMustBeNumber); | ||
| return false; | ||
| } | ||
| if (this.$total.isClean()) { | ||
| alert(MESSAGE.pleaseEnterNumberBeforeOperation); | ||
| return false; | ||
| } | ||
| if (validator.isOperation(this.state.lastClickedButton)) { | ||
| alert(MESSAGE.operationCannotBeEnteredConsecutively); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Operation 관련해서는 예외 상황이 많아서 따로 함수로 추출해서 검증했습니다!
| setState(nextState) { | ||
| this.state = nextState; | ||
| this.render(); | ||
| } | ||
|
|
||
| render() { | ||
| this.$target.innerHTML = this.state; | ||
| } |
There was a problem hiding this comment.
제가 예전에 프로그래머스 과제인 고양이 사진첩을 구현하고, 해설 블로그를 봤는데
자식 컴포넌트들은 자신의 state를 기준으로 렌더링을 진행하기 때문에 별도의 인자를 받지 않는다고 하더라구요!
이게 정답은 아니지만, 부모 컴포넌트에서 setState를 진행을 하면 그 state를 기준으로 render가 진행이 된다. 라고 생각을 하니까 조금 더 깔끔해서 이 방식을 사용하고 있습니다!
- 3개의 숫자에 대해 덧셈이 가능한 지 테스트 - 4개의 숫자에 대해 덧셈이 가능한 지 테스트
- 모든 테스트 파일에 선언할 필요없이 support/commands 파일에만 선언해주면 됨
- 더하기 곱셈, 곱셈 빼기와 같이 우선순위가 다른 연산자들이 섞였을 때의 테스트 생성
This reverts commit dae667e.
|
복잡한 연산들 예를들어 |
There was a problem hiding this comment.
현수님! 원래도 구현을 잘 해주셨는데 코드가 더 깔끔해졌네요!
테스트 코드도 성공 케이스부터 예외까지 잘 작성해주셨네요~
complexOperation.spec 에 해당하는 내용은 아직 구현하시기 전으로 인지했는데 맞을까요?
이 부분은 우선 넘어가도 좋을 것 같습니다! 미션의 취지를 넘어서는 영역인 것 같아서요~
확인하시면 머지하도록 하겠습니다!
질문에 대한 답변입니다.
-
저는 개인적으로 지금 구현하신 방법을 선호하긴 합니다. 컴포넌트가 특정 dom을 알고 있는 것보다 인자로 받는 것이 재사용에 유리하니까요~
사실 지금도 Digits, Modifiers, Operations가 하는 일이 다 같아서 DOM selector만 추가로 인자로 주입받으면 하나의 컴포넌트를 재사용해 모두 대체 가능합니다. 그리고 더 나아가 class로 구현할 필요도 없이 함수 하나로 다 가능합니다. -
우선 1번의 답변을 참고해주시구요~
사실 계산기는 하나의 '컴포넌트'로 충분한 어플리케이션이라 생각합니다.
어떤 컴포넌트로 나눌지 고민하기 이전에 컴포넌트가 무엇이고, 어떤 역할을 하기 바라는지 명확해야할 것 같습니다! -
지금 상황에선 크게 문제 없어보입니다.
- window.Function을 이용해서 구현
|
@Tanney-102 내용 확인했습니다! 다음 미션 때부터는 컴포넌트를 구분하는거에 대해서 조금 더 깊게 생각을 해봐야겠군요! 복잡한 연산 테스트로직은 꼼꼼한 리뷰 정말 감사했습니다!~ |
이번 클린코드 2기 진행하게된 정현수입니다!
기능 구현과 간단한 리팩토링을 마쳐서 우선 리뷰를 요청드립니다.
추후에 테스트 코드 추가와 리팩토링을 지속적으로 할 예정입니다.
데모 링크
🎯 기능 요구사항
🚀 추가적으로 구현해야 할 사항들
✅ 생각해볼 사항들