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

[2단계 - 행운의 로또 미션] 하루(김하루) 미션 제출합니다. #33

Merged
merged 32 commits into from
Feb 26, 2021

Conversation

365kim
Copy link

@365kim 365kim commented Feb 23, 2021

Jbee님, 안녕하세요!

2단계를 들고 나타난 🙋🏻‍♀️ 하루(@365kim) 입니다. 이번에도 잘 부탁드리겠습니다!🙏


요약

  • 데모  : 로또미션 2단계 - 하루
  • 구현 순서: 테스트 전체 작성 후, 각 테스트를 통과하기 위한 단위 기능 구현
  • 구현 결과: 작성한 cypress 테스트 모두 통과 ✔
    git clone https://github.com/365kim/javascript-lotto.git
    cd javascript-lotto
    git checkout step2
    npm install
    npm run cypress
    

1단계 - merge이후 추가 반영사항

  • 페어👩🏻‍🦰 (@SunYoungKwon)가 1단계에서 받은 리뷰 내용을 다음과 같이 함께 반영했습니다.

    • 번호보기 토글 시 루프를 통해 모든 .lotto-number의 CSS 속성을 수정하던 방법에서, 상위 엘리먼트 하나만을 수정하는 방법으로 반복문을 제거했습니다. (해당 리뷰 코멘트, 해당 커밋)

    • 번호보기 토글 시 변경이 필요한 모든 CSS 속성('display: inline', 'flex-direction: column')을 각각 수정하던 방법에서, 단일 클래스만을 수정하도록 조정했습니다. (해당 리뷰 코멘트, 해당 커밋)


2단계 - 구현기능 목록

  • 로또 구매 후 당첨번호를 정상적으로 입력하면 결과 확인버튼을 클릭할 수 있다.

    • 로또를 구매하면 당첨번호를 입력할 수 있는 창이 표시된다.
    • 결과 확인 버튼은 기본적으로 비활성화 되어있다.
    • 각 번호가 1 ~ 45 사이의 값이 아닌 경우 ➡️ 입력칸 하단에 재입력 요청 메세지 표시
    • 각 번호가 서로 중복되는 경우 ➡️ 입력칸 하단에 재입력 요청 메세지 표시
    • 6개의 '당첨번호'와 1개의 '보너스번호' 모두 알맞게 입력하면 결과확인 버튼이 활성화된다.
  • 결과확인 버튼을 누르면 당첨 통계, 수익률을 모달로 확인할 수 있다.

    • 구매한 로또의 '당첨번호' 일치 개수를 판별한다.
    • 일치 개수가 5개일 경우에만 '보너스번호' 일치여부를 확인한다.
    • 일치하는 번호 개수가 3, 4, 5, 5.5(보너스번호가 일치하는 경우), 6인 경우 당첨된 로또 장수를 표시한다.
    • 수익률을 표시한다.
      • 수익률은 백분율로 표시한다.
      • 수익률이 정수가 아닌 경우 소수점 둘째 자리까지 표기한다.
    • 다시시작 버튼을 누르면 초기화 되서 다시 구매를 시작할 수 있다.

구조도 작성

  • 구조도를 그려보니, 상태가 변했을 때 후속 작업을 해주는 메서드들이 여기저기 산재해 있는 것을 가시적으로 확인할 수 있었습니다. 다음 단계에서 lottoTickets, winningNumber를 한군데에서 관리하고 상태변화에 따른 후속작업도 통일성있게 관리할 수 있도록 해보려고 합니다...!
    구조도(2단계)



바쁘신 와중에 시간내서 리뷰해 주셔서 감사드립니다! 행복한 2월의 끝자락 되세요😆😆

SunYoungKwon and others added 27 commits February 23, 2021 16:18
Comment on lines 28 to 59
export const RESULT_TABLE_DISPLAY_KEY = [3, 4, 5, 5.5, 6];
export const WINNING_PRIZE = {
6: {
PRIZE: 2000000000,
DESCRIPTION: '6개',
},
5.5: {
PRIZE: 30000000,
DESCRIPTION: '5개 + 보너스볼',
},
5: {
PRIZE: 1500000,
DESCRIPTION: '5개',
},
4: {
PRIZE: 50000,
DESCRIPTION: '4개',
},
3: {
PRIZE: 5000,
DESCRIPTION: '3개',
},
2: {
PRIZE: 0,
},
1: {
PRIZE: 0,
},
0: {
PRIZE: 0,
},
};
Copy link
Author

@365kim 365kim Feb 24, 2021

Choose a reason for hiding this comment

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

질문드립니다! 🙋🏻‍♀️ - 적절한 자료구조...!

당첨결과 테이블은 위 그림처럼 상금이 작은 row 부터 렌더되어야 합니다. 이를 위해 28행과 같이 RESULT_TABLE_DISPLAY_KEY라는 상수를 두었습니다. 이보다 조금 더 효과적으로(?) 테이블 렌더 순서를 맞춰주는 방법이 있을지 궁금해서 질문드립니다...! RESULT_TABLE_DISPLAY_KEY는 아래 코드와 같이 사용했습니다.

테이블의 순서를 맞추기 위해서는 상수들중에 누군가는 '순서가 있는 자료구조(배열)'이어야 할텐데, 만약 WINNING_PRIZE를 배열로 만들면 로또번호일치 갯수(6, 5.5, 5, 4, ...)를 key값으로 쓸 수 없어서 결국 WINNING_PRIZE는 객체로 남겨두고 RESULT_TABLE_DISPLAY_KEY과 같은 요상한(?) 배열을 만들게 되었습니다. 조금 더 일반적인(?) 방법이 있을 것 같아서 질문드려봅니다!!

요약해서, 객체의 key값에 순서가 있다면 이를 자료구조로 어떻게 풀어내면 좋을지 조언 부탁드리겠습니다... 🙏

// ResultModal.js
createTableBodyHTML() {
  return RESULT_TABLE_DISPLAY_KEY.map((key) => {
    const { DESCRIPTION, PRIZE } = WINNING_PRIZE[key];

    return this.createTableRowHTML({
      DESCRIPTION,
      PRIZE,
      numOfWinningTicket: this.lottoTickets.filter((lottoTicket) => 
        lottoTicket.totalMatchCount === key).length,
    });
  }).join('');
}

Choose a reason for hiding this comment

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

WINNING_PRIZE의 key 기준으로 sort하면 어떨까요? 🤔

Object.keys(WINNING_PRIZE).sort({ ... })

Copy link
Author

@365kim 365kim Feb 24, 2021

Choose a reason for hiding this comment

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

말씀해주신 방법대로 하면 RESULT_TABLE_DISPLAY_KEY를 없앨 수 있겠네요...! 👍👍👍 감사합니다!

한가지 더 여쭤보겠습니다...!! 🙋🏻‍♀️🙋🏻‍♀️ 현재 WINNING_PRIZE의 key값이 숫자(6, 5.5, ...)인데, 이를 상수화하지 않고 그대로 써도 괜찮을까요? 아래와 같이 상수화를 한다면 상수를 다시 상수로 이중 포장하는 것처럼 느껴지다가도, key값으로 쓰이는 6이라는 숫자가 상수이긴 해서 이 부분이 헷갈립니다...!!! 🤧

export const FIRST_PRIZE_MATCH_COUNT = 6;
export const SECOND_PRIZE_MATCH_COUNT = 5.5;
export const THIRD_PRIZE_MATCH_COUNT = 5;
...

Choose a reason for hiding this comment

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

6이라는 값을 동일한 의미로 다른 곳에서도 사용한다면 상수화해서 가져다 쓰면 좋지 않을까요? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

코드 내에서 6이라는 숫자가 직접적으로 쓰이는(written) 곳이 있으면 상수화하는 것이 좋다는 의미로 말씀해주신 것으로 이해하고, '6'이라고 쓰이는 곳은 없어서 따로 상수화하지 않았습니다...!

Copy link

@JaeYeopHan JaeYeopHan left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! comment 남겨봤어요~ 👏 👏 👏

index.html Outdated
<div class="d-flex justify-center mt-5">
<button type="button" class="btn btn-cyan">다시 시작하기</button>
<button type="button" class="restart-button btn btn-cyan">다시 시작하기</button>

Choose a reason for hiding this comment

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

type="reset"은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

리뷰어님께서 요렇게 짚어주신 덕분에 어물쩍 넘어가지 않고 집중해서 찾아볼 수 있었습니다. 감사합니다!!👍👍👍 앞으로는 <form>을 잘 사용할 수 있도록 말씀해주신 키워드 참고해서 내용 정리해보고 적용해 보았습니다. 하나의 reset버튼으로 여러 개의 <form>을 연결할 수는 없어서, 가장 reset할 입력필드가 가장 많은 로또 당첨번호 입력폼과 연결해서 reset버튼을 활용해 보았습니다.

<!-- 로또 당첨번호 입력 폼 -->
<form id="winning-number-form">
</form>

...
<!-- 결과모달 내 다시시작하기 버튼 -->
<button type="reset" class="reset-button btn btn-cyan" form="winning-number-form">다시 시작하기</button>

Choose a reason for hiding this comment

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

와 정리 + 블로그 멋지네요. 제 페이스북 페이지나 SNS에 공유해도 되나요?ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

헛,,,그럼요! Jbee님께서 공유해주신다면 저야 영광입니당 ☺️☺️☺️

src/js/components/App.js Outdated Show resolved Hide resolved
src/js/components/WinningNumberInput.js Outdated Show resolved Hide resolved
this.$openResultModalButton.addEventListener('click', this.onShowModal.bind(this));
}

onChangeWinningNumberInput(e) {

Choose a reason for hiding this comment

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

혹시 controlled, uncontrolled 라는 개념을 알고 계셔서 요렇게 진행하셨는지가 궁금해지네요!

Copy link
Author

@365kim 365kim Feb 25, 2021

Choose a reason for hiding this comment

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

우선 답변을 드리자면 아니요....!🙅🏻‍♀️ controlled, uncontrolled 키워드는 말씀해주셔서 처음 알았습니다.

이 키워드를 언급해주신 이유가 (1) 당첨번호 입력칸 자체 때문인지 (2) 입력값에 따라 실시간으로 변하는 체크메세지 때문인지도 가늠하지 못하고 있습니다...ㅎㅎㅎ

찾아보니 uncontrolled 컴포넌트는 '사용자가 키보드 입력 - 화면에 입력값이 보임 - 이벤트 핸들러가 입력값을 컴포넌트에 전달 - setState' 순서로 진행되는 일반적인(?) HTML form 사용방식이고, controlled 컴포넌트는 '사용자가 키보드입력 - 이벤트핸들러가 입력값을 컴포넌트에 전달 - setState - 화면에 입력값(state)이 보임' 순서로 진행되는 방식으로 이해했습니다. 사용자가 키보드에 입력되는 글자를 바로 보여주는게 아니라 컴포넌트를 거쳤다가 나온 글자를 보여줄 수 있다는게 신기했습니다...

제가 이해한 바로는 당첨번호 입력칸은 uncontrolled, 체크메세지는 (input이 아니라 div이지만 굳이 구분하자면) controlled라고 할 수 있을 것 같습니다.

uncontrolled와 controlled 둘을 각각 언제 쓰면 좋을지 비교해주는 요 아티클에는 controlled방식이 더 리액트스러운 방법이고 데이터(state)와 UI(input)의 싱크를 맞출 수 있어서 좋다는 내용이 있었습니다. 아티클에 나와있는 경우 중 입력하는 데이터의 포맷을 가공해서 보여줘야할 경우에 매우 유용할 것 같습니다. 하지만 다른 경우에는 controlled 방식이 어떤 의미가 있는지 아직은 크게 와닿지 않았습니다...!

제가 잘못 이해한 부분, 간과한 부분, 요 키워드를 말씀해주신 의도에 대해서 더 코멘트 해주시면 감사하겠습니다! 🙏

Choose a reason for hiding this comment

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

winningNumbers, bonusNumber 의 값을 change event에서 메모리에 저장하고 있어서 일부러 controlled 방식을 사용하셨나가 궁금했어요ㅎㅎ

그럴 필요없다면 input의 각 값은 submit 이벤트에서 접근하면 되지 않을까 하는 생각이 들어요!

Choose a reason for hiding this comment

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

데모를 보니 observing해야하는 작업이 있어서 요렇게 처리하신 것으로 이해할게요! 👍

);
this.setState({ checkMessage });

if (this.checkMessage === WINNING_NUMBER_CHECK_MESSAGE.COMPLETED) {

Choose a reason for hiding this comment

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

이런 flag 없이 winningNumber를 가져올 수 없었나요?

Copy link
Author

Choose a reason for hiding this comment

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

상태를 확인하는 다른 방법을 생각해보자는 말씀으로 이해하고 isFulfilled라는 Boolean 타입 변수로 확인하도록 변경해보았습니다. 혹시 이게 아니라 다른 방향으로 생각해보도록 의도하신 거라면 힌트 조금 더 부탁드리겠습니다...! 🙏

// before
const checkMessage = this.validateInput(inputValues);
...
if (this.checkMessage === WINNING_NUMBER_CHECK_MESSAGE.COMPLETED) {  // *****
  this.setState(winningNumber)
}

// after
const { checkMessage, isFulfilled } = validateInput(inputValues);
...
if (!this.isFulfilled) {  // *****
  return;
}
this.setState(winningNumber)

Choose a reason for hiding this comment

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

onChangeWinningNumberInput가 너무 많은 일을 하고 있다. 가 의도였어요!
지금 다시 보니 observing하느라 요런 로직을 하셨구나 싶네요!

after 코드도 👍 입니다!

src/js/components/WinningNumberInput.js Outdated Show resolved Hide resolved
}

attachEvents() {
this.$modalClose?.addEventListener('click', this.closeModal.bind(this));

Choose a reason for hiding this comment

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

왜 optional chaining을 사용해주셨나요?

Copy link
Author

Choose a reason for hiding this comment

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

위 테스트 에러를 해결하기 위해 옵셔널 체이닝을 넣었습니다.

수익률을 구하는 getLottoRateOfReturn()를 호출하기 위해 테스트코드에서 ResultModal를 import해서 생성하면 DOM요소를 찾지 못해 위와 같은 에러가 발생했습니다. 옵셔널 체이닝을 여기저기 넣으면 어디서 나중에 에러가 발생했는지 추적이 어려워져서 어려워서 남발하지 않는 것이 좋은 것으로 알고 있습니다. 이 경우에 브라우저 환경에서는 $modalClose 요소 탐색이 문제가 되지 않을 것이라는 확신이 있었고, 테스트 통과를 위해 간단한 해결방법이라고 판단해서 넣게되었습니다.

다음 단계에서는 이런 상황이 발생하지 않도록 lottoTickets와 winningNumber라는 전체적인 상태관리 구조를 조금 더 단순하게 다듬고 더 testable한 코드를 작성해보려고 합니다...!

Choose a reason for hiding this comment

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

네! 다음 미션 때 좀더 개선될 수 있는 부분일 것 같아요 ㅎㅎ

살짝 첨언드리자면 테스트를 위해 프로덕션 코드에 trick이 가해지는 상황은 없어야 합니다!ㅎㅎ

@365kim
Copy link
Author

365kim commented Feb 26, 2021

📌 리뷰반영 요약표

번호 리뷰내용 관련코멘트 반영커밋
1 Boolean 타입으로 중복 변환하는 부분 제거 - 9ab3fca
2 Object.keys()메서드 활용, 불필요한 상수 제거 바로가기 4b45e65
3 HTML문서 작성 신경쓰기, type="reset"사용 바로가기 9ab3fca
4 클래스 내 유틸성 함수 분리 바로가기 a047941, dd7e93f
+ optional chaining 사용한 이유 바로가기
+ controlled, uncontrolled 개념 인지여부 바로가기

Copy link

@JaeYeopHan JaeYeopHan left a comment

Choose a reason for hiding this comment

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

다음 스텝에서도 이야기 이어가요! 수고하셨어요! 👏 👏 👏

@JaeYeopHan JaeYeopHan merged commit 1fdeed4 into woowacourse:365kim Feb 26, 2021
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