[클린코드 4기 우경준] 계산기 구현 #167
[클린코드 4기 우경준] 계산기 구현 #167
Conversation
There was a problem hiding this comment.
안녕하세요, 경준님! 리뷰어 소인성입니다.
리뷰가 늦어져서 죄송합니다🙇♂️
전체적인 구조화에 신경을 많이 쓰셨네요. 몇 가지 리뷰 남기니 확인해주세요!
중요한 로직들이 선언적으로 읽힐 수 있도록 신경썼습니다.
- 이 부분을 많이 공들이셨는데, 결국 고도화된 선언적 코드는 자바스크립트 네이티브를 표방하게 됩니다. 간단한 규모의 앱에서 래핑된 요소가 많이 보였어요. 그 외에는 잘 읽히고, 의도를 알 수 있는 코드들이라고 생각 됩니다😁
불필요한 부분들이 노출되어 복잡한 코드가 되는 것을 줄이기 위해 노력했습니다.
- 확실히 외부에서 의존하거나 사용하는 함수, 변수에 대한 일관성을 지키기 위해 노력해주셨는데, 그러다보니 각 파일 내부에서의 복잡도가 커진 느낌이 있어요. 너무 파편화 되어 있어 어떤 객체에 어떤 함수들이 있다던가, 함수명과는 다른 기능을 한다던가... 해당 내용은 리뷰로 확인해주세요!
가독성은 괜찮은지? 혹은 읽기에는 불편함이 없었는지?
- detph를 깊게 가져가지 않기 위해 리팩토링을 진행하거나 고민한 부분이 많이 보였습니다.
변수나 파일 이름들이 직관적이었는지
- 괜찮았습니다! 다만 자연스러운 느낌보다는 경준님의 컨벤션을 지키려고 조금 더 길어진 느낌이 있습니다.
코드 스타일에 대한 리뷰가 많은 것이며 그 외적으로는 크게 문제가 되는 부분이나 잘못이라고 여겨지는 곳은 없었어요!
미션 수행하느라 고생 많으셨습니다😎
| cy.get('#digits').contains('4').click(); | ||
| cy.get('#digits').contains('5').click(); | ||
| cy.get('#operations').contains('+').click(); | ||
| cy.get('#digits').contains('1').click(); | ||
| cy.get('#digits').contains('0').click(); | ||
| cy.get('#operations').contains('=').click(); | ||
|
|
||
| cy.get('#total').should('have.text', 55); |
There was a problem hiding this comment.
cy.get('#digits').contains('4').click(); 의 함수 라인이 중복되어 특정 테스트를 구현할 때 상당히 피로할 것 같아요. custom command를 통해 계산식을 입력 하는 함수를 만들면 어떨까요?
cy.inputOperation('45', '+', '10').then((total) => total.should('have.text', 55));형태처럼요!
또한 셀렉터를 상수화하여 상단에 정의하면 좀 더 직관적으로 읽을 수 있을 것 같아요!
| <button class="operation">/</button> | ||
| <button class="operation">X</button> | ||
| <button class="operation">x</button> | ||
| <button class="operation">-</button> |
| digitViews.forEach((digitView) => { | ||
| digitView.onClick((e) => { | ||
| const buttonNumber = e.target.textContent; | ||
|
|
||
| appendNumberToCurrentNumber(buttonNumber); | ||
| totalView.appendTextContent(getCurrentNumber()); | ||
| setIsOperateState(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
View 클래스에서 많은 게 할당되는 것 같네요! onClick이 마치 자바스크립트 네이티브한 이벤트 프로퍼티 할당 방식처럼 느껴지기 때문에, 함수명을 바꾸는 것을 추천드려요!
그리고 과한 래핑 느낌이 드는데... 아래에서 계속 리뷰하겠습니다!😊
| modifierView.onClick((e) => { | ||
| initStore(); | ||
| totalView.appendTextContent(getCurrentNumber()); | ||
| }); |
There was a problem hiding this comment.
e 파라미터가 사용되지 않으면 생략해주는 것이 좋을 것 같아요!
| totalView.appendTextContent(getCurrentNumber()); | ||
| setIsOperateState(); |
There was a problem hiding this comment.
숫자만 눌러도 store 의 연산식 여부를 지속적으로 수정하네요. false, false, false, false... 반대로 말하자면 연산식을 여러번 누르면 true, true, true, true... 이런 프로세스를 조금 개선해볼 수 없을까요?
There was a problem hiding this comment.
그러네요! 불필요한 set이 있네요!
받아들인 parameter가 기존 state와 다를때만 set하는 방식으로 대응 할 수 있을 것 같습니다 👍
| const newCurrentNumber = Number(String(currentNumber) + newNumber); | ||
|
|
||
| if (checkIsValidNewCurrentNumber(newCurrentNumber)) return; | ||
|
|
||
| setCurrentNumber(Number(String(currentNumber) + newNumber)); |
There was a problem hiding this comment.
문자열 템플릿을 이용하면 아래도 좋을 것 같아요!
const newCurrentNumber = Number(`${currentNumber}${newNumber}`);
if (checkIsValidNewCurrentNumber(newCurrentNumber)) return;
setCurrentNumber(newCurrentNumber);| export function setIsOperateState(newState) { | ||
| isOperateState = newState || false; | ||
| } |
There was a problem hiding this comment.
newState에 OR 연산자 대신 default paramter를 주면 좋을 것 같네요😊
| export function initCurrentNumber() { | ||
| currentNumber = defaultCurrentNumber; | ||
| } | ||
|
|
||
| function initIsOperateState() { | ||
| isOperateState = defaultControlState; | ||
| } |
There was a problem hiding this comment.
상단의 setCurrentNumber 함수는 currentNumber를 반환하는데, 일관된 동작을 개발자에게 주는 것이 더 좋아보입니다. return 구문을 추가해보면 어떨까요?
There was a problem hiding this comment.
음,,, 그러네요!
확실히 일관성이 없네요,,, 감사합니다 👍
| } | ||
|
|
||
| #validateSelector(selector) { | ||
| if (selector && typeof selector !== 'string') throw new Error('event selector only can be string'); |
| #getTargetElementWithSelector(selector) { | ||
| return selector ? this.rootElement.querySelector(selector) : this.rootElement; | ||
| } |
There was a problem hiding this comment.
selector default 파라미터를 this.rootElement로 넣어도 좋을 것 같네요!
안녕하세요 리뷰어님!
정말 엄청나게 늦어버렸지만 ㅠㅠ 이제서야 완성해서 드립니다.
잘부탁드리겠습니다. 🙏
신경썼던 부분
선언적 코드
중요한 로직들이 선언적으로 읽힐 수 있도록 신경썼습니다.
불필요한 부분이 노출되지 않도록 노력
불필요한 부분들이 노출되어 복잡한 코드가 되는 것을 줄이기 위해 노력했습니다.
View의 interface를 일정하게 유지하고, DOM api를 노출하지 않기 위해 노력
View는 반드시 DOM api에 의존할 수 밖에 없는데, View 이외의 로직에서도 DOM api에 의존하게 되면, 코드 변경시, 불필요하게 변경되는 부분이 발생합니다.
따라서, DOM api는 View 내부적으로만 해결할 수 있도록 하며, View의 interface를 일정하게 유지해, 어떤 instance를 재사용해도 기존 코드는 수정하지 않아도 되도록 했습니다.
걱정되는 부분
항상 어떤 PR을 올릴때나 마찬가지이지만,
등등등...의 정말 클린코드를 위한 고민을 합니다!
잘 부탁드리겠습니다! 🙏