[2단계 - 행운의 로또 미션] 지그(송지은) 미션 제출합니다. #43

zigsong · 2021-02-25T02:41:19Z

안녕하세요 😊 페어 티케와 로또 2단계 미션을 진행한 지그입니다.
아직 1단계가 머지되지 않아 변경한 코드들이 같이 따라와서 변경 파일이 조금 방대해졌습니다 😢
커밋은 1단계와 분리하였으니 참고해주시면 좋을 것 같습니다.
(먼저 머지된 페어의 merge commit에서부터 시작했습니다 :D)

잘 부탁드리겠습니다! 🙇‍♂️

🎰 지그's 로또 2단계 데모

✅ 2단계 미션 TODO

기능 구현 목록

Cypress 테스트

정상 로직
- 모든 숫자의 입력을 완료하면, 결과 확인 버튼이 활성화된다.
- 두개의 숫자를 입력하면, 자동으로 다음 숫자 칸으로 focus가 이동한다.
- 결과 확인 버튼을 누르면 modal 창이 보인다.
- modal의 X 버튼을 누르면 modal 창이 닫힌다.
- modal 영역 밖을 click 하면 modal 창이 닫힌다.
- ~~[ ] modal은 당첨 개수와 수익률을 보여준다.~~
- 임의의 로또 번호에 대하여 당첨 개수를 올바르게 계산하여 보여준다.
- 임의의 로또 번호에 대하여 수익률을 올바르게 계산하여 보여준다.
- 다시 시작하기 버튼을 누르면 프로그램 input이 모두 초기화된다.
예외상황
- 금액 입력 예외상황
  - ~~[ ] 입력할 수 있는 당첨 숫자의 범위는 1 ~ 45 사이이다.~~
  - 중복된 숫자는 입력할 수 없다.

🗂 Directory

🛠 View & 구조도

🤓 고민한 점

InputPriceView, PurchasedLottosView, WinningResultView 3개 view의 공통 부모 클래스인 View를 만들었습니다. View에서 각각의 xxxView가 상황에 따라 show, hide될 수 있도록 메소드를 구현했고, on과 emit을 통하여 event를 view가 아닌 controller에서 처리할 수 있게끔 분리해보았습니다.
위 과정에서 CustomEvent와 dispatchEvent를 이용하였는데, 처음 접하는 방식이었지만 페어의 도움을 받아 개념을 이해하여 사용할 수 있었습니다. 또한 각 메소드에 return this를 사용하여 모듈을 import하여 사용할 때 chaining이 가능하게끔 하는 방식을 배웠습니다.
그동안 진지하게 생각하지 않았던 html 태그의 역할들에 대해서도 생각해 보았습니다. 무조건 class를 쓰기보다는 dataset을 사용하는 방법, input 태그의 required와 min, max를 알게 되어 사용했습니다. 또 input에 명시적인 label이 필요하지 않은 경우, aria-label로 대체할 수 있다는 것을 배웠습니다. 마지막으로 section과 main 등, 페이지 전체를 아우르는 태그들에 대해서는 실제 사용하는 케이스들을 더 살펴보며 고민해봐야겠다고 생각했습니다.
class에서 getter와 setter의 역할과 사용법에 대해 한번 더 고민해보았습니다. 페어와 해당 이슈를 다룬 영상을 함께 시청하고 실제 사용 사례를 검토하여 적용해 보았는데, 옳은 방식이라고 확신이 들지는 않습니다.
reduce, switch~case 문 등 javascript의 기본적인 문법이지만 자주 사용하지 않아서 사소하게 실수하게 되는 점을 반성하게 되었습니다. 좀 더 가독성 좋은 코드를 위해 끊임없이 연습해야 할 것입니다.
1단계 리뷰에서 말씀 주신 #을 이용한 class의 private property는 다양한 시도 끝에 꼬이는 부분들이 많아 이번 미션에서는 제대로 활용하지 못했습니다. 가능하다면 리팩토링을 하면서 바꿔보도록 하겠습니다.

🤔 궁금한 점

InputPriceView, PurchasedLottosView, WinningResultView가 상속하는 클래스인 View에서는 각 view들이 공통적으로 갖고 있어야 하는 메소드(show, hide 및 이벤트 전달과 관련된 메소드)들에서 마지막에 return this를 하고 있습니다. controller에서 사용할 때 view.show().renderLottos()와 같이 메소드 체이닝을 하기 위함인데, 권장되는 사용 방식인지 여쭤보고 싶습니다.
controller에는 이벤트 핸들링과 관련된 메소드들, 그리고 createLottos와 같이 앱 전체에서 중요하다고 생각되는 메소드들만 넣고 실제 당첨 결과를 확인하는 함수들은 utils로 분리시켰습니다. 적절하게 분리한 것인지 평가 받고 싶습니다!
이전에 리뷰 주셨던 클로저를 이용한 모듈 패턴 대신에 현재 모든 view가 상속 받는 View class를 만들어 그곳에 show와 hide 메소드를 만들어 주었는데, 어떻게 생각하시는지 궁금합니다! 🙃

코멘트 이어 나가겠습니다!

zigsong

리뷰 부탁드리곘습니다. 감사합니다 🙇‍♂️

zigsong · 2021-02-25T03:04:18Z

index.html

@@ -53,44 +63,84 @@ <h4 class="mt-0 mb-3 text-center">당첨 번호</h4>
                  <input
                    type="number"
                    class="winning-number mx-1 text-center"
+                    aria-label="winning-number-1"


input에는 label이 함께 있는 것이 좋은 방식이지만, 명시적인 label이 필요하지 않은 경우 aria 속성을 사용하는 방법이 있다고 하여 적용해보았습니다!

aria 프리픽스가 붙은 속성들은 ARIA(Accessible Rich Inernet Applications) 라고... 웹접근성을 위한 속성입니다.
그래서 명시적인 label이 필요하지 않은 경우 aria 를 사용하는 방법이라는 말이 틀렸다고 할 수는 없지만 근본적으로 '웹접근성'을 위한것이라는것을 모르면 오해 할 소지가 있어서 이 부분 첨언 드립니다. :)

진지하게 생각해보지 않고 갖다 붙인 것 같네요 😂 웹 접근성에 대해 시간 내서 공부해보겠습니다! 감사합니다 😄

zigsong · 2021-02-25T03:05:57Z

src/js/Lotto.js

+    this._numbers = new Set();
+    this.matchingNumbers = 0;
+    this.isMatchBonus = false;
+    this._rank = Infinity;


로또가 생성되었을 때의 default 순위(rank)는 Infinity값으로 했으며, 당첨 숫자들과의 일치 개수에 따라 rank를 update시켜줬습니다.

랭크가 과연 로또의 관심사 일까? 라는 관점에서 한 번 생각해보시면 좋을 것 같습니다. (matchingNumbers, isMatchBonus도 마찬가지 입니다.)
로또는 그냥 겹치지 않는(Set) 숫자가 적힌 종이 쪽지인데 다른 정보들이 들어오면서 역할이 비대 해졌습니다.
이미 발행된 로또를 가지고 뭔가를 하는 역할이 따로 필요할 것 같습니다.

zigsong · 2021-02-25T03:07:01Z

src/js/Lotto.js

+  // Test를 위한 setter 생성
+  set numbers(numsArray) {
+    this._numbers = numsArray;
+  }


cypress test에서 임의로 숫자를 넣어 비교해주기 위한 함수이며, 실제 기능에는 사용되지 않습니다.
혹시 이런 방식(실제 기능과는 무관하나, 테스트를 위한 메소드 작성)이 문제가 있을까요? 🙄

네 문제가 있습니다.
이미 발행된 로또의 this._numbers에다가 외부에서 직접 값을 할당하는 조작을 막기위해(실제로 코드가 아직 그렇게 되지는 않았지만) setter 없이 getter만 두신것인데, 단지 테스트를 위해 setter를 열어야 한다면 처음의 의도가 망가집니다.
setter 열어야만 테스트가 된다면 그것은 setter가 없어서 테스트 되지 않는게 아니라 근본적인 다른 문제가 있어서 그럴 수 있으니 다른 부분을 살펴보시는게 더 좋을 것 같습니다.
(테스트가 잘못되었든지, Lotto의 컨스트럭터가 잘못되었든지, getter만 두려고 했던 애초의 의도를 원복해서 numbers를 공개하도록 하든지 기타 등등)

결론: 오로지 테스트를 위해서 소스코드에서 사용하면 안되는 공개 메서드를 만들지는 않습니다.

zigsong · 2021-02-25T03:08:05Z

src/js/Lotto.js

+      case 5:
+        this._rank = this.isMatchBonus ? 2 : 3;
+        break;


switch-case문에서 특정 case 안에 처음에는 중첩 case를 작성하였으나, 삼항연산자를 이용하여 보기 좋게 작성했습니다.
switch-case문 안에서 분기를 또 나눈 것이 처음이라, 혹시 이렇게 작성하면 예상치 못한 문제가 발생할 수 있을지 궁금합니다!

스위치 케이스 내에서의 조건 분기에 따른 문제는 없습니다.

if () { } else if () { if () { // 여기 } }

위와 근본적으로 다르지 않습니다.
중첩이 깊어지면 코드를 읽기 힘들고 유지보수도 힘들어지기 때문에 중첩이 깊어지지만 않게만 관리하면 별다른 문제는 없습니다.
일반적으로 중첩이 3~4단계를 넘어가면 문제가 있다고 볼 수 있습니다.
지금은 2단계이니 괜찮습니다.

zigsong · 2021-02-25T03:11:47Z

src/js/LottoController.js

+    this.inputPriceView.show().resetInputPrice();
+    this.purchasedLottosView.hide().resetToggleSwitch();
+    this.winningResultView.hide().resetWinningNumbers();


각각의 view의 부모 클래스인 View는 show와 hide 메소드를 갖고 있고, 해당 메소드들은 this를 반환하게끔 하여 모듈 밖에서 사용할 때 메소드 체이닝이 가능하게끔 만들었습니다.
클래스의 메소드 내에서 return this를 하는 것은 페어가 알려주었고, 독특하고 유용한 방식이라고 생각했습니다.
이것이 일반적으로 괜찮은 패턴인지, 다른 사이드 이펙트가 발생하지는 않는지 여쭤보고 싶습니다 😯

상속을 할 경우 부모의 this는 상속받은 자식을 가르킨다는 점을 이용해서 공통적인 메서드를 부모에 두는 패턴으로 중복을 제거할 때 유용하게 쓰입니다.
부모의 show, hide 가 없다면 view 로쓰이는 모든 클래스에서 매번 동일하게 show와 hide를 구현해야 하니까 중복을 제거 할 때 용이합니다.
상속은 이런 장점이 있는 반면에 잘못된 설계를 하게 되면 나중에 고치기가 너무 어려워지는 문제가 있습니다.
'상속 vs 합성' 으로 검색해보시면 관련 정보를 얻으실 수 있으며 쉽지 않은 내용이니 너무 급하게 생각하실 필요는 없습니다.
this를 리턴하는 '메서드 체이닝'도 장단점이 함께 존재 하니 검색해서 같이 보시면 좋을 것 같습니다. :)

현재 코드 구성에서는 나쁘지 않은것 같습니다.

키워드 추천 감사합니다!

zigsong · 2021-02-25T03:15:39Z

src/js/LottoController.js

+  reset() {
+    this.lottos = [];
+    this.purchasedPrice = 0;
+    this.rankCounts = Array(5).fill(0);


초기값은 [0, 0, 0, 0, 0]이며, 구매한 로또와 실제 당첨 번호들을 비교하여
[1등의 개수, 2등의 개수, 3등~~, 4등~~, 5등~~]으로 업데이트되도록 하였습니다.

utils의 countByRank() 함수에서 매칭되는 등수들을 계산하여 업데이트합니다.

zigsong · 2021-02-25T03:16:41Z

src/js/utils/utils.js

+function checkMatchingNums(lottos, winningNumbers) {
+  let i = 0;
+  while (i++ < 6) {
+    const currNum = winningNumbers[i];
+    lottos.forEach(lotto => {
+      if (lotto.numbers.has(currNum)) {
+        lotto.addMatchNumbers();
+      }
+    });
+  }
+}


로또 인스턴스가 갖고 있는 6개의 숫자들과 실제 당첨 숫자들을 비교하여, 매칭되는 숫자가 있다면 해당 로또 인스턴스의 matchNumbers의 개수를 증가시켜 줍니다.

lotto의 책임에 변화가 생기면 이 부분도 같이 바뀌게 됩니다.
맞는 숫자가 있는지 여부는 로또 자체가 가진 책임이 아닌것으로 보입니다.

일반적으로 util은 특정 도메인(로또)을 알지 못하고 범용적(로또와 상관없는 전혀 다른 프로젝트)으로 사용 할 수 있는 것들의 모음입니다.
따라서 파라미터로 로또를 받아서 로또의 상태를 바꾸는 함수는 로또를 강하게 알고 있는것이기 때문에 유틸이라고 보기 어렵고 로또의 어떤 문제를 해결하는 역할이어야 합니다.
따라서 이 utils.js 에서 로또를 알고 있는 모든 함수는 다 다른곳으로 옮겨져야 할 것으로 보입니다.

zigsong · 2021-02-25T03:17:44Z

src/js/views/View.js

+  show() {
+    this.$element.style.display = 'block';
+    return this;
+  }
+
+  hide() {
+    this.$element.style.display = 'none';
+    return this;
+  }
+
+  on(event, eventHandler) {
+    this.$element.addEventListener(event, eventHandler);
+    return this;
+  }
+
+  emit(event, data) {
+    const newEvent = new CustomEvent(event, { detail: data });
+    this.$element.dispatchEvent(newEvent);
+    return this;
+  }


모든 view가 상속 받고 있는 View class의 메소드들에 return this를 작성하여, view를 생성하여 사용할 때 메소드 체이닝이 가능하게끔 했습니다.

zigsong · 2021-02-25T03:20:08Z

src/js/views/WinningResultView.js

+    $('main').addEventListener('click', () => this.closeModal());
+    $('.modal-inner').addEventListener('click', e => e.stopPropagation());


modal이 열려있을 때 modal 영역 바깥(main)을 클릭하면 modal이 닫히고,
이때 main은 modal-inner(modal 영역 안)를 포함하고 있기 때문에 modal-inner에는 event의 전파를 막는 방식(stopPropagation)을 적용했습니다. 의도대로 만들기 위해 이렇게 작성하는 것이 괜찮아보이는지 여쭤보고 싶습니다!

보통 모달을 띄울 때 화면 전체를 가리는 모달의 백그라운드용 div 를 모달과 함께 띄우고 그 백그라운드에 이벤트리스너를 걸어서 모달 바깥 클릭을 제어하는식으로 구현 합니다.
그 과정중에 이벤트 전파로 인해 의도와 다르게 동작을 하면 그 때 stopPropagation 을 걸어줄 지점을 찾고는 합니다.

zigsong · 2021-02-25T03:24:04Z

src/js/views/WinningResultView.js

+  constructor($element) {
+    super($element);
+    this.$modal = $('.modal');
+    this.winningNumbers = {};


당첨 번호 7개(6개 + 보너스)가 들어오는 곳으로,
{1: 27, 2: 41, 3: 4, 4: 15, 5: 32, 6: 18, 7: 20}과 같은 식으로 채워집니다.
단순 array가 아닌 객체로 만든 이유는, 사용자가 특정 입력 칸에 숫자를 입력했다가 바꾸고 싶어 다시 입력할 경우 해당 순서에 들어갈 숫자를 바꿔줘야 하기 때문에 당첨 번호의 순서에 매칭되는 key를 만들면 좋겠다고 생각했기 때문입니다.

좀 더 나은 방법을 사용해보고 싶었는데, 떠오르지 않아 간단하게 객체로 해결했습니다.
다른 좋은 방법이 있을까요? 🤔

당첨번호는 숫자 6개와 보너스 숫자 하나로 구분되므로 코드에서도
[27, 41, 4, 15, 32, 18]
20
과 같은식으로 이를 그대로 표현해주는게 좋을 것 같습니다. (두 숫자를 구분 할 네이밍은 적절히..)
input 에서 들어온 값을 바꿔줘야 하는것과 같은식으로 화면의 어떤 상황 때문에 그를 해결하기 위해 모델을 설계하면 화면 디자인이 바뀔때마다 js 코드도 바꿔야 하는 경우가 생깁니다.

zigsong · 2021-02-25T06:35:07Z

src/js/LottoController.js

+    if (!this.isResultCalculated) {
+      compareNumbers(this.lottos, winningNumbers);
+      this.lottos.forEach(lotto => lotto.updateRank());
+      countByRank(this.lottos, this.rankCounts);
+    }


'결과 확인하기' 버튼을 누르면 inputNumbersHandler가 실행되어 결과를 계산하고 모달을 보여줍니다.
그런데 모달을 닫은 후 (구매한 로또들과 당첨 번호를 변경하지 않은 상태에서) 다시 '결과 확인하기'를 누르면 한 번 더 계산이 되어 결과가 계속 누적되어 이상하게 꼬여버려서, isResultCalculated라는 bool flag를 두어 '결과 확인하기'를 처음 눌렀을 때만 계산이 실행되도록 하였습니다.

removeEventListener 등의 방법도 고려해보았으나, '결과 확인하기' 버튼이 수행하는 1. 계산하기 2. 모달 띄우기 두 개의 기능이 모두 없어져 버려서 사용하지 못했습니다. (계산은 1번만 하되, 모달은 계속 보여줘야 합니다.)
이렇게 bool 변수를 두는 것도이아직 크게 문제 될 것이라고 생각하지 않아서 사용했는데, 혹시 다른 좋은 방법이 있을지 궁금합니다!

말씀하신것처럼 모달을 닫은 후 다시 '결과 확인하기'를 눌렀다는 것은 인풋이 바뀐것이 아니기 때문에 동일한 결과가 나와야 합니다.
누적이 되어 이상한 결과가 나왔다면 누적을 시키는 그 로직에 문제가 있거나 누적을 하게끔 진행되는 코드의 전체 프로세스가 문제이므로 문제가 되는 근본적인 원인을 찾아서 수정해야 합니다.
isResultCalculated 같은 flag를 사용하는 것은 별것 아닌것 같지만 임시방편으로 문제를 덮어두는것이라서 폭탄 같은(언제 터질지 모르는) 위험한 코드 입니다 :)
당장 이것부터 해결하는것은 쉽지 않아보이니 흩어져 있는 코드들을 역할에 맞게 적절히 부여해준다면 나중에는 의외로 쉽게 풀릴 수도 있습니다.

wow9144

Lotto가 본인의 관심사가 아닌것을 너무 많은것을 알고 있습니다.
피드백 드린것에 따라 수정하시게 되면 코드가 좀 많이 바뀌게 될 것 같은데, Lotto의 책임을 덜어내고 그 책임을 누가 질 것인가 부터 생각을 시작 하시면 도움이 될 것 같습니다. :)

wow9144 · 2021-02-27T05:10:20Z

index.html

@@ -53,44 +63,84 @@ <h4 class="mt-0 mb-3 text-center">당첨 번호</h4>
                  <input
                    type="number"
                    class="winning-number mx-1 text-center"
+                    aria-label="winning-number-1"


aria 프리픽스가 붙은 속성들은 ARIA(Accessible Rich Inernet Applications) 라고... 웹접근성을 위한 속성입니다.
그래서 명시적인 label이 필요하지 않은 경우 aria 를 사용하는 방법이라는 말이 틀렸다고 할 수는 없지만 근본적으로 '웹접근성'을 위한것이라는것을 모르면 오해 할 소지가 있어서 이 부분 첨언 드립니다. :)

wow9144 · 2021-02-27T05:37:42Z

src/js/Lotto.js

+    this._numbers = new Set();
+    this.matchingNumbers = 0;
+    this.isMatchBonus = false;
+    this._rank = Infinity;


랭크가 과연 로또의 관심사 일까? 라는 관점에서 한 번 생각해보시면 좋을 것 같습니다. (matchingNumbers, isMatchBonus도 마찬가지 입니다.)
로또는 그냥 겹치지 않는(Set) 숫자가 적힌 종이 쪽지인데 다른 정보들이 들어오면서 역할이 비대 해졌습니다.
이미 발행된 로또를 가지고 뭔가를 하는 역할이 따로 필요할 것 같습니다.

wow9144 · 2021-02-27T05:52:25Z

src/js/Lotto.js

+  // Test를 위한 setter 생성
+  set numbers(numsArray) {
+    this._numbers = numsArray;
+  }


네 문제가 있습니다.
이미 발행된 로또의 this._numbers에다가 외부에서 직접 값을 할당하는 조작을 막기위해(실제로 코드가 아직 그렇게 되지는 않았지만) setter 없이 getter만 두신것인데, 단지 테스트를 위해 setter를 열어야 한다면 처음의 의도가 망가집니다.
setter 열어야만 테스트가 된다면 그것은 setter가 없어서 테스트 되지 않는게 아니라 근본적인 다른 문제가 있어서 그럴 수 있으니 다른 부분을 살펴보시는게 더 좋을 것 같습니다.
(테스트가 잘못되었든지, Lotto의 컨스트럭터가 잘못되었든지, getter만 두려고 했던 애초의 의도를 원복해서 numbers를 공개하도록 하든지 기타 등등)

결론: 오로지 테스트를 위해서 소스코드에서 사용하면 안되는 공개 메서드를 만들지는 않습니다.

wow9144 · 2021-02-27T05:57:37Z

src/js/Lotto.js

+      case 5:
+        this._rank = this.isMatchBonus ? 2 : 3;
+        break;


스위치 케이스 내에서의 조건 분기에 따른 문제는 없습니다.

if () { } else if () { if () { // 여기 } }

위와 근본적으로 다르지 않습니다.
중첩이 깊어지면 코드를 읽기 힘들고 유지보수도 힘들어지기 때문에 중첩이 깊어지지만 않게만 관리하면 별다른 문제는 없습니다.
일반적으로 중첩이 3~4단계를 넘어가면 문제가 있다고 볼 수 있습니다.
지금은 2단계이니 괜찮습니다.

wow9144 · 2021-02-27T05:58:36Z

src/js/Lotto.js

+        this._rank = 5;
+        break;
+      default:
+        this._rank = Infinity;


랭크가 인피니티 보다는 랭크가 없다는게(null) 개념적으로 받아들여지기 쉬울 것 같습니다.

wow9144 · 2021-02-27T07:00:09Z

src/js/LottoController.js

+    this.inputPriceView.show().resetInputPrice();
+    this.purchasedLottosView.hide().resetToggleSwitch();
+    this.winningResultView.hide().resetWinningNumbers();


상속을 할 경우 부모의 this는 상속받은 자식을 가르킨다는 점을 이용해서 공통적인 메서드를 부모에 두는 패턴으로 중복을 제거할 때 유용하게 쓰입니다.
부모의 show, hide 가 없다면 view 로쓰이는 모든 클래스에서 매번 동일하게 show와 hide를 구현해야 하니까 중복을 제거 할 때 용이합니다.
상속은 이런 장점이 있는 반면에 잘못된 설계를 하게 되면 나중에 고치기가 너무 어려워지는 문제가 있습니다.
'상속 vs 합성' 으로 검색해보시면 관련 정보를 얻으실 수 있으며 쉽지 않은 내용이니 너무 급하게 생각하실 필요는 없습니다.
this를 리턴하는 '메서드 체이닝'도 장단점이 함께 존재 하니 검색해서 같이 보시면 좋을 것 같습니다. :)

현재 코드 구성에서는 나쁘지 않은것 같습니다.

wow9144 · 2021-02-27T07:06:53Z

src/js/utils/utils.js

+function checkMatchingNums(lottos, winningNumbers) {
+  let i = 0;
+  while (i++ < 6) {
+    const currNum = winningNumbers[i];
+    lottos.forEach(lotto => {
+      if (lotto.numbers.has(currNum)) {
+        lotto.addMatchNumbers();
+      }
+    });
+  }
+}


일반적으로 util은 특정 도메인(로또)을 알지 못하고 범용적(로또와 상관없는 전혀 다른 프로젝트)으로 사용 할 수 있는 것들의 모음입니다.
따라서 파라미터로 로또를 받아서 로또의 상태를 바꾸는 함수는 로또를 강하게 알고 있는것이기 때문에 유틸이라고 보기 어렵고 로또의 어떤 문제를 해결하는 역할이어야 합니다.
따라서 이 utils.js 에서 로또를 알고 있는 모든 함수는 다 다른곳으로 옮겨져야 할 것으로 보입니다.

wow9144 · 2021-02-27T07:08:34Z

src/js/utils/utils.js

+import { LOTTO_NUMBERS, LOTTO_WINNING_PRICE } from './constants.js';
+
+export function getRandomNumber() {
+  return Math.floor(Math.random() * LOTTO_NUMBERS.LOTTO_MAX_NUM) + 1;


LOTTO_NUMBERS.LOTTO_MAX_NUM 을 내부에서 알고 있지 않고 외부에서 인자로 주입한다면 getRandomNumber 는 보다 범용적으로 쓰일 수 있을 것입니다.
getRandomNumber의 파라미터 이름에는 당연히 로또가 들어가면 안되고요.

범용적으로 사용되기 때문에 utils로 뺐습니다!
혹시 말씀하신 것에 제가 캐치하지 못한 다른 의도가 있을까요? 😮

getRandomNumber라는 이름은 말그대로 random한 숫자를 반환 할 것으로 기대되는 이름인데,
정작 로직은 1~45까지의 숫자중 랜덤하게 반환하는것으로써 숫자의 구간이 존재합니다.
따라서 이름과 로직이 다릅니다.
그래서 getRandomNumber(min, max) {...} 같은식으로 구간을 준다든지 해서 좀 더 범용적으로 쓸 수 있는 함수를 만드셨으면 하는 의도였습니다.

wow9144 · 2021-02-27T07:17:04Z

src/js/views/WinningResultView.js

+  constructor($element) {
+    super($element);
+    this.$modal = $('.modal');
+    this.winningNumbers = {};


당첨번호는 숫자 6개와 보너스 숫자 하나로 구분되므로 코드에서도
[27, 41, 4, 15, 32, 18]
20
과 같은식으로 이를 그대로 표현해주는게 좋을 것 같습니다. (두 숫자를 구분 할 네이밍은 적절히..)
input 에서 들어온 값을 바꿔줘야 하는것과 같은식으로 화면의 어떤 상황 때문에 그를 해결하기 위해 모델을 설계하면 화면 디자인이 바뀔때마다 js 코드도 바꿔야 하는 경우가 생깁니다.

wow9144 · 2021-02-27T07:37:42Z

src/js/LottoController.js

+    if (!this.isResultCalculated) {
+      compareNumbers(this.lottos, winningNumbers);
+      this.lottos.forEach(lotto => lotto.updateRank());
+      countByRank(this.lottos, this.rankCounts);
+    }


말씀하신것처럼 모달을 닫은 후 다시 '결과 확인하기'를 눌렀다는 것은 인풋이 바뀐것이 아니기 때문에 동일한 결과가 나와야 합니다.
누적이 되어 이상한 결과가 나왔다면 누적을 시키는 그 로직에 문제가 있거나 누적을 하게끔 진행되는 코드의 전체 프로세스가 문제이므로 문제가 되는 근본적인 원인을 찾아서 수정해야 합니다.
isResultCalculated 같은 flag를 사용하는 것은 별것 아닌것 같지만 임시방편으로 문제를 덮어두는것이라서 폭탄 같은(언제 터질지 모르는) 위험한 코드 입니다 :)
당장 이것부터 해결하는것은 쉽지 않아보이니 흩어져 있는 코드들을 역할에 맞게 적절히 부여해준다면 나중에는 의외로 쉽게 풀릴 수도 있습니다.

zigsong

Lotto 모델에서 불필요한 부분들을 빼고, 당첨 결과 로직을 단순화해 보았습니다.
혹시 제 코드가 리뷰 주신 의도와 다르다면 지적 부탁드리겠습니다! 🤓

zigsong · 2021-02-27T15:31:38Z

src/js/Lotto.js

+  inputManualNumbers(numbers) {
+    this._numbers = new Set(numbers);
+  }


이 함수는 현재 진행 중인 3단계 미션(수동구매)에 이미 사용하고 있기 때문에, 오직 테스트만을 위한 set numbers()함수를 지우고 새로 넣었습니다.

zigsong · 2021-02-27T15:32:41Z

src/js/utils/lottoProcessor.js

@@ -0,0 +1,51 @@
+import { LOTTO_NUMBERS, LOTTO_WINNING_PRICE } from './constants.js';
+
+export default class LottoProcessor {


로또 당첨 결과를 계산하는 함수들을 utils 파일에서 모두 분리하여 새로운 모듈로 만들었습니다.

zigsong · 2021-02-27T15:33:26Z

src/js/LottoController.js

@@ -71,21 +66,22 @@ export default class LottoController {
  }

  inputNumbersHandler(winningNumbers) {
+    console.log(winningNumbers);


앗 여기는 지우고 다시 올리겠습니다...😂

zigsong · 2021-02-27T15:36:12Z

src/js/utils/lottoProcessor.js

+  checkMatchingNums() {
+    this.lottos.forEach(lotto => {
+      const matchNumbers = [...lotto.numbers].filter(number =>
+        this.winningNumbers.includes(number)
+      );
+      const isMatchBonus = lotto.numbers.has(this.bonusNumber);
+      this.updateRankCounts(matchNumbers.length, isMatchBonus);
+    });
+  }


말씀주신 대로 Lotto 모델의 matchingNumbers, isMatchBonus, rank 등의 프로퍼티를 제거하고,
controller 자체에서 등수를 계산할 수 있도록 로직을 바꿨습니다.
리뷰의 의도대로 잘 작성한지는 모르겠지만, Lotto 모델이 더욱 단순해지고 당첨 결과 계산 로직이 간결해진 것 같은 느낌입니다 🙄🙃

zigsong · 2021-02-27T15:37:15Z

src/js/utils/utils.js

+import { LOTTO_NUMBERS, LOTTO_WINNING_PRICE } from './constants.js';
+
+export function getRandomNumber() {
+  return Math.floor(Math.random() * LOTTO_NUMBERS.LOTTO_MAX_NUM) + 1;


범용적으로 사용되기 때문에 utils로 뺐습니다!
혹시 말씀하신 것에 제가 캐치하지 못한 다른 의도가 있을까요? 😮

zigsong · 2021-02-27T15:38:45Z

src/js/views/WinningResultView.js

+    $('#winning-numbers-form').addEventListener('submit', e => {
+      e.preventDefault();
+      this.winningNumbers = [
+        ...$('#winning-numbers-form').getElementsByTagName('input'),
+      ].map(input => Number(input.value));
+    });


winningNumbers를 object에서 key가 없는 array로 바꾸고, 각 input의 값들을 제대로 반영하게끔 했습니다.
현재 진행 중인 3단계에서는 이렇게 썼는데, 왜 2단계에서는 여러 개의 input 값들을 이상하게 받아왔을까요 😞
이렇게 수정했습니다!

zigsong

.

zigsong · 2021-02-27T15:44:35Z

src/js/LottoController.js

-    if (!this.isResultCalculated) {
-      lottoProcessor.checkMatchingNums();
-      lottoProcessor.calculateEarningRate(this.purchasedPrice);
-      this.isResultCalculated = true;
-    }


앞의 커밋에서
Lotto 모델의 역할을 단순화하고 당첨 결과 계산 로직을 분리하니 isResultCalculated를 쓰지 않아도 결과 확인 버튼을 누를 때마다 계산 결과를 누적하여 덮어씌우는 문제가 사라졌습니다. lotto 객체 자체에는 변화가 없고, lottoProcessor 인스턴스를 계속 새롭게 만들어 계산하고 있기 때문입니다.
하지만 이 말은, 여전히 결과 확인 버튼을 누를 때마다 당첨 결과를 내부적으로 계속 계산하고 있다는 뜻이기도 합니다.
이 부분은 어쩔 수 없는 걸까요? 🤔

- toggle switch off when reset - calculate result only once

- rename winning number form - separate lotto calculate(processor) logics - change winningnumbers type (from object to array) - remove logics from Lotto object

zigsong · 2021-02-28T07:52:11Z

1단계 수정한 내용 rebase하여 리뷰 요청 다시 드립니다!

wow9144

역할이 나눠져서 많이 좋아진 것 같습니다.
코멘트 드린 부분은 3단계에서 계속 이어가시면 될 것 같습니다.
화이팅!

wow9144 · 2021-02-28T13:55:21Z

src/js/utils/utils.js

+import { LOTTO_NUMBERS, LOTTO_WINNING_PRICE } from './constants.js';
+
+export function getRandomNumber() {
+  return Math.floor(Math.random() * LOTTO_NUMBERS.LOTTO_MAX_NUM) + 1;


getRandomNumber라는 이름은 말그대로 random한 숫자를 반환 할 것으로 기대되는 이름인데,
정작 로직은 1~45까지의 숫자중 랜덤하게 반환하는것으로써 숫자의 구간이 존재합니다.
따라서 이름과 로직이 다릅니다.
그래서 getRandomNumber(min, max) {...} 같은식으로 구간을 준다든지 해서 좀 더 범용적으로 쓸 수 있는 함수를 만드셨으면 하는 의도였습니다.

wow9144 · 2021-02-28T14:04:53Z

src/js/views/WinningResultView.js

+    $('main').addEventListener('click', () => this.closeModal());
+    $('.modal-inner').addEventListener('click', e => e.stopPropagation());


보통 모달을 띄울 때 화면 전체를 가리는 모달의 백그라운드용 div 를 모달과 함께 띄우고 그 백그라운드에 이벤트리스너를 걸어서 모달 바깥 클릭을 제어하는식으로 구현 합니다.
그 과정중에 이벤트 전파로 인해 의도와 다르게 동작을 하면 그 때 stopPropagation 을 걸어줄 지점을 찾고는 합니다.

zigsong · 2021-04-24T09:04:50Z

학습로그

모든 View들이 상속 받는 parent View class

chaining을 위한 return this

export default class View {
 constructor() { ... }

  show() {
  // ... 
    return this;
  }

  hide() {
  // ... 
    return this;
  }

class SomeView extends View { ... }

const someView = new SomeView();
someView.show().hide(); // chaining

상속과 합성 (ref)

상속

is-a 관계
공통적인 메서드를 부모에 두는 패턴으로 작성하여 중복 제거에 유리하다.
부모 클래스와 자식 클래스 간의 결합도가 높아지고 자식 클래스는 부모 클래스의 구현에 의존한다. 이는 코드의 수정을 어렵게 만든다. 부모 클래스의 구조가 변하면 자식 클래스가 영향을 받는다.

합성

has-a 관계
이미 구현되어 있는 기능을 빌려 쓴다
관련 코드 링크
링크
- https://mingcoder.me/2020/07/19/Programming/JavaScript/inheritance-vs-composition/

CustomEvent, dispatchEvent

이벤트의 이름과 전달 데이터를 명시하여 이벤트를 사용자화할 수 있다.

obj.addEventListener("go", function(e) { console.log(e + 'going' });

const event = new CustomEvent("go", { detail: 'zig' });

obj.dispatchEvent(event);

링크
- https://developer.mozilla.org/ko/docs/Web/API/CustomEvent/CustomEvent

(웹접근성) html aria-label

ARIA

접근가능한 리치 인터넷 어플리케이션(Accessible Rich Internet Applications, ARIA)
장애를 가진 사용자가 웹 콘텐츠와 웹 어플리케이션(특히 JavaScript를 사용하여 개발한 경우)에 더 쉽게 접근할 수 있는 방법을 정의하는 여러 특성
role과 aria-* 속성을 사용한다.
ex) 스크린 리더 등의 보조기기 사용 시 요소를 명확하게 식별하기 위해 사용

aria-label

label의 텍스트를 표시하지 않을 경우 사용한다. 태그의 목적을 설명한다.

<input
  type="number"
  class="winning-number mx-1 text-center"
  aria-label="winning-number-1"
/>

aria-labelledby가 우선순위가 더 높다
링크
- https://developer.mozilla.org/ko/docs/Web/Accessibility/ARIA

기타

util의 의미 ref

하나의 프로젝트에서 util은 특정 도메인은 알지 못하고 범용적으로 사용할 수 있는 것들의 모음

input change vs input event

change - input 요소의 값이 바뀌었을 때 발생. 포커스를 잃거나, 엔터를 입력하는 등의 경우에 발생한다.
input - input 요소의 value 속성이 바뀔 때마다 발생

zigsong changed the title ~~Zig step2~~ [2단계 - 행운의 로또 미션] 지그(송지은) 미션 제출합니다. Feb 25, 2021

pocojang requested a review from wow9144 February 25, 2021 03:05

pocojang assigned wow9144 Feb 25, 2021

zigsong commented Feb 25, 2021

View reviewed changes

wow9144 suggested changes Feb 27, 2021

View reviewed changes

zigsong commented Feb 27, 2021

View reviewed changes

zigsong force-pushed the zig-step2 branch from 948b71e to 146b3e0 Compare February 27, 2021 15:40

zigsong commented Feb 27, 2021

View reviewed changes

zigsong requested a review from wow9144 February 27, 2021 15:46

devhyun637 and others added 20 commits February 28, 2021 16:48

[1단계 - 행운의 로또 미션] 티케(조은현) 미션 제출합니다 (woowacourse#26)

a025f39

docs : update README.md for step 2 mission

2c9a3b1

feat : create View module

10d2fda

feat : separate winningNumberInput View

42ef459

refactor : separate InputPriceView module

cf6df4a

refactor : separate PurchasedLottosView module

3e1b379

refactor : add attributes to input tags

6ab8ae7

feat : create modal open event in WinningNumberInput

3552554

test(refactor) : select specific input by id

9bff3f6

test : enable result button if all winning nums filled

33f17f8

refactor : update default export to export

c59832c

feat : enable result button if all winning nums filled

19914a9

test : show modal when click result button

5c9dd46

feat : show modal and check unique validation

b1182c6

test : winning number unique validation

820e874

test : close modal when click x button

9631e61

test : add mock data for compare numbers

d5132eb

feat : create compare winning numbers logic

8927346

test : get correct result count

1663e86

feat : show correct result count in modal

7e2f1d1

devhyun637 and others added 13 commits February 28, 2021 16:49

test : calculate earning rate

b645d84

feat : calculate and show earning rate

2378df1

test : restart when click reset button

74a755a

feat : restart when click reset button

9af3261

test : move input focus when type 2 letters

070e0fc

test : move input focus when type 2 letters

3dd44fc

test : close modal when clicked outside of modal

9fd710b

feat : close modal when clicked outside of modal

052b8ab

refactor : rename WinningNumbersInput to WinningResultView

df50b90

refactor : rearrange code

4c8ea46

fix : fix bugs

35564b9

- toggle switch off when reset - calculate result only once

refactor:

1dbc84c

- rename winning number form - separate lotto calculate(processor) logics - change winningnumbers type (from object to array) - remove logics from Lotto object

refactor: remove isResultCalculated logic

26636a8

zigsong force-pushed the zig-step2 branch from 146b3e0 to 26636a8 Compare February 28, 2021 07:51

wow9144 approved these changes Feb 28, 2021

View reviewed changes

wow9144 merged commit fc4bf70 into woowacourse:zigsong Feb 28, 2021

		$('main').addEventListener('click', () => this.closeModal());
		$('.modal-inner').addEventListener('click', e => e.stopPropagation());

		@@ -0,0 +1,51 @@
		import { LOTTO_NUMBERS, LOTTO_WINNING_PRICE } from './constants.js';

		export default class LottoProcessor {

[2단계 - 행운의 로또 미션] 지그(송지은) 미션 제출합니다. #43

[2단계 - 행운의 로또 미션] 지그(송지은) 미션 제출합니다. #43

Conversation

zigsong commented Feb 25, 2021 • edited Loading

✅ 2단계 미션 TODO

🗂 Directory

🛠 View & 구조도

🤓 고민한 점

🤔 궁금한 점

zigsong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigsong Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigsong Feb 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wow9144 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigsong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigsong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigsong commented Feb 28, 2021

wow9144 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigsong commented Apr 24, 2021

학습로그

모든 View들이 상속 받는 parent View class

상속과 합성 (ref)

상속

합성

CustomEvent, dispatchEvent

(웹접근성) html aria-label

ARIA

aria-label

기타

util의 의미 ref

input change vs input event

zigsong commented Feb 25, 2021 •

edited

Loading

zigsong Feb 25, 2021 •

edited

Loading

zigsong Feb 27, 2021 •

edited

Loading