[클린코드 2기 위영민] 계산기 미션 #90
Conversation
+ yujo11 님의 vanilla-javascript-boilerplate 가이드를 적용해봤습니다. + Cypress 설정은 옵션으로 남겨두었습니다. + 참고 : https://github.com/yujo11/vanilla-javascript-boilerplate
There was a problem hiding this comment.
안녕하세요 영민님! 리뷰어 조은규입니다. 🙇
미션뿐만 아니라 테스트코드까지 모두 구현해주시느라 고생 많으셨습니다!
계산기가 구현만 한다면 간단하겠지만 가독성 좋은 코드로 작성하는건 쉬운 일이 아니라고 생각해요.
클린코드 미션의 시작점인 만큼 대략적인 갈피를 잡을 수 있게 피드백을 드리려고 생각하고 있어요.
질문해주신 부분부터 답변드리겠습니다!
질문사항
어떤 기준으로 파일을 분리하는 것이 좋을까?
말씀해주신 것 처럼 React에 익숙하시다 보니 `React처럼 파일 구조를 가져가도 될까?'라는 부분에 대해 의문점이 많으셨을거라 생각해요. 현재 상수 파일을 제외하고 index.js라는 파일안에 어플리케이션이 가지고 있어야하는 상태, 변화되는 상태에 따라 화면의 DOM을 조작, 각 데이터를 검증하는 등 다양한 기능들이 한 파일에 작성되어 있어요.
위는 제 생각이기에 영민님이 생각하시는 연관된 기능 단위별로 파일이나 함수를 개선해보는 것도 좋다고 생각해요.
말씀하신대로 미션이 점점 진행되며 구조가 복잡해질텐데 best practice를 찾아 작성해보시기보다 지금처럼 영민님이 생각하시는 관점대로 작성을 하고나서 이렇게 작성하면 유지보수 어렵고 기능을 확장할 때 ~ 문제가 있구나라는걸 느껴본 뒤 개선해보시는것도 추천드리고 싶어요. 미리 답을 드려도 좋겠지만 작성하고 피드백을 통해 코드를 개선해나가는 과정 속에서 영민님이 느끼는 바가 더 클거라고 생각해요.
cypress(이하, cy) 의 테스트코드 파일 관리는 어떻게 하는 편이신가요 ?
Cypress의 경우에 저도 영민님이 작성해주신 구조처럼 integration에 작성하고 있어요.
지금처럼 간단한 테스트 단위라면 calculator로 분리할 것 같아요.
게시판처럼 스크린이 많을 경우 페이지별로 폴더 구조를 나눈 뒤 해당 페이지 내부에서도 테스트할 케이스가 많다면 글 작성, 글 삭제, 글 조회 등 다양한 시뮬레이션별로 파일을 분리해서 할거에요.
테스트 코드를 작성하면서, 물론 테스트 코드 문법도 그렇고 아직 무지해서 코드가 이런건가..싶은것이, 비슷한 과정의 사칙연산 을 테스트하는 데 반복적인 코드 ? 가 너무 훤히 보입니다..
반복되는 부분을 줄이면서 가독성도 개선해볼 수 있을만한 코멘트를 아래에 남겨드렸어요 👀
또한 현재 사칙연산의 경우 특정 버튼만 가져와서, 미리 생각한 예상 값을 테스트 하는데요. 테스트를 이러한 접근으로 하는 것이 올바른 방식인가요..?
올바른 작성방식이 맞다고 생각해요.
말씀해주신대로 프로그램이 의도하지 않은 상태가 되지 않게 예외 케이스를 최대한 예측해서 테스트 케이스를 작성하는게 중요하다고 생각해요.
요구사항에 있는 테스트 케이스를 제외하고 추가적으로 예외 케이스들을 생각해보고 구현해보는게 좋다고 생각해요.
예를 들어 이미 예외처리가 된 연산자를 두번 눌렀을때 경고창을 검사한다던가 현재 두번째 수를 0으로 나누는 조건에 대해 추가적으로 테스트 해보면 좋을 것 같아요. (이외에 또 인성님이 생각하신 유저의 예상치 못한 입력이 있다면 테스트 케이스를 추가해보는 것도 좋겠네요!)
0 / 0을 하면 NaN이 뜨는 문제라거나 1 / 0을 하면 Infinity가 뜨는 문제도 있을 것 같은데 테스트 케이스에 해당 조건을 추가해서 경고창에 대한 expect 테스트 케이스를 우선 작성한 뒤 해당 테스트 케이스가 통과하게 해보는 것도 도움이 될거라 생각해요.
위 질문사항 답변 외에도 아래 코드에 추가로 남긴 코멘트 확인해주시면 되겠습니다~
아무래도 피드백에서 요청드린 부분이 많아 걱정이 되는데 너무 부담갖지 마시고 영민님이 진행하실 수 있는 만큼만 진행해주시면 되겠습니다~
다른 미션들에서도 충분히 배울 수 있다고 생각해요.
온보딩 미션인 만큼 영민님의 속도로 진행해주시면 되겠습니다.
화이팅입니다 😀
src/index.js
Outdated
| const canNotProceedOperationHandler = (clickedOperator) => { | ||
| let canNotProceed = false; | ||
|
|
||
| if ((isEmptyLeftNumber() && isEmptyRightNumber()) || (isNotEmptyOperator() && isEmptyRightNumber())) { |
There was a problem hiding this comment.
아래에 있는 isRequireCalculateFirst 처럼 조건문을 캡슐화 해보시면 더 좋아보이네요.
조건문을 파악하는데 피로할 수 있겠다고 생각이 들어요.
그리고 첫번째 조건문인 isEmptyLeftNumber() && isEmptyRightNumber()에서 isEmptyRightNumber는 어떤 의도로 함께 사용하신지 여쭤볼 수 있을까요? 👀
There was a problem hiding this comment.
안녕하세요 은규님 ! 위영민입니다. 이제막 리팩토링 시작해서 하나씩 개선해보고 리뷰 재요청해보려합니다.
isEmptyLeftNumber() && isEmptyRightNumber() 로지은 제가 생각한 계산기 로직이 원인일 수도 있겠습니다.
leftNumber( 이하, A ) , rightNumber( 이하, B ) 가 있다고 생각했을 때
A, 연산자, B 순으로 데이터가 존재한다고 보시면 될 것 같습니다.
이 때 맨 처음 연산 시에 좌항이 존재하지 않는데도 연산자가 입력이 받아지는 버그가 있었습니다.
아마도 저의 연산 방식이 A 와 B 가 있을 때 A(좌항) = A 연산 B 의 과정으로 진행되는데요. 초반에 A와 B가 초반에 null(빈값) 으로 초기화해 두었는데, 이 때 null 값임에도 불구하고 연산자를 누를 경우 연산이 진행되는 것을 막기 위해 위에 조건문이 필요하게 되었습니다.
const dispatchOperator = (newOperator) => {
operator = newOperator;
leftNumber = leftNumber ?? rightNumber; << 이 부분 초기 작동 시 비어있는 좌항 값을 위해 작성했습니다.
rightNumber = null;
paintTotalNumberDisplay();
};맨 처음 연산 시를 고려해서 = 연산을 제외한 다른 연산이 입력될 경우 A(좌항)이 null(비어있는)이라면 현재 입력중인 B(우항, 새로 입력중인 값) 으로 초기화시키는 작업이 필요하다 생각했습니다.
설명이 길어졌는데 이해가 가실지 걱정이 되네요 .. 😥
이것과 별개로, 말씀해주신 추상화 작업은 진행하는 것이 아래 부분과 같이 추상화한 일관성있고, 가독성 측면에서 좋겠다는 생각입니다 ! 수정해보고 리뷰요청 다시 보내겠습니다.
src/index.js
Outdated
| let canNotProceed = false; | ||
|
|
||
| if ((isEmptyLeftNumber() && isEmptyRightNumber()) || (isNotEmptyOperator() && isEmptyRightNumber())) { | ||
| showAlertMessage(ALERT_MESSAGES.OPERAND_FIRST_REQUIREMENT); |
There was a problem hiding this comment.
showAlertMessage로 alert를 호출해주셨네요
alert를 그대로 사용할 수 있다고 생각되는데 분리해주신 이유가 있을까요??
There was a problem hiding this comment.
아하.. ㅎㅎ 이것은 저의 개인적인 선호? 였던 것 같습니다.
코드의 양만 많아지고, 불필요한 작업일까요 ?
There was a problem hiding this comment.
기본적으로 익숙한 API를 두는게 더 명확하다고 생각해서요!
alert를 호출하면 인자로 message가 오는걸 알고있고 경고창이 뜬다는 내용도 대부분의 개발자가 알고있다고 생각되어 드린 코멘트였어요
협업하는 개발자들이 모두 인지하고있다면 그대로 두어도 상관없어요!
src/index.js
Outdated
| `${leftNumber === null ? '' : leftNumber}${operator === '' ? '' : operator}${ | ||
| rightNumber === null ? '' : rightNumber |
There was a problem hiding this comment.
operator의 삼항연산자가 의미있을지 다시 한번 생각해봅시다 🥲
그리고 병합 연산자에 대해 알아보시고 개선해보면 좋을 것 같아요
There was a problem hiding this comment.
?? 연산자, nullish coalescing operator 를 사용하면 좀 더 코드가 간편해질 수 있군요 !
각각의 세 가지 데이터들이 비어있는 상황을 고려한다고 진행해본 것인데.. falsy 한 값( 예를 들어, 결과값이 0 이 될 경우 ) 일 경우에 화면에서 결과값이 비어버리게 되는 상황이 있더군요..
그것을 해결하기위해 방법을 생각해봤는데 ?? 연산자를 사용할 생각을 못하고 || 연산자만 사용해보았다가 버그가 보여서 삼항연산자만 생각하게 되었던 것 같습니다.
falsy 값이 아닌 좀 더 구체적으로 null 또는 undefined 값을 경우를 생각한다면 병합 연산자 ??(nullish coalescing operator) 를 사용해야겠습니다.
보완해보겠습니다 !
src/index.js
Outdated
| let canNotProceed = false; | ||
|
|
||
| if ((isEmptyLeftNumber() && isEmptyRightNumber()) || (isNotEmptyOperator() && isEmptyRightNumber())) { | ||
| showAlertMessage(ALERT_MESSAGES.OPERAND_FIRST_REQUIREMENT); |
There was a problem hiding this comment.
Error을 명시적으로 throw해서 명시적으로 문제가 있다는걸 알려주는것도 좋다고 생각해요.
에러 핸들링을 통해 경고창을 띄워주는 것도 좋은 방향성이라고 생각합니다.
There was a problem hiding this comment.
생각해보니 필요없는 임시 변수 ? 를 사용하고 있는 느낌이 번쩍 들었습니다.
canNotProceedOperationHandler 함수에서는 명확하게 Error 를 throw 해주고
해당함수를 호출하는 함수에서는 try - catch 문을 통해 Error 가 발생할 경우 catch 문을 통해 유저에게는 alert 창을, 사용자(개발자 ?) 에게는 콘솔에 error 메시지까지 보여주도록 변경해보면 개선된 코드가 될 수 있겠다 생각이 듭니다 !
보완해보겠습니다 !
문득, 궁금한 부분을 앞으로 중간중간 함께 남겨보려고 합니다.
위와 같이 eslint 컨벤션 때문에 발생하는 경고 가 개발하면서 환경설정을 하다보면 여럿 만날 수 있습니다.
저는 이럴 때 (제 기준이지만) 개발하는 당시에는 무시해도 된다는 수준의 경고메시지는 config 파일에서 off 을 하기도 합니다.
이러한 부분은, 만약 실제 협업에서 팀 개발을 진행할 경우에는 팀원들과 커뮤니케이션을 통해 적정선을 찾는 편이신가요 ? 아니면 eslint 에서 설정한 규칙에 맞지 않는 형식은 아예 사용하지 않는 편인가요 ?
There was a problem hiding this comment.
eslint rule은 아무래도 프로젝트를 시작할 때 설정하고 가는 경우가 많은데요~
보통 rule을 설정할때 다 협의가 된 내용이기 때문에 off할때는 팀원들과 커뮤니케이션을 통해 제거하는게 맞다고 생각해요. 팀원과 약속한 컨벤션이니 임의로 제거하는건 맞지 않다고 생각해요.
자주 사용하는 함수에서 린트 에러를 유발할 하거나 컨벤션을 지켜 일관성을 얻는 부분보다 불편하게 느껴져서 피로감이 클때는 항상 커뮤니케이션을 하는편입니다 😄
처음 설정한 rule 그대로 사용하는 경우는 잘 없더라구요 💦 (쌓여가는 off rule들... 👋 )
| }); | ||
|
|
||
| it('두 수에 대한 덧셈이 가능한가 ?', () => { | ||
| cy.get('[data-test=btn-3]').click(); |
There was a problem hiding this comment.
data-test로 이벤트 케이스를 핸들링 해주셨네요 👍 👍
해당 라이브러리 문서에서도 best practice로 나와있는걸로 알고있는데 잘 적용해주셨습니다!
There was a problem hiding this comment.
오...!! 정확히 공식문서를 참고해보고 해당 내용을 적용해보았습니다.
https://docs.cypress.io/guides/references/best-practices
Cypress 를 떠나서 테스트 코드 작성 자체가 처음인데, 퀄리티를 떠나서 칭찬받으니 기부니가 좋아지네요 😋
감사합니다.
아직 테스트 스킬이 부족하다곤 생각하는데 궁금증이 있습니다.
위와 같이 data- 속성으로 테스트 코드에서 DOM 을 가져와야하는 경우에 HTML tag 에 일일히 data- 속성을 기재해줘야하는 걸까요 ?
더 좋은 방법이 있는지 궁금했습니다.
There was a problem hiding this comment.
번거로우시겠지만 data-속성을 전부 기재해줘야하는걸로 알고있어요 😢
src/index.js
Outdated
|
|
||
| if ((isEmptyLeftNumber() && isEmptyRightNumber()) || (isNotEmptyOperator() && isEmptyRightNumber())) { | ||
| showAlertMessage(ALERT_MESSAGES.OPERAND_FIRST_REQUIREMENT); | ||
| canNotProceed = true; |
There was a problem hiding this comment.
이런식으로 이후 조건문을 진행할 필요가 없는 경우에는 early return을 통해 더 깔끔한 코드를 만들 수 있어요
There was a problem hiding this comment.
해당 부분과 관련된 로직을 수정해보았습니다.
수정된 버전으로 push 하겠습니다.
말씀해주신 의도와 다른 부분이 있다면 말씀 부탁드립니다 !
src/index.js
Outdated
| const isEmptyLeftNumber = () => leftNumber === null; | ||
| const isEmptyRightNumber = () => rightNumber === null; | ||
| const isNotEmptyOperator = () => operator !== ''; | ||
| const isReadyCalculate = () => operator !== '' && rightNumber !== null; | ||
| const isRequireCalculateFirst = (clickedOperator) => | ||
| leftNumber !== null && operator !== '' && rightNumber !== null && clickedOperator !== '='; |
There was a problem hiding this comment.
validator라는 파일을 따로 분리해서 검증 로직을 분리해봐도 좋을 것 같아요!
There was a problem hiding this comment.
validator 모듈을 만든다면, 어느 디렉터리에 보관하시는 편인가요 ?
현재 새로 push 한 버전 기준으로
coomon ( 일반적으로 사용한다는 의미해서.. ) 디렉터리 내부에 constants 파일과 같은 depth 에 위치시켰습니다.
validator 를 떠나서, 혹시 은규님이 나름의 컨벤션으로 지키고 있는 디렉터리 구조가 있는지요..ㅎㅎ
There was a problem hiding this comment.
특정 컴포넌트에서만 사용하는 validator라면 해당 컴포넌트 폴더 주위에 있는게 편하다고 생각해요.
공통적으로 사용할 validator가 있다면 말씀하신대로 common 폴더에 정리해볼 것 같구요 ㅎㅎ
실제로 컴포넌트구조를 나눌 때 확장성을 생각해서 컴포넌트 이름을 가진 폴더를 만들고 해당 컴포넌트에 관련된 type파일, test파일, style파일같은 부가적인 파일을 만들 때 해당 폴더 내부에 생성하면 작업할 때 편해서 말씀드린 구조로 작업하고 있어요.
부가적인 공통 유틸같은 경우 utils 폴더에서 만들고 core폴더에 특정 라이브러리에 대한 폴더나 주요 기능에 관련된 모듈들을 만들어 관리하고 있구요.
폴더 구조는 프로젝트 시작 단계에서 정하기 나름이라 매번 다르겠지만 우선은 이러한 구조로 작성하고 있어요.
src/index.js
Outdated
| let leftNumber = null; | ||
| let rightNumber = null; | ||
| let operator = ''; |
There was a problem hiding this comment.
해당 계산기의 데이터를 전역 상태로 관리하다보니 많은 함수들이 이 변수에 의존성이 생긴 것 같아요.
함수가 전역변수에 의존하게되면 현재처럼 E2E 테스트에는 문제가 없지만 단위테스트를 할 때 어려워진다는 문제점이 있어요.
테스트 뿐만 아니라 데이터의 타입을 변경해서 할당하면 프로그램이 의도했던 방식대로 동작하지 않고 오류를 발생시킬 수 있어요.
total에 있는 데이터를 기반으로 데이터를 핸들링하거나 전역 상태가 아닌 class나 function을 기반으로 안전하게 변경할 수 있게 데이터를 관리하는 역할을 갖는 방식으로 구현해보면 좋을 것 같아요.
여러 함수에 나뉘어져있는 데이터 변경의 책임을 한 모듈에 옮겨서 해당 파일 내부에서만 관리하게 되면 유지보수, 가독성 측면에서 많은 개선이 있을거라 생각해요!
There was a problem hiding this comment.
해당 피드백이 이번 리뷰 중에서 가장 중요한 부분이라 생각합니다.
보완해보겠습니다.
There was a problem hiding this comment.
가장 혼란스러운 부분이라 질문을 하는 게 좋을 것 같습니다.
말씀해주신 피드백 포인트에 대해 고민을 해봤습니다.
여러 함수에 나뉘어져있는 데이터 변경의 책임을 한 모듈에 옮겨서 해당 파일 내부에서만 관리하게 되면 유지보수, 가독성 측면에서 많은 개선이 있을거라 생각해요!
- 생각해본 방식 1 :
index.js 파일에서 전역 데이터 관리 -> 클래스 or 생성자 함수 형태의 객체 안에서 구현
import { $, SELECTORS } from '../utils/selector.js';
export default function Calculator() {
this.$totalDisplay = $(SELECTORS.ID.TOTAL);
this.$digitsPanels = $(SELECTORS.CLASS.DIGITS);
this.$modifierPanel = $(SELECTORS.CLASS.MODIFIER);
this.$operationsPanels = $(SELECTORS.CLASS.OPERATIONS);
// digits panel function
this.digitsPanelHandler = (event) => {
// ...
};
// operations panel function
this.operationsPanelHandler = (event) => {
// ...
};
// AC(reset) panel function
this.modifierPanelHandler = () => {
// ...
};
this.$digitsPanels.addEventListener('click', this.digitsPanelHandler);
this.$modifierPanel.addEventListener('click', this.modifierPanelHandler);
this.$operationsPanels.addEventListener('click', this.operationsPanelHandler);
}-
생각해본 방식 2 :
각각의 컴포넌트 단위로 나누는 방식- 각각의 분리한 컴포넌트 내부에는
방법 1에서 구현한 기능들이 분할되어 구현된다는 가정하에
- 각각의 분리한 컴포넌트 내부에는
import TotalDisplay from './totalDisplay.js';
import DigitsPanels from './digitsPanels.js';
import ModifierPanel from './modifierPanel.js';
import OperationsPanels from './operationsPanels.js';
export default function Calculator() {
this.$totalDisplay = new TotalDisplay();
this.$digitsPanels = new DigitsPanels();
this.$modifierPanel = new ModifierPanel();
this.$operationsPanels = new OperationsPanels();
// ... root 컴포넌트 개념인 Calculator 컴포넌트에서는 중앙적으로 관리할 `데이터` 와 관련된 로직만 작성해놓기
}-
방법 1의 경우는
Calculator라는 객체 형태 내부에서 로직이 구현되겠지만, 실질적으로는 현재index.js의 내용이Calculator객체 내부에서 관리되는 것 외에는 큰 차이점이 없지 않은가 ? 생각이 됩니다. -
방법 2의 경우가 말씀해주신 방법에 가깝다고 생각이 드는데 확신이 들지 않습니다.
- 각각의 분리되어 import 되고, 모든 컴포넌트들이 모이는
Calculator객체( 이또한 컴포넌트 ? ) 에서는state개념과 같이 관리하는 데이터를 선언하고, 데이터와 직접적인 로직만 정의해놓고, 각각의 컴포넌트에 필요한 로직을props개념처럼 파라미터로 넘겨서 사용하도록 구현하는 방식입니다.
- 각각의 분리되어 import 되고, 모든 컴포넌트들이 모이는
생각이 깊어 가기만해서 질문을 남겨봅니다.. 😓
@EungyuCho
There was a problem hiding this comment.
2번의 형식이 제가 말씀드린 피드백과 가까워요.
2번에 작성해주신 코드처럼 UI를 기준으로 분리할수도 있겠지만 제가 생각하기엔 이번 미션에서는 오버 엔지니어링이라고 생각되요 👀 (그래도 시도해보셔도 좋아요!)
이번 미션에서 시도해보시면 좋겠다 싶었던 포인트는 전역변수에 관리되고있던 화면 구성에 필요한 데이터(계산에 필요한 변수 및 연산자)들을 별도의 클래스에 분리해보면 좋겠다고 생각했어요.
현재 코드에서 유효성 검증에 대한 부분이 많아 분리해보면 좋을 것 같고 추가적으로 앞에서 말씀드린 계산에 관련된 필요한 데이터와 함수를 갖고있는 별도의 모돌을 만들어서 계산에 관련된 책임은 온전히 해당 모듈에서 할 수 있게 만들어보면 좋을 것 같아요.
유효성 검증에 성공하면 계산에 필요한 숫자나 연산자를 계산 모듈의 필드에 set하고 해당 계산 모듈에서 calculate(현재 해당 모듈에서 가지고 있는 숫자와 연산자를 기반으로 계산)을 할 수있게 별도의 모듈로 빼면 좋겠다고 생각했어요. 이런식으로 분리하게되면 index.js에 있던 계산에 대한 책임을 외부 클래스에 위임할 수 있어서요 💦
src/index.js
Outdated
| case '+': | ||
| leftNumber += rightNumber; | ||
| break; | ||
| case '-': | ||
| leftNumber -= rightNumber; | ||
| break; | ||
| case 'X': | ||
| leftNumber *= rightNumber; | ||
| break; | ||
| case '/': | ||
| leftNumber = Math.floor(leftNumber / rightNumber); |
There was a problem hiding this comment.
Lookup Table을 통해 구현해보시는걸 추천드릴게요!
switch문보다 가독성도 좋고 분기 케이스를 모두 검사하지 않아도 되어 빠르다는 장점이 있어요
There was a problem hiding this comment.
lookup table 이란 패턴이 있었군요. 훨씬 가독성있어보입니다.
자바스크립트의 object 를 이용한 패턴인가 봅니다.
안그래도 swtich 문이 정말 코드라인도 그렇고 묵직한 느낌이 있었는데 이것을 보완할 수 있을 것 같네요 !
보완해보겠습니다.
src/index.js
Outdated
|
|
||
| const clickModifierHandler = () => { | ||
| resetNumbersAndOperator(); | ||
| $total.innerHTML = '0'; |
There was a problem hiding this comment.
textContent로 변경하면 될 것 같은데 innerHTML로 변경하신 이유가 있을까요?
추가로 textContent와 innerHTML의 차이점을 알아보시면 좋을 것 같아요 😄
There was a problem hiding this comment.
구현할 당시에는 특별한 이유가 없었습니다..
말씀해주신대로 innerHTML, innerText, textContent 의 차이점을 다시 확인해본 결과, innerHTML 의 경우는 해당 Element 의 HTML 을 가져오는 액션이 발생하네요. 텍스트 값만을 가져오는 innerText 또는 textContent 액션에 비해 무거운 작업이라고 생각하게 되었습니다. ( 맞을까요..? )
단순 텍스트만 변경하는 작업이기에 textContent 로 변경하는 것이 좋겠다는 생각이 듭니다 !
그렇다면, 미션의 경우 결과창 에 텍스트만이 변경되어 보여지는 데이터라고 생각하는데요 !
// 현재 상태 ( innerHTML )
const paintTotalNumberDisplay = () => {
$total.innerHTML= getNewTotalValue();
};
// 변경할 경우 ( textContent )
const paintTotalNumberDisplay = () => {
$total.textContent = getNewTotalValue();
};연산의 결과로 인한 결과창 을 다시 계산 그려야하는 함수의 경우에도 innerHTML 대신, 이 또한 텍스트만 변경하는 가벼운 작업이니깐 textContent 를 사용하는 것이 좋을까요 ?
There was a problem hiding this comment.
텍스트만 교체하고 별도의 스타일링(줄바꿈)이 없다면 textContent로 하시는걸 추천드려요~
말씀하신대로 textContent는 text/plain으로 파싱을 하기때문에 innerHTML보다 빨라서 textContent를 추천드려요 👍
innerHTML이 무겁다는 단점 뿐만 아니라 XSS라는 큰 문제가 있어서 지양하시는걸 추천드려요!
우회법이 있긴하지만요 ㅎㅎㅎ;
+ 삼항 연산자 -> 병합 연산자 (nollish coalescing operator) + 피연산자 필요 검사 로직 -> 함수 추상화 + 에러 핸들링 or Early Return 으로 특정 함수에 불필요한 임시변수 제거 + switch 문 -> lookup table pattern 으로 가독성과 빠른 처리 속도 효과 + innerHTML -> textContent 변경
src/index.js
Outdated
| }; | ||
|
|
||
| const clickOperationHandler = (e) => { | ||
| const clickedOperator = e.target.textContent; | ||
|
|
||
| if (canNotProceedOperationHandler(clickedOperator)) { | ||
| try { | ||
| canNotProceedOperationHandler(clickedOperator); |
There was a problem hiding this comment.
checkAbleToPressOperator 요론 네이밍은 어떨까요?
현재 오퍼레이터가 누를 수 있는 상태인지 검사하는 유효성 검사 메서드라고 생각되어 check라는 prefix를 붙혀 네이밍해봤어요
There was a problem hiding this comment.
적절한 네이밍 생각해내는 것도 쉽지 않네요.
은규님은 네이밍 결정하는 기준 같은 것이 있으실까요 ?
There was a problem hiding this comment.
우선 동사를 prefix로 사용해서 이 함수가 어떤 행동을 할것인지 정해요.
동사만 잘 정하면 뒤에있는 해당 행동에 대한 목적어는 자연스럽게 따라온다고 생각해요. 👀
canNotProceedOperationHandler로 지어주신 함수를 보게되면 validation에 관한 prefix인 check를 우선 확정지었고 연산자를 누를 수 있는 상태인지 체크하는 네이밍을 뒤에 덧붙혀서 checkAbleToPressOperator라는 함수명이 나왔어요
src/index.js
Outdated
| const calculateExpression = () => { | ||
| switch (operator) { | ||
| case '+': | ||
| const expressionCalculate = () => { |
src/index.js
Outdated
| if (isRequireOperandFirst()) { | ||
| throw new Error(ALERT_MESSAGES.OPERAND_FIRST_REQUIREMENT); | ||
| } | ||
|
|
||
| if (isRequireCalculateFirst(clickedOperator)) { | ||
| showAlertMessage(ALERT_MESSAGES.CALCULATE_FIRST_REQUIREMENT); | ||
| canNotProceed = true; | ||
| throw new Error(ALERT_MESSAGES.CALCULATE_FIRST_REQUIREMENT); |
There was a problem hiding this comment.
세부적인 구현사항에 집중하지 않고 추상화를 통해 코드를 읽기 수월해졌어요!
그리고 error를 명시적으로 던져 문제가 있단걸 파악할 수 있게되었네요 👏
+ 하나의 index.js 형태 -> UI 별로 모듈 분할
+ Cypress Custom Command 로 Command 추상화
There was a problem hiding this comment.
피드백 반영하시느라 고생하셨습니다 영민님 😄
이전에 하나의 모듈에서 책임지던 구조에서 적절히 컴포넌트별로 역할을 나눠주셔서 각 컴포넌트가 해야할 이벤트에 대한 콜백이나 상태를 선언적으로 작성해주셔서 코드를 읽는데 어려움이 없었어요. 유지보수면에서도 보다 수월해졌다고 생각해요.
이번 계산기 미션에서 요구하는 조건도 다 충족하셨고 구조를 개선하는 과정을 경험해보셔서 바로 다음미션으로 넘어가면 되겠습니다
고생하셨어요! merge 하겠습니다~
(혹시 궁금하신점이 더 있으시다면 코멘트로 대화 이어갈게요! 👋 )
| context('Confirm Component initial render ', () => { | ||
| it('total display', () => { | ||
| cy.getTotalDisplay().contains(0); | ||
| }); | ||
|
|
||
| cy.get('[data-test=total]').contains(1); | ||
| }); | ||
| it('Number panels', () => { | ||
| cy.getNumberPanel(0).contains(0); | ||
| cy.getNumberPanel(1).contains(1); | ||
| cy.getNumberPanel(2).contains(2); | ||
| cy.getNumberPanel(3).contains(3); | ||
| cy.getNumberPanel(4).contains(4); | ||
| cy.getNumberPanel(5).contains(5); | ||
| cy.getNumberPanel(6).contains(6); | ||
| cy.getNumberPanel(7).contains(7); | ||
| cy.getNumberPanel(8).contains(8); | ||
| cy.getNumberPanel(9).contains(9); | ||
| }); | ||
|
|
||
| it('두 수에 대한 곱셈이 가능한가 ?', () => { | ||
| cy.get('[data-test=btn-3]').click(); | ||
| cy.get('[data-test=btn-mul]').click(); | ||
| cy.get('[data-test=btn-2]').click(); | ||
| cy.get('[data-test=btn-equal]').click(); | ||
| it('Operator panels', () => { | ||
| cy.getOperatorPanel('+').contains('+'); | ||
| cy.getOperatorPanel('-').contains('-'); | ||
| cy.getOperatorPanel('X').contains('X'); | ||
| cy.getOperatorPanel('/').contains('/'); | ||
| cy.getOperatorPanel('=').contains('='); | ||
| }); | ||
|
|
||
| cy.get('[data-test=total]').contains(6); | ||
| it('Modifier panel', () => { | ||
| cy.getModifierPanel().contains('AC'); | ||
| }); |
There was a problem hiding this comment.
첫 렌더링 때 계산기의 UI를 테스트를 해주셨는데 유의미한 테스트일까요??
렌더링때 바뀔 여지가 없어보이고 만약 UI 요구사항이 바뀐다고해도 맞춰서 테스트까지 변경해줘야하는 번거로움이 있어서 의미가 있을지 의문이 들어요.
There was a problem hiding this comment.
렌더링때 바뀔 여지가 없어보이고 만약 UI 요구사항이 바뀐다고해도 맞춰서 테스트까지 변경해줘야하는 번거로움이 있어서 의미가 있을지 의문이 들어요.
맞는 말씀인 것 같습니다.
테스트 코드의 의미가 살아있지 않네요.
사용해야할 UI 가 정상적으로 잘 가져와 지는가 ? 확인해보는 사전 과정이라 생각하고 작성해보았는데요.
UI 요구사항이 바뀌면 단순한 이유로 해당 테스트 코드가 변경되어야 된다는 적절하지 못한 이유가 있을 지 생각 못했네요 !
| this.$totalDisplay = new TotalDisplay({ | ||
| initState: this.state.total, | ||
| }); | ||
|
|
||
| this.$modifierPanels = new ModifierPanel({ | ||
| onClick: () => { | ||
| this.setState({ | ||
| ...this.state, | ||
| total: INITIAL_VALUES.INITIAL_TOTAL, | ||
| operand: INITIAL_VALUES.INITIAL_OPERAND, | ||
| operator: INITIAL_VALUES.INITIAL_OPERATOR, | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| this.$digitsPanels = new DigitsPanels({ | ||
| onClick: (keypadNumber) => { | ||
| try { | ||
| validators.operandValidtor.checkOperandSize(this.state.operand); | ||
| validators.operandValidtor.checkImmediateAfterCalulate(this.state); | ||
| } catch (error) { | ||
| alert(error.message); | ||
| return; | ||
| } | ||
|
|
||
| this.setState({ | ||
| ...this.state, | ||
| total: this.state.total + keypadNumber, | ||
| operand: this.state.operand + keypadNumber, | ||
| operator: INITIAL_VALUES.INITIAL_OPERATOR, | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
함수형 컴포넌트 작성에 익숙하신 것 같아요!
리액트스러운 컴포넌트들로 변경되었네요 😄
There was a problem hiding this comment.
이전에 말씀하신대로 이번 미션은 굳이 UI 기준으로 분리할 필요가 있을까 ? 생각했습니다.
유효성 검사 모듈과, 데이터만 관리하는 모듈, 계산만 전담하는 모듈로 분리해서 작성하면 좋을 것 같다는 피드백을 주셨는데요.
그와 같이 구현을 할 경우, 코드 분할을 어떻게 해야할 지 감이 오질 않았습니다.
때문에, 오버엔지니어링이라 생각할 수 있겠지만, 현재 수준에선 확실히 구조를 분리할 수 있는 (말씀하신 리액트스러운 ?) 방식으로 구현해보았습니다.
감사합니다 ! 😃
There was a problem hiding this comment.
유효성 검사 모듈과, 데이터만 관리하는 모듈, 계산만 전담하는 모듈로 분리해서 작성하면 좋을 것 같다는 피드백을 주셨는데요.
이 피드백 자체가 특정 모듈에 대한 구조를 나누는 것 보다 너무 큰 앱을 분할해보면 좋겠다는 생각이 커서 드린 코멘트여서.. 😅
이번에 구조를 나눠보는 연습만으로 충분합니다!
다음 미션에서 로또 관리에 대한 책임을 지는 모듈을 만들어보시면 이해가 되실거라 생각해요. 로또를 생성할 때 요구되는 조건들이 많아 이번 미션보다 수월하게 작성하실수 있을거라 생각해요. 😄
There was a problem hiding this comment.
로또 미션을 진행해봐야 말씀해주신 이슈를 정확히 알 수 있을 것 같습니다..ㅠㅠ
다음 미션도 은규님께서 멘토가 되신다면, 많이 질문해보겠습니다 !
고생하셨습니다. 😃
클린코드 2기 위영민입니다.
잘 부탁드립니다 🙂
주안점
🎯 기능 요구사항
🎯 테스트 요구사항
기능 요구사항에 대한 테스트 코드
질문사항
비교적규모가 작은 미션이었기 때문에 파일을 효과적으로 분리하는 작업이 이뤄지지 않았습니다. 그러므로 소신껏함수를 어떻게하면 잘 분리할 수 있을까 ?,함수의 depth 를 어떻게 줄일 수 있을까 ?을 중점으로 1차적인 구현을 해보았습니다.state개념을 유지하면서클래스or생성자 함수를 통해 컴포넌트 기반으로 파일을 분리한 형태라 보시면 되겠습니다.어떤 기준으로 파일을 분리하는 것이 좋을까 ?에 대한 해답이 아직은 떠오르지 않습니다.(cf. 개인적으로 드는 생각은 이래서 React 같은 라이브러리의 구현 형태에만 익숙해져서 본질을 잘 구분하는 방법이 부족하다고 생각이 드네요..자꾸 익숙하게 사용하는 React 의 작동방식과 비슷하게만 만들려고하는 습관?이 보이는 것 같습니다.)
테스트 코드 작성하는 것 자체가 처음이라 Cypress 가 아니어도 테스트에 대해 낯설은 것은 어쩔 수 없는 것 같네요..ㅎㅎ 많이 해보는 것 많이 정답인 것 같습니다. 1차적으로 테스트코드를 작성해보면서 생긴 궁금증을 남겨봅니다.
src디렉터리 내부에서 특정 컴포넌트 파일depth 와 동일한 곳에 생성해서 관리하기도 하더라구요 !calcuator(계산기)라는 하나의 작은 주제만 구현하는 것이어서calculator.spec.js라는 이름으로 파일 존재하는데요. 예를 들어게시판이라는 프로젝트를 진행한다면 어떤 기준으로 테스트 파일을 분할 하는게 좋을까요 ?글 등록 페이지,글 목록 페이지.. 처럼페이지단위로 나눌까요 ? 아니면 명확한 기준이 있다기보다 테스트 크기의 따라 분할 하나요 ?현재 작성된 테스트 코드를 작성하면서, 물론 테스트 코드 문법도 그렇고 아직 무지해서 코드가 이런건가..싶은것이, 비슷한 과정의
사칙연산을 테스트하는 데 반복적인 코드 ? 가 너무 훤히 보입니다.. 이렇게 작성하면 재활용성이 전혀 지켜지지 않은 코드인지 궁금합니다..또한 현재 사칙연산의 경우
특정 버튼만 가져와서, 미리 생각한 예상 값을 테스트하는데요. 테스트를 이러한 접근으로 하는 것이 올바른 방식인가요..? 저의 머리에서 정의한 특정 기능의 테스트 케이스가 현재는덧셈이라는 비교적 간단한 주제이지만, 이 또한 혹시 모를 다른 테스트케이스에선 에러가 발생할 수도 있다 생각이 들어서요 !