-
Notifications
You must be signed in to change notification settings - Fork 88
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
[호프-도리] 계산기 미션 제출합니다. #43
[호프-도리] 계산기 미션 제출합니다. #43
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나름 리팩토링 한다고 했는데 피드백 받고 나니 바꿀 곳이 생각보다 많네요....ㅎ
} | ||
|
||
const isStartZero = (number, newNumber) => { | ||
return number.length === 0 && newNumber === '0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return만 할 경우 const validateNumber = (number) => typeof number === 'number'로 수정 가능
TOTAL:'#total' | ||
} | ||
|
||
const ERROR = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예약어와 비슷한 네이밍을 사용을 지양하기
if(!validateNumberLength(this.right)) return alert(ERROR.DIGIT_LENGTH); | ||
if (isStartZero(this.left, number)) return alert(ERROR.START_ZERO); | ||
this.right += number; | ||
total.innerHTML+= number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
innerHTML대신 insertAdjacentHTML을 사용하기
import './commands' | ||
|
||
// Alternatively you can use CommonJS syntax: | ||
// require('./commands') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 주석은 삭제하고, 불필요한 파일들은 삭제 또는 .gitignore를 통해 올리지 않기
operationClick("="); | ||
checkAlertMessage(ERROR.WRONG_EXPRESSION); | ||
}); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EOL 제거
'/' : (left,right) => {if (isValidateInput(left, right)) return Math.floor(left / right);} | ||
} | ||
|
||
export { validateNumberLength, isStartZero, calculate} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EOL 제거
cy.on('window:alert', (str) => { | ||
expect(str).to.equal(message) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오류가 alert인지 테스트코드에서는 알 수 없기 때문에 stub을 사용하기
|
||
const SELECTOR = { | ||
CALCULATOR: '.calculator', | ||
TOTAL:'#total' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일정한 기준의 constant를 생성하기
if (eventClassName === CLASS.OPERATION) { | ||
return this.operationHandler(event.target.innerHTML); | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조금 더 작은 단위의 listener를 사용하기
digitHandler(number) { | ||
const total = document.querySelector(SELECTOR.TOTAL); | ||
if (!this.operator) { | ||
console.log(this.operator, this.left) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log 지우기!
페어프로그래밍이 처음이라서, 시간이 조금 오래걸리기도 했지만, 혼자 할 때보다 두배는 더 빠르게 에러를 해결하고 좋은 코드에 대한 고민을 할 수 있었던 시간이었습니다 :)
다만 아쉬운점은 테스트코드를 추후에 작성해서 비효율적인 수동 테스트를 거쳐야했는데요, 다음 미션에서는 꼭 테스트 먼저 작성하는 TDD를 도입해보고싶습니다.