Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 36 commits into from
Mar 2, 2021

Conversation

zigsong
Copy link

@zigsong zigsong commented Mar 1, 2021

안녕하세요! 마지막 로또 3단계 미션 제출합니다 😆

지그's 로또 데모

2단계가 머지되기 전에 3단계를 이미 진행하고 있어서, 2단계 머지 후 새로운 브랜치에서 기존 3단계 작업 내용들을 cherry-pick하여 가져왔는데
중간에 커밋이 올바르게 딸려오지 않은 부분들이 있을 수 있습니다. (전체 코드 상에서는 문제가 없지만, 각 커밋에 들어간 코드가 100% 내용을 반영하지 않는 부분도 있습니다.) 코드 상세 내용들 코멘트로 달아놓도록 하겠습니다!

✅ 3단계 TODO

기능

  • 수동 구매, 자동 구매 선택지 UI 만들기
  • 수동 구매, 자동 구매 선택지 중 현재 선택 옵션 하이라트
  • 수동 구매 선택 시, 번호 입력 폼 UI 만들기
  • 소비자가 입력한 수동 구매 번호 저장하기
    • 검증: 번호는 1~45 사이의 숫자만 가능하다.
    • 검증: 로또 한 장에 중복된 번호가 없어야 한다.
  • 수동 구매 후 남는 금액이 있다면 자동 구매를 진행한다는 확인창 띄우기
  • 자동 구매 취소 시 계속해서 수동 구매 번호 입력 폼 채우기
  • 자동 구매 동의 시 남는 금액으로 자동 구매 진행하기
  • 소비자가 구매한 로또로 당첨 결과를 계산하여 모달에 보여주기

테스트

  • 수동 구매, 자동 구매 선택지를 보여준다.
  • 수동 구매, 자동 구매 선택지 중 현재 선택 옵션을 표시한다.
  • 수동 구매를 선택하고 금액을 입력하면, 금액만큼의 번호 입력 폼을 보여준다.
    • [ ] 검증: 1~45 사이의 숫자 입력 시 알럿이 뜬다.
    • 검증: 로또 한 장에 중복된 번호 입력 시 알럿이 뜬다.
    • 수동 구매 번호를 모두 올바르게 입력하면, 확정된 로또 티켓을 보여준다.
  • 수동 구매 후 남는 금액 발생 시 자동 구매 진행 확인창이 뜬다.
  • [ ] 자동 구매 취소 시 계속해서 수동 구매 번호 입력 폼을 채울 수 있다.
  • 자동 구매 동의 시 자동구매를 진행한다.
  • 소비자가 구매한 로또의 당첨 결과가 모달에 올바르게 표시된다.

🛠 컴포넌트 및 구조도

lotto-step3-component

lotto-step3-map

🗂 디렉토리 구조

lotto-step3-dir

🤔 고민한 점

  1. 아무래도 UI를 혼자 제작하다보니, 사용성을 해치지 않으면서도 기존의 UI에서 크게 벗어나지 않는 선에서 제작하는 것을 많이 고민했습니다. 그런데 아직 플레이어가 저밖에 없다보니, 괜찮게 만든 것인지 확실한 판단이 서지는 않습니다. 😅
  2. select type을 번갈아가며 클릭했을 때 (자동구매를 선택해놓고 갑자기 수동구매를 하고싶어질 수도 있으므로) 화면을 거의 새로고침해야 하지만 reload는 사용하지 않는 것이 좋겠다고 판단하여 기존에 담겨 있던 값들과 화면들을 적절히 비우는 데 은근히 애를 썼던 것 같습니다.

🙄 궁금한 점

  1. 2단계에서 Lotto 모델이 너무 많은 일을 한다는 피드백을 바탕으로 LottoProcessor 클래스를 만들어서 당첨 결과를 계산하게끔 했는데, 해당 계산 담당 로직들을 class로 만들어 내부 메소드들로 구현한 것이 괜찮은 방식인지 궁금합니다! 각 게임마다 new 연산자로 각 view를 새로 만들어내는 것처럼, 계산하는 로직도 현재 로또 배열과 당첨 번호를 담고 있는 유일한 인스턴스라고 생각해서 그렇게 작성했는데, 어떻게 생각하시는지 피드백 받고 싶습니다.
  2. ManualInputView가 처음 생각보다 조금 길어진 것 같습니다. html 템플릿이 많아서 그런 것 같기도 한데, 자동 구매 시에는 해당 view가 필요하지 않기 때문에 index.html 파일에 미리 넣어 놓고 show, hide하기보다는 수동 구매 선택 시에만 ManualInputView를 append하는 방식으로 사용하고 있습니다. 이에 대한 리뷰 부탁드리고 싶습니다!

그 밖의 내용들은 코멘트로 달았습니다 🤓

devhyun637 and others added 29 commits February 21, 2021 00:03
- refactor spacings
- rename selector to dom
- remove unnecessary variable
Copy link
Author

@zigsong zigsong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존의 틀을 해치지 않는 선에서 나름대로 UI를 만들어서 제작해보았습니다.
피드백 기다리겠습니다! 감사합니다 ☺️

@@ -25,29 +32,32 @@ export default class LottoController {
}

reset() {
this.isAutoPurchase = true;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

페이지 로드 시 처음 purchase type은 default로 auto가 되도록 설정했습니다.

Comment on lines 87 to 97
purchaseManually() {
this.manualInputView
.show()
.init(this.purchasedPrice / LOTTO_NUMBERS.LOTTO_UNIT);
this.manualInputView.on('submitNumbers', e =>
this.inputManualNumbersHandler(e.detail)
);
this.manualInputView.on('confirmAll', e =>
this.confirmManualPurchaseHandler(e.detail)
);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수동구매 선택 시 실행됩니다.

Comment on lines 99 to 108
inputManualNumbersHandler(eventDetail) {
const ticketNumbers = eventDetail.numbers.map(manualNumber =>
Number(manualNumber.value)
);
if (!isUniqueManualNumber(ticketNumbers)) {
alert(ALERT_MESSAGES.DUPLICATE_NUMS);
return;
}
this.createManualLottos(ticketNumbers, eventDetail.ticketNumber);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수동 구매 선택 후 각 티켓에 6개의 숫자를 입력 시 한 장씩 생성됩니다.

manual-ticket

Comment on lines 110 to 125
confirmManualPurchaseHandler(manualCount) {
if (manualCount < this.purchasedPrice / LOTTO_NUMBERS.LOTTO_UNIT) {
const agreeAutoPurchase = confirm(ALERT_MESSAGES.CONVERT_TO_AUTO_ALERT);
if (agreeAutoPurchase) {
this.manualInputView.hide();
this.purchaseAutomatically(
this.purchasedPrice / LOTTO_NUMBERS.LOTTO_UNIT - manualCount
);
}
} else {
this.manualInputView.hide();
this.purchasedLottosView.show();
this.purchasedLottosView.renderLottos(this.lottos);
this.winningResultView.show();
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수동 구매를 선택하였으나 숫자를 (구입 티켓의 수만큼) 모두 입력하지 않은 경우, confirm 창으로 자동구매 전환을 안내합니다.
동의 시 자동 구매로 전환되고 남은 티켓의 개수만큼 로또를 자동으로 구매합니다.

const newLottos = Array.from({ length: lottoCount }, () => {
return new Lotto();
});
this.lottos = [...this.lottos, ...newLottos];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수동 구매 후 남는 금액으로 자동 구매를 하는 경우도 있기 때문에 스프레드 연산자를 사용하여 this.lottos에 새로운 lotto들을 추가하는 방식으로 바꾸었습니다.

}

createManualLottos() {
const lottoTicekts = [...Array(this.count)]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의미 없는 index for문을 사용하는 것보다 이렇게 스프레드 연산자를 활용하여 map이나 forEach를 돌리는 것이 낫다고 배웠는데,
정확하게 이 문법으로 작성하는 것인지를 까먹었습니다 😭 마땅한 키워드가 떠오르지 않아 검색하기가 어려워서 일단 작성해보았습니다.
지적 부탁드립니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for 문 보다 Array 의 메서드를 사용하는게 대부분의 경우에 더 낫습니다.
이 경우는
const lottoTickets = Array.from({length: this.count}, (_, idx) => `<li>....</li>`).join('')
이렇게 하시면 될 것 같네요 :)

Comment on lines 79 to 81
Number($element.dataset.manualIndex) ===
LOTTO_NUMBERS.LOTTO_MANUAL_COUNT - 1
) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수동 구매 각 티켓의 마지막 숫자에서는 moveFocus 이벤트가 발생하지 않습니다.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의도가 잘 드러나는 코드가 좋은 코드입니다.
이 댓글을 달아주신 이유가 만약 제가(혹은 이 코드를 보는 다른 누군가가) 이 코드를 더 잘 이해하도록(혹은 정확히 이해하지 못할까봐) 설명(마치 주석처럼)을 해주신것이라면, 코드 자체에서 그 설명이 표현되는게 좋습니다.

수동 구매에서는 포커스다음으로 이동된다.
마지막은 제외한다

라는 의도를, 코드를 읽는 사람이 별도 설명 없이 코드만 보고 알아차릴 수 있다면 완벽한 코드입니다.

사용성을 위해 포커스를 다음으로 옮기시는것은 굉장히 좋습니다.
실무에서도 디자이너나 기획자가 따로 언급하지 않는 경우도 많은데 개발자가 사용성을 생각해서 고민하고 추가로 구현하는것은 굉장히 좋은 태도라고 생각합니다 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

간단하게 메서드를 따로 분리하여 네이밍해보겠습니다!

Comment on lines 96 to 97
const allConfirmBtn = document.createElement('template');
const convertToAutoCaption = document.createElement('template');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createElement('template')을 사용하여 아래에서처럼
this.$element.append(convertToAutoCaption.content, allConfirmBtn.content);으로 작성하면 html을 붙일 수 있다는 것을 알게 되었는데, 처음 접한 방식이라 괜찮은 방식인지 궁금합니다! 🙄

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createElement() 하면 DOM 엘리먼트를 만들어 낼 수 있으므로 해당 엘리먼트를 통해 DOM API를 이용할 수 있는 이점이 있습니다.
안타깝게도 template 엘리먼트가 IE 에서 지원되지 않으니 이점 추가로 알아두시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE 대응은 어렵군요... 😭 다른 방식으로 수정해보도록 하겠습니다!

Comment on lines 112 to 119
bindConfirmEvent() {
$('#manual-confirm-btn').addEventListener('click', () => {
this.emit(
'confirmAll',
[...$$('.manual-wrapper > .lotto-detail')].length
);
});
}
Copy link
Author

@zigsong zigsong Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수동 구매 시 전체 확정 버튼을 누르면, 현재 확정된 로또(확정된 로또는 class명으로 lotto-detail을 가집니다.)의 개수(아래 사진에서는 1개)를 controller에 보내줍니다. 3000원을 입력했으나 수동 구매로 1개만 입력한 경우, 나머지는 자동 구매로 전환하기 위해 현재 확정된 수동 티켓의 개수를 보내주고 있습니다.

manual-all-confirm

import View from './View.js';
import { $ } from '../utils/dom.js';

export default class PurchaseTypeSelectView extends View {
Copy link
Author

@zigsong zigsong Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음 앱 접속 시 보여지는 자동/수동 구매 선택지입니다.

typeselector

- remove yarn conflicts
- update validation constant
- resolve eol errors in css files
Copy link

@wow9144 wow9144 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용성을 올리기 위해 고민하신 흔적이 보여서 좋았습니다. 👍
다만 전체 프로세스가 진행되는 과정중에서 약간의 논리 오류가 발견 되었으니 코멘트 확인하시어 이 부분을 중점적으로 수정 하시면 될 것 같습니다.
프로세스의 각 단계를 나눠서 생각해보시면 수정 하시는데 도움이 될 것 같습니다.
용지에 번호를 적는 단계, 로또가 발행되는 단계, 발행된걸 확인하는 단계 등등..

화이팅!

Comment on lines 96 to 97
const allConfirmBtn = document.createElement('template');
const convertToAutoCaption = document.createElement('template');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createElement() 하면 DOM 엘리먼트를 만들어 낼 수 있으므로 해당 엘리먼트를 통해 DOM API를 이용할 수 있는 이점이 있습니다.
안타깝게도 template 엘리먼트가 IE 에서 지원되지 않으니 이점 추가로 알아두시면 좋을 것 같습니다.

}

createManualLottos() {
const lottoTicekts = [...Array(this.count)]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for 문 보다 Array 의 메서드를 사용하는게 대부분의 경우에 더 낫습니다.
이 경우는
const lottoTickets = Array.from({length: this.count}, (_, idx) => `<li>....</li>`).join('')
이렇게 하시면 될 것 같네요 :)

Comment on lines 79 to 81
Number($element.dataset.manualIndex) ===
LOTTO_NUMBERS.LOTTO_MANUAL_COUNT - 1
) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의도가 잘 드러나는 코드가 좋은 코드입니다.
이 댓글을 달아주신 이유가 만약 제가(혹은 이 코드를 보는 다른 누군가가) 이 코드를 더 잘 이해하도록(혹은 정확히 이해하지 못할까봐) 설명(마치 주석처럼)을 해주신것이라면, 코드 자체에서 그 설명이 표현되는게 좋습니다.

수동 구매에서는 포커스다음으로 이동된다.
마지막은 제외한다

라는 의도를, 코드를 읽는 사람이 별도 설명 없이 코드만 보고 알아차릴 수 있다면 완벽한 코드입니다.

사용성을 위해 포커스를 다음으로 옮기시는것은 굉장히 좋습니다.
실무에서도 디자이너나 기획자가 따로 언급하지 않는 경우도 많은데 개발자가 사용성을 생각해서 고민하고 추가로 구현하는것은 굉장히 좋은 태도라고 생각합니다 :)


createManualLottos(manualNumbers, ticketNumber) {
const lotto = new Lotto();
lotto.inputManualNumbers(manualNumbers);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputManualNumbers 라는 메서드를 별도로 두기보다는 Lotto를 생성 할 때 numbers를 넘기는게 의미상으로도 더 좋을 것 같습니다.
넘기면 그 숫자로 로또를 생성하고(수동) 안넘기면 랜덤한 숫자로 생성하고(자동) ...
현재 구조라면 랜덤 숫자로 이미 생성된 로또에 뒤늦게 숫자를 덮어 씌우는듯 하네요 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

덮어 씌우는 것이 맞긴 합니다 😅 그냥 넘어갔는데 짚어 주셔서 감사합니다!
다른 로직으로 수정해보도록 하겠습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇게 하면 Lotto의 constructor에 분기를 나눠줘야 할 것 같아서 inputManualNumbers를 넣은 것인데,
좀 더 고민 후 반영해보겠습니답

}

confirmManualPurchaseHandler(manualCount) {
if (manualCount < this.purchasedPrice / LOTTO_NUMBERS.LOTTO_UNIT) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if 의 조건에 다양한(다른) 연산이 섞여 있으면 가능하면 분리해주는게 좋습니다.
< 비교 연산과
/ 산술 연산이 같이 존재하므로
산술 연산을 하여 하나의 값으로 귀결 시켜 변수에 할당(변수의 이름을 통해 의미 부여)하고 manualCount 와 이 변수와 비교 연산을 하는식으로 2개의 다른 연산을 나누면 코드의 가독성이 올라갑니다.

alert(ALERT_MESSAGES.DUPLICATE_NUMS);
return;
}
this.createManualLottos(ticketNumbers, eventDetail.ticketNumber);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 시점에 createManualLottos 를 하는게 맞는지 한 번 고민해보실 필요가 있을 것 같습니다.
숫자를 쓰고 확정을 누른 시점은 로또 용지에 번호를 다 적은 상태일뿐 로또가 발행되는것은 '구매 확정하기'를 누른 뒤어야 하지 않을까 싶습니다.

실무에서 이것과 비슷한 예를 찾는다면 회원가입을 하기 위해 아이디를 입력하고 아이디 중복 검사를 눌렀는데 아이디로 회원가입이 되어버린것과 같은 상황이라고 볼 수 있습니다.
아직 비밀번호도 주소도 기타 다른 필드들이 입력되지 않았고 가입하기 버튼을 누르지도 않았는데 말이죠 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

각각의 수동 로또 티켓에 확정을 누르면 Lotto 객체를 생성하여 번호들을 보여주는 로직을 간단하게 하려고 했던 건데,
말씀 주신 부분이 일리가 있는 것 같습니다. 좀 더 자연스러운 실행 흐름으로 바꿔보겠습니다!

@wow9144
Copy link

wow9144 commented Mar 1, 2021

궁금한점 답변

2단계에서 Lotto 모델이 너무 많은 일을 한다는 피드백을 바탕으로 LottoProcessor 클래스를 만들어서 당첨 결과를 계산하게끔 했는데, 해당 계산 담당 로직들을 class로 만들어 내부 메소드들로 구현한 것이 괜찮은 방식인지 궁금합니다! 각 게임마다 new 연산자로 각 view를 새로 만들어내는 것처럼, 계산하는 로직도 현재 로또 배열과 당첨 번호를 담고 있는 유일한 인스턴스라고 생각해서 그렇게 작성했는데, 어떻게 생각하시는지 피드백 받고 싶습니다.
-> 잘하셨습니다.(Processor 라는 이름은 전체 발행 과정을 관장하는 것 같아서 조금 수정이 되면 더 좋을 것 같긴 합니다. 로또 발행이 끝나고 결과를 확인할 때 필요한 애니까요) 이렇게 역할(로또 분석)을 나누고 그 녀석(객체)에게 내가 가진게 이건데 이걸로(인자) 처리(메서드 이름에 해당 일)좀 해줄래? 라고 하는 것이 객체 지향 프로그래밍의 기본입니다.

ManualInputView가 처음 생각보다 조금 길어진 것 같습니다. html 템플릿이 많아서 그런 것 같기도 한데, 자동 구매 시에는 해당 view가 필요하지 않기 때문에 index.html 파일에 미리 넣어 놓고 show, hide하기보다는 수동 구매 선택 시에만 ManualInputView를 append하는 방식으로 사용하고 있습니다. 이에 대한 리뷰 부탁드리고 싶습니다!
-> 사용성을 올리기 위한 고민이 고스란히 묻어나는게 ManualInputView 가 아닌가 싶습니다. 미리 넣어놓고 show, hide를 하는 경우가 append 하는 경우보다 빠릅니다. show, hide 는 이미 만들어진 돔트리에 조작만 해서 렌더트리만 변경이 일어나고 append 는 돔트리와 렌더트리가 전부 변경이 되니까요.
그래서 미리 보여져도 별 문제가 없다면(단순히 유저의 편의를 위한 인터페이스 같은것) show, hide 만 하는 편이고 서버를 통해 어떤 정보가 있어야 해서 미리 보여주면 안된다든지(개발자도구로 숨겨져있는걸 조작해서 미리 보면 문제가 생긴다든지) 하는것은 show, hide로만 조작하면 안되기도 합니다.
결론적으로 지금과 같은 경우는 show, hide 를 하든지 append 를 하든지 크게 차이는 없으니 편한대로 하셔도 됩니다.

Copy link
Author

@zigsong zigsong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 주신 부분들 반영하여 수정해보았습니다.
좋은 피드백 감사드립니다 😆

`
)
.join('');
// const lottoTickets = Array.from({length: this.count}, (_, idx) => `<li>....</li>`).join('')
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실수로 올라갔네요 😂 다음 커밋에서 지웠습니다!

Comment on lines +22 to +24
const manualForms = Array.from(
{ length: this.count },
(_, idx) =>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array로 loop을 도는 로직을 말씀주신 대로 바꾸었습니다.

Comment on lines +91 to +93
isLastManualNumber(manualIndex) {
return Number(manualIndex) === LOTTO_NUMBERS.LOTTO_MANUAL_COUNT - 1;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마지막 input에서는 moveFocus가 되지 않는 코드를, 명시적으로 설명하기 위해 함수로 한번 분리하였는데
말씀주신 의도대로 잘 이해한 것이 맞을까요? 😅

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 이정도만 되어도 가독성이 훨씬 나아진것 같습니다 👍

}

showAllConfirmButton() {
const allConfirmBtn = document.createElement('div');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE 대응을 위해 createElement('template') 대신 createElement('div')를 사용했습니다.

Comment on lines +97 to +109
inputManualTicketHandler(eventDetail) {
const ticketNumbers = eventDetail.numberInputs.map(input =>
Number(input.value)
);
if (!isUniqueManualNumber(ticketNumbers)) {
alert(ALERT_MESSAGES.DUPLICATE_NUMS);
return;
}
this.manualInputView.saveTemporaryTicket(
ticketNumbers,
eventDetail.ticketIdx
);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수동 로또 1장 확정 시 로또를 바로 생성하지 않고, 확정된 숫자들의 목록으로만 보여지게끔( this.manualInputView.saveTemporaryTicket) 로직을 수정했습니다.

this.purchaseAutomatically(lottoTotalCount - manualCount);
}
} else {
this.createManualLottos(manualTickets);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수동 구매 시 전체 구매 확정을 눌렀을 때 createManualTickets를 실행하도록 바꿨습니다.

this.initNumbers();
constructor(numbers) {
this._numbers = new Set(numbers) || new Set();
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자동일 때는 빈 Set이 만들어지고 아래의 initRandomNumbers를 실행하며,
수동일 때는 인자로 받은 숫자들로 로또를 생성합니다.

Copy link

@wow9144 wow9144 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3단계까지 완료 하시느라 수고 많으셨습니다.
앞으로 진행 될 다음 미션들도 계속 기대 할게요 화이팅! :)

Comment on lines +91 to +93
isLastManualNumber(manualIndex) {
return Number(manualIndex) === LOTTO_NUMBERS.LOTTO_MANUAL_COUNT - 1;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 이정도만 되어도 가독성이 훨씬 나아진것 같습니다 👍

@wow9144 wow9144 merged commit 5b30f1f into woowacourse:zigsong Mar 2, 2021
Copy link
Author

@zigsong zigsong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[3/23 셀프 리뷰]

while (this._numbers.size < Lotto.LOTT0_LENGTH) {
this._numbers.add(getRandomNumber());
this._numbers.add(getRandomNumber(1, LOTTO_NUMBERS.LOTTO_MAX_NUM));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getRandomNumber와 같은 유틸성 함수는 최대/최소값 인자를 받아서 여러 곳에서 사용할 수 있도록 작성하는 게 좋은 것 같다.

Comment on lines +136 to +137
lotto.initRandomNumbers();
return lotto;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return 문 위는 한 줄 띄는 게 가독성이 좋다.

}

createAutoLottos(lottoCount) {
const newLottos = Array.from({ length: lottoCount }, () => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

length만 알고 있는 Array를 채우는 방식을 몇 번 더 연습해서 적재적소에 적용해봐야겠다.

Comment on lines +8 to 13
LOTTO_NUMBERS.LOTTO_MANUAL_COUNT
);
this.bonusNumber = winningNumbers[LOTTO_NUMBERS.WINNING_NUMBER_COUNT - 1];
this.bonusNumber = winningNumbers[LOTTO_NUMBERS.LOTTO_MANUAL_COUNT];
this.rankCounts = Array(5).fill(0);
this.earningRate = 0;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 파일의 이름은 LottoManager 등으로 했어도 좋을 것 같다.
그때도 리뷰어님께서 말씀 주셨는데, 까먹고 바꾸지 않았다 😅

Comment on lines +1 to +2
export function getRandomNumber(min, max) {
return Math.floor(Math.random() * max) + min;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

moveFocus($element) {
if ($element.value.length === 2) {
if (this.isLastManualNumber($element.dataset.manualIndex)) return;
$element.nextElementSibling.focus();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextElementSibling는 써놓고 또 잊고 있었음! 유용한 DOM selector 기억기억

Comment on lines +29 to +32
$('#auto-btn').classList.toggle('btn-default');
$('#auto-btn').classList.toggle('btn-cyan');
$('#manual-btn').classList.toggle('btn-default');
$('#manual-btn').classList.toggle('btn-cyan');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

단순히 toggle만 하다보면 리로딩이나 악의적인 css 변경 등 외부 변화에 취약하여 앱의 상태와 실제 UI가 맞지 않을 수 있기 때문에,
isAutoPurchase bool 값에 따라 조건문 분기로 class의 추가/삭제를 해주는 것이 좋아 보인다.

@@ -5,11 +5,13 @@ export default class View {
}

show() {
if (!this.$element) return;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(이전 변경 내역이라 아래에 코멘트가 안 달린다.)

CustomEvent를 여기서 선언해 놓고 class 자체를 상속 받아 사용했기 때문에
실제로 dispatchEvent가 어디에 붙어 사용되는지, 왜 이곳에 붙여주는지 생각지 않고 사용했던 것 같다.

이번 지하철 노선도 미션에서 window.dispatchEventwindow.addEventListener를 사용하며 조금 더 CustomEvent의 작동 방식과 친해진 것 같다.

Comment on lines 52 to 57
moveFocus($element, idx) {
if ($element.value.length === 2) {
if (idx === LOTTO_NUMBERS.WINNING_NUMBER_COUNT - 1) return;
if (this.isLastWinningNumber(idx)) return;
$$('.winning-number')[idx + 1].focus();
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(이전 변경 내역이라 위에 코멘트가 안 달린다.)

bindNumberInputEvent 메소드에서

this.winningNumbers = [
    ...$('#winning-numbers-form').getElementsByTagName('input'),
].map(input => Number(input.value));

대신

this.winningNumbers = [
    ...$('#winning-numbers-form').elements
].map(input => Number(input.value));

이런 식으로 돌았어도 될 것 같다. form 요소 내부의 form 컨트롤이 되는 대상들을 모아보기!

<<<<<<< HEAD
debug@^4.0.1, debug@^4.1.0, debug@^4.1.1:
=======

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge conflict 시 yarn.lock 파일도 잘 보자 🤦‍♀️

@zigsong
Copy link
Author

zigsong commented Apr 24, 2021

학습로그

객체 프로그래밍의 기본 (ref)

show/hide와 append (ref)

웹페이지 렌더 과정

  1. DOM, CSSOM 생성
  2. render tree 생성
  3. layout (reflow) - 브라우저 뷰포트 내에서 각 노드들의 정확한 위치와 크기를 계산
  4. (re)paint

기타

Array 내장 메서드 사용하기 (ref)

const lottoTickets = Array.from({ length: this.count }, (_, idx) => `<li>....</li>`).join('') 

nextElementSibling (ref)

form.elements (ref)

const inputs = document.getElementById("my-form").elements;
const inputByIndex = inputs[0];
const inputByName = inputs["username"];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants