[1단계 - 행운의 로또 미션] 우디(류정우) 제출합니다. #163

jw-r · 2023-02-16T07:36:34Z

안녕하세요!!
요술토끼와 페어로 진행한 우디입니다

처음으로 TDD 방식으로 구현해봤는데요
생각보다 막힘없이 진행되었다고 느껴졌지만 코드의 퀄리티는 어떨지 모르겠습니다😭

util 함수들은 최대한 도메인과 분리하여 범용성을 높혀 봤습니다
에어비앤비 코딩 컨벤션을 최대한 지키려고 했습니다
프로덕션 코드 내에서 중복을 최소화 하도록 했습니다

코드 리뷰 잘 부탁드리겠습니다 🤩

추가적인 질문사항

"> 구입금액을 입력해 주세요."와 같은 문자열은 크게 고민하지않고 상수화를 진행했습니다

하지만 아래와 같은 것들은 어떻게 상수화를 진행할 수 있을까요?

`${data.length}개를 구매했습니다.`
`총 수익률은 ${formattedRate}%입니다.`

현재 코드에서는 상수화를 진행하지 않고 놔두었지만 굳이 진행하자면 아래와 같은 방법이 있을 것 같습니다.

const RATE = {
  FRONT: '총 수익률은 ',
  BACK: '%입니다',
}

`${RATE.FRONT}${formattedRate}${RATE.BACK}`

하지만 가독성 측면에서도 좋아보이지 않고, 오버 핸들링이라는 생각이 듭니다
이에 대해 리트의 생각이 궁금해요!

- 로또 지정된 횟수만큼 발행 - 총 당첨금을 계산 -> 수익률 계산

…주어진다면 내림차순으로 정렬

lsw1164

우디, 코드 잘봤어요. 막힘 없이 진행하셨다고 하셨는데 코드 잘 짜셨네요 👏
코멘트 확인해주세요~

lsw1164 · 2023-02-16T14:25:37Z

src/util/getSortedNumbers.js

@@ -0,0 +1,15 @@
+import { SORT_TYPE } from './constants.js';
+
+const getSortedNumbers = (numbers, orderType = SORT_TYPE.INCREASING) => {


order, sort 같은 의미로 쓰고있다면 하나만 써도 좋지 않을까요?

lsw1164 · 2023-02-16T14:26:44Z

src/util/getSortedNumbers.js

+import { SORT_TYPE } from './constants.js';
+
+const getSortedNumbers = (numbers, orderType = SORT_TYPE.INCREASING) => {
+  const copiedNumbers = numbers.slice();


불변성 챙기셨군요👍

프리코스 때부터 자바스크립트 Airbnb 코딩 컨벤션을 지키라는 요구사항이 있어서 잘 지키려고 노력했었는데.. 단지 가독성을 위함이 아닌 불변성을 지키는 등의 이유도 있었군요!

리트 덕분에 불변성에 대해서 공부하고 정리할 수 있었어요🤩

lsw1164 · 2023-02-16T14:31:06Z

src/constants/index.js

+  BONUS_NUMBER: /^([1-9]|[1-3]\d|4[0-5])$/,
+});
+
+const RANK = {


Object.freeze 사용 기준이 있을까요?

마지막에 추가하느라 깜빡하고 넣어주지 않았습니다..앞으로 리뷰를 요청하기전에 꼼꼼하게 확인하도록 하겠습니다!!

Object.freeze() 자체는 온보딩 미션과 다른 시도를 해보고 싶어서 사용했습니다!
이 객체안의 값은 변하지 않는다는 것을 명시적으로 알려줄 수도 있고, 실제로 외부에서 변경할 수 없으니까요

하지만 폴더명이나 파일명으로 이미 변하지 않는 값이라는 걸 명시하고 있고, 굳이 constants 폴더안의 객체를 변경할 일이 없으니 꼭 Object.freeze()를 사용할 필요는 없었다는 생각이 듭니다

이에 대해서는 조금 더 고민해보고 싶습니다!

자바스크립에서는 Object.freeze() 사용하는 게 더 안전할 거 같아요.

나중에 타입스크립트 적용하시면 문법에서 불변성 지원합니다.
그때는 Object.freeze()를 안써도 되겠죠. :)

lsw1164 · 2023-02-16T14:35:51Z

src/domain/LottoGame.js

+  }
+
+  checkBonusNumber(lotto, bonusNumber) {
+    if (lotto.includes(bonusNumber)) {


includes 자체가 bool 값을 반환하고 있어서 if문이 없어도 되곘군요

lsw1164 · 2023-02-16T14:39:51Z

src/domain/LottoGame.js

+    if (matchedCount === 6) return RANK.FIRST;
+    if (matchedCount === 5 && hasBonusNumber) return RANK.SECOND;
+    if (matchedCount === 5) return RANK.THIRD;
+    if (matchedCount === 4) return RANK.FOURTH;
+    if (matchedCount === 3) return RANK.FIFTH;


이렇게 반복적으로 동일성 체크하는 if문 비교에는
차라리 switch 문이 더 낫다는 생각입니다 :)

프로그래밍 요구사항에서 인덴트에 대한 조건(1칸)과 메소드의 길이(10줄)에 대한 조건이 있어서 switch문을 사용하지 않았습니다

물론 반드시 만족시켜야 할 기능 요구사항과는 달리, 프로그래밍 요구사항은 필요에 따라 조금은 유연하게 적용시켜도 된다고 생각하고 있지만, 이번 미션에서는 if문을 적용시켜보고 싶습니다!

lsw1164 · 2023-02-16T15:17:57Z

src/view/output.js

+5개 일치 (${GAME_VALUE.PRIZE[2].toLocaleString()}원) - ${data[RANK.THIRD]}개
+5개 일치, 보너스 볼 일치 (${GAME_VALUE.PRIZE[1].toLocaleString()}원) - ${data[RANK.SECOND]}개
+6개 일치 (${GAME_VALUE.PRIZE[0].toLocaleString()}원) - ${data[RANK.FIRST]}개`);
+  }


return문이 빠진걸까요?

허걱.. 미처 발견하지 못했네요
꼼꼼한 리뷰 감사합니다!!

lsw1164 · 2023-02-16T15:19:06Z

src/view/output.js

@@ -0,0 +1,29 @@
+import { FORMATTING_TYPE, GAME_VALUE, RANK } from '../constants/index.js';
+
+const output = (data, formattingType = '') => {


포맷 타입 별로 다른 로직이 수행되는군요.
앞으로 함수가 더 복잡해질 예정인데 개선할 수 있을까요?

현재는 리액트의 컴포넌트화와 같은 방식으로 해결하는게 어떨까라는 고민을 하고있습니다

서로 관련이 있는 UI 혹은 중복되는 UI에 대한 출력 로직을 하나의 파일로 관리하는 방식입니다
그렇게 분리한 파일들을 최종적으로 output 객체의 메소드의 형식으로 넣어주면 좋지 않을까 생각하고 있습니다

view.output.board(data, type="WINNING_STATE")와 같이 해당 메소드를 실행할 수 있습니다

이 부분에 대해서는 리트의 생각도 궁금해요!!

2단계 기준으로 관련 있는 컴포넌트는 하나로 합치고 그렇지 않은 컴포넌트는 분리하겠다고 이해했는데 맞을까요?

현재는 하나의 파일에서 모든 로직을 구현했지만, 연관성이 높은 로직들끼리 하나의 독립적인 파일로 분리해서 구현한 후, output이라는 함수(객체)에 내부 함수(메서드) 형태로 넣어주는 방식으로 생각했었습니다!

구현 로직이 외부에 있다고 하더라도 output 객체가 모든 타입을 알고 있어야 되는 구조일까요?
연관성 높은 로직들끼리 잘 분리 했다면 다시 output으로 감싸는 대신 분리한 로직을 그대로 사용해도 될 거 같다는 생각이 들었어요 🤔

lsw1164 · 2023-02-16T15:22:33Z

src/LottoController.js

+    view.close();
+  }
+
+  handleCatchedError(message, callback) {


VSCODE에 스펠체크하는 익스텐션을 깔면 오타 잡아줘요!
handleCaughtError

오우.. 이런 확장 프로그램이.. 꿀팁 감사합니다🥹
Code Spell Checker 바로 적용해 볼게요!!

lsw1164 · 2023-02-16T15:30:53Z

src/LottoController.js

+    [COMMAND.RETRY]: this.startGame.bind(this),
+    [COMMAND.EXIT]: this.#exitGame.bind(this),


bind 처리 해주신 의도가 궁금해요.
화살표 함수도 고려 대상이었을까요?

[COMMAND.RETRY]: this.startGame.bind(this),

위의 부분에서 bind 처리를 해주지 않으면 TypeError: attempted to get private field on non-instance에러가 발생했습니다
bind처리를 해주면 정상 작동했기 때문에 #commandHandler오브젝트를 통해 메소드를 실행할 때 this를 잃어버린다고 생각했습니다

하지만 아래에 대해서는 에러가 발생하지 않으니 제가 잘못 생각했던 것 같습니다

[COMMAND.EXIT]: this.#exitGame,

두 메소드는 private 메소드이냐 아니냐의 차이가 있을 뿐인데, 왜 private메소드에서는 bind처리 없이도 잘 동작하면서, 그렇지 않을 때는 위의 에러가 발생하는지 잘 모르겠습니다ㅠㅠ

작은 힌트를 던져주셨으면 좋겠습니다🥹

다른 차이가 있어요.
힌트: 해당 메서드 내부에서 this 접근 여부

드디어 이 피드백을 설명할 수 있게 됐습니다🥳
callback 함수는 기본적으로는 함수를 실행한 것과 같기 때문에 호출될 때 binding되는 this는 전역 객체를 가리키게 돼요
때문에 위와 같은 에러가 발생하게 된거죠

생성자 함수를 사용해서 호출하는 LottoController 클래스 내의 this는 인스턴스 객체 그 자체가 되기 때문에

[COMMAND.RETRY]: this.startGame.bind(this),

위의 코드의 this는 인스턴스 객체를 가리키고, 콜백 함수에 직접 this를 바인딩 해주는 것으로 startGame 메서드 내의 this가 인스턴스 객체를 가리키도록 할 수 있습니다!

callback 함수는 기본적으로는 함수를 실행한 것과 같기 때문에 호출될 때 binding되는 this는 전역 객체를 가리키게 돼요

'this가 전역 객체를 가리킨다'는 경우에 따라 다릅니다.
지금은 엄격모드가 아니라 전역 객체를 가리키는 거 같군요?
this를 더 자세히 알고 싶으시면 나중에 '실행 컨텍스트'라는 키워드와 함께 공부해보시면 좋습니다.

lsw1164 · 2023-02-16T15:33:47Z

__tests__/validator.test.js

@@ -0,0 +1,41 @@
+import validator from '../src/domain/validator';
+describe('입력받은 로또 구매 비용에 대한 유효성 검사 테스트', () => {


테스트 전반적으로 깔끔하네요 👏

…ind처리 제거

lsw1164

우디, 이해가 안되는 부분은 의문을 갖고 한 번 더 고민하려는 모습 좋네요👍

스텝1은 merge 할게요! 남은 피드백은 다음 스텝에서 마저 반영부탁드려요!

lsw1164 · 2023-02-20T15:21:33Z

src/LottoController.js

+    [COMMAND.RETRY]: this.startGame.bind(this),
+    [COMMAND.EXIT]: this.#exitGame.bind(this),


다른 차이가 있어요.
힌트: 해당 메서드 내부에서 this 접근 여부

lsw1164 · 2023-02-20T15:22:14Z

src/domain/ScoreBoard.js

+  getBoard() {
+    return this.#board;
+  }


정확히 이해하셨습니다!

lsw1164 · 2023-02-20T15:27:28Z

src/domain/pickLotto.js

+  while (lotto.size < GAME_VALUE.LOTTO_SIZE) {
+    lotto.add(pickRandomNumber(GAME_VALUE.MAX_LOTTO_NUMBER));
+  }


오... 그렇군요. 공부해보시면 정상적으로 섞이지 않는구나를 알게되실 겁니다.

제레미가 결과를 추출해서 첨부해주셨더라고요.
#161 (comment)
뭔가 나오던 숫자가 계속 나오는 거 같은... 찝찝하죠?

sort를 공부할 좋은 기회군요!

lsw1164 · 2023-02-20T15:40:48Z

src/view/output.js

@@ -0,0 +1,29 @@
+import { FORMATTING_TYPE, GAME_VALUE, RANK } from '../constants/index.js';
+
+const output = (data, formattingType = '') => {


2단계 기준으로 관련 있는 컴포넌트는 하나로 합치고 그렇지 않은 컴포넌트는 분리하겠다고 이해했는데 맞을까요?

lsw1164 · 2023-02-20T15:46:29Z

"> 구입금액을 입력해 주세요."와 같은 문자열은 크게 고민하지않고 상수화를 진행했습니다

하지만 아래와 같은 것들은 어떻게 상수화를 진행할 수 있을까요?
`${data.length}개를 구매했습니다.`
`총 수익률은 ${formattedRate}%입니다.`
현재 코드에서는 상수화를 진행하지 않고 놔두었지만 굳이 진행하자면 아래와 같은 방법이 있을 것 같습니다.
const RATE = {
  FRONT: '총 수익률은 ',
  BACK: '%입니다',
}

`${RATE.FRONT}${formattedRate}${RATE.BACK}`
하지만 가독성 측면에서도 좋아보이지 않고, 오버 핸들링이라는 생각이 듭니다 이에 대해 리트의 생각이 궁금해요!

제 생각을 말씀드리자면 가독성이 우선이라는 생각이 들어요! 가독성이 너무 나빠진다면 꼭 상수로 만들 필요는 없다고 생각해요

jw-r · 2023-02-23T14:35:52Z

@lsw1164
추가 코맨트도 한번 더 확인 부탁드리고 싶습니다🥹

jw-r added 28 commits February 14, 2023 17:07

docs: 기능 목록 작성

9e55d98

test: getSortedNumbers에 대한 테스트 코드 작성

d28e962

feat: 정렬된 배열을 반환하는 기능

f6ff2a6

feat: 랜덤 숫자를 반환하는 기능

30122b2

chor: type model 추가

b127cd3

feat: 입력받은 구입 금액에 대한 유효성 검사 구현

51f0ebc

feat: 입력받은 당첨 번호에 대한 유효성 검사 구현

80856a5

feat: 보너스 번호에 대한 유효성 검사 구현

e09f88d

feat: 로또 한장을 발행하는 기능 구현

866851f

feat: 로또에 보너스 번호가 포함되어 있는지를 반환하는 기능 구현

e6d3755

feat: 일치하는 일반 로또의 개수를 반환하는 기능 구현

f4d3314

feat: 일치하는 로또 숫자의 개수와 보너스 번호의 포함 여부를 반환하는 기능 구현

00f884a

feat: 일치하는 번호의 수를 이용해 등수를 판별하는 기능 구현

5990440

feat: 총 수익률 계산 기능 구현

b1b3467

- 로또 지정된 횟수만큼 발행 - 총 당첨금을 계산 -> 수익률 계산

feat: 입출력을 담당하는 view 구현

617e4d1

feat: Controller 구현

1aa5c9c

feat: 재시작 커맨드에 대한 유효성 검사 구현

ab05ef6

refactor: 로또 게임 내 상수 제거

8a3db04

fix: 보너스 번호를 제대로 판별하지 못 하는 문제 수정

0f35a56

refactor: 매개변수가 3개 이상인 메소드의 기능 통합

a45341e

refactor(getSortedNumbers): orderType이 매개변수로 주어지지 않는다면 오름차순, 'DESC'가 …

4c33d0d

…주어진다면 내림차순으로 정렬

refactor(ScoreBoard): board를 오브젝트가 아닌 랭크 + 1 만큼의 크기를 가진 배열로 수정

ffd1df6

refactor: 변수명 restart를 retry로 수정

3137d92

refactor(Controller): 당첨 번호와 보너스 번호를 받아오는 메서드를 각각 받아오는 메소드로 분리

dc98bb4

chor: Given When Then 주석 제거

3796a50

refactor: 당첨 번호와 보너스 번호를 입력하는 메소드 통합

ee4de07

chor: build를 위해 type: module 제거

fed9dfe

refactor: 변수명 money를 buget으로 수정

b83d7b9

jw-r changed the base branch from main to evencoding February 16, 2023 07:36

lsw1164 self-assigned this Feb 16, 2023

lsw1164 requested changes Feb 16, 2023

View reviewed changes

jw-r added 6 commits February 19, 2023 00:05

refactor: catched 오타를 caught로 수정

93c3988

refactor(output): 어떤 조건문에 해당할 때, 함수가 조기에 종료될 수 있도록 return문을 추가

1351c41

refactor(validator): throwErrorIf~ 네이밍을 간결하게 validate~로 수정

2a2b4ef

refactor(pickLotto): 최댓값과 로또 길이를 매개변수로 넘겨받도록 수정, 랜덤 숫자 배열을 생성하는 방식 변경

90289c9

refactor(ScoreBoard): 로또의 당첨 현황을 반환할 때, 불변성을 지키도록 수정

a403217

refactor(LottoController): #commandHandler 객체에서 실행할 private 메소드에 대해 b…

8e2bf4a

…ind처리 제거

lsw1164 approved these changes Feb 20, 2023

View reviewed changes

lsw1164 merged commit 7a3f5d2 into woowacourse:evencoding Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1단계 - 행운의 로또 미션] 우디(류정우) 제출합니다. #163

[1단계 - 행운의 로또 미션] 우디(류정우) 제출합니다. #163

jw-r commented Feb 16, 2023 •

edited

Loading

lsw1164 left a comment

lsw1164 Feb 16, 2023

lsw1164 Feb 16, 2023

jw-r Feb 18, 2023

lsw1164 Feb 16, 2023

jw-r Feb 18, 2023

lsw1164 Mar 1, 2023

lsw1164 Feb 16, 2023

lsw1164 Feb 16, 2023

jw-r Feb 20, 2023

lsw1164 Feb 16, 2023

jw-r Feb 18, 2023

lsw1164 Feb 16, 2023

jw-r Feb 20, 2023

lsw1164 Feb 20, 2023

jw-r Feb 23, 2023

lsw1164 Feb 23, 2023 •

edited

Loading

lsw1164 Feb 16, 2023

jw-r Feb 18, 2023 •

edited

Loading

lsw1164 Feb 16, 2023

jw-r Feb 20, 2023

lsw1164 Feb 20, 2023

jw-r Feb 23, 2023 •

edited

Loading

lsw1164 Feb 23, 2023 •

edited

Loading

lsw1164 Feb 16, 2023

lsw1164 left a comment

lsw1164 Feb 20, 2023

lsw1164 Feb 20, 2023

lsw1164 Feb 20, 2023

lsw1164 Feb 20, 2023

lsw1164 commented Feb 20, 2023

jw-r commented Feb 23, 2023

		@@ -0,0 +1,15 @@
		import { SORT_TYPE } from './constants.js';

		const getSortedNumbers = (numbers, orderType = SORT_TYPE.INCREASING) => {

		@@ -0,0 +1,29 @@
		import { FORMATTING_TYPE, GAME_VALUE, RANK } from '../constants/index.js';

		const output = (data, formattingType = '') => {

		[COMMAND.RETRY]: this.startGame.bind(this),
		[COMMAND.EXIT]: this.#exitGame.bind(this),

		@@ -0,0 +1,41 @@
		import validator from '../src/domain/validator';
		describe('입력받은 로또 구매 비용에 대한 유효성 검사 테스트', () => {

[1단계 - 행운의 로또 미션] 우디(류정우) 제출합니다. #163

[1단계 - 행운의 로또 미션] 우디(류정우) 제출합니다. #163

Conversation

jw-r commented Feb 16, 2023 • edited Loading

추가적인 질문사항

lsw1164 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsw1164 Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jw-r Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jw-r Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

lsw1164 Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsw1164 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsw1164 commented Feb 20, 2023

jw-r commented Feb 23, 2023

jw-r commented Feb 16, 2023 •

edited

Loading

lsw1164 Feb 23, 2023 •

edited

Loading

jw-r Feb 18, 2023 •

edited

Loading

jw-r Feb 23, 2023 •

edited

Loading

lsw1164 Feb 23, 2023 •

edited

Loading