[1단계 - 자동차 경주 구현] 윤생(이윤성) 미션 제출합니다. #154

2yunseong · 2023-02-09T08:03:06Z

No description provided.

- @woowacourse/mission-utils 설치

Co-authored-by: Gabriel Ju Hyun, Yoon <gabrielyoon7@gmail.com>

- 원래 콜백패턴에서 사용해 의존성이 입력 로직에 얽혀있었는데, play 메서드로 이동했다.

- 일회성으로 사용한 tryCount를 제거

Puterism

안녕하세요 윤생 😃
이번 미션의 리뷰어를 맡게 된 체프라고 합니다. 반갑습니다!

첫번째 미션 구현하시느라 고생 많으셨어요. 전체적으로 깔끔하게 코드를 짜주셔서 떠오르는 피드백이 많지 않았네요 😅 피드백과 더불어 질문을 몇 가지 남겨놓았으니 확인 부탁드릴게요.

추가로 package-lock.json과 yarn.lock 파일이 같이 있어서 yarn과 npm을 둘 다 사용하는 것으로 보여져요. 패키지 관리자는 하나만 사용하시는 것이 좋으니 통일 부탁드릴게요! (왜 그런지도 생각해보시면 좋을 것 같네요!)

Puterism · 2023-02-09T09:33:36Z

src/Car.js

+  move(go) {
+    if (go) {
+      this.#position += 1;
+    }
+  }


move라는 메소드에서는 go의 값에 따라서 this.#position의 값을 더할지 말지 결정하고 있는데요.
이렇게 되면, move라는 메소드가 다음과 같이 두 가지 역할을 하게 된다고 생각해요.

go의 truthy/falsy 여부를 판별하는 역할

this.#position의 숫자를 1 증가시키는 역할

move라는 메소드의 네이밍 맥락 상 this.#position의 숫자를 1 증가시키는 역할까지만 해도 충분하지 않을까 생각하는데요. 윤생의 생각은 어떠신지 궁금해요.

위 피드백과는 별개로 go라는 네이밍만으로는 어떤 역할을 하는 파라미터를 받는 것인지 유추하기 어려울 것 같아요. 네이밍만으로 어떤 역할인지 유추할 수 있게 하려면 어떻게 이름을 짓는 게 좋을까요?

move라는 메소드의 네이밍 맥락 상 this.#position의 숫자를 1 증가시키는 역할까지만 해도 충분하지 않을까 생각하는데요. 윤생의 생각은 어떠신지 궁금해요.

저 코드를 구현할 때는 go 라는 결과에 따라 자동차가 스스로 갈지 안갈지 판단하는 메서드라고 생각했습니다! 그래서 move에 함께 넣었습니다. go의 상태에 따라 갈지 안갈지는 다른 메서드에서 해주는게 좋을 것 같네요. 체프 말씀대로 move라는 메서드가 두가지 책임을 가지고 있다고 생각이 듭니다!

위 피드백과는 별개로 go라는 네이밍만으로는 어떤 역할을 하는 파라미터를 받는 것인지 유추하기 어려울 것 같아요. 네이밍만으로 어떤 역할인지 유추할 수 있게 하려면 어떻게 이름을 짓는 게 좋을까요?

제 생각으로는 읽었을 때 의도가 명확하게 들어나야 될 것 같습니다! 지금은 go가 갈지 안갈지 판단할 때 필요한 값이니 이 뜻을 담을 수 있는 역할인지는 2단계 리팩토링할 때 고민해 반영해보겠습니다. 개인적인 생각으로는 먼저 책임 분리를 한 다음, 지금의 go가 하는 역할에 대해 고민해보아야 겠네요.

체프의 피드백을 읽고 고민한 부분이 있는데,go의 truthy/falsy 여부를 판별하는 역할 은 일단 Car 클래스가 가져야 된다고 생각합니다. 그 이유는 이동할지/안할지의 결정은 차 객체의 책임이라고 생각하는데요,

그러면 랜덤한 값에 따라 현재 go라는 truthy/falsy 여부를 결정하는 로직은 GameManager가 갖는다고 생각합니다. 게임에서 차에게 이동할지 말지 메세지를 보낸다고 생각하기 때문입니다.

체프는 제 의견에 대해 어떻게 생각하시나요?

저는 개인적으로 이동할지/안할지의 결정 또한 GameManager가 가지고 있어야 한다고 생각했어요. GameManager가 차를 움직이게 할지 말지를 결정할 수 있는 주체가 되는 것이 좋다고 생각했거든요. 마치 GameManager를 심판의 역할로 봤달까요?

물론 정답은 없습니다. 차 객체가 어디까지 책임을 가지게 할 것인가에 따라서 달라질 것 같아요. 결정은 윤생이 해주시면 됩니다 :)

어떤 경우가 더 나을 것인지 고민해봐야겠네요! 감사합니다 체프:D

Puterism · 2023-02-09T14:37:29Z

src/GameManager.js

+const Console = require('./utils/Console');
+const { isValidCarNames, isValidTryCount } = require('./utils/Validation');
+const Car = require('./Car');
+// const { pickRandomNumber } = require('./utils/RandomGenerator');


미사용되는 코드 주석은 잘 지워주세요!

Puterism · 2023-02-09T14:40:54Z

src/GameManager.js

+  #cars = [];
+
+  isForward() {
+    return RandomGenerator.pickRandomNumber() >= 4;


전체적으로 상수화를 잘해주셨는데요.
숫자 4에 대한 상수화도 진행해주시면 좋을 것 같아요. 숫자 4는 무슨 역할을 하나요?

문제의 요구사항에서 4 이상일 경우 전진한다 를 보아, 4는 전진할지 말지 결정하는 역할을 한다고 생각하였습니다! 해당 의도에 맞게 상수화를 진행해야겠네요.

Puterism · 2023-02-09T15:07:42Z

src/GameManager.js

+    cars.sort((a, b) => b.getPosition() - a.getPosition());
+    const max = cars[0].getPosition();
+    const winners = cars
+      .filter((car) => car.getPosition() === max)
+      .map((car) => car.getName());
+    return winners;


우승자를 판정하는 로직은 sort와 filter, map을 이용해서 깔끔하게 잘 짜주신 것 같아요! 좋습니다 👍

Puterism · 2023-02-09T15:10:06Z

src/GameManager.js

+  }
+}
+
+module.exports = GameManager;


ES6 문법으로 export하지 않은 이유가 혹시 있으셨을까요?

공통 피드백에서도 깨달았지만, 습관적으로 사용하였습니다. require/ module.exports 와 import/ export의 차이점에 대해서는 한번 알아볼 예정입니다!
혹시 체프께서 하실 말씀 있으시다면 코멘트 남겨주세요!!

Puterism · 2023-02-09T15:13:43Z

__tests__/GameManager.test.js

+    const cars = gameManager.generateCars(carNames);
+    mockRandoms(moves);
+    gameManager.moveCars(cars);
+    expect(gameManager.judgeWinners([...cars])).toEqual(winners);


toBe와 toEqual을 둘 다 사용해주셨는데요. toBe와 toEqual의 차이점은 무엇일까요?

toBe는 Object.is 처럼 객체의 참조(동일성)를 비교하고, toEqual은 서로 같은 정보를 가지고 있는지(동등성)을 판단하는 차이점이 있다고 생각합니다!

Puterism · 2023-02-09T15:27:13Z

__tests__/Validation.test.js

+/* eslint-disable no-undef */
+const { isValidCarNames, isValidTryCount } = require('../src/utils/Validation');
+
+describe('Validation Test', () => {


입력값 유효성 검사 테스트까지 진행하시다니 좋네요 👍

감사합니다 :D

Puterism · 2023-02-09T15:35:38Z

src/utils/Validation.js

@@ -0,0 +1,10 @@
+const Validation = {
+  isValidCarNames(carNames) {
+    return carNames.every((name) => name.length > 0 && name.length <= 5);


이렇게 숫자값 그대로 코드에 사용되는 것을 매직 넘버라고 하는데요.
매직 넘버 0과 5에 대해서도 상수화를 진행해주시면 좋을 것 같아요!

넵! 감사합니다. 별개로 하나 생각해본 점이 있는데요!

일단 작성한 Validation.test.js중 일부를 잠깐 가져오겠습니다!

test.each([ [['yunseong', 'gabriel'], false], [['aa', 'bb', 'cc'], true], [['aa', 'bb', ''], false], // [['aa', 'bb', '윤생이😁😁'], true], => 고민할 부분(서로게이트 쌍) ])('이름 유효성 검사(%s: %s)', (names, expected) => { expect(isValidCarNames(names)).toBe(expected); });

위 Validation.test.js에서 보시듯이, 윤생이😁😁는 사람이 느끼기에는 다섯글자입니다.
하지만, 이모티콘은 서로게이트 쌍으로, 실제로 문자열의 길이는 7이 됩니다. (1 + 1 + 1 + 2 + 2)
그런데 만약 이름이 중국어인 친구나 서로게이트 쌍으로 표현되는 문자열은 사람이 느끼기에는 5글자 이하이지만 프로그램에서는 에러가 발생하는 문제가 있다고 판단하였습니다.

그래서 이걸 어떻게 판단할지 고민하다가 미션 제출 시간이 와 일단은 넘어갔는데.. 아쉬움이 남네요.
이 아쉬움을 체프께 공유하고자 글 남겨보았습니다! 제가 아직은 깊게 찾아보진 않았는데 찾아볼 예정입니다!
혹시 체프께서도 여기 관해서 생각해볼 거리나 시도해볼 방법이 있으시다면 말씀해주시기 바랍니다!!

저도 잘 모르겠어서 조사해봤는데요. 다행히 방법이 있는 것 같네요. Intl.Segmenter에 대해서 알아보시면 좋을 것 같습니다.

Puterism · 2023-02-09T15:36:25Z

src/utils/Validation.js

@@ -0,0 +1,10 @@
+const Validation = {
+  isValidCarNames(carNames) {
+    return carNames.every((name) => name.length > 0 && name.length <= 5);


every와 some의 차이점은 무엇일까요?

every는 배열의 모든 요소가 인자로 전달한 callback에 따라 true로 평가되는지 판단하고, some은 적어도 한 요소라도 인자로 전달한 callback에 따라 true로 평가되는지 판단하는 메서드로 알고 있습니다!

2yunseong

안녕하세요 체프!  인사가 늦었네요 🚰 ·̫🚰
먼저 PR메세지를 먼저 작성했어야 했는데 그러지 못한 점 죄송합니다 ㅠㅠ 다음 번에는 주의해서 작성할께요.
많은 고민거리나 생각할 주제를 던져주셔서 너무 감사드립니다!
거기에 이어 제가 몇가지 질문이랑 답변을 남겼습니다!!
즐거운 금요일과 함께 주말도 잘 보내시길 바랍니다 :D

2yunseong · 2023-02-10T01:26:44Z

__tests__/GameManager.test.js

+    const cars = gameManager.generateCars(carNames);
+    mockRandoms(moves);
+    gameManager.moveCars(cars);
+    expect(gameManager.judgeWinners([...cars])).toEqual(winners);


toBe는 Object.is 처럼 객체의 참조(동일성)를 비교하고, toEqual은 서로 같은 정보를 가지고 있는지(동등성)을 판단하는 차이점이 있다고 생각합니다!

2yunseong · 2023-02-10T01:26:53Z

__tests__/Validation.test.js

+/* eslint-disable no-undef */
+const { isValidCarNames, isValidTryCount } = require('../src/utils/Validation');
+
+describe('Validation Test', () => {


감사합니다 :D

2yunseong · 2023-02-10T07:17:08Z

src/GameManager.js

+const Console = require('./utils/Console');
+const { isValidCarNames, isValidTryCount } = require('./utils/Validation');
+const Car = require('./Car');
+// const { pickRandomNumber } = require('./utils/RandomGenerator');


2yunseong · 2023-02-10T07:26:08Z

src/GameManager.js

+  }
+}
+
+module.exports = GameManager;


공통 피드백에서도 깨달았지만, 습관적으로 사용하였습니다. require/ module.exports 와 import/ export의 차이점에 대해서는 한번 알아볼 예정입니다!
혹시 체프께서 하실 말씀 있으시다면 코멘트 남겨주세요!!

2yunseong · 2023-02-10T07:36:59Z

src/utils/Validation.js

@@ -0,0 +1,10 @@
+const Validation = {
+  isValidCarNames(carNames) {
+    return carNames.every((name) => name.length > 0 && name.length <= 5);


넵! 감사합니다. 별개로 하나 생각해본 점이 있는데요!

일단 작성한 Validation.test.js중 일부를 잠깐 가져오겠습니다!

test.each([ [['yunseong', 'gabriel'], false], [['aa', 'bb', 'cc'], true], [['aa', 'bb', ''], false], // [['aa', 'bb', '윤생이😁😁'], true], => 고민할 부분(서로게이트 쌍) ])('이름 유효성 검사(%s: %s)', (names, expected) => { expect(isValidCarNames(names)).toBe(expected); });

위 Validation.test.js에서 보시듯이, 윤생이😁😁는 사람이 느끼기에는 다섯글자입니다.
하지만, 이모티콘은 서로게이트 쌍으로, 실제로 문자열의 길이는 7이 됩니다. (1 + 1 + 1 + 2 + 2)
그런데 만약 이름이 중국어인 친구나 서로게이트 쌍으로 표현되는 문자열은 사람이 느끼기에는 5글자 이하이지만 프로그램에서는 에러가 발생하는 문제가 있다고 판단하였습니다.

그래서 이걸 어떻게 판단할지 고민하다가 미션 제출 시간이 와 일단은 넘어갔는데.. 아쉬움이 남네요.
이 아쉬움을 체프께 공유하고자 글 남겨보았습니다! 제가 아직은 깊게 찾아보진 않았는데 찾아볼 예정입니다!
혹시 체프께서도 여기 관해서 생각해볼 거리나 시도해볼 방법이 있으시다면 말씀해주시기 바랍니다!!

2yunseong · 2023-02-10T07:39:41Z

src/utils/Validation.js

@@ -0,0 +1,10 @@
+const Validation = {
+  isValidCarNames(carNames) {
+    return carNames.every((name) => name.length > 0 && name.length <= 5);


every는 배열의 모든 요소가 인자로 전달한 callback에 따라 true로 평가되는지 판단하고, some은 적어도 한 요소라도 인자로 전달한 callback에 따라 true로 평가되는지 판단하는 메서드로 알고 있습니다!

2yunseong · 2023-02-10T07:39:59Z

src/GameManager.js

+  #cars = [];
+
+  isForward() {
+    return RandomGenerator.pickRandomNumber() >= 4;


문제의 요구사항에서 4 이상일 경우 전진한다 를 보아, 4는 전진할지 말지 결정하는 역할을 한다고 생각하였습니다! 해당 의도에 맞게 상수화를 진행해야겠네요.

2yunseong · 2023-02-10T07:56:11Z

src/Car.js

+  move(go) {
+    if (go) {
+      this.#position += 1;
+    }
+  }


move라는 메소드의 네이밍 맥락 상 this.#position의 숫자를 1 증가시키는 역할까지만 해도 충분하지 않을까 생각하는데요. 윤생의 생각은 어떠신지 궁금해요.

저 코드를 구현할 때는 go 라는 결과에 따라 자동차가 스스로 갈지 안갈지 판단하는 메서드라고 생각했습니다! 그래서 move에 함께 넣었습니다. go의 상태에 따라 갈지 안갈지는 다른 메서드에서 해주는게 좋을 것 같네요. 체프 말씀대로 move라는 메서드가 두가지 책임을 가지고 있다고 생각이 듭니다!

위 피드백과는 별개로 go라는 네이밍만으로는 어떤 역할을 하는 파라미터를 받는 것인지 유추하기 어려울 것 같아요. 네이밍만으로 어떤 역할인지 유추할 수 있게 하려면 어떻게 이름을 짓는 게 좋을까요?

제 생각으로는 읽었을 때 의도가 명확하게 들어나야 될 것 같습니다! 지금은 go가 갈지 안갈지 판단할 때 필요한 값이니 이 뜻을 담을 수 있는 역할인지는 2단계 리팩토링할 때 고민해 반영해보겠습니다. 개인적인 생각으로는 먼저 책임 분리를 한 다음, 지금의 go가 하는 역할에 대해 고민해보아야 겠네요.

체프의 피드백을 읽고 고민한 부분이 있는데,go의 truthy/falsy 여부를 판별하는 역할 은 일단 Car 클래스가 가져야 된다고 생각합니다. 그 이유는 이동할지/안할지의 결정은 차 객체의 책임이라고 생각하는데요,

그러면 랜덤한 값에 따라 현재 go라는 truthy/falsy 여부를 결정하는 로직은 GameManager가 갖는다고 생각합니다. 게임에서 차에게 이동할지 말지 메세지를 보낸다고 생각하기 때문입니다.

체프는 제 의견에 대해 어떻게 생각하시나요?

2yunseong · 2023-02-10T07:57:23Z

src/Car.js

+  getName() {
+    return this.#name;
+  }
+
+  getPosition() {
+    return this.#position;
+  }


자바스크립트에서 제공하는 getter를 사용하지 않고 메서드로 만들어 사용했는데, 어느 방법이 나을까요? 저는 구현해놓고 보니 언어에서 기본적으로 제공하는 getter 기능을 사용해 보이는게 더 좋은 방법 같습니다.

저도 언어에서 제공하는 getter 기능을 사용하는 게 더 좋을 것 같다는 생각에 동의합니다 :)

답변 감사합니다~!

Puterism

수고 많으셨습니다! 다음 2단계에서 뵈어요~

2yunseong · 2023-02-12T08:33:53Z

넵!! 감사합니다~~

2yunseong and others added 30 commits February 7, 2023 15:36

chore: npm 모듈 설치

0864c02

- @woowacourse/mission-utils 설치

docs: 요구사항 문서 작성

e4448fb

docs: 프리코스 피드백 및 페어프로그래밍 규칙 작성

80e324d

chore: 불필요한 npm 모듈 삭제

3ee13de

feat: 입출력 담당 모듈 구현

97cf66d

feat: n대의 자동차를 입력 받는 기능 구현

d8da869

Co-authored-by: Gabriel Ju Hyun, Yoon <gabrielyoon7@gmail.com>

feat: 각 자동차 이름이 1자 이상 5자 이하의 글자인지 판별하는 기능 구현

fbfb1cb

test: 각 자동차 이름이 1자 이상 5자 이하의 글자인지 판별하는 기능 테스트

35663a4

feat: 자동차 생성 기능 구현

84db7c1

feat: 시도 횟수를 입력 받는 기능 구현

ead5f2b

feat: 시도 횟수를 입력이 자연수인지 판별하는 기능 구현

658fd36

test: 시도 횟수를 입력이 자연수인지 판별하는 기능 테스트

e4806a1

feat: 입력받은 시도 횟수 만큼 모든 자동차의 이동을 시도하는 기능 구현

a6df78d

feat: 0부터 9까지 랜덤으로 숫자를 발생시키는 기능 구현

079ed07

feat: 차가 전진하는 기능 구현

a1457ee

feat: 자동차의 현재 이동 상태를 출력하는 기능 구현

06f1107

feat: 최종 우승자를 판별하고 출력하는 기능 구현

f12c5f9

docs: 기능목록 최신화

93d4d82

feat: 잘못된 이름 입력이 들어왔을 경우 에러 처리 구현

6e0c83c

Co-authored-by: Gabriel Ju Hyun, Yoon <gabrielyoon7@gmail.com>

feat: 잘못된 횟수 입력이 들어왔을 경우 에러 처리 구현

16a0534

feat: 게임 종료 후 cleanup 기능 구현

90bd66a

test: 자동차 이름 유효성 검사 테스트 케이스 추가

f0134b8

test: 시도 횟수 검사 테스트 케이스 추가

e21ed1a

refactor: 콜백 패턴의 입력 로직을 async/await로 변경

a990424

Co-authored-by: Gabriel Ju Hyun, Yoon <gabrielyoon7@gmail.com>

refactor: 형 변환 책임 분리

926a75b

refactor: 게임 동작/제어에 관한 책임 분리

ebf28d5

- 원래 콜백패턴에서 사용해 의존성이 입력 로직에 얽혀있었는데, play 메서드로 이동했다.

refactor: UI 출력을 담당하는 로직을 분리

0f1e56e

refactor: 콘솔을 담당하는 책임 분리

c083e0d

style: 비구조화 할당 문법으로 변경

c802c7c

refactor: 문자열 리터럴 상수화

29ed5f6

2yunseong added 14 commits February 9, 2023 14:43

refactor: 불필요한 필드 삭제

c0e56f3

- 일회성으로 사용한 tryCount를 제거

refactor: 사용하지 않는 import 문 삭제

71e97bf

refactor: 자동차 생성 로직 분리

57a4612

refactor: 메서드에서 필드로 사용하던 변수를 매개변수로 변경

5c72db0

refactor: 불필요한 import 문 제거

454395d

test: Car 클래스 테스트 작성

43913b2

test: 테스트를 위한 형변환 로직 추가

78dd042

chore: eslint 설정 변경

c7d7fe3

test: 랜덤 기능 mock 함수 생성

f975546

test: 전진할지 안할지 판단하는 기능 테스트

3ea8cd9

fix: mocking 이 안되는 이슈 해결

5b95d00

test: 승자들을 판단하는 기능 테스트

096d43d

chore: lint 설정 변경

bfe05fc

docs: 요구사항 문서 작성

1aa7e7a

Puterism requested changes Feb 9, 2023

View reviewed changes

2yunseong commented Feb 10, 2023

View reviewed changes

2yunseong added 4 commits February 10, 2023 17:28

refactor: 미사용 주석 제거

4af06ce

refactor: CommonJS 모듈 import/export ES6 방식으로 변경

db293a1

refactor: 매직 넘버 상수화

98dabaf

refactor: move 책임 분리

1d268c1

Puterism approved these changes Feb 11, 2023

View reviewed changes

Puterism merged commit cfb8ddb into woowacourse:2yunseong Feb 11, 2023

[1단계 - 자동차 경주 구현] 윤생(이윤성) 미션 제출합니다. #154

[1단계 - 자동차 경주 구현] 윤생(이윤성) 미션 제출합니다. #154

Conversation

2yunseong commented Feb 9, 2023

Puterism 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

Puterism Feb 10, 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

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

2yunseong 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

Puterism left a comment

Choose a reason for hiding this comment

2yunseong commented Feb 12, 2023

Puterism Feb 10, 2023 •

edited

Loading