[클린코드 6기 홍석주] 로또 미션 STEP 3 #287
Conversation
There was a problem hiding this comment.
안녕하세요 석주님 리뷰가 늦어 죄송합니다..!
무거운 step-web-index.js
step-web-index.js 파일 내부에서 모든 일을 처리하려다 보니 라인 수도 많고 여러 역할을 수행하고 있어요. 뷰에서 바로 모델을(lottos) 업데이트하고(이벤트 리스너), 또 참조하기도(예: showLottoStats) 하는 등, 관심사 분리가 필요한 부분들이 보였습니다.
개인적으로 프론트엔드에서 MVC 패턴을 권장하지는 않지만, 어떤 관심사 분리가 적절한지 고민되신다면 현재 상황에서는 컨트롤러를 만드는 것도 괜찮아보입니다.
앱 상태
앱이 현재 로또 구입을 하였는지, 당첨 확인을 하여 재시작이 가능한지 등 앱 상태도 고려해보시면 좋을 것 같습니다.
| throw new Error("로또를 구입해주세요."); | ||
| } | ||
| }; | ||
|
|
||
| export const checkNumbers = (numbers) => { |
There was a problem hiding this comment.
지난 미션에서 놓친 부분같은데요, winningNumbers와 bonusNumbers에 중복된 번호가 있을 때 중복체크가 되지 않고 있어 수정 및 테스트 코드 추가가 필요해 보입니다
| const $buyButton = document.getElementById("buyButton"); | ||
| const $moneyInput = document.getElementById("moneyInput"); | ||
| const $totalCountLabel = document.getElementById("totalCountLabel"); | ||
| const $lottosDiv = document.getElementById("lottosDiv"); | ||
| const $numbersToggle = document.getElementById("numbersToggle"); | ||
| const $winningNumberInputs = document.getElementsByClassName("winning-number"); | ||
| const $bonusNumberInput = document.getElementsByClassName("bonus-number")[0]; | ||
| const $resultButton = document.getElementById("resultButton"); | ||
|
|
||
| const $fifthRankCountSpan = document.getElementById("fifthRankCount"); | ||
| const $fourthRankCountSpan = document.getElementById("fourthRankCount"); | ||
| const $thirdRankCountSpan = document.getElementById("thirdRankCount"); | ||
| const $secondRankCountSpan = document.getElementById("secondRankCount"); | ||
| const $firstRankCountSpan = document.getElementById("firstRankCount"); | ||
| const $profitRateSpan = document.getElementById("profitRate"); | ||
|
|
||
| const $restartButton = document.getElementById("restartButton"); |
There was a problem hiding this comment.
DOM 조작, 접근, 브라우저 이벤트와 관련한 로직들은 index 파일에 작성하는 것보다는 view로 분리해버리면 좋을 것 같습니다 😄
| } | ||
| }); | ||
|
|
||
| $restartButton.addEventListener("click", () => location.reload()); |
There was a problem hiding this comment.
참고) 새로고침으로 앱을 초기화하는게 간단하고 좋은 방법이지만 사용자 경험에는 좋지 않습니다
이번 미션에서 수정하실 필요는 없습니다!
|
|
||
| $resultButton.addEventListener("click", showLottoStats); | ||
|
|
||
| [...$winningNumberInputs].forEach((e) => { |
There was a problem hiding this comment.
요소에 대한 인자명을 e로 하게 되면 event, error와 혼동될 여지가 많아 보여요
| function buyLottosEvent() { | ||
| try { | ||
| const money = getMoney(); | ||
| lottos = buyLottos(money); | ||
| setTotalCountLabel(lottos.length); | ||
| setLottosDiv(lottos.map((e) => e.numbers)); | ||
| $winningNumberInputs[0].focus(); | ||
| } catch (err) { | ||
| alert(err.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
이벤트에 대한 핸들러 함수명을 지을 때, handle~, on~과 같은 접두사를 이용해 표현하여 해당 로직을 담은 함수(예: buyLottos)와 구분하는 컨벤션을 많이 사용합니다.
만일 구입금액과 구입 버튼을 form으로 감싸고 submit 이벤트를 활용한다면 아래와 같은 네이밍이 되겠네요.
| function buyLottosEvent() { | |
| try { | |
| const money = getMoney(); | |
| lottos = buyLottos(money); | |
| setTotalCountLabel(lottos.length); | |
| setLottosDiv(lottos.map((e) => e.numbers)); | |
| $winningNumberInputs[0].focus(); | |
| } catch (err) { | |
| alert(err.message); | |
| } | |
| } | |
| function handleSubmitPurchaseAmount(e) { | |
| e.preventDefault(); | |
| try { | |
| ... | |
| } catch (err) { | |
| ... | |
| } | |
| } |
| } | ||
|
|
||
| function getWinningNumbers() { | ||
| return [...$winningNumberInputs].map((e) => parseInt(e.value)); |
|
|
||
| $showResultButton.addEventListener('click', onModalShow) | ||
| $modalClose.addEventListener('click', onModalClose) | ||
| // $showResultButton.addEventListener("click", onModalShow); |
| function showLottoStats() { | ||
| try { | ||
| const winningNumbers = getWinningNumbers(); | ||
| const bonusNumber = getBonusNumber(); | ||
|
|
||
| const lottoRanks = getLottoRanks(lottos, winningNumbers, bonusNumber); | ||
|
|
||
| const { rankCount, totalCount, totalReward } = new LottoStats(lottoRanks); | ||
|
|
||
| $fifthRankCountSpan.innerHTML = rankCount.get(LottoRank.FIFTH.rank) ?? 0; | ||
| $fourthRankCountSpan.innerHTML = rankCount.get(LottoRank.FOURTH.rank) ?? 0; | ||
| $thirdRankCountSpan.innerHTML = rankCount.get(LottoRank.THIRD.rank) ?? 0; | ||
| $secondRankCountSpan.innerHTML = rankCount.get(LottoRank.SECOND.rank) ?? 0; | ||
| $firstRankCountSpan.innerHTML = rankCount.get(LottoRank.FIRST.rank) ?? 0; | ||
|
|
||
| const profitRate = calculateProfitRate(totalCount, totalReward); | ||
| $profitRateSpan.innerHTML = profitRate; | ||
|
|
||
| onModalShow(); | ||
| } catch (err) { | ||
| alert(err.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
step-web-index.js에서 가장 긴 함수인데요, 함수 내부의 여러 관심사를 분리할 수 없을까요?
|
|
||
| function buyLottosEvent() { | ||
| try { | ||
| const money = getMoney(); |
There was a problem hiding this comment.
정수가 아닌 수를 입력해도 구매가 가능하네요 (1234.567)
There was a problem hiding this comment.
로또 번호 input에 세자리 이상의 수도 입력이 가능한데 제한할 수 없을까요?
안녕하세요 리뷰어님! 로또 미션 STEP 3 PR 드립니다.
많이 부족한 코드라고 생각하지만... 리뷰를 먼저 받는 것이 좋을 것 같아 리뷰 요청 드립니다.
감사합니다. :)