[2단계 - 행운의 로또 미션] 하리(이아현) 미션 제출합니다. by LAH1203 · Pull Request #122 · woowacourse/javascript-lotto

LAH1203 · 2022-03-02T12:17:07Z

안녕하세요 Vallista님~😁

✨데모 페이지✨

📦src
 ┣ 📂css
 ┃ ┣ 📜global.css
 ┃ ┣ 📜index.css
 ┃ ┣ 📜popup.css
 ┃ ┣ 📜purchaseMoney.css
 ┃ ┣ 📜purchasedLotto.css
 ┃ ┣ 📜reset.css
 ┃ ┣ 📜toggle.css
 ┃ ┗ 📜winningNumber.css
 ┣ 📂js
 ┃ ┣ 📂__tests__
 ┃ ┃ ┗ 📜util.test.js
 ┃ ┣ 📂constants
 ┃ ┃ ┗ 📜index.js
 ┃ ┣ 📂controller
 ┃ ┃ ┗ 📜LottoMachineController.js
 ┃ ┣ 📂model
 ┃ ┃ ┗ 📜Lotto.js
 ┃ ┣ 📂util
 ┃ ┃ ┣ 📜common.js
 ┃ ┃ ┗ 📜validator.js
 ┃ ┗ 📂view
 ┃ ┃ ┣ 📜PopupView.js
 ┃ ┃ ┣ 📜PurchaseMoneyView.js
 ┃ ┃ ┣ 📜PurchasedLottoView.js
 ┃ ┃ ┣ 📜WinningNumberView.js
 ┃ ┃ ┗ 📜template.js
 ┗ 📜index.js

우선 저번 1단계 미션에서 피드백 해주신 것을 기반으로 고쳐본걸 간단하게 말해볼까 합니다!

#winning-number-container p 수정

👍 말씀해주신대로 새로운 클래스를 만들어 관리하는게 좋을 것 같아 winning-number-label 클래스명을 붙여주었습니다.

private method 지정

👍 외부에 노출되지 않는 메서드들은 private으로 지정해주었습니다.

Lottos 모델

👍 로또 매니저가 필요할까요? 라고 말씀하신거에서 저도 모르게 아니요! 라고 답했습니다 😂 저도 굳이 필요하진 않다고 느꼈기에 로또 머신 안으로 합쳐주었습니다 :)

테스트 코드

👍 말씀해주신 대로 '잘 동작하지 않는 경우'에 맞춰 추가적인 테스트 코드를 작성해주었습니다.
👎 Lottos 모델이 로또 머신에 합쳐졌으므로 기존의 Lottos 검증 테스트 코드를 로또 머신 생성으로 테스트해봐야 하나, 테스트 환경을 jsdom으로 설정하고 mock을 사용하여 생성 및 테스트를 시도해보았음에도 로또 머신 내부에서 발생하는 alert를 감지하지 못하는 오류가 발생하였습니다 :( 테스트 코드 내부에서 발생시키는 alert는 감지하는데 왜 로또 머신 내부에서 발생하는 alert는 감지하지 못하는지, 그 이유를 잘 모르겠습니다..

toggle의 중복

👍 배열의 forEach를 사용하여 중복을 없애주었습니다.

그리고 저번에 질문을 남겨주셨는데 제가 못 봐서 답을 못 했던 부분이 있었습니다. 그 부분에 대한 답변입니다 :)

MVC를 쓰신 이유가 있을까요? 있다면 왜 써야했을까요!? 다양한 패턴들이 있는데 MVC를 사용하신 이유가 궁금합니다!

솔직히 말하면 프리코스 때부터 사용했던 패턴이라 익숙해서 썼다는 이유가 가장 클 것 같습니다.
또, 제가 패턴을 꼭 써야한다는 강박관념에 사로잡혀 있었어요. 그래서인지 처음에 구조를 잡을 때부터 UI와 도메인을 분리하는 디자인 패턴 중에 무엇을 고를까 고민하고 있었습니다.
앞으로 패턴에 대한 사용은 지양하는 방향으로 가려고 합니다! (패턴을 아예 배척하겠다는 것은 아니지만, 패턴을 사용함으로 제가 짤 수 있는 코드 구조의 한계를 저 스스로 만들어버린 느낌이었어요..)

감사합니다 😊

LAH1203 · 2022-03-02T13:49:31Z

다시 보니 구현 기능 문서에 팝업 기능은 적어놓지 않아서 추가하였습니다 :)

Vallista

안녕하세요 하리님!
빠르게 반영해주셔서 너무 감사합니다!

코드 적용된 것을 보았고, 수정할 사항을 코맨트로 넣어두었습니다!
MVC를 기반으로 하시는 것도 좋지만, 프로젝트 규모에 적합한 구조로 코드를 짜는게 더 좋겠습니다!

그리고 for 보단 reduce로 해결할 수 있는 상황인지 확인해보시고,
왜 고차함수를 쓰는게 for 보다 좋을지 생각해보면 좋겠습니다!

Vallista · 2022-03-04T06:02:02Z

src/css/popup.css

+  font-size: 20px;
+  font-weight: 600;
+  letter-spacing: 0.15px;
+  margin: 30px 0 32px 0;


30px 0 32px 로도 되겠습니다~

나름 정리를 다 했다고 생각했는데 아직 css에서 테스트의 흔적들이 많이 있었군요,,😭 수정하겠습니다 !

Vallista · 2022-03-04T06:02:30Z

src/css/popup.css

+  left: 320px;
+  top: 16px;
+  border-radius: 0px;
+  color: #323232;


inline으로 color를 지정해주기보다 variable로 만드는건 어떨까요?

아닛.. 아직 빼지 않은 색이 더 있었네요 :(
찾아보니 global로 분리하지 않은 색이 하나가 더 있어서 그 친구랑 같이 빼주도록 하겠습니다 ! 알려주셔서 감사합니다 :)

Vallista · 2022-03-04T06:02:52Z

src/css/popup.css

+th, td {
+  border-top: 2px solid var(--table-line-color);
+  border-bottom: 2px solid var(--table-line-color);
+  padding: 10px 10px;


padding: 10px 10px 이면, padding 10px 과 다른게 없을 것 같네요!

그러네요... 제가 왜 저 상태로 놔뒀을까요.... 바로 수정하겠습니다 !

Vallista · 2022-03-04T06:03:01Z

src/css/popup.css

+}
+
+#result-percent {
+  margin: 16px 0 32px 0;


16px 0 32px

앗 감사합니다 :)

Vallista · 2022-03-04T06:04:21Z

src/js/__tests__/util.test.js

+});
+
+describe('당첨번호 테스트', () => {
+  it('값을 전부 입력하지 않으면 에러가 발생한다.', () => {


ㅋㅋㅋㅋㅋ 에러에 대한 테스트가 필요하다고 피드백 드렸더니 정말 성실하게 다 해주셨군요!!

좋습니다..! 그런데 성공하는 경우도 함께 있으면 더 좋겠네요 ㅋㅋㅋㅋ (죄송)

제가 너무 에러인 경우만 테스트했군요 😂 성공 케이스 테스트도 바로 추가하겠습니다!

Vallista · 2022-03-04T06:12:27Z

src/js/util/common.js

+const countCorrectNumber = (lottoList, winningNumbers) => {
+  let correctCount = 0;
+
+  for (let i = 0; i < winningNumbers.length; i++) {


여기도 reduce가 될 것 같..

ㅋㅋㅋㅋ 저 항상 reduce는 어디에 쓰이는거지, 모호하다는 생각을 갖고 있었는데, 덕분에 어디서 어떻게 쓰일지 감이 잡혀가는 것 같아요! 감사합니다 😊

Vallista · 2022-03-04T06:12:35Z

src/js/controller/LottoMachineController.js

+
+    const results = getRanks(this.lottos, numbers);
+    const totalReward = calculateTotalReward(results);
+    // 수익률 = (총 상금 / 구매 금액)을 퍼센트 환산


주석 좋네요!

지난번 1단계 피드백에서 주석을 달아주면 좋을 것 같다고 남겨주신 부분을 저는 본인이 짠 (수학적?) 계산이 들어간 부분은 주석 설명을 달아주는게 좋을 것 같다고 생각했어요 ! 그래서 수익률 계산 부분도 주석으로 남겼는데, 알아주신 것 같아서 조금 뿌듯하네요 ☺️

Vallista · 2022-03-04T06:15:20Z

src/js/controller/LottoMachineController.js

-  #setEventHandler() {
-    this.view.purchaseMoneyView.addSubmitEvent(this.#onSubmitHandler.bind(this));
+  #makeLottos(lottoCount) {
+    const newLottos = Array.from({ length: lottoCount }).map(() => new Lotto());


바로 this.lottos = 넣어도 되겠어요!

오 넵 바로 넣는걸로 수정하겠습니다 👍🏻👍🏻

Vallista · 2022-03-04T07:14:27Z

src/css/popup.css

+  width: 14px;
+  left: 320px;
+  top: 16px;
+  border-radius: 0px;


Vallista · 2022-03-04T07:15:04Z

src/js/view/PopupView.js

+    this.closeButton = document.getElementById('close-button');
+  }
+
+  render(result, rewardRate) {


private과 public의 메소드 순서가 막 얽혀있네요!

순서 정리를 해보는게 어떨까요?

보기가 좀 난해하긴 하네요 😕
하는 김에 다른 파일들도 같이 순서 정리 해보겠습니다 !

LAH1203 · 2022-03-04T11:17:14Z

안녕하세요 Vallista님 😸

우선 코멘트 남겨주신거 확인하고 수정하였습니다! 추가적인 피드백이나 이건 좀 수정하거나 도입했으면 좋겠다 하는 부분이 있으면 알려주세요!

항상 친절하고 꼼꼼한 리뷰 감사합니다~ :)

+) 추가적으로, 프로젝트에 적합한 구조로 짰으면 좋겠다고 해주셔서 어떤 구조로 하면 좋을지 계속 생각해보고 있는데 자꾸 MVC 패턴밖에 생각이 안나네요ㅠㅠ.. '소규모에선 이런 구조가 좋다' 할만한 조언을 받을 수 있을까요?

Vallista · 2022-03-06T03:46:32Z

안녕하세요 하리님!
리뷰하자마자 코맨트를 달아주셔서 너무 감사합니다!

구조는 처음부터 설계하고 들어갈 수도 있지만, 프로그래밍을 잘 하는것은 "구조를 작성하는 것 (탑)", "실제 로직을 작성하는 것 (바텀)" 이러한 탑 다운, 바텀업이 잘 일어나야 진짜 프로그래밍을 잘 짜는거라 볼 수 있습니다 :)

MVC 패턴은 거시적은 목적 단위 (모델 = 도메인 (비즈니스), 뷰 = 사용자에게 보이는 영역, 컨트롤러 = 두 영역의 합친 로직을 짜는 것) 이라고 볼 수 있는데요! 목적 단위를 이렇게 생각해보지 말고, 하나의 컴포넌트 단위로 생각해봐도 좋겠습니다 :)

예를 들자면, 지금은 뷰 로직과 모델 로직이 분리되어있지만, 이 로직을 분리하지않고 합쳐보면 어떨까? 라던지요!
실제로 프론트엔드는 백앤드와 다르게, 컴포넌트에 비즈니스 로직이 녹여들 수 있기 때문에 이러한 형태로 구현을 해봐도 좋을 것 같습니다 :)

요 과제는 먼저, 2단계 미션 제가 머지를 한 후, (3단계도 있는진 모르겠지만) 추후 하리님이 생각하신 방식으로 개선해보고, 제게 DM을 주시면 따로 리뷰 해보도록 하겠습니다 :) 감사합니다!

LAH1203 added 17 commits February 28, 2022 14:11

style: label 태그에 클래스명 추가

6bc85bd

refactor: lottos 모델을 컨트롤러로 병합

c16602f

refactor: classList.toggle 코드 중복 제거

be83cad

feat: 결과 확인 팝업 템플릿 추가

024ae9e

feat: 결과 도출 및 상금 계산 로직 구현

d8dc186

feat: 결과 보여주는 팝업 추가

ad8a15f

feat: 당첨 번호 입력 시 자동으로 다음 입력창 포커싱하도록 추가

4d8f32d

style: 팝업 시 불투명 배경 설정

1046625

test: 금액 및 당첨번호 테스트 추가

f69b6ac

refactor: 결과 계산 로직 util로 분리 및 변경

c87e384

docs: 구현 기능 업데이트

8128f7a

style: 팝업 스타일 변경

abae69d

feat: 결과 팝업 보여줄 때 가장 위로 스크롤 되도록 추가

ef8c66f

docs: 로또 구입 제한 기능 삭제

2c8f5ae

feat: 재시작 시 금액 입력창 리셋 추가

dd0ed9d

style: 결과 확인 버튼 밑 공백 추가

7b896f8

style: private method 지정

b754fff

pocojang requested a review from Vallista March 2, 2022 12:17

pocojang assigned Vallista Mar 2, 2022

docs: 팝업 기능 설명 추가

0148a8c

Vallista suggested changes Mar 4, 2022

View reviewed changes

LAH1203 added 5 commits March 4, 2022 18:04

style: css 색상 분리

8d06a0a

refactor: reduce, switch-case 도입

04dfe0b

test: 정상 수행 테스트 케이스 추가

0d0d8e5

refactor: toLocaleString('ko-KR')을 Number의 메서드로 정의

acc528b

style: public, private method 위치 조정

4abee6f

Vallista merged commit b3e51a4 into woowacourse:lah1203 Mar 6, 2022

Conversation

LAH1203 commented Mar 2, 2022

✨데모 페이지✨

Uh oh!

LAH1203 commented Mar 2, 2022

Uh oh!

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

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!

LAH1203 commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vallista commented Mar 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LAH1203 commented Mar 4, 2022 •

edited

Loading