[1단계 - 행운의 로또 미션] 유세지(김용래) 미션 제출합니다 by usageness · Pull Request #82 · woowacourse/javascript-lotto

usageness · 2022-02-23T07:33:14Z

유세지(김용래) 행운의 로또 미션 1단계 PR 제출합니다.

리뷰어님, 안녕하세요!

이번 미션은 실제로 구현하기 전 페어와 활발히 의견을 주고 받으며 문서화를 통해 프로젝트의 개략적인 설계를 머리속에 그려둔 덕분에 빠르게 완료할 수 있었던 것 같아요. 그래서 추가적으로 몇 가지 시도를 해보았습니다. 질문이 조금 있어서 밑에 정리해두었는데 괜찮으시다면 고민을 해결할 수 있는 힌트를 조금 알려주시면 좋을 것 같습니다! 감사합니다 😊

데모 페이지

궁금한 점

프로토타입의 사용

src/utils/customPrototypeMethod 파일의 deepCopy 메서드는 Array의 프로토타입에 정의되어 있습니다. 이렇게 기존에 존재하는 객체에 프로토타입을 붙여 사용하는 방식은 괜찮을까요? 문제가 된다면 어떤 점에서 발생할 수 있을까요!

깊은 복사

두 번째는 마찬가지로 deepCopy 메서드에 관한 부분인데, 어떤 값을 가져올때 깊은 복사를 통해 원본의 불변성을 유지시키는 방법은 다양한 곳에서 값을 사용하여 원본이 변경되는 경우 사이드 이펙트가 발생할 수 있을때 유용할 것이라고 생각됩니다. 다만 이 경우 가져오는 데이터가 많다면 프로그램의 퍼포먼스가 떨어질 수도 있다는 생각이 들어 약간 고민이 되는데... 만약 그렇다면 어떻게 처리하는 것이 좋을까요?

프로젝트 설계도

MVC 패턴을 이용해 각자의 역할을 나누었습니다.

roy-jung

유세지님 안녕하세요, 리뷰어 정재남입니다.
코드 잘 봤습니다. 고생하셨어요!!

deepCopy 퍼포먼스 질문 관련
깊은복사는 불변성을 통해 얻는 이점이 이를 위한 퍼포먼스 손실에 비해 큰 경우에 취하는 전략입니다. 특히 레퍼런스에 대한 성능최적화가 고도화된 V8엔진 하에서는 이로부터 얻는 퍼포먼스 손실은 생각보다 미미합니다.
그렇다면 무조건 불변객체를 사용하는게 좋느냐 하면, 반드시 그렇지는 않다고 생각합니다. nested한 객체의 경우 전체 뎁스를 모두 깊은복사해야만 하는 경우도 물론 있지만, 1뎁스까지만 깊은복사를 해도 충분한 경우도 있고, 얕은복사로도 아무 문제 없는 경우도 많습니다. 성능이 고민되신다면 각각의 상황에 따라 전략을 취하시면 되겠습니다.
Models의 각 네이밍이 확 와닿지 않네요. Lotto 클래스는 어떤 정보를 담는 녀석인지, LottoGame은 또 어떤 녀석인지, 이름만 봤을 때 대략적으로라도 유추할 수 있는 네이밍을 고민해보시면 좋겠어요.

그밖에는 코멘트로 대체합니다. 요청사항 확인해보시고 반영 / 이견제시 / 질문 등 해주세요!

roy-jung · 2022-02-24T13:24:44Z

@@ -0,0 +1,6 @@
+export const NUMBER = {
+  LOTTO_NUMBER_LENGTH: 6,


length는 '자릿수'를 의미하는걸로 오해할 여지가 있으니 'amount'가 어떨까요?

좋은 지적 감사합니다! 배열에 사용되는 상수라 무의식적으로 Length를 사용했는데
말씀을 듣고 나니 amount가 오해의 여지가 적은 네이밍인 것 같습니다.

roy-jung · 2022-02-24T13:24:58Z

+export const SELECTOR = {
+  CHARGE_INPUT_FORM: '#charge-input-form',
+  CHARGE_INPUT: '#charge-input',
+
+  ALIGN_CONVERTER: '#align-converter',
+  PURCHASED_MESSAGE: '#purchased-message',
+  LOTTO_CONTAINER: '#lotto-container',
+};


많은 크루분들이 이렇게 셀렉터를 상수화해서 쓰고 계시던데, 개인적으로 이렇게 할 가치가 있는지 의문이에요.

저도 들이는 시간에 비해선 큰 효과가 있다고 생각하지는 않지만,
개인적으로는 선택자 앞에 붙는 #과 .이 통일되지 않는 점이 다소 거슬렸습니다.
상수화 시킨 셀렉터를 사용하니 미관상 깔끔해 보여서 이후로 쭉 사용하게 되었네요 😭

roy-jung · 2022-02-24T13:25:57Z

+  constructor(lottoNumbers) {
+    this.lottoNumbers = lottoNumbers;
+  }
+
+  static create(lottoNumbers) {
+    if (isValidNumber(lottoNumbers) && isValidLength(lottoNumbers)) {
+      return new Lotto(lottoNumbers);
+    }
+    throw new Error(ERROR_MESSAGE.LOTTO_NUMBER_IS_INVALIDATE);
+  }


new 연산자 사용을 제한하고 반드시 create로만 쓰도록 의도하신거라면
constructor에서 new로 접근하는걸 제한하시면 더 좋을 것 같아요.
[MDN - new.target]
(그런데 왜 이렇게 의도하셨는지가 궁금하긴 합니다).

로또를 구매한다는 미션의 내용으로 미루어보아,
현재 요구사항에는 자동으로 번호가 정해지는 로또만 구매할 수 있지만
이후에 수동으로 번호를 찍어 구매하는 기능이 생길수도 있겠다는 생각을 하였습니다.

이런 경우에 constructor만 이용하여 로또를 생성하는것 보다는,
별도의 이름이 있는 메서드를 통해 구매할 수 있도록 구조를 짜두면
여러가지 방법으로 로또를 생성하기 쉬울 것 같다고 생각합니다.

만약 그렇게 된다면, create 의 이름은 createAuto 정도로 바뀌게 될 것 같습니다 :)
new 접근 제한도 있었군요. 좋은 방법을 알려주셔서 감사합니다!

roy-jung · 2022-02-24T13:27:01Z

+        const lottoNumbers = this.createLottoNumbers();
+        const lotto = Lotto.create(lottoNumbers);
+        lottoList.push(lotto);
+      }


for문을 쓰지 않고 처리할 수 있는 방법은 없을까요?

for문을 아래처럼 map을 이용해서 대체 해주었습니다.
그런데 for문을 사용하지 말아야하는 이유가 있을까요?

const newLottoList = new Array(availableLottoAmount).fill().map(() => { const lottoNumbers = this.createLottoNumbers(); return Lotto.create(lottoNumbers); });

https://techcourse.woowahan.com/s/dSWvXWYI/ls/DYFbCq8T
"반복문보다는 Array 내장 메서드 사용하기" 부분을 참고하세요 :)

roy-jung · 2022-02-24T13:27:35Z

+  constructor(lottoNumbers) {
+    this.lottoNumbers = lottoNumbers;
+  }
+
+  static create(lottoNumbers) {
+    if (isValidNumber(lottoNumbers) && isValidLength(lottoNumbers)) {
+      return new Lotto(lottoNumbers);
+    }
+    throw new Error(ERROR_MESSAGE.LOTTO_NUMBER_IS_INVALIDATE);
+  }


createLottoList가 먼저 호출된 이후에만 getLottoList 호출이 이뤄진다는게 명확하다면 문제는 없겠지만,
클래스 구조로만 봤을 때는 이대로는 문제가 있어 보여요.
lottoList 값이 null인 경우엔 error를 던지게 될테니까요.

실제로 잘못된 값을 주어 lottoList의 값이 갱신되지 않으면 에러가 발생했습니다.
error를 방지하기 위해 lottoList의 기본값을 null 대신 빈 배열 [] 으로 주었습니다!

roy-jung · 2022-02-24T14:08:40Z

+  #initializeDOM() {
+    this.$purchasedMessage = findElement(SELECTOR.PURCHASED_MESSAGE);
+    this.$lottoContainer = findElement(SELECTOR.LOTTO_CONTAINER);
+  }


많이들 이렇게 작성하시던데, 이건 그냥 constructor에 넣어두어도 되지 않나요?

constructor 내부는 깨끗하게 유지해야 하고, 분리할 수 있는 코드들은 모두 분리를 해두어야 한다는 강박관념이 있었던 것 같습니다. 필요없는 메서드 분리는 최대한 지양하도록 하겠습니다!

roy-jung · 2022-02-24T14:11:15Z

+  renderLottoSection(lottoList) {
+    this.renderPurchasedMessage(lottoList.length);
+    this.renderLottoList(lottoList);
+  }
+
+  renderLottoList(lottoList) {
+    this.$lottoContainer.innerHTML = lottoList
+      .map((lotto) => this.generateLottoTemplate(lotto))
+      .join('');
+  }
+
+  renderPurchasedMessage(lottoAmount) {
+    this.$purchasedMessage.innerText = `총 ${lottoAmount}개를 구매하였습니다.`;
+  }


지나치게 잘게 분리하였습니다. renderLottoList, renderPurchasedMessage는 모두 renderLottoSection 한군데에서만 호출하고 있는, 분리의 목적이 불분명한 메서드들입니다. 이런 메서드들은 협업시 다른 사람들로 하여금 혹시 다른데서도 호출하는 곳이 있나? 하는 혼란만 주게 됩니다.
함수/메서드를 분리해야 하는 목적은, [1) 외부에서의 접근수단 제공, 2) 여러 곳에서 호출] 이 두 가지 기준을 위주로 판단하세요. 단순히 기능이 다르다는 이유만으로 쪼개는건 지양하시기 바랍니다.

그동안 기능 위주의 메서드 분리를 이어와서 그런지 기능이 다르면 분리를 해야한다고 생각했었습니다.
함수/메서드를 분리할때 판단의 분명한 근거를 두는 연습을 해야할 것 같습니다. 😭

roy-jung · 2022-02-24T14:15:21Z

+
+- [✅] 로또 게임 모델에 금액이 정상적으로 입력되면, 구매할 수 있는 로또의 수를 반환할 수 있어야 한다.
+- [✅] 금액은 1000이상의 숫자여야한다.
+- [✅] 로또 번호 배열들을 입력하여 로또 모델을 생성하고 관리할 수 있어야 한다.


여기 있는 내용들은 모두 '나중에 할 일'을 정리해두신 거죠? 각각에 checked가 되어있어서 이미 구현하신줄 알았네요...

앗 맞습니다 😅 step2 내용까지 실수로 체크 해버렸네요..

roy-jung · 2022-02-24T14:18:29Z

+
+    const lottoListFromGetterFunc = lottoGame.getLottoList();
+
+    expect(lottoListFromGetterFunc !== lottoGame.lottoList).toBe(true);


깊은복사여부를 판단하려면 to.deep.equal을 쓰시면 됩니다.
deepCopy에 대한 unit 테스트는 별개로 진행하시면 되고, 위 테스트는 이 자리에 있는게 의미가 있죠.
실제로 deepCopy가 호출되었는지 여부는 여기선 관심사가 아니고, 오직 결과값이 deepEqual인지만 알면 되니까요.

그런데 전체 코드를 둘러보니, lottoList의 값들에 변경을 가하는 그 어떤 동작도 없는 것 같은데, 굳이 deepCopy를 구현하신 이유가 궁금하네요.

모든 기능이 구현된 이후에 get 메서드가 깊은 복사를 하도록 수정되어서 그런지
테스트를 추가할때 혼동이 있었던 것 같습니다.
그리고 toEqual 메서드를 말씀하신 것 같아 해당 테스트를 이걸로 수정했습니다!
to.deep.equal은 아마 chai.js 의 문법인 것 같아요 👍

굳이 깊은 복사를 구현했던 이유는, 이후의 구현에서 값을 변경하는 동작이 생기더라도
별다른 오류 없이 진행할 수 있도록 미리 구현해 놓자는 생각이었습니다.
당장의 코드에서는 깊은 복사가 필요하지는 않은게 맞네요 :)

roy-jung · 2022-02-24T14:21:13Z

+  #initializeGame() {
+    this.lottoGameModel = new LottoGameModel();
+    this.lottoGameView = new LottoGameView();
+  }
+
+  #initializeDOM() {
+    this.$chargeForm = findElement(SELECTOR.CHARGE_INPUT_FORM);
+    this.$chargeInput = findElement(SELECTOR.CHARGE_INPUT);
+    this.$alignConverter = findElement(SELECTOR.ALIGN_CONVERTER);
+  }
+
+  #initializeHandler() {
+    this.$chargeForm.addEventListener('submit', this.onSubmitChargeInputForm);
+    this.$alignConverter.addEventListener('change', this.onChangeAlignState);
+  }


앞서 말씀드린 것과 마찬가지입니다. 기능별로 분리를 해두셨지만 실제 호출은 start메서드 한군데에서만 이뤄지고 있기 때문에 '과도한 분리'로 보입니다.

usageness · 2022-02-26T13:19:09Z

안녕하세요 재남님!

꼼꼼히 리뷰해주시고, 궁금했던 점을 쉽게 설명해주셔서 이해가 잘 되었습니다.
특히 메서드 분리나, 코드를 사용해야 할 때를 가르는 명확한 기준이 생긴 것 같아요.
리뷰해주신 내용 토대로 코드를 수정했습니다!
다음 리뷰에서도 잘 부탁드립니다 😊

roy-jung

변경사항 확인했습니다. 고생하셨고, step-2에서 봬요!

usageness and others added 22 commits February 22, 2022 15:17

docs: todo list 정리

77c2296

test: model 테스트 케이스 구현

b90e001

feat: Lotto, LottoGame 모델 구현

c0ad6c3

feat: 마크업, css 적용

a0cb9ff

feat: LottoGame controller 구현

4841ed6

refactor: DOM 요소, 에러 메시지 상수화

272bbc6

chore: eslint, prettier 적용

6f1cb92

chore: lint 설정으로 인한 변경사항

3084723

feat: 정상적인 금액을 입력하면, 구매한 로또가 보인다

1058e9b

feat: 번호 보기를 클릭하면, 로또 보기 상태가 변경된다

988cb4a

refactor: 상수화 진행, align-state 의 dataset property를 변경

0121c1a

chore: 최신버전의 ecmaVersion으로 변경

e09d6ce

style: converter 스타일 코드를 추가

521d5f7

chore: eslint 설정 변경으로 인한 수정사항

af546a4

refactor: onSubmitChargeInputForm 함수의 로직을 다른 메소드로 정의

d91222e

refactor: converter css 파일에서 레이아웃 코드를 분리

42e1984

refactor: LottoGameModel의 숫자 리터럴 상수화, LottoModel의 주석을 제거

01be1a1

refactor: LottoGameModel에서 불필요한 메소드 삭제, 메소드의 네이밍 수정

a6e9dd3

test: 모델의 변경사항을 테스트 파일에 적용 및 상수화 진행

0c6252a

docs: todo.md 수정

a5f2a45

refactor: import css 문에 확장자 추가

6851773

refactor: getter 함수는 복사된 값을 리턴, createLottoList의 로직을 수정, 에레메세지 텍스트 변경

b4f543c

pocojang requested a review from roy-jung February 24, 2022 07:45

pocojang assigned roy-jung Feb 24, 2022

roy-jung suggested changes Feb 24, 2022

View reviewed changes

roy-jung mentioned this pull request Feb 24, 2022

[1단계 - 행운의 로또 미션] 록바(김상록) 미션 제출합니다 #103

Merged

refactor: LOTTO_NUMBER_LENGTH 상수 이름 변경

ae4be3c

roy-jung mentioned this pull request Feb 25, 2022

[1단계 - 행운의 로또 미션] 안(안수진) 미션 제출합니다 #114

Merged

usageness added 2 commits February 26, 2022 16:43

refactor: createLottoList 로직의 for문을 map()으로 대체

8184bb9

chore: github pages 사용을 위한 세팅

5b0faf9

usageness added 4 commits February 26, 2022 17:47

refactor: randomNumber() 의 호출 횟수를 줄이도록 변경

a2a8e71

refactor: isValidNumber 로직 검색 효율 개선

f3568c2

refactor: 필요없는 메서드 일원화

b9b9997

chore: 깊은 복사 test 문법 변경

b4724f9

roy-jung approved these changes Feb 26, 2022

View reviewed changes

roy-jung merged commit b1948b4 into woowacourse:usageness Feb 26, 2022

		@@ -0,0 +1,6 @@
		export const NUMBER = {
		LOTTO_NUMBER_LENGTH: 6,


		const lottoListFromGetterFunc = lottoGame.getLottoList();

		expect(lottoListFromGetterFunc !== lottoGame.lottoList).toBe(true);

Conversation

usageness commented Feb 23, 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!

usageness 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!

usageness 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!

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

usageness Feb 26, 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!

usageness commented Feb 26, 2022

Uh oh!

roy-jung left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

usageness commented Feb 23, 2022 •

edited

Loading

usageness Feb 25, 2022 •

edited

Loading

usageness Feb 25, 2022 •

edited

Loading

roy-jung Feb 26, 2022 •

edited

Loading

usageness Feb 26, 2022 •

edited

Loading