[클린코드 2기 소인성] 로또 미션 STEP1 #83

InSeong-So · 2022-03-19T06:11:23Z

안녕하세요, 클린코드 2기 소인성입니다.
잘 부탁드려요!😁

1차 피드백

Cypress 테스트 함수를 대폭 수정했습니다.
- Command를 추가하고 상황에 맞는 케이스를 늘렸습니다.
컴포넌트를 일관되게 수정했습니다.
렌더링과 이벤트 바인딩을 담당할 함수를 작성했습니다.
- 현재 네이밍이 이상할 수 있습니다. 엘리먼트를 반환하는데 렌더링은 옳지 않은데 라는 생각이 들고 있어요.
valid 함수의 조건문을 변경했습니다.
- 에러 메세지를 상수화하였습니다.

구현

실행화면 바로가기

웹 VSCode로 보기

기능 요구사항

로또 구입 금액을 입력하면, 금액에 해당하는 로또를 발급해야 한다.
로또 1장의 가격은 1,000원이다.
소비자는 자동 구매를 할 수 있어야 한다.
복권 번호는 번호보기 토글 버튼을 클릭하면, 볼 수 있어야 한다.

테스트 요구사항

위 기능들이 정상적으로 동작하는지 Cypress를 이용해 테스트한다.

질문

cypress의 stub 함수를 사용해 구현한 기능을 테스트하려고 했는데... document를 찾아도 잘 적용을 못하겠네요. 이 부분에 대해 예제나 좋은 방법론을 소개 받을 수 있을까요?

Tanney-102

인성님 안녕하세요!
로또 미션 리뷰어 오태은입니다!

전반적으로 역할에 맞게 모듈을 잘 분리해주셨고, 코드도 깔끔하게 잘 작성해주셔서 읽기 좋았습니다!
다만 컴포넌트들을 조금 더 일관성 있게 작성해주셨으면 어땠을까 싶네요~
어떤 컴포넌트는 DOM 엘리먼트를 반환하고 어떤 컴포넌트는 단순히 유틸함수만을 포함하는 객체를 반환하기도 합니다.(이 경우 사실 컴포넌트라고 불러도 될지 모르겠네요~)
인성님이 정의하시는 컴포넌트는 무엇인지 먼저 정리해보시면 좋을 것 같습니다

자세한 코멘트 남겼으니 확인 부탁드려요!

Tanney-102 · 2022-03-19T10:48:38Z

cypress/integration/app.test.js

+const lotto = {
+  numbers: [],
+  getNumbers: count => {
+    const array = [


testSet으로 사용될 array인가요? 사소한 로직이더라도 분명한 이름을 주는 습관을 가지시면 좋을 것 같습니다!

purchasedNumber로 변경해보겠습니다!👍

purchaseNumbers 제안해봅니다 ㅎㅎ 배열이라!

테스트를 수정하다 보니 mock 함수의 필요성을 느끼지 못해 제거했습니다!

Tanney-102 · 2022-03-19T10:56:44Z

cypress/integration/app.test.js

+
+const lotto = {
+  numbers: [],
+  getNumbers: count => {


메서드인데 화살표함수를 사용하신 이유가 있나요?

그리고 getNumbers가 이름과는 다르게 많은 일을하고 있네요.
일반적으로 get~ 라는 네이밍은 특정 값이나 멤버에 대한 접근자를 의미하는데 멤버를 초기화하는 로직까지 포함하는 것 같습니다.

cypress mock 테스트를 해보려다가 실패하면서 시간에 쫓겨 커밋했네요. 일관된 책임 부여와 역할 분리를 더 신경 쓰겠습니다. 그리고 화살표 함수를 너무 남용했네요😅

이미 알고 계실지도 모르겠지만 기왕 언급된거 function 키워드 vs 화살표 함수 vs 메서드 축약표현의 차이를 정리해보시면 좋겠습니다!

Tanney-102 · 2022-03-19T10:59:37Z

cypress/integration/app.test.js

+    for (let i = 0; i < count; i++) {
+      lotto.numbers.push(array[i]);
+    }


getNumbers가 화살표 함수가 아니었다면 this.numbers로 나타낼 수 있었을 것 같아요!

array[i]는 배열아닌가요?
현재 상황에서는 크게 상관 없긴 하겠지만 객체나 배열의 참조값을 직접 전달하는 것은 좋은 습관이 아닙니다.
같은 참조 값이 여러 곳에서 관리되면 언제 어떻게 변경될지 예측할 수 없으니까요~

Suggested change

for (let i = 0; i < count; i++) {

lotto.numbers.push(array[i]);

}

for (let i = 0; i < count; i++) {

lotto.numbers.push([...array[i]]);

}

맞는 말입니다! 현재 이 함수는 상수로써 사용하려고 만들어둔 변수라 참조값을 사용했고, 몇 천개가 넘어가는 배열을 넣으려고 한 게 아니라 빠르게 쓰는 것만 생각했어요. 조심해야겠습니다🙇‍♂️

Tanney-102 · 2022-03-19T11:10:48Z

cypress/integration/app.test.js

+  fail: () => {
+    throw new Error('fail get number');
+  },


fail은 언제 호출되나요?

fail 테스트 커맨드도 만들어 두었다가... 사용을 제대로 못해 제거했는데 선언부를 놓쳤네요😅 mock 테스트를 좀 더 찾아보고 적용해볼게요!

Tanney-102 · 2022-03-19T11:14:23Z

cypress/integration/app.test.js

+    it('로또 구입 금액을 입력하면, 금액에 해당하는 로또를 발급해야 한다.', () => {
+      cy.get('[data-props="amount-input"]').type('3000');
+      cy.get('[data-props="confirm-button"]').click();
+      cy.get('[data-props="count-span"]').should('have.text', '3');
+    });


실제 로또 티켓(.lotto-numbers-wrapper > span)은 확인 안해도 괜찮을까요?

.lotto-numbers-wrapper > span은 소비자는 자동 구매를 할 수 있어야 한다.에서 확인하기 위해 분리해두었습니다. 하지만 테스트 코드 자체를 제대로 작성하지 못해 혼란이 야기된 것 같네요😅

금액에 해당하는 로또 구매 와 자동구매라는 표현을 비교해봤을때 구체적으로 어떤점이 다른지 조금 헷갈립니다!
발급한 로또 수를 보여준다 라든지 실제 테스트 되는 내용을 적어주면 어떨까요?

Tanney-102 · 2022-03-19T12:15:25Z

src/js/helper/utils.js

+import { isSpecial } from './valid.js';
+
+export const validCount = amount => {
+  if (isNaN(amount) || isSpecial(amount)) throw new Error('숫자를 입력해주세요.');


isSpecial이 꼭 필요한 validation일까요?
만약 음수와 소수를 거르기 위함이라면 L5와 L7로 충분할 것 같습니다

+1 이라는 문구가 허용되므로 추가했습니다! 그 외의 특수문자는 isNaN에서 걸러지긴 합니다 ㅎㅎ

Number로 형변환을 하는데도 예외처리를 해주어야하나요? 🤔
혹시 필요하다고 하더라도 필요한 부분에 대해서만 정규식을 작성해주는 게 어떨까요?

Tanney-102 · 2022-03-19T12:16:34Z

src/js/helper/utils.js

+  const count = amount / 1000;
+  if (!Number.isInteger(amount / 1000))
+    throw new Error('로또 구입 금액을 1,000원 단위로 입력해 주세요.');
+  return Math.trunc(count);


여기까지 도달하면 이미 integer일텐데 trunc가 필요한가요?

const count = amount / 1000; if (!Number.isInteger(count)) throw new Error('로또 구입 금액을 1,000원 단위로 입력해 주세요.'); return count;

이게 맞겠네요!

Tanney-102 · 2022-03-19T12:19:56Z

src/js/helper/valid.js

@@ -0,0 +1,7 @@
+import { REGEXP_SPECIAL } from '../constants.js';
+
+export const isNull = value => value === null || value === undefined;


함수 이름과 로직이 잘 호응하지 않는 것 같습니다.
사용처에서 isNull이라는 이름을 보고 undefined가 걸러진다는 사실까지 예측해야하는 걸까요?

lodash에도 isUndefined로 분리해놨네요! 수정하겠습니다.

말씀하신 lodash에서는 null과 undefined를 함께 확인해주는 메서드로 isNil을 정의하긴합니다! 참고하시면 좋을 것 같아용

Tanney-102 · 2022-03-19T12:20:36Z

src/js/helper/valid.js

+
+export const isNull = value => value === null || value === undefined;
+
+export const isEquals = (target1, target2) => target1 === target2;


꼭 함수로 정의해야하는 로직인지 잘 모르겠습니다.

맞습니다! 이 부분은 과도한 리팩토링이라고 생각 되네요.

Tanney-102 · 2022-03-19T12:22:01Z

src/js/helper/valid.js

+
+export const isEquals = (target1, target2) => target1 === target2;
+
+export const isSpecial = value => REGEXP_SPECIAL.test(value);


isSpecial이라는 이름은 좀 모호하네요~
일반적으로 special이라는 표현을 보고 특수문자를 떠올릴 수 있을까요?

어떤 네이밍이 좋을까요? isSpecialCharacter를 고민하다가 줄였는데 네이밍을 어떻게 지어야 할까 답이 나오진 않았습니다🙄

isSpecialCharacter 로만 네이밍 되어도 훨씬 이해하기 쉬울 것 같아요!

혹은 special symbol이라고도 표현하는 것 같습니다!

- 도메인을 분리하고 안정 장치를 마련한다.

- 유틸 함수는 도메인에 상관이 없어야 하므로 도메인 로직으로 분리한다.

- 컴포넌트는 일관된 동작만을 가진다.

SINHOLEE

클래스형과는 또 다른 느낌의 스타일이네요. 배워갑니다.

SINHOLEE · 2022-03-22T16:00:17Z

src/js/helper/utils.js

@@ -0,0 +1,10 @@
+export const safeExecutor =


safeExcutor 참신하군요. 해당 방식은 유저경험이 좋아지겠네요.

SINHOLEE · 2022-03-22T16:00:38Z

src/js/helper/dom.js

+};
+
+// TODO: DOM Parser를 만들어 렌더링을 루트 앱에서 진행합니다.
+export const $eventBindedComponent = getElement => args => {


여기엔 safeExcutor 안입힌 이유가 있을까요?

한 번 커링을 적용한 녀석이라 너무 깊어지지 않을까 하는 마음에 분리해뒀습니다ㅎㅎ

SINHOLEE · 2022-03-22T16:04:04Z

src/js/components/App.js

+  const $events = [
+    {
+      type: 'submit',
+      callback: event => {


여기 callback에도 safeExcutor안붙인 이유가 있을까용?

여기는 미처 생각하지 못했네요...! $addEvent 부분에 적용해봐야겠네요☺

SINHOLEE · 2022-03-22T16:13:53Z

src/js/components/App.js

+          const lottoList = LottoList(count);
+          const lottoCheck = LottoCheck();
+
+          $('.lotto-section').replaceChildren(lottoList, lottoCheck);


$('.lotto-section') 를 두 번 함수실행 하는것보다,

$elementRemoveClass($('.lotto-section'), 'hidden').replaceChildren(lottoList, lottoCheck);

이게 더 함수형 같은 느낌의 코드일거같아여

이렇게 바꿀게요! 체이닝을 할 수 있음에도 생각하지 못했습니다🙇‍♂️

SINHOLEE · 2022-03-22T16:22:25Z

src/js/services/lotto.js

+import { AMOUNT_UNIT, ERROR_MESSAGE } from './constants.js';
+import { isEmpty } from '../helper/index.js';
+
+const useLottoService = () => {


혹시 리액트처럼 훅 스럽게 만드신 이유가 있을까요?
결국 lottoService에 public메서드 두개와 private 메서드 하나를 지닌 서비스 함수라서. 파일 단위로 모듈화 하면 똑같은 효과일거같은데 따로 의도한 내용이 있나 궁금합니다.

프로젝트가 깊어지거나 DOM 렌더링을 "변경 사항만 비교하며" 작업할 때를 대비한 네이밍이에요! 훅처럼 만들어두고 외부에 useCallback 등의 메모이제이션 함수로 이를 개선해보고자 했습니다 ㅎㅎ 하지만 현재의 상태는 시노리님 말대로 클래스형 모듈이나 객체가 더 맞아요!

아하 최대한 리액트와 친근하게 접근하려는 의도였군요.

SINHOLEE · 2022-03-22T16:24:52Z

src/js/services/lotto.js

+    return Array.from(numbers);
+  };
+
+  const purchasesLotto = number => {


그리고 만약 확장 가능하게 만들것 같으면 getLottoNumbers를 lottoAlgorithm이라고 바꾸고, purchasesLotto에서 인자로 알고리즘을 받는 형식으로 해서 로또 넘버를 구하는 방법은 purchasesLotto와 의존성을 약하게 관리하는게 유연한 코드가 될거같아요. 지금 당장은 오버엔지니어링 이겠네요 ㅋㅋㅋㅋ큐ㅠㅠㅠㅠ

아니에요! 그런 세세한 생각을 했어야 현재 코드에도 잘 녹아났을텐데... 네이밍부터 이 부분은 제 미스입니다 ㅎㅎ

SINHOLEE · 2022-03-22T16:31:23Z

src/js/helper/dom.js

+    const [$element, $events = []] = getElement(args);
+    if (!isDOMElement($element)) throw new Error('첫 번째 인자로 DOM 요소를 선언해주세요.');
+    $events.forEach(({ type, callback }) => $addEvent($element, type, callback));
+    return $element;


계속 뭔가 $element에 데코레이팅 혹은 HOC느낌의 동작을 하는 함수들이네요. 이게 함수형인가? 뭔가 아웃풋은 결국 $element라는 일관성이 가독성을 높여주는 느낌입니다.

Tanney-102

인성님! 확인이 늦어 죄송합니다ㅠㅠ
피드백 반영 뿐만 아니라 많은 부분에서 개선이 있었네요!
safeExecutor를 구현하신 게 인상 깊습니다! ㅎㅎ
component들도 이제 나름 일관성을 갖추게 되어 좋아보이네요~
이만 머지할테니 다음 스텝 진행하시면 되겠습니다 ㅎㅎ

InSeong-So · 2022-03-23T00:29:55Z

클래스형과는 또 다른 느낌의 스타일이네요. 배워갑니다.

리뷰 부탁 들어줘서 고마워욯ㅎㅎ🙇‍♂️

InSeong-So added 9 commits March 17, 2022 01:33

🔬Chore: 개발 환경을 설정한다.

4c8a485

🎆Style: 스탭 1 기능에 대한 HTML/CSS를 수정한다.

fed0928

🧪Test: TDD를 적용한다. 미션(기존 커밋에 스탭이라고 표현했다)1에 대한 테스트 케이스를 작성한다.

9e99167

🎆Style: 템플릿 리터럴로 분리한 lotto-section을 정리하고 css를 추가한다.

9dae411

✒️Feat: 로또 리스트 컴포넌트를 분리한다.

b3a0554

✒️Feat: 컴포넌트를 분리하고 템플릿/컨테이너로 기능을 구현한다.

209611b

🧪Test: 테스트 코드를 기능에 맞게 수정한다.

56f1e05

✒️Feat: 입력 금액에 대한 유효성 검사를 추가한다.

4e9c922

🧺Remove: 잘못 커밋된 파일 제거

8e02929

InSeong-So changed the base branch from main to inseong-so March 19, 2022 06:17

🎆Style: EOF 제거

43a0a30

pocojang requested a review from Tanney-102 March 19, 2022 06:37

pocojang assigned Tanney-102 Mar 19, 2022

pocojang changed the title ~~[클린코드 2기 소인성] 로또 미션~~ [클린코드 2기 소인성] 로또 미션 STEP1 Mar 19, 2022

Tanney-102 requested changes Mar 19, 2022

View reviewed changes

InSeong-So added 7 commits March 20, 2022 23:51

🧪Test: 피드백을 반영하여 테스트 코드를 수정한다.

061fb0d

🗃Refactor: 서비스 로직을 일관되게 수정한다.

275fa6c

✒️Feat: isNil과 isEmpty를 추가하고 기존 함수를 수정한다.

47aaae0

🗃Refactor: 중복되는 조건이 없도록 수정하고 상수화 된 에러메세지를 import 한다.

4fe08c6

✒️Feat: 렌더링과 이벤트 바인딩을 담당할 함수를 커링을 활용하여 추가한다.

3c9d033

✒️Feat: 컴포넌트를 일관되게 작성하고 렌더링과 바인딩을 위임하는 함수를 호출하여 감싼다.

17c1d17

🗃Refactor: 수정된 컴포넌트에 따른 코드를 반영한다.

7ecbaeb

InSeong-So requested a review from Tanney-102 March 20, 2022 14:59

InSeong-So and others added 6 commits March 21, 2022 00:20

🧺Remove: 불필요 함수와 상수를 제거한다.

839354d

✒️Feat: 3주차 클린코드 내용을 반영한다.

0bbec63

- 도메인을 분리하고 안정 장치를 마련한다.

✒️Feat: 3주차 클린코드 내용을 반영한다.

c40dab0

- 유틸 함수는 도메인에 상관이 없어야 하므로 도메인 로직으로 분리한다.

✒️Feat: 3주차 클린코드 내용을 반영한다.

55696ea

- 컴포넌트는 일관된 동작만을 가진다.

🧪Test: 테스트 코드의 상수 경로를 수정한다.

2d29c31

🗃Refactor: 직관적인 네이밍으로 변경한다.

87d33b2

SINHOLEE reviewed Mar 22, 2022

View reviewed changes

Tanney-102 approved these changes Mar 22, 2022

View reviewed changes

Tanney-102 merged commit 5beae85 into next-step:inseong-so Mar 22, 2022

		@@ -0,0 +1,7 @@
		import { REGEXP_SPECIAL } from '../constants.js';

		export const isNull = value => value === null \|\| value === undefined;


		export const isNull = value => value === null \|\| value === undefined;

		export const isEquals = (target1, target2) => target1 === target2;


		export const isEquals = (target1, target2) => target1 === target2;

		export const isSpecial = value => REGEXP_SPECIAL.test(value);

[클린코드 2기 소인성] 로또 미션 STEP1 #83

[클린코드 2기 소인성] 로또 미션 STEP1 #83

Uh oh!

Conversation

InSeong-So commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1차 피드백

구현

기능 요구사항

테스트 요구사항

질문

Uh oh!

Tanney-102 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InSeong-So Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SINHOLEE left a comment

Choose a reason for hiding this comment

Uh oh!

InSeong-So commented Mar 19, 2022 •

edited

Loading

InSeong-So Mar 20, 2022 •

edited

Loading