[2단계 - 행운의 로또 미션] 유세지(김용래) 미션 제출합니다. #141

usageness · 2022-03-04T15:29:24Z

유세지(김용래) 행운의 로또 미션 2단계 PR 제출합니다.

재남님 안녕하세요!
지난 리뷰에서 받았던 피드백들을 되새기고, 다른 코치분들의 말씀도 생각하며 코드를 작성하느라
조금 늦게 제출하게 되었네요 😥

2단계에서는 지난 PR의 구조는 유지하되, 불필요한 패턴 적용을 최대한 줄이려고 노력했습니다.
MVC니, MVP니 하는건 지금 미션에서는 오버 엔지니어링일뿐더러 패턴이 해결하려고 했던 문제를
깊게 생각해보지도 않고 적용했던터라 제대로 된 방식도 아니었다는 생각이 들었어요.

간혹 코드 길이가 15라인을 초과하는 메서드가 존재합니다.
1, 2줄이 아닌 30라인을 훌쩍 넘어 너무 길어지는 코드는 분리했을때 얻는 이익이 더 크다고 판단하여
우선 기능에 따라 분리하였고 (지적받은 부분이었는데 죄송합니다) 주석으로 표기하였습니다.

그 외에 17줄, 18줄 메서드는... 그냥 두었습니다.
이 정도는 괜찮겠지요...?

좋은 주말 보내시길 바라겠습니다 😊

데모 페이지

이곳에서 확인 가능합니다.

roy-jung

유세지님, 2단계도 잘 완수하셨네요! 고생하셨습니다.

확인해보니 함수내 '줄수 제한'은 요구사항이 아닙니다. 애초에 모든 함수가 15줄 이내여야 한다는 건 너무 가혹하기도 하고, 도리어 불필요한 짧은 함수를 남발하여 가독성을 떨어뜨리는 결과를 초래할 수도 있어요.
UX적으로 신경쓰고자 한 부분 좋았습니다. 특히 invalid시 input 색상을 변경해주는 시도가 돋보였어요. 다만 오히려 생각을 너무 많이 하셔서 사용성이 저해된 측면도 있는 것 같고, 기왕 UX를 신경쓰는 거라면 보다 명확하게 에러메시지를 노출해주는 방법을 더 고민해보시면 좋았겠다- 하는 아쉬움이 있네요. (이 내용은 향후 유세지님의 성장을 기원하는 차원에서 말씀드린 것이고 수정을 요청드리는 건 아닙니다.)

수정요청사항은 코멘트 남겼으니 확인해 주세요 :)

roy-jung · 2022-03-05T14:07:56Z

src/css/index.css

+#lotto-section[data-visible-state='true'] {
  display: flex;
  flex-direction: row;
+  height: 237.84px;


이 애매한 수치는 뭔가요..? 피그마를 그대로 입력하신걸까요?

테스트 환경에서 요소 5개가 들어갈 수 있는 기본 높이를 개발자 도구에서 찍어서 (...) 확인했습니다...
적당한 요소 값을 지정해주는 방법을 모르겠네요ㅠㅠ rem 이나 em 단위로 바꾸는게 좋을까요?

해당 라인은 로직상 중복되는 부분이라 제거하였습니다!

개발자도구에 소숫점자리값이 읽히나요?
그렇더라도 반올림을 해주시는편이.. 어차피 1px 이하로는 표현 못하니까요

소수점은 표현이 안되는군요... 개발자 도구에서 보이길래 적용되는 줄 알았네요ㅠㅠ

roy-jung · 2022-03-05T14:10:00Z

src/js/__tests__/lotto.test.js

+    const winningNumbers = [1, 2, 3, 4, 5, 7, 6];
+
+    const lotto = Lotto.create(lottoNumbers);
+    expect(lotto.result(winningNumbers)).toBe(NUMBER.SECOND_GRADE_INDEX);


이 테스트를 통과한다면 어지간해선 잘 동작할 것이라고 안심해도 될까요?

구현된 기능들에 비해 하나만 테스트하는건 부실하다는 느낌이 있네요...!
보너스 번호를 포함하는 케이스(2등), 포함하지 않는 케이스(1등), 실패하는 케이스(꽝) 을 모두 포함시켰습니다!

roy-jung · 2022-03-05T14:13:12Z

src/js/__tests__/lottoGame.test.js

    } catch ({ message }) {
      expect(message).toEqual(ERROR_MESSAGE.CHARGE_IS_INVALIDATE);
    }
  });


테스트코드만 봐도 1001, 1000.5, 1500 같은 수들이 다 괜찮을지 어떨지 알 수 있게 해주시면 좋겠어요.

기존의 코드로 해당 입력들을 걸러낼 수 없어서
나누어 떨어지는 수만 입력할 수 있도록 유효성 검사를 추가했습니다!

roy-jung · 2022-03-05T14:16:40Z

src/js/__tests__/lottoGame.test.js

+    lottoRound.createLottoList(charge);
+
+    const lottoListFromGetterFunc = lottoRound.getLottoList();
+    expect(lottoListFromGetterFunc).toEqual(lottoRound.lottoList);


이 테스트는 중첩 객체의 각 프로퍼티가 모두 같은 값으로 구성되어 있음을 확인할 수는 있으나, '깊은복사'가 된 것인지 여부를 알 수는 없습니다. 예를들어 getLottoList 메소드가 다음과 같이 되어 있더라도 위 테스트는 문제 없이 통과됩니다.

getLottoList() { return this.lottoList }

이전 피드백에서 말씀해주신만큼 값이 변경될 일이 없는 현재의 요구사항으로는
깊은 복사는 무의미한 기능인 것 같아 코드에서 제거하였습니다.

별개로, 깊은 복사를 테스트하도록 수정한다면 아래처럼 테스트 할 수 있을 것 같습니다!

it('lottoList의 getter는 깊게 복사된 값을 반환한다.', () => { const lottoRound = new LottoRound(); lottoRound.createLottoList(charge); const lottoListFromGetterFunc = lottoRound.getLottoList(); expect(lottoListFromGetterFunc).toEqual(lottoRound.lottoList); lottoRound.lottoList = null; expect(lottoListFromGetterFunc).not.toEqual(lottoRound.lottoList); });

roy-jung · 2022-03-05T14:18:19Z

src/js/__tests__/lottoGame.test.js

+    const lottoRound = new LottoRound();
+    const lottoList = [];
+    const winningNumbers = [1, 2, 3, 4, 5, 6, 10];
+    const result = [1, 0, 1, 1, 0, 66718333, 0];


이 result 배열은 무슨 값들인가요? 66718333은 어떤걸 의미하는거죠?

result는 예상되는 결과값이 들어있는 배열인데 표현력이 부족했던 것 같습니다.
각 배열의 값에 어떤 의미인지 알 수 있는 이름과 함께 할당했습니다!

const expectResult = [ firstGradeCount, secondGradeCount, thirdGradeCount, fourthGradeCount, fifthGradeCount, earningRate, notWinningCount, ];

roy-jung · 2022-03-05T14:57:51Z

src/js/views/index.js

+  #calculateVisibleLottoSectionHeight(lottoAmount) {
+    return `${lottoAmount * ELEMENT_PROPERTY.HEIGHT_OF_ONE_LOTTO_ICON_LINE}px`;
+  }
+
+  #calculateInvisibleLottoSectionHeight(lottoAmount) {
+    if (lottoAmount > NUMBER.LOTTO_SECTIONS_DEFALUT_CAPACITY_IN_ICON) {
+      const linesOfLottoIcon = Math.ceil(
+        (lottoAmount - NUMBER.LOTTO_SECTIONS_DEFALUT_CAPACITY_IN_ICON) /
+          NUMBER.LOTTO_ELEMENT_PER_LINE
+      );
+      return `${
+        linesOfLottoIcon *
+          (ELEMENT_PROPERTY.HEIGHT_OF_ONE_LOTTO_ICON_LINE + ELEMENT_PROPERTY.GAP_OF_LOTTO_ITEM) +
+        ELEMENT_PROPERTY.DEFAULT_HEIGHT_OF_LOTTO_SECTION
+      }px`;
+    }
+    return `${ELEMENT_PROPERTY.DEFAULT_HEIGHT_OF_LOTTO_SECTION}px`;
+  }


어떤 의도로 이런 작업을 하셨는지는 알겠으나,

이럴만한 가치가 있나 싶은 생각이 듭니다.

높이 계산이 제대로 되지 않고 있는 것 같습니다. (번호보기를 눌렀을때 하단에 여백이 많이 잡힘. 누르지 않은 경우에도 하단 여백이 많음.)

이런 측면에서 검토해보시고 의견 주세요.

이럴만한 가치가 있나 싶은 생각이 듭니다.

요구사항에 명시되지 않은 부분이긴 하지만, 애니메이션 효과를 주기 위해 사용했습니다.
사용자의 시선이 자연스럽게 아래쪽 섹션(lottoSection) 으로 따라가도록 하는 장치였는데,
말씀하신대로 그럴만한 가치가 없다면 없는 작업입니다. 개인적인 욕심이었던 것 같아요 😥

높이 계산이 제대로 되지 않고 있는 것 같습니다. (번호보기를 눌렀을때 하단에 여백이 많이 잡힘. 누르지 않은 경우에도 하단 여백이 많음.)

기본값으로 설정된 높이(DEFAULT_HEIGHT_OF_LOTTO_SECTION)가 있습니다.
로또 요소가 다섯개 정도 들어갈 수 있는 크기인데, 이보다 로또 요소가 차지하는 공간이 작으면
항상 이 값으로 고정되도록 구현하였습니다.
요소가 적은데 너무 붙어있어서 답답하게 보일뿐더러,
아래쪽 영역과 구분해주려는 의도가 있었습니다.

이 부분은 개인적인 취향에 맞춘 방식이라 어떤게 더 좋은 방법인지 잘 모르겠네요...
재남님의 의견이 궁금합니다!

번호숨김인 상태에서 최소값을 지정한건 (여백이 너무 큰 느낌이 있긴 하지만) 납득이 가요. 그런데 번호보기를 눌러 전부 펼친 상태에서도 하단 여백이 많이 잡히는건 오류가 아닌가 싶은 느낌이 들더라구요.

애니메이션 효과를 주는것 자체는 좋습니다. 그런데 5개 이하로 구매했을 경우, 번호숨김 상태에서 이미 높이값이 늘어나는 애니메이션이 실행되는건 문제 아닐까요? 이후 숨김<->보기를 번갈아 눌러보면 여백은 그대로 있고 숫자만 보였다 사라졌다 하고 있거든요.. '이럴 가치가 있나' 라고 말씀드린 포인트가 이 부분입니다.

다시 생각해보니 일반 사용자 입장에서는 오류처럼 느껴질 수 있겠네요. 기본값을 줘야한다 라는 생각에 너무 잡혀서 이런 부분을 제대로 생각하지 못했던것 같아요! 이 부분을 제거하고 나니 오히려 코드도 정리된것 같고 보기 좋습니다.

/* index.js 48~60 line, DEFAULT 제거 */ renderAlignState(visibleState, lottoAmount = 0) { if (visibleState) { this.$lottoSection.style.height = this.#calculateVisibleLottoSectionHeight(lottoAmount); this.$alignConverter.setAttribute('disabled', true); setTimeout(() => { this.$lottoContainer.setAttribute('data-visible-state', visibleState); this.$alignConverter.removeAttribute('disabled'); }, NUMBER.ANIMATION_TIME); return; } this.$lottoContainer.setAttribute('data-visible-state', visibleState); this.$lottoSection.style.height = this.#calculateInvisibleLottoSectionHeight(lottoAmount); }

roy-jung · 2022-03-05T15:03:47Z

src/js/views/index.js

+  setInvalidInputState(target) {
+    target.setAttribute('data-invalid-state', true);
+  }
+
+  setValidInputState(target) {
+    target.setAttribute('data-invalid-state', false);
+  }


일반적으로 html의 data attribute는 정적인 값 위주로 넣어주고,
이 상황과 같이 style 처리를 위한 경우에는 class를 이용합니다.
css에서 [data-invalid-state]와 같은 선택자는 class에 비해 성능이 떨어지기 때문입니다.

앗... 성능이 떨어지는 특징이 있는지 몰랐네요... 감사합니다!!

roy-jung · 2022-03-05T15:06:16Z

src/js/app.js

+    if (isNotValidNumber(Number(e.target.value))) {
+      this.lottoRoundView.setInvalidInputState(e.target);
+      e.target.value = '';
+      return;
+    }
+    this.lottoRoundView.setValidInputState(e.target);
+    if (e.target.value.length === 2) {
+      if (e.target.nextElementSibling !== null) {
+        e.target.nextElementSibling.focus();
+        return;
+      }
+      if (e.target === this.$lastWinNumberInput) {
+        this.$bonusNumberInput.focus();
+      }
+    }


e.target 을 변수로 하면 더 좋을 것 같아요.

if문에 의한 indent 깊이를 더 줄여주세요 (early return)

수정했습니다!

roy-jung · 2022-03-05T15:21:14Z

src/js/app.js

+      Number(e.path[0][0].value),
+      Number(e.path[0][1].value),
+      Number(e.path[0][2].value),
+      Number(e.path[0][3].value),
+      Number(e.path[0][4].value),
+      Number(e.path[0][5].value),
+      Number(e.path[0][6].value),


e.path는 크롬에서만 제공하는 비표준 프로퍼티입니다. 데모를 사파리, 파이어폭스 등에서 실행해 보세요.

낯선 속성이다 싶었는데 비표준 프로퍼티였군요 😅
querySelector를 이용한 방식으로 변경했습니다!

roy-jung · 2022-03-05T15:24:46Z

src/js/models/Lotto.js

+        if (number === winningNumbers[NUMBER.BONUS_NUMBER]) {
+          isContainBonusNumber = true;
+        }


이 부분이 여기에 있는게 맞나요? "5개 일치" 조건을 갖추지 않은 다른 경우들에는 불필요한 조건문 아닌가요?

불필요한 조건문이긴 하나 아래와 같은 이유로 여기에서 판별하는게 가장 효율적이라고 생각했습니다.

보너스 번호 판별이 불필요한 최악의 경우 (보너스 포함 4개 일치)
lottoNumbers.forEach : 비교 6회 (로또 번호 원소)
winningNumbers.includes : 비교 7회 (당첨 번호 원소)
이 중 4개의 hit -> 비교 4회
=> ( 6 * 7 ) + 4 = 46회의 비교 (최악)

아래쪽으로 분리하여 번호 5개가 일치할때만 보너스 번호 비교

this.lottoNumbers.forEach((number) => { if (winningNumbers.includes(number)) { countMatchNumber += 1; } }); if (countMatchNumber === 5) { this.lottoNumbers.forEach((number) => { if (number === winningNumbers[NUMBER.BONUS_NUMBER]) { isContainBonusNumber = true; break; } } }

첫 번째 lottoNumbers.forEach : 비교 6회 (로또 번호 원소)
winningNumbers.includes : 비교 7회 (당첨 번호 원소)
이 중 5개의 hit -> 추가 비교 없음
if (countMatchNumber === 5) : 비교 1회
두 번째 lottoNumbers.forEach : 비교 3회 (평균 3회 순회 : 중간에 있었을 경우)
=> ( 6 * 7 ) + 4 = 46회의 비교 (평균)

더 좋은 방법이 있다면 코드가 바뀔 수 있을 것 같지만 생각나지 않네요...ㅠㅠ
재남님이라면 어떻게 하셨을지 궁금합니다!

getLottoRank의 case 5에서 처리하면 되지 않나요?

그렇네요...! 코드도 훨씬 깔끔하고 좋습니다 😊

case 6: if (this.lottoNumbers.some((number) => number === bonusNumber)) { return NUMBER.SECOND_GRADE_INDEX; } return NUMBER.FIRST_GRADE_INDEX;

usageness · 2022-03-06T12:18:03Z

주말에도 꼼꼼한 리뷰 남겨주셔서 감사합니다! 생각할거리가 많았던 것 같아요 😊
피드백 사항들 수정했고, 코멘트로 몇 가지 의견 남겨보았습니다. 확인 부탁드려요!

usageness · 2022-03-07T12:23:51Z

답변 감사합니다!! 말씀해주신 부분도 추가로 적용했습니다!
그동안 너무 한쪽만 생각하고 있었던 것 같네요... 😅

데모페이지

roy-jung · 2022-03-07T12:34:37Z

수정사항 모두 확인했습니다. UX적으로 신경 많이 쓰셔서 사용자 입장에서 참 좋네요! 승인 및 머지할게요. 고생하셨습니다 :)

usageness added 12 commits February 28, 2022 12:09

docs: todo list 정리

6224d54

feat: 로또 모델에서 등수 반환 구현

f1e648a

feat: 당첨 결과를 이용하여 통계와 수익률을 반환할 수 있다

d4cadf9

feat: 게임 초기화가 가능해야 한다.

19039ca

feat: 결과 모달창 마크 업 작성

fc6104f

feat: 결과를 보여주는 modal view 구현

ce1ecb4

feat: css 유저 편의성 개선

26dc565

feat: 당첨 번호 유효성 검사 로직 추가

d34d366

feat: 로또 컨테이너의 높이 값이 늘어날 경우 애니메이션 적용

b64913d

fix: 애니메이션중 클릭시 버그 수정

cc065f2

feat: 당첨 번호 유효성 검사 및 다음 번호 자동 포커싱 구현

bdeff9f

refactor: LottoGame.js -> LottoRound.js 파일 이름 및 메소드 이름 변경

cb33016

pocojang requested a review from roy-jung March 5, 2022 10:57

pocojang assigned roy-jung Mar 5, 2022

roy-jung suggested changes Mar 5, 2022

View reviewed changes

usageness added 9 commits March 6, 2022 13:16

chore: 중복되는 css 할당 수정

47df9c7

test: 로또 모델 테스트 케이스 보완

c88a799

feat: 로또 구매시 조건(로또 가격으로 나누어 떨어져야 한다) 추가

69cb517

refactor: 깊은 복사 메서드 제거

e8ca7e7

refactor: 수익률 반환 test에서 결과 배열의 값 역할 명시

6e19d03

refactor: data attribute 방식 class 할당으로 변경

a4dff61

fix: 비표준 프로퍼티 제거 후 querySelector로 대체

4555bf6

refactor: early return 을 이용한 depth 제거

f097ca7

fix: 수익률 연산 로직 오류 수정

e405782

usageness added 2 commits March 7, 2022 21:01

refactor: 보너스 번호 확인 로직 변경

6e8b087

refactor: lottoSection의 default state 제거

5493d95

roy-jung merged commit 2fe2552 into woowacourse:usageness Mar 7, 2022

[2단계 - 행운의 로또 미션] 유세지(김용래) 미션 제출합니다. #141

[2단계 - 행운의 로또 미션] 유세지(김용래) 미션 제출합니다. #141

Uh oh!

Conversation

usageness commented Mar 4, 2022

데모 페이지

Uh oh!

roy-jung 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!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

usageness commented Mar 6, 2022

Uh oh!

usageness commented Mar 7, 2022

Uh oh!

roy-jung commented Mar 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

usageness Mar 6, 2022 •

edited

Loading

usageness Mar 6, 2022 •

edited

Loading

usageness Mar 6, 2022 •

edited

Loading