[1단계 - 행운의 로또 미션] 태태(김태윤) 미션 제출합니다 by nan-noo · Pull Request #91 · woowacourse/javascript-lotto

nan-noo · 2022-02-24T04:57:42Z

이번 미션에서 TDD에 처음 도전해 봤습니다.
실패하는 테스트 작성 -> 성공하도록 빠르게 구현 -> 리팩토링 과정을 거쳐서 유닛테스트 코드를 만들었는데, 작은 단위부터 구현해나가니까 개발에 부담이 없어서 좋았습니다. 정석대로 하고 있는 지는 잘 모르겠지만..
실무에서도 TDD로 개발하는 경우가 많은가요?
클래스 분리를 중점으로 진행했습니다.
구조는 리드미에 있습니다!ㅎㅎ
각 클래스가 들고 있어야 하는 데이터를 constructor에 넣어둬야 좋을지(보기 편하려고..?), 아니면 지금처럼 메소드에서 this.aa 로 저장하는 게 좋을지 궁금합니다.
그리고 또, 우테코 강의에서 백엔드는 정말 세세하게 클래스로 만들던데(예를 들어, 이번 미션에서는 각 로또가 들고 있는 숫자 하나하나까지) 프론트는 어디까지 클래스로 만들어야 하는 지 기준이 있나요?
이번에 this 때문에 발생한 오류로 시간을 좀 잡아먹어서 this 범위(+ 화살표함수)를 공부할 생각입니다
데모 페이지

Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

기능 구현 목록 추가 Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

공백, 1000원 단위, 범위 확인 Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

lottoController와 lottoManager를 추가하였다. 컨트롤러에서 입력을 받고 매니저에게 입력값을 넘겨 준다(예외 검사)

게임 관련 상수, 메세지 분리 및 메소드 수정

로또 모델 추가 및 매니저에 로또 생성 메소드 추가

로또 뷰에 렌더링함수 추가 index.html 수정

로또뷰와 상수 파일 리팩토링

css로 구현

토글 버튼이 활성화 됐을 때, 뷰가 화면에 숫자를 보여주도록 구현. 비활성화시 숨김.

잘못된 금액을 입력했을 때, 화면에 렌더링 되는 것을 수정

구입 버튼을 눌렀을 때, 구입 버튼과 금액 입력창 비활성화

시맨틱 태그 사용 및 메소드와 변수 이름 수정. 스타일 수정

구조 설계도 및 파일 의존성 도식 추가

roy-jung · 2022-02-24T12:58:39Z

안녕하세요, gh-page로 데모페이지 올려주시면 감사하겠습니다.

nan-noo · 2022-02-25T01:07:57Z

안녕하세요, gh-page로 데모페이지 올려주시면 감사하겠습니다.

리드미에 데모 페이지 있습니다!
https://nan-noo.github.io/javascript-lotto/

roy-jung

안녕하세요 태태님, 리뷰어 정재남입니다.
코드 잘 봤고, 여기서는 질문주신 내용에 대해 답변 적어보겠습니다.
코멘트 요청사항 확인해보시고 반영 / 이견제시 / 질문 등 해주세요!

TDD를 해보셨다니 대단하시네요! 전 아직 TDD에 대한 경험이 많지 않아 부끄럽습니다. 실무에서도 회사들마다 TDD를 하는 회사가 많이 있는것 같긴 한데, 제가 경험해보진 못했네요.
TDD는 정석이랄게 없고 다만 딱 필요한 상황에서 정확한 테스트를 할 수 있다면 가장 좋겠죠. 작은 단위로부터 시작하신건 아주 좋습니다!
나중에 typescript를 접해보시면, constructor 또는 field상에서 선언/할당하지 않은 member는 에러를 던집니다. javascript가 자율성을 많이 열어두고 있어서 그 외의 경우에도 문제없이 동작하고 있지만, 기본적으로는 constructor 호출시점에 모든 멤버들의 초기화가 이뤄지는게 정석이긴 합니다.
실무에서 프론트엔드쪽에서 클래스를 만드는 케이스는 생각보다 많지 않습니다. 리액트나 뷰를 쓰는 경우에는 더더욱 없고, 다만 뷰 전용 모델/서비스 등을 다룰 때 쓰는 경우가 있긴 합니다. 객체지향을 필수로 하는 자바 등과는 아무래도 생태계가 달라요.
추가로 디자인 패턴에 관한 내용은 링크로 대체합니다 (#103 (review))

roy-jung · 2022-02-25T01:42:48Z

src/index.js

-import "./css/index";
-import "./js/app";
+import './css/index.css';
+import './js/app';


lottoController를 여기서 직접 호출하지 않고 app이라는 파일을 한 번 더 거치는 이유가 뭔가요?

처음 클론했을 때, index.js와 app.js가 분리되어 있어서 그대로 사용했습니다.
피드백 받고 생각해보니까 아무래도 index.js는 스타일과 js가 import 되는 곳이어서 확실하게 나누기 위해 app.js에서 컨트롤러를 호출하는 게 나은 것 같습니다.

// index.js import './css/index.css'; import './js/app'; // app.js import lottoMachine from './controller/lottoMachine'; lottoMachine.startLotto();

이렇게 작성하면 '스타일과 js가 import되는 곳(index.js)과 js만 import되는 곳(app.js)'이 확실하게 나누어지는 건가요? 그렇게 해야 하는 이유는 뭔가요? css-module을 쓰는 경우에도 '스타일과 js를 import'하기 위한 별도의 파일을 만들어서 분리하는게 좋다고 생각하시나요?

태태님이 '틀렸다'는게 아니라, 말씀하신 내용을 바탕으로 생각을 확장해보고, 파일분리의 적절한 기준에 대해 함께 고민해보자는 취지로 말씀드리는 거예요.

roy-jung · 2022-02-25T01:44:51Z

src/js/__tests__/app.test.js

+        message: () => `expected ${received} not to be within range ${floor} - ${ceiling}`,
+        pass: true,
+      };
+    }
+    return {
+      message: () => `expected ${received} to be within range ${floor} - ${ceiling}`,
+      pass: false,
+    };


메시지가 반대로 된 것 아닌지요?

공식 문서에 있던 예시 복붙한건데, 지금 보니 반대로 되어 있는 것 같네요. 수정했습니다!

roy-jung · 2022-02-25T01:45:26Z

src/js/__tests__/app.test.js

+    expect(() => lottoManger.buyLotto(cashInput)).toThrow();
+  });
+  test('입력 값이 1000원 단위인지 검증한다.', () => {
+    const cashInput = '1500';
+    expect(() => lottoManger.buyLotto(cashInput)).toThrow();
+  });
+  test('입력 값의 범위가 1000 - 50000인지 검증한다.', () => {
+    const cashInput = '55000';
+    expect(() => lottoManger.buyLotto(cashInput)).toThrow();
+  });
+  test('올바른 입력 값을 입력하면 오류가 발생하지 않는다.', () => {
+    const cashInput = '2000';
+    expect(() => lottoManger.buyLotto(cashInput)).not.toThrow();


throw시 에러메시지까지 확인해주면 더 좋을것 같아요.

수정했습니다!

roy-jung · 2022-02-25T01:46:24Z

src/js/__tests__/app.test.js

+  test('범위가 1 - 45인 고유한 숫자 6개가 생성되는지 확인한다.', () => {
+    const lotto = new Lotto();
+    lotto.lottoNumberSet.forEach((number) =>
+      expect(number).toBeWithinRange(LOTTO_NUMBER_RANGE.MIN, LOTTO_NUMBER_RANGE.MAX)
+    );
+  });


테스트가 확인하고자 하는 바를 온전히 수행하지는 못하는 것 같아요.

원래 고유한 숫자 6개인 것을 확인하려고 했던 테스트코드인데, 1-45 사이인지만 확인했네요. 타이틀이랑 내용 모두 수정했습니다.ㅎㅎ

roy-jung · 2022-02-25T01:49:08Z

src/js/constants/constants.js

+  OUT_OF_RANGE_MESSAGE: `${CASH_INPUT_RANGE.MIN}원-${CASH_INPUT_RANGE.MAX}원 사이의 금액을 입력해주세요.`,
+});
+
+export const LOTTO_IMAGE = '🎟️';


어떤것은 Object.freeze가 있고 어떤것은 없는데, 그 이유가 궁금합니다.
#103 (comment)

앗 실수입니다ㅠㅠ 수정했습니다. Object.freeze를 쓰는 이유가 외부에서 수정하지 못하도록 하는 것으로 알고 있는데, 이런 상수 모음 객체에는 가능하면 쓰는 게 맞을까요?

위 링크 코멘트 확인해보시고, 태태님만의 기준을 정하시면 될 것 같습니다.

roy-jung · 2022-02-25T06:41:02Z

src/js/view/lottoView.js

+    if (isVisible) {
+      lottoNumberClassList.add(SELECTOR.ONE_COLUMN_GRID_CLASSNAME);
+      lottoNumberClassList.remove(SELECTOR.HIDE_NUMBERS_CLASSNAME);
+      return;
+    }
+    lottoNumberClassList.remove(SELECTOR.ONE_COLUMN_GRID_CLASSNAME);
+    lottoNumberClassList.add(SELECTOR.HIDE_NUMBERS_CLASSNAME);


두개의 클래스가 동시에 공존하지 못하는 것 같은데, 그렇다면 하나의 클래스명으로 통합관리하는 방법은 없을까요?

그 생각을 못했었는데, 하나의 클래스로 작동하도록 수정했습니다:D

roy-jung · 2022-02-25T06:41:57Z

src/js/view/lottoView.js

+  beforeRenderLottos() {
+    const cashInput = selectDom(SELECTOR.CASH_INPUT_CLASS, this.cashInputSection);
+    const cashInputButton = selectDom(SELECTOR.CASH_INPUT_BUTTON_CLASS, this.cashInputSection);
+    cashInput.disabled = true;
+    cashInputButton.disabled = true;
+    cashInputButton.textContent = DISABLED_PURCHASE_BUTTON_TEXT;
+  }
+
+  renderLottos(lottos) {
+    this.purchasedLottoSection.classList.remove(SELECTOR.HIDE_CLASSNAME);
+    this.winnerNumberSection.classList.remove(SELECTOR.HIDE_CLASSNAME);
+
+    this.lottoShowContainer.prepend(LottoView.generatePurchasedLabel(lottos.length));
+    this.lottoNumberContainer.append(...LottoView.generateLottoElementsArray(lottos));
+  }


이 둘이 분리되어 있어야 할 필요성이 있나요?

아무래도 함수 네이밍을 잘못한 거 같아서 beforeRenderLottos를 disableCashInputSection으로 수정했습니다. 캐시인풋섹션을 비활성화 하는 것과 로또를 렌더링하는 것은 다른 기능이기 때문에 분리했습니다!

roy-jung · 2022-02-25T06:42:51Z

src/js/view/lottoView.js

+  static generatePurchasedLabel(length) {
+    const labelElement = document.createElement('label');
+    labelElement.textContent = `총 ${length}개를 구매하였습니다.`;
+    return labelElement;
+  }
+
+  static generateLottoElementsArray(lottos) {
+    return lottos.map((lotto) => LottoView.generateLottoElement(lotto));
+  }
+
+  static generateLottoElement(lotto) {
+    const lottoElement = createElementWithClassName('div', SELECTOR.LOTTO_CLASSNAME);
+
+    const lottoImage = createElementWithClassName('p', SELECTOR.LOTTO_IMAGE_CLASSNAME);
+    lottoImage.textContent = LOTTO_IMAGE;
+
+    const lottoNumbers = createElementWithClassName('p', SELECTOR.LOTTO_NUMBERS_CLASSNAME);
+    lottoNumbers.textContent = Array.from(lotto.lottoNumberSet).join(', ');
+
+    lottoElement.append(lottoImage, lottoNumbers);
+    return lottoElement;
+  }
+}


static으로 선언한 특별한 이유가 있나요?

eslint 에어비앤비 베이스로 하다보니 this를 클래스 메소드에 사용하지 않으면 계속 경고를 띄워서 일단 Static으로 하긴 했는데, 지금 보니 굳이 static일 필요가 없어 보여서 수정했습니다.
경고 뜨는 게 보기 싫어서 "class-methods-use-this": "off"로 하긴 했는데, 혹시 이렇게 기본적으로 메소드 this를 안 쓰면 static으로 하라는 데에는 이유가 있을까요?
공식 문서를 봐도 이유는 안 적혀있어서 질문합니다!

class는 기본적으로 인스턴스를 생성하는 틀입니다. 인스턴스 내부 '메소드'의 정의는 인스턴스 '자신'의 내부 동작을 수행하는 함수입니다. '자신'에 관여하는 내용이 없다면 이는 '메서드'가 아닌 '함수'로 불러야 하겠죠. 그런 맥락에서 보면, this를 사용하지 않는 메서드는 인스턴스에서 접근해야할 필요가 없습니다. 인스턴스에 제공될 필요가 없으니 굳이 상속시키지 않게끔 static으로 분리하라는 것이죠.
그런데 전부 static 메서드만 있는 클래스의 경우, 이를 바탕으로 인스턴스로 만들어봐야 그 인스턴스는 빈껍데기만 있는 객체에 불과합니다. 실제로 인스턴스로 만드는 시도조차 안하실거구요. 그렇다면 해당 클래스는 클래스로서가 아니라 일반 객체로서의 의미밖에 없는 것이니, static으로만 구성된 클래스는 차라리 처음부터 객체로 만드는 편이 좋다고 생각합니다.

roy-jung · 2022-02-25T06:45:11Z

src/js/constants/constants.js

+export const SELECTOR = Object.freeze({
+  CASH_INPUT_SECTION_CLASS: '.cash-input-section',
+  CASH_INPUT_CLASS: '.cash-input',
+  CASH_INPUT_BUTTON_CLASS: '.cash-input-button',
+  CASH_INPUT_BUTTON_CLASSNAME: 'cash-input-button',
+
+  PURCHASED_LOTTO_SECTION_CLASS: '.purchased-lotto-section',
+  WINNER_NUMBER_SECTION_CLASS: '.winner-number-section',
+
+  LOTTO_SHOW_CONTAINER_CLASS: '.lotto-container',
+  LOTTO_NUMBER_CONTAINER_CLASS: '.lotto-grid',
+
+  SHOW_NUMBER_TOGGLE_BUTTON_CLASS: '.show-number-toggle-button',
+
+  LOTTO_CLASSNAME: 'lotto',
+  LOTTO_IMAGE_CLASSNAME: 'lotto-image',
+  LOTTO_NUMBERS_CLASSNAME: 'lotto-numbers',
+
+  HIDE_CLASSNAME: 'hide',
+  ONE_COLUMN_GRID_CLASSNAME: 'one-column-grid',
+  HIDE_NUMBERS_CLASSNAME: 'hide-numbers',
+});


Selector를 상수로 받아놓는 것이 어떤 가치가 있는지 의문이에요. 차라리 selecting을 마친 결과물을 변수로 담아서 쓰는 방법이 더 낫지 않을까 싶은데, 어떻게 생각하시나요?

�처음엔 일단 상수고 반복해서 쓰이니까 분리했는데, 리팩토링을 거치고 나니까 뷰에서 어차피 멤버로 가지고 있는데 상수로 분리할 필요가 없어 보여서 수정할 생각입니다!

roy-jung · 2022-02-25T06:58:26Z

src/js/model/lottoManager.js

+  }
+}
+
+export default LottoManager;


Lotto와 LottoManager가 정확히 어떤 역할을 맡고 있는지, 이름만 봐서는 잘 모르겠습니다. 코드를 찬찬히 뜯어보고서야 이해할 수 있는 네이밍은 바람직하지 않다고 생각해요. 이름만 봤을 때 대략적으로라도 유추할 수 있는 네이밍을 고민해보시면 좋겠어요.

이름 수정했습니다!
로또 -> 로또숫자
로또매니저 -> 로또생성기

각각 클래스에 constructor 추가해서 필드 초기화

전체적으로 선택자는 컨트롤러가 아닌 뷰에서 다루게 바꾸었고, 스태틱 메소드일 필요가 없다고 느껴 은닉할 수 있으면 은닉했다.

역할에 맞게 이름을 수정했다. LottoController -> LottoMachine LottoManager -> LottoGenerator Lotto -> LottoNumber LottoView -> LottoMachineView

lottoPrice 삭제

one-column-grid 삭제 및 lotto-grid 스타일 수정

one-column-grid가 필요 없어졌으므로 삭제

상수 변수 명 변경 뷰에서 직접 선택자를 멤버로 가지고 있기 때문에 constants.js에서 셀렉터 삭제

roy-jung

고생하셨습니다. 코멘트 답변 추가했으니 확인해보시고, step-2에서 계속 발전시켜보면 좋을 것 같아요. step-2때 뵙겠습니다!

nan-noo and others added 29 commits February 22, 2022 15:54

doc: update README.md

55fedb6

Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

feat: update markup

ce1e990

Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

style: update css style

cf26bf0

Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

build: install packages

934894c

Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

doc: update README.md

83f1c74

기능 구현 목록 추가 Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

build: install prettier and eslint

c5ed472

Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

test: write cash input validation test

64ab303

공백, 1000원 단위, 범위 확인 Co-Authored-By: Yo Wook Kim <uk960214@gmail.com>

feat: get user cash input

c4d78cc

lottoController와 lottoManager를 추가하였다. 컨트롤러에서 입력을 받고 매니저에게 입력값을 넘겨 준다(예외 검사)

refactor: separate constants and methods

8606b47

게임 관련 상수, 메세지 분리 및 메소드 수정

test: refactor & add test code

cc05ee1

test: write lotto number generation test

68753b7

test: write multiple lotto numbers generation test

487c388

feat: generate lottos

d020271

로또 모델 추가 및 매니저에 로또 생성 메소드 추가

test: write multiple lottos generation test

e2d5233

feat: generate lottos

6534dbe

feat: render screen when click 구매 button

e858a73

feat: render purchased lottos

908abb5

로또 뷰에 렌더링함수 추가 index.html 수정

refactor: separate constants & methods

9954efa

로또뷰와 상수 파일 리팩토링

feat: make toggle button

d108951

css로 구현

feat: show & hide lotto numbers

4f4d0a5

토글 버튼이 활성화 됐을 때, 뷰가 화면에 숫자를 보여주도록 구현. 비활성화시 숨김.

refactor: separate constants & methods

ee7d2fd

build: install github-pages

116c872

doc: add demo-page

64aec35

fix: render issue

788d889

잘못된 금액을 입력했을 때, 화면에 렌더링 되는 것을 수정

feat: disable button and input

a670392

구입 버튼을 눌렀을 때, 구입 버튼과 금액 입력창 비활성화

refactor: rename method and change tags

e7b8082

시맨틱 태그 사용 및 메소드와 변수 이름 수정. 스타일 수정

refactor: separate css files

219cd1e

refactor: fix eslint error

b018d08

doc: update README.md

90e4a9d

구조 설계도 및 파일 의존성 도식 추가

pocojang requested a review from roy-jung February 24, 2022 05:32

pocojang assigned roy-jung Feb 24, 2022

roy-jung suggested changes Feb 25, 2022

View reviewed changes

nan-noo added 11 commits February 27, 2022 14:29

refactor: add constructor

18965ab

각각 클래스에 constructor 추가해서 필드 초기화

build: add eslint rule

4b46ce4

refactor: freeze object

99b1612

refactor: change methods

7ac6e64

전체적으로 선택자는 컨트롤러가 아닌 뷰에서 다루게 바꾸었고, 스태틱 메소드일 필요가 없다고 느껴 은닉할 수 있으면 은닉했다.

refactor: rename classes

5fae6a7

역할에 맞게 이름을 수정했다. LottoController -> LottoMachine LottoManager -> LottoGenerator Lotto -> LottoNumber LottoView -> LottoMachineView

refactor: modify test code

33b2364

refactor: delete useless field

b328e18

lottoPrice 삭제

style: update css stlye

ab754f0

one-column-grid 삭제 및 lotto-grid 스타일 수정

refactor: modify method

1272345

one-column-grid가 필요 없어졌으므로 삭제

refactor: modify test code

03a716d

refactor: rename variables

8a4bd0f

상수 변수 명 변경 뷰에서 직접 선택자를 멤버로 가지고 있기 때문에 constants.js에서 셀렉터 삭제

roy-jung approved these changes Feb 27, 2022

View reviewed changes

roy-jung merged commit 81c8fa6 into woowacourse:nan-noo Feb 27, 2022

Conversation

nan-noo commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roy-jung commented Feb 24, 2022

Uh oh!

nan-noo commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roy-jung 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!

roy-jung Feb 25, 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!

roy-jung Feb 27, 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!

roy-jung left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nan-noo commented Feb 24, 2022 •

edited

Loading

nan-noo commented Feb 25, 2022 •

edited

Loading

roy-jung Feb 25, 2022 •

edited

Loading

roy-jung Feb 27, 2022 •

edited

Loading