[2단계 - 웹 기반 로또 게임] 윤생(이윤성) 미션 제출합니다. #207
Conversation
- 강의에서 피드백 시간에 나온 수정사항 및 빌드방법 반영
- 다소 buy와 amount가 중복된 의미를 들고 있어, buy가 별로 큰 의미를 갖지 않는 단어라고 생각함.
There was a problem hiding this comment.
드뎌 웹의 세계로 넘어오셨군요!
피드백
도식화
구조 그리기~! 잘 하셨어요
매번 이런 활동을 하셨으면 좋겠습니다 :)
어떤 그림을 그려내느냐 훌륭한 구조를 만들어내느냐는 중요치 않아요.
이런 활동을 하시는게 중요합니다
Change View
제가 알기로는 이번 미션이.. 콘솔 기반의 앱을 웹 기반의 HTML로 옮겨내는 것으로 알고 있어요.
그런데도 콘솔 로직과 InputView.. OutputView.. 는 왜 아직도 존재하는걸까요?
혹시 콘솔과 HTML을 모두 지원하는.. 하이브리드 앱인가요?!
N회 렌더링
N회 렌더링이 굉장히 많아요.
요 부분은 필히 고치시는게 좋습니다.
온라인에서 무언가 구매하실때 배송비를 채우기 위해 추가 구매하여 무료배송으로 구매해보신 경험이 있으신가요?
지금 만드신 로직이 모두 배송비를 내면서 불필요하게 N회 구매하는 것과 비슷한 상황입니다.
즉 반복문 내부에 DOM에 접근하는 코드를 넣으시면 왜 좋지 않을까 진지하게 고민해보시고 고쳐보시는게 가장 시급해보입니다 :)
사용성
많은 걸 바라지는 않아요!!
다만 입력창에서 엔터 키는 동작하는게 좋지 않을까요?
또한... 애초에 입력창에 숫자만 입력되는게 좋지 않을까요?
그럼 이만..!
다시 리뷰요청 해주세요~
__tests__/LottoRank.test.js
Outdated
| [0, true, LottoRank.PRIZE.MISS.RANK], | ||
| [0, false, LottoRank.PRIZE.MISS.RANK], | ||
| [1, true, LottoRank.PRIZE.MISS.RANK], | ||
| [1, false, LottoRank.PRIZE.MISS.RANK], | ||
| [2, true, LottoRank.PRIZE.MISS.RANK], | ||
| [2, false, LottoRank.PRIZE.MISS.RANK], | ||
| [3, true, LottoRank.PRIZE.FIFTH.RANK], | ||
| [3, false, LottoRank.PRIZE.FIFTH.RANK], | ||
| [4, true, LottoRank.PRIZE.FOURTH.RANK], | ||
| [4, false, LottoRank.PRIZE.FOURTH.RANK], | ||
| [5, true, LottoRank.PRIZE.SECOND.RANK], | ||
| [5, false, LottoRank.PRIZE.THIRD.RANK], | ||
| [6, false, LottoRank.PRIZE.FIRST.RANK], |
There was a problem hiding this comment.
음 암호문처럼 이해하기 어려운 구조인데 어떻게 생각하세요?
There was a problem hiding this comment.
가능한 모든 테스트를 작성해보려 적어 보았는데, 가독성이 많이 떨어지네요. 테스트 코드도 코드인 점을 중요하게 생각하지 않았던 것 같습니다. 감사합니다!
index.html
Outdated
| <tbody class="result-table-body"> | ||
| <tr> | ||
| <td>3개</td> | ||
| <td>5,000</td> | ||
| </tr> | ||
| <tr> | ||
| <td>4개</td> | ||
| <td>50,000</td> | ||
| </tr> | ||
| <tr> | ||
| <td>5개</td> | ||
| <td>1,500,000</td> | ||
| </tr> | ||
| <tr> | ||
| <td>5개 + 보너스 볼</td> | ||
| <td>30,000,000</td> | ||
| </tr> | ||
| <tr> | ||
| <td>6개</td> | ||
| <td>2,000,000,000</td> | ||
| </tr> | ||
| </tbody> |
There was a problem hiding this comment.
이 부분은 결과를 출력하는 테이블에서 잘 변경되지 않는 부분을 HTML으로 작성하고 당첨된 갯수 부분만 동적으로 넣어줄려고 의도 했습니다!
index.html
Outdated
| <h3 class="lotto-subtitle">🎱 내 번호 당첨 확인 🎱</h3> | ||
| <div class="payments-container"> | ||
| <p class="payments-notice">구입할 금액을 입력해주세요.</p> | ||
| <input class="payments-input" type="text" placeholder="금액"> |
There was a problem hiding this comment.
input type을 금액을 위한 무언가로 바꾸는게 어떨까요?
index.html
Outdated
| <main> | ||
| <div id="app"> | ||
| <h3 class="lotto-subtitle">🎱 내 번호 당첨 확인 🎱</h3> | ||
| <div class="payments-container"> |
There was a problem hiding this comment.
그저 div를 사용해도 동작은 하겠지만 더 의미적인 요소를 사용하는게 어떨까요?
src/LottoController.js
Outdated
| handlePayments(payments) { | ||
| this.#lottoMachine = new LottoMachine(payments); | ||
| } | ||
|
|
||
| handleWinNumbers(winNumbers) { | ||
| this.#lottoMachine.generateWinningLotto(winNumbers); | ||
| } | ||
|
|
||
| handleBonusNumber(bonusNumber) { | ||
| this.#lottoMachine.setBonusNumber(bonusNumber); | ||
| } |
There was a problem hiding this comment.
이렇게 추가적으로 래핑하는 함수를 만들때의 이점이 무엇이 있을까요?
There was a problem hiding this comment.
추가적인 로직이 필요할 때 handle ~ 에만 추가를 해주면 편할 것이라는 생각을 했습니다.
또한 이런 함수를 호출하는 부분은 실행 흐름만 관리하는데 세부적인 로직은 알 필요가 없다고 판단해서 이것도 하나의 이점이라고 생각합니다.
There was a problem hiding this comment.
음 저는 그저 함수를 한번 감싸주는 보조의 역할을 수행하는 것으로 밖에는 안보여서 뭔가 아쉽긴 하네요 😢
src/validation.js
Outdated
| LOTTO_NUMBER_RANGE_MAX, | ||
| LOTTO_NUMBER_SIZE, | ||
| } from './util/constants'; | ||
|
|
||
| const isInteger = (value) => Number.isInteger(value); |
There was a problem hiding this comment.
움.. 뒤늦게 봤지만 이건 너무 과한 추상화 같은데 어떻게 생각하시나요?
src/util/getNumberValue.js
Outdated
| @@ -0,0 +1,4 @@ | |||
| const getNumberValue = (numberInput) => { | |||
There was a problem hiding this comment.
요것도.. 굳이 추상화 되어야할 녀석인지 잘 모르겠군요
There was a problem hiding this comment.
동의합니다! 다시 보니 추상화의 의미가 없군요
이하 동문입니다!
src/validation.js
Outdated
| const isInteger = (value) => Number.isInteger(value); | ||
| const isPositiveNumber = (number) => number > 0; | ||
| const isValidLottoNumberRange = (number) => { | ||
| return number >= LOTTO_NUMBER_RANGE_MIN && number <= LOTTO_NUMBER_RANGE_MAX; | ||
| }; | ||
|
|
||
| export const isPositiveInteger = (value) => isInteger(value) && isPositiveNumber(value); | ||
|
|
||
| export const isValidRestartCommand = (command) => command === 'y' || command === 'n'; |
There was a problem hiding this comment.
이미 만들어놓은 RESTART_COMMAND, EXIT_COMMAND를 사용하셔야하지 않을까요?
There was a problem hiding this comment.
헉 맞네요.. 리팩토링할때 미쳐 살피지 못했습니다 감사합니다!
src/step2-index.js
Outdated
| webController.receiveBonusNumberInput(bonusNumber); | ||
| }; | ||
|
|
||
| const changeCSSByResultBtnClick = () => { |
There was a problem hiding this comment.
- 굳이 CSS를 변경한다는 의미를 부여할 필요가 있을까요?
- 이 함수가 클릭이 아닌 다른 이벤트에 호출된다면 이름을 변경하시는걸까요?
There was a problem hiding this comment.
CSS의visibility속성을 이용해 입력 전/후를 보여주므로, 따로 분리하면 좋지 않을까 생각하여 의미를 부여했습니다. 또한 버튼을 비활성화 한다던지, 그런 일련의 동작들을 메서드로 분리해내면 파악하기 좋지 않을까 생각해 의도하였습니다.
(여기서도 네이밍이 참 안좋았네요 ㅠ)
- 엇 미쳐 생각못했네요 ㅠ 코멘트로 피드백 주신 사용성과 관련해서 리팩터링 하면서 이 것에 관한 문제점도 한번 느껴보겠습니다!
index.html
Outdated
| <body> | ||
| <header>🎱 행운의 로또</header> | ||
| <main> |
- webController보다, Web 과 LottoMachine 사이의 메세지를 중재해주는 meditator로 변경
- enter 를 누르면 금액이 입력되도록 구현
|
안녕하세요 포코~ 윤생입니다 :D 말씀 주신 피드백 몇 가지 고쳐 봤어요!! Change View
N회 렌더링
먼저 반복문 안에 제일 많은 태그를 불러오는 위험성이 큰 로또 목록을 출력하는 부분을 고쳤습니다. 기존은 for문이 돌때마다 존재하는 태그에 접근해 append를 했지만 변경한 코드는 넣을 로또 리스트를 한꺼번에 만들어서 한꺼번에 넣었습니다! 사용성
|
| const winRankByMatchCount = (matchCount, hasBonus, expected) => { | ||
| expect(LottoRank.getWinRank(matchCount, hasBonus)).toEqual(expected); | ||
| }; | ||
|
|
||
| test.each([ | ||
| [0, true, LottoRank.PRIZE.MISS.RANK], | ||
| [0, false, LottoRank.PRIZE.MISS.RANK], | ||
| [1, true, LottoRank.PRIZE.MISS.RANK], | ||
| [1, false, LottoRank.PRIZE.MISS.RANK], | ||
| [2, true, LottoRank.PRIZE.MISS.RANK], | ||
| [2, false, LottoRank.PRIZE.MISS.RANK], | ||
| ])('로또등수가 낙첨인지 판단', winRankByMatchCount); |
| @@ -1,16 +1,89 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
html indent... 적용해보았습니다.
vscode에서 전체 선택해서 command + K , command + F 하면 되었어요!! 감사합니다!
There was a problem hiding this comment.
좋습니다
다음 번에는 그것보다도 더 자동적인 방법을 알아보세요 :)
| <h3 class="lotto-subtitle">🎱 내 번호 당첨 확인 🎱</h3> | ||
| <form class="payments-container"> | ||
| <p class="payments-notice">구입할 금액을 입력해주세요.</p> | ||
| <input class="payments-input" type="number" placeholder="금액" step="1000" min="0"> |
There was a problem hiding this comment.
다양한 input 속성을 사용해보았습니다. 로또 금액에 맞게 step도 1000원으로 적용시켰고, 최소 금액은 마이너스가 되지 않도록 min = 0을 적용시켰습니다.
| <script type="module" src="./src/js/index.js"></script> | ||
| <header>🎱 행운의 로또</header> | ||
| <main> | ||
| <article id="app"> |
There was a problem hiding this comment.
시맨틱 태그도 사용해보았습니다!
아무래도 div만 떡칠하기 보다는 눈에 들어오는게 편했습니다!
src/view/web/render.js
Outdated
| const renderLottoList = (lottoNumbers) => { | ||
| const paymentsContainer = document.querySelector('.payments-container'); | ||
|
|
||
| const lottosContainer = document.createElement('section'); | ||
| lottosContainer.className = 'lottos-container'; | ||
|
|
||
| lottoNumbers.forEach((lottoNumber) => { | ||
| const lottoElement = document.createElement('div'); | ||
| const lottoNumberElement = document.createElement('p'); | ||
|
|
||
| lottoElement.className = 'lotto-numbers'; | ||
| lottoNumberElement.innerText = `🎟️ ${lottoNumber.join(', ')}`; | ||
|
|
||
| lottoElement.append(lottoNumberElement); | ||
| lottosContainer.append(lottoElement); | ||
| }); | ||
| paymentsContainer.after(lottosContainer); | ||
| }; |
There was a problem hiding this comment.
과한 렌더링이 들어갔다고 생각하는 부분이여서 수정하였습니다.
There was a problem hiding this comment.
createElement로 로또 리스트가 들어갈 lottosContainer를 만들고,
배열을 forEach를 통해 요소들을 lottosContainer 에 삽입해주었습니다.
그 이후 실제 DOM 트리에 있는 paymentsContainer의 뒤에 삽입해주어도 N회 렌더링이 일어나는 걸까요?
아니면 다른 부분에 문제가 있는걸까요? 혹시 힌트를 주시면 그 부분에 대해 알아보겠습니다!
모던 자바스크립트 Deep Dive 725~726p 를 참고했습니다.
There was a problem hiding this comment.
document.createElement(), appendChild()도 리렌더에 영향을 끼치는 대표적인 케이스인데요.
다만 모던 브라우저 생태계가 정말 빠르게 발전하고 있어 최적화해주는 부분이 워낙 많긴합니다.
때문에 문서의 명세 내용 조차 얼마나 유효한건지 계속 애매한 순간이 나타나는데요. 덕분에 저도 복습할 수 있는 좋은 시간을 가졌습니다.
거두절미하고 위의 예제를 직접 찾아서 제가 실험해봤어요
const btn = document.querySelector("button");
const $fruits = document.getElementById("fruits");
// 컨테이너 요소 노드 생성
const $container = document.createElement('div');
btn.addEventListener("click", function () {
for (let i = 0; i < 10000; i++) {
// 1. 요소 노드 생성
const $li = document.createElement("li");
// 2. 텍스트 노드 생성
const textNode = document.createTextNode(i + "회");
// 3. 텍스트 노드를 $li 요소 노드의 자식 노드로 추가
$li.appendChild(textNode);
// 4. $li 요소 노드를 컨테이너 요소의 마지막 자식 노드로 추가
$container.appendChild($li);
}
// 5. 컨테이너 요소 노드를 #fruits 요소 노드의 마지막 자식 노드로 추가
$fruits.appendChild($container);
});
💯 결론적으로 @2yunseong 의 말대로 렌더링 성능에 큰 영향이 없었답니다 💯
그렇지만 말씀드리자면 반복문 내에서 DOM을 CRUD하는 작업은 모두 좋지 않습니다 :)
특히 무거운 작업이나 연산이 포함되었을때는 꼭 주의해주시는게 좋아요.
마지막으로 책이나 블로그 등의 지식도 좋지만 꼭 실험과 결과를 통해서 체크하는 습관을 가지셨으면 좋겠어요 👍
There was a problem hiding this comment.
넵 감사합니다! 포코 덕분에 이번 미션에서 많은 것을 찾아보고 생각해 봤던 것 같아요!!
추가로 앞으로는 DevTools를 잘 이용해 보겠습니다! 사실 DevTools를 잘 다뤄본 적이 없어 Consoles나 CSS 볼 때만 사용하는데 유용하게 사용해 보겠습니다!! (실험까지 직접 해주셔서 감사드립니다)
| changeCSSByPaymentsEvent(); | ||
| }); | ||
|
|
||
| winningLottoContainer.addEventListener('submit', (e) => { |
There was a problem hiding this comment.
form 태그를 이용해 enter를 사용할 수 있도록 구현하였습니다!
src/LottoController.js
Outdated
| handlePayments(payments) { | ||
| this.#lottoMachine = new LottoMachine(payments); | ||
| } | ||
|
|
||
| handleWinNumbers(winNumbers) { | ||
| this.#lottoMachine.generateWinningLotto(winNumbers); | ||
| } | ||
|
|
||
| handleBonusNumber(bonusNumber) { | ||
| this.#lottoMachine.setBonusNumber(bonusNumber); | ||
| } |
There was a problem hiding this comment.
음 저는 그저 함수를 한번 감싸주는 보조의 역할을 수행하는 것으로 밖에는 안보여서 뭔가 아쉽긴 하네요 😢
| const winRankByMatchCount = (matchCount, hasBonus, expected) => { | ||
| expect(LottoRank.getWinRank(matchCount, hasBonus)).toEqual(expected); | ||
| }; | ||
|
|
||
| test.each([ | ||
| [0, true, LottoRank.PRIZE.MISS.RANK], | ||
| [0, false, LottoRank.PRIZE.MISS.RANK], | ||
| [1, true, LottoRank.PRIZE.MISS.RANK], | ||
| [1, false, LottoRank.PRIZE.MISS.RANK], | ||
| [2, true, LottoRank.PRIZE.MISS.RANK], | ||
| [2, false, LottoRank.PRIZE.MISS.RANK], | ||
| ])('로또등수가 낙첨인지 판단', winRankByMatchCount); |
| @@ -1,16 +1,89 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
좋습니다
다음 번에는 그것보다도 더 자동적인 방법을 알아보세요 :)
| <h3 class="lotto-subtitle">🎱 내 번호 당첨 확인 🎱</h3> | ||
| <form class="payments-container"> | ||
| <p class="payments-notice">구입할 금액을 입력해주세요.</p> | ||
| <input class="payments-input" type="number" placeholder="금액" step="1000" min="0"> |
| @@ -0,0 +1,27 @@ | |||
| import LottoMachine from './domain/LottoMachine'; | |||
|
|
|||
| class LottoMeditator { | |||
There was a problem hiding this comment.
Meditator는 어떤 의미를 가질까요..?
아무리 봐도 모르겠군요 ㅠㅠ 영어 사전도 뒤적뒤적 해보고 있었답니다.
There was a problem hiding this comment.
중재자(Mediator)라는 의미를 담고 싶었는데.. 거기에 또 오타를 냈었네요 ㅠ
중간에서 메세지를 받아 전달을 해주는 역할을 의도했습니다!
src/view/web/render.js
Outdated
| const paymentsContainer = document.querySelector('.payments-container'); | ||
|
|
||
| const lottosContainer = document.createElement('section'); | ||
| lottosContainer.className = 'lottos-container'; |
There was a problem hiding this comment.
classname을 활용할때는 전용 메서드도 활용할 수 있답니다 :)
src/view/web/render.js
Outdated
| const renderLottoList = (lottoNumbers) => { | ||
| const paymentsContainer = document.querySelector('.payments-container'); | ||
|
|
||
| const lottosContainer = document.createElement('section'); | ||
| lottosContainer.className = 'lottos-container'; | ||
|
|
||
| lottoNumbers.forEach((lottoNumber) => { | ||
| const lottoElement = document.createElement('div'); | ||
| const lottoNumberElement = document.createElement('p'); | ||
|
|
||
| lottoElement.className = 'lotto-numbers'; | ||
| lottoNumberElement.innerText = `🎟️ ${lottoNumber.join(', ')}`; | ||
|
|
||
| lottoElement.append(lottoNumberElement); | ||
| lottosContainer.append(lottoElement); | ||
| }); | ||
| paymentsContainer.after(lottosContainer); | ||
| }; |
src/view/console/OutputView.js
Outdated
| return `${amount}개를 구매했습니다.`; | ||
| }, | ||
|
|
||
| generateProfitRateMessage(profitRate) { | ||
| return `총 수익률은 ${profitRate.toFixed(2)}%입니다.`; | ||
| }, | ||
| // to-do : 메서드명 나중에 변경하기 | ||
|
|
||
| printBuyLottos(lottos) { |
There was a problem hiding this comment.
print-* 네이밍과 Console.print는.. 계속 필요할까요?
src/WebController.js
Outdated
| @@ -0,0 +1,27 @@ | |||
| import LottoMachine from './domain/LottoMachine'; | |||
|
|
|||
| class WebController { | |||
There was a problem hiding this comment.
네이밍의 문제보다는.. 그 역할을 못하고 있는게 문제라고 봅니다 😢
src/view/web/render.js
Outdated
| lottoElement.appendChild(lottoNumberElement); | ||
| lottosContainer.appendChild(lottoElement); |
There was a problem hiding this comment.
연달아 렌더링을 추가할때 사용할 수 있답니다 ㅎㅎ
|
안녕하세요 포코!!
포코가 시간 내로 다시 수정 요청 드린 걸 전부 반영하지 못해 아쉬움이 남네요ㅠ 일단은 부족한 상태지만 재리뷰 요청 날립니다!! 제 다짐이 르블랑의 법칙대로 되지 않기를 바라며... 마지막 리뷰를 아쉬운 마음으로 날려봅니다. |
안녕하세요 포코~! 윤생입니다.
날씨가 갑자기 부쩍 추워졌네요 ㅠㅠ 감기 조심 하시기 바랍니다!!
이번 2단계는 다음을 중심으로 진행해 보았습니다.
LottoController까지 도메인으로 보고 재사용하려 했더니.. 애초에LottoController에서InputView와OutputView를import함으로 웹에서 사용할 수 없는readline라이브러리를import해 오류가 나 새로운WebController클래스를 구현하였습니다.WebController는lottoMachine과step2-index.js사이에 인터페이스 역할을 한다고 보시면 될 것 같아요.아래 내용은 리뷰하실 때 참고해주시기 바랍니다!
피드백 반영 내용
미션 2 요구사항 이외로 다음 부분을 반영하였습니다.
static상수나 변수들이 꽤나 다른 클래스에서도 많이 필요했습니다. 곰곰히 생각하길 이렇게 사용해버리면static의 의미가 없어졌다 판단해constant로 빼버렸습니다.TDD로 하였다고 했지만 중간부터 단위 테스트가 아니라고 생각해 놓쳤던 메서드나 기능 구현을 먼저 해버린 기능들에 테스트를 추가하였습니다.(추가로..)+ 추천해주신 그림그리기를 해보았습니다. 정확한 건 아니지만 구현할 때 잠깐씩 참고하면 리팩터링 하기도 편하고 새로운걸 넣기 전에 고민할 때도 편하더라구요. 감사합니다. (상당히 악필입니다.. 죄송합니다..ㅠ)
고민중이고 해결 중인 내용
에러 처리를 어디서 할까?
두 가지 상반된 고민을 하고 있는데요… 다음과 같은 고민을 하고 있습니다.
그래서 아직 반영하지 않았습니다. 좀 더 고민하고 싶어서요. 또한 "도메인 로직을 최대한 고쳐보지 말고 뷰 로직을 작성하자" 라는 학습 목표 부터 이루고 싶어 기존의 코드를 놔둔 상태에서 작성해보고 싶어 후 순위로 미룬 이유도 있습니다.
이상입니다! 잘 부탁드립니다 😊
실행 방법
로컬
배포된 웹사이트
https://2yunseong.github.io/