[1단계 - 자동차 경주 구현] 하루(김하루) 미션 제출합니다. #6

365kim · 2021-02-08T15:12:00Z

Chris님, 안녕하세요!

처음 리뷰요청 드리는 🙋🏻‍♀️ 하루(@365kim) 입니다.
저는 🙆🏻‍♂️ 유조(@yujo42)와 페어 프로그래밍을 진행하고 약간의 리팩토링을 거쳐 PR 올리게 되었습니다.

요약

구현 순서: 테스트 전체 작성 후, 각 테스트를 통과하기 위한 단위 기능 구현
구현 결과: 작성한 cypress 테스트 모두 통과 ✔
협업 방법: 30분 단위로 턴을 돌려 각 커밋에 공동책임을 갖도록 페어프로그래밍 진행
협업 환경: Zoom 화면 공유 & VSCode 라이브셰어

git clone https://github.com/365kim/javascript-racingcar.git
cd javascript-racingcar
git checkout 365kim
npm install
npm run cypress

구현기능 목록

자동차 이름 입력
- 올바른 자동차 이름을 입력한 후 확인버튼을 누르면 시도횟수 입력창을 화면에 출력한다.
  - 'EAST, WEST, SOUTH, NORTH'를 입력하면 화면에 시도횟수 입력창이 표시되는지 테스트한다.
- 올바르지 않은 자동차 이름을 입력한 경우 경고메세지를 출력한다.
  - 'YUJO_YOONHO'을 입력한 경우 '이름은 5글자 이하로 입력해 주세요.' 경고메세지를 출력하고 입력창을 초기화 하는지 테스트한다.
  - 공백만 입력된 경우나 아무것도 입력되지 않은 경우 '공백만으로는 이름을 구성할 수 없습니다.' 경고메세지를 출력하고 입력창을 초기화 하는지 테스트한다.
시도 횟수 입력
- 양의 정수만을 시도횟수로 입력할 수 있게 한다.
  - '-7'을 입력한 경우 '1 이상의 숫자를 입력해주세요.' 경고메세지를 출력하고 입력창을 초기화 하는지 테스트한다.
  - 공백만 입력된 경우나 아무것도 입력되지 않은 경우 '1 이상의 숫자를 입력해주세요.' 경고메세지를 출력하고 입력창을 초기화 하는지 테스트한다.
- 올바른 시도횟수를 입력한 후 확인버튼을 누르면 앞서 자동차 경주 진행화면을 표시해준다.
  - 자동차 경주 진행화면이 visible 상태인지 테스트한다.
  - 화면에 표시된 자동차 대수와 입력된 자동차 대수가 일치하는지 테스트한다.
  - 화면에 표시된 자동차 이름과 입력된 자동차 이름이 일치하는지 테스트한다.
자동자 경주 진행
- 자동차 경주가 정상적으로 진행되는지 테스트 한다.
  - 난수를 생성하는 함수가 0 ~ 9 사이의 정수를 반환하는지 테스트한다.
  - 전진횟수를 결정하는 함수가 '[1, 3, 3, 7]'을 입력받았을 때 '1'을 반환하는지 테스트한다.
  - 게임진행 결과에 따라 각 자동차에 화살표 갯수가 제대로 표시되는지 테스트한다.
결과 출력
- 자동차 경주 게임을 완료한 후 누가 우승했는지를 화면에 출력한다. 우승자가 여러명일 경우 ,를 이용하여 구분해서 출력한다.
  - 최종 우승자가 🏆 최종 우승자: ${쉼표로 구분된 우승자 목록} 🏆와 일치하는지 테스트한다.
게임 다시 시작
- 다시 시작버튼을 누르면 초기 화면을 출력해서 게임을 다시시작할 수 있도록 한다.
  - 시도횟수 입력창, 게임진행 화면, 게임 결과 화면이 모두 보이지 않는 상태인지 테스트한다.
  - '#game-process-screen' 노드가 비어있음을 확인하여 게임 진행 내용을 초기화 되었는지 테스트한다.
  - 자동차이름 입력창과 시도횟수 입력창이 초기화 되었는지 테스트한다.

고민사항 / 미해결과제

"테스트 코드도 기능 코드만큼 리팩토링에 공을 들이는 것이 맞을까요?
테스트 코드의 품질을 얼마나 신경쓰는 것이 바람직한지 궁금합니다."

저희가 작성한 테스트에서는 'input을 클릭하고 키워드를 입력하는 동작'이 거의 모든 테스트에서 반복적으로 필요했습니다. 따라서 typeCarNameAndSubmit(), typeRacingCountAndSubmit()라는 메서드를 따로 분리해서 사용했는데 실은 이 두 함수도 매우 유사해서 다시 한번 별도의 메서드로 분리할 수 있어 보였습니다. 뿐만 아니라 얼럿창의 메세지 내용을 확인하는 테스트에도 같은 구문이 반복되어서 사용되었기 때문에 이 부분도 분리할 수 있을 것 같았습니다.
기능코드가 아닌 테스트코드의 경우에도 이런 식으로 리팩토링 하고 모듈화 해서 메서드당 15줄을 넘지 않는 등의 기능코드와 같은 원칙을 적용하는 방향을 지향해야 할지, 아니면 '테스트 코드 작성도 비용이다.’라는 명제에 따라 아니면 가독성을 크게 해치지 않는 어느 적정선에서 정리하는 방향을 지향해야 할지 궁금합니다...!

시간 내서 리뷰해주셔서 감사합니다.
크고 작은 코멘트 많이 많이 부탁드리겠습니다!
그리고 새해 복 많이받으세요 😆

… into 365kim

ysm-dev

안녕하세요! 👋
하루님 코드를 리뷰하게된 Chris(양성민)입니다.
리뷰가 조금 늦어져서 죄송합니다.

첫번째 미션 수행하시느라 고생하셨습니다.

세부적인 코멘트는 각 라인에 달았습니다.

전체적인 구조 피드백은

$cars 등 직접 DOM을 다루고 DOM에 직접 접근해서 data 값을 변경하다보니 Model과 View가 너무 강결합 되어있다는 생각이 듭니다.
다른 수강생들의 코드도 참고해보고, Car 를 Object나 Class로 작성해서 Model과 View를 분리하려고 노력해보세요. 👍
Car, Game 등을 객체나 클래스로 선언했을 때 각각 어떤 속성들을 가질 수 있는지, 그리고 왜 Model과 View를 분리하는게 중요한지 좀더 고민해보면 좋을 것같습니다. 👍

고민사항 - 추상화하고, 함수로 분리하고, 중복을 제거하는 작업은 항상 미래의 유지보수성에 도움이 됩니다. 하지만 이는 가독성을 해칠 수 있습니다. 이 둘은 tradeoff입니다.
중복이 보인다고, 중복이 생긴다고 섣부르게 추상화하거나, 과도하게 추상화하면 오히려 요구사항이 추가되거나 바뀌었을 때, 중복을 허용하고 작성하는 것보다 더 더러운 코드가 될 수 있습니다.

어느시점에 중복을 제거하고 함수로 분리해야할지, 추상화해야할지는 많은 코드를 작성하고 연습하면서 몸으로 체득해야합니다. 이건 저도 아직 잘 못하는 부분이예요.

위와 같은 고민을 하고있다면
Kent C.Dodds 의 AHA Programming 이라는 발표를 꼭 들어보세요.
(https://www.youtube.com/watch?v=wuVy7rwkCfc)
다른 수강생 분들께도 정말 추천합니다.

그 외 함수의 네이밍이나 컨벤션, 테스트코드는 훌륭합니다 👍

첫번째 미션 정말 수고하셨습니다. :)

ysm-dev · 2021-02-10T04:52:46Z

src/js/utils/constant.js

+export const VALIDATOR = {
+  MAX_NAME_LENGTH: 5,
+  MIN_NAME_LENGTH: 1,
+  MIN_COUNT: 1,
+};


VALIDATOR라는 이름은 일반적으로 어떤 값을 검증하는 작업을 수행하는 함수나 객체 이름으로 쓰입니다.
여기서는 그냥 상수의 역할만 하고있는것같아서 CAR나 CAR_NAME 같은 이름이 더 적절할 것 같습니다! 👍

ysm-dev · 2021-02-10T04:55:27Z

src/js/utils/constant.js

+
+export const ERR_MESSAGE = {
+  NAME_TOO_LONG: `이름은 ${VALIDATOR.MAX_NAME_LENGTH}글자 이하로 입력해 주세요.`,
+  NAME_BLANK: '공백만으로는 이름을 구성할 수 없습니다.',


에러메시지의 key값은 조금 길어지더라도 어떤 에러메시지인지 key값만 보고도 명확하게 알 수 있도록
NAME_CANNOT_BE_BLANK와 같은 좀더 명확한 이름으로 짓는게 어떨까요? 👍

ysm-dev · 2021-02-10T04:59:42Z

src/js/input/handleRacingCountInput.js

+const isValidRacingCount = (racingCount) => {
+  if (racingCount < VALIDATOR.MIN_COUNT) {
+    alert(ERR_MESSAGE.COUNT_TOO_SMALL);
+    return false;
+  }
+  return true;
+};


함수의 이름은 값을 검증만 하는 것처럼 작성하셨는데,
실제로 함수 내부에서는 alert이라는 사이드이펙트가 존재하는 것 같아요.
함수의 이름만 보고도 함수가 무슨 일을 하는지 명확하게 알 수 있도록하고,
그 이름에 맞게 한가지의 일만 수행하도록 작성하는게 좋습니다!

alert 함수를 함수 외부로 추출하는게 좋겠어요! 👍

ysm-dev · 2021-02-10T05:18:08Z

src/js/input/handleRacingCountInput.js

+  if (!isValidRacingCount(racingCount)) {
+    return ($racingCountInput.value = '');
+  }


이렇게 작성하더라도 handleRacingCountInput 함수를 사용하는 쪽에서 항상 return 값 없이 사용만 한다면 문제가 없겠지만,
실제 이 $racingCountInput.value 값을 함수 외부에서 받아서 사용할 의도가 아니라면 그 의도가 명확히 드러나도록 아래와 같이 분리해서 작성하게 좋을 것 같습니다. 👍
코드를 항상 짧게 작성하는 것이 좋은 코드라는 생각을 버리고, 코드에 의도를 명확히 드러내도록 작성하는 습관을 들여보세요! 👍

Suggested change

if (!isValidRacingCount(racingCount)) {

return ($racingCountInput.value = '');

}

if (!isValidRacingCount(racingCount)) {

$racingCountInput.value = '';

return

}

ysm-dev · 2021-02-10T05:19:56Z

src/js/input/handleCarNameInput.js

+const isValidCarName = (carNames) => {
+  if (!carNames.every((carName) => isValidLength(carName))) {
+    alert(ERR_MESSAGE.NAME_TOO_LONG);
+    return false;
+  }
+  if (!carNames.every((carName) => isNotBlank(carName))) {
+    alert(ERR_MESSAGE.NAME_BLANK);
+    return false;
+  }
+  return true;
+};


여기도 마찬가지로 함수이름에 드러난 의도(CarName을 검증)에 비해서 함수가 그외 다른일(alert)들을 하고있는 것 같습니다.
alert 을 함수 외부로 분리하면 좋을것같아요 👍

ysm-dev · 2021-02-10T06:16:40Z

cypress/integration/racingGame.spec.js

+  const typeCarNameAndSubmit = (carNames = defaultCarNames) => {
+    cy.get('#car-name-input').type(carNames.join(','));
+    cy.get('#car-name-submit').click();
+    cy.get('#car-name-submit').click();
+    return cy.get('#car-name-submit').click();
+  };


몇줄이 중복되어 불필요한 라인이 들어간것같네요!

ysm-dev · 2021-02-10T06:33:09Z

cypress/integration/racingGame.spec.js

+    cy.get('.car-player').each(($div, index) =>
+      cy.get($div).should('have.text', defaultCarNames[index]),
+    );


ysm-dev · 2021-02-10T06:36:28Z

cypress/integration/racingGame.spec.js

+    }
+  });
+
+  it('전진횟수를 결정하는 함수가 "[1, 3, 3, 7]"을 입력받았을 때 "1"을 반환하는지 확인하는 테스트한다.', () => {


테스트 설명이 잘못된 것 같습니다!
전진을 결정하는 함수가 3이하의 값을 받았을 때 false를 반환하고, 4이상의 값을 받았을때 true를 반환하는지 확인한다
가 아래 코드를 더 잘 설명할 것 같습니다.

ysm-dev · 2021-02-10T06:44:00Z

cypress/integration/racingGame.spec.js

+    cy.get('.car').then(($cars) => {
+      const counts = [...$cars].map(($car) => {
+        return $car.querySelectorAll('.forward-icon').length;
+      });
+      const maxScore = Math.max(...counts);
+      const winners = [];
+
+      counts.forEach((carCount, index) => {
+        if (carCount === maxScore) {
+          winners.push(defaultCarNames[index]);
+        }
+      });
+      cy.get('#game-result-text').should(
+        'have.text',
+        `🏆 최종 우승자: ${winners.join(', ')} 🏆`,
+      );
+    });


utils/getWinner 함수에서 하는일과 거의 동일한 로직으로 보이는데요.
이렇게 똑같은 로직을 다시 작성하기보다는
getWinner 함수를 리팩토링하여 DOM에 직접 접근하는 부분과 maxScore인 car들을 뽑아내는 로직을 분리하고,
위 isEffectiveScore 함수를 테스트하신 것처럼, getWinner 함수를 여기서 사용하여 테스트하면 같은 로직을 두번 작성할 필요가 없어질 것 같아요. 👍

ysm-dev · 2021-02-10T07:16:04Z

src/js/utils/toggleVisibility.js

+export const toggleVisibility = (target) => {
+  const sectionList = {
+    $racingCountSection,
+    $gameProcessSection,
+    $gameResultSection,
+  };
+
+  sectionList[target].toggleAttribute('hidden');
+};


일반적으로 List라는 이름은 배열 자료구조를 담을때 많이 쓰입니다. 현재는 object 형태의 자료구조를 List라는 이름의 변수에 담고있네요.
또한 target 파라미터가 $racingCountSection, $gameProcessSection, $gameResultSection 셋중 하나가 아닐 경우에 대한 예외처리도 되어있지 않은 것같은데요.

위에서 말씀드린대로 setVisibility라는 이름의 다른 형태의 함수로 리팩토링해보시는걸 추천드립니다! 👍
(dom 과 Visibility(boolean) 값을 받아서 해당 값을 적용하는 형태)

365kim · 2021-02-10T19:40:52Z

ysm-dev · 2021-02-11T04:07:54Z

src/js/util-view/setVisibility.js

+export const setVisibility = ($target, isToBeVisible) => {
+  if (isToBeVisible) {
+    $target.hidden = false;
+  } else {
+    $target.hidden = true;
+  }
+};


Suggested change

export const setVisibility = ($target, isToBeVisible) => {

if (isToBeVisible) {

$target.hidden = false;

} else {

$target.hidden = true;

}

};

export const setVisibility = ($target, visibility) => {

$target.hidden = !visibility;

};

또는

Suggested change

export const setVisibility = ($target, isToBeVisible) => {

if (isToBeVisible) {

$target.hidden = false;

} else {

$target.hidden = true;

}

};

export const setHidden = ($target, hidden) => {

$target.hidden = hidden

};

이렇게 짧게 작성하는게 더 직관적이겠죠?

헛 한 줄로 더 가독성 좋게 작성할 수 있었군요..!! 감사합니다!! 👍👍👍

ysm-dev

LGTM 👍 첫번째 미션 피드백까지 잘 수행해주셨습니다.
벌써 많이 나아진게 보이네요!

코드를 작성하는 시간을 줄이고, 다른 수강생은 어떻게 작성했는지, 왜그렇게 작성했는지, 다른 리뷰어분들께 어떤 피드백을 받았는지, 좋은 코드를 더 많이 보고, 왜 그게 더 좋은 코드인지 고민하는데에 시간을 많이 투자하시면 좋을 것 같아요. 👍

365kim · 2021-02-11T06:00:35Z

@ysm0622 우와 첫 머지 감격스럽네요...!! 말씀해주신대로 무작정 코드를 작성하기보다 좋은 코드와 리뷰를 보고 앞으로 더 많이 흡수하겠습니다! 도움이되는 조언 감사합니다 :) 새해복 많이 받으세요 🥰🥰🥰

365kim and others added 30 commits February 5, 2021 15:21

chore: 초기 환경설정

a0bff84

chore: cypress 초기설정

c2fc85c

docs: 구현기능 목록 작성

14e21a6

docs: 테스트 목록 작성

4f7f462

test: 자동차 이름 화면 정상출력 테스트 코드 작성

39a3c42

Merge branch '365kim' of https://github.com/365kim/javascript-racingcar…

b76cd78

… into 365kim

feat: HTML 요소 id 부여

1033611

chore: eslint config 수정

3929d67

feat: 자동차 이름을 입력받는 기능

c7b916b

docs: 올바르지 않은 자동차이름 입력시 입력창 초기화 기능 추가

84ad1dc

test: 자동차 이름이 올바르지 않을 경우의 테스트 코드 작성

598a47d

test: 양의 정수가 아닌 시도횟수 입력 시 경고메세지 출력 테스트 코드 작성

708a554

docs: 올바르지 않은 시도횟수 입력시 입력창 초기화 기능 추가

70201ff

refactor: 테스트에서 자동차 이름을 입력시 화면에 표시하는 요소 변경

dcaf130

test: 시도횟수가 올바르게 입력된 경우 경주 화면이 보이는지 테스트 하는 코드 추가

31e8023

test: getRandomNumber 함수가 정상적으로 랜덤값을 생성하는지 테스트 하는 코드 작성

a88ec82

test: 경주를 마쳤을 때 우승자를 정상적으로 출력하는지 확인하는 테스트 코드 작성

01a4b71

test: 전진횟수 결정함수 및 화살표 정상표시 테스트 코드 추가

3253a48

feat: 자동차 이름 입력 후 제출 시 시도횟수 입력창 표시

b0cac3f

feat: 자동차 이름을 입력받았을 때 유효한지 검사하는 기능

bba0d5f

refactor: 자동차 이름 유효성 검사 함수 분리

b314e6b

feat: 입력받은 시도횟수 유효성 검사 기능

c24f621

feat: 자동차경주 진행화면 표시 기능

3ac062c

feat: 난수 생성 함수, 전진 횟수 체크 함수 구현

0574b0c

feat: 유효한 점수일 경우 화살표 추가 표시 기능

bac0bac

fix: 선택자 오타 및 dataset 오타 수정

b8665cb

fix: 자동차 경주 진행 중 화살표 갯수를 테스트에서 파싱하지 못 하던 오류 수정

72dcad8

feat: 최종결과 화면 표시 기능

d3e88e6

fix: 우승자를 정상적으로 얻어오지 못하는 오류 수정

bb81061

fix: cypress 메서드 버그 수정

9210ecd

365kim reopened this Feb 8, 2021

365kim changed the base branch from main to 365kim February 8, 2021 15:24

pocojang requested a review from ysm-dev February 9, 2021 05:38

ysm-dev self-assigned this Feb 10, 2021

ysm-dev suggested changes Feb 10, 2021

View reviewed changes

365kim added 17 commits February 10, 2021 22:52

docs: 1단계 피드백 반영 목록 작성

57c6228

refactor: 더 명확한 변수명으로 개선

152fe84

refactor: 함수명이 의도를 숨기지 않도록 개선

53d8cef

refactor: 숨어있던 alert 분리, 사용하지 않는 반환값 return문에서 분리

1f14e4e

refactor: 불필요한 new 연산자 삭제

6576dd1

refactor: cypress 테스트 설명 간결화, 오타 수정

2ccb976

refactor: NAME_BLANK, VALIDATOR 등 모호한 변수명을 더 명확하게 개선

401efdf

refactor: toggleVisibility 함수명 수정 & true/false 인자를 받아 의도 명시

4f01ca7

refactor: 숨어있던 alert 분리, 사용하지 않는 반환값 return문에서 분리

73a54b7

refactor: 불필요한 new 연산자 삭제

0ef5945

test: 기능코드와 중복되는 코드 제거, 순수 getWinners() 테스트 추가

0b2746c

refactor: Car 클래스 생성

de64ee0

refactor: Car 클래스를 이용하여 Model과 View 분리 적용

a4d258f

refactor: 불필요한 행 삭제, 오타 수정

31056d3

refactor: Car 클래스에 역할 위임

7b43ee2

fix: git pull 충돌 해결

4e3281f

refactor: 변수명 일부 의미 명확화 하기 위해 재수정

3eac631

365kim requested a review from ysm-dev February 11, 2021 02:44

ysm-dev reviewed Feb 11, 2021

View reviewed changes

ysm-dev approved these changes Feb 11, 2021

View reviewed changes

ysm-dev merged commit e94876b into woowacourse:365kim Feb 11, 2021

ysm-dev mentioned this pull request Feb 11, 2021

[1단계 - 자동차 경주 구현] 크리스(이송원) 미션 제출합니다. #11

Merged

365kim mentioned this pull request Jun 1, 2021

[1단계 - 지하철 노선도 미션] 하루(김하루) 미션 제출합니다. woowacourse/react-subway-map#20

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1단계 - 자동차 경주 구현] 하루(김하루) 미션 제출합니다. #6

[1단계 - 자동차 경주 구현] 하루(김하루) 미션 제출합니다. #6

365kim commented Feb 8, 2021 •

edited

Loading

ysm-dev left a comment

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

ysm-dev Feb 10, 2021

365kim commented Feb 10, 2021

ysm-dev Feb 11, 2021

365kim Feb 11, 2021

ysm-dev left a comment

365kim commented Feb 11, 2021

[1단계 - 자동차 경주 구현] 하루(김하루) 미션 제출합니다. #6

[1단계 - 자동차 경주 구현] 하루(김하루) 미션 제출합니다. #6

Conversation

365kim commented Feb 8, 2021 • edited Loading

요약

구현기능 목록

고민사항 / 미해결과제

ysm-dev 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

365kim commented Feb 10, 2021

📝 1단계 피드백 반영 목록

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysm-dev left a comment

Choose a reason for hiding this comment

365kim commented Feb 11, 2021

365kim commented Feb 8, 2021 •

edited

Loading