[1단계 - 자동차 경주 미션] 호프(김문희) 미션 제출합니다. #61

moonheekim0118 · 2022-02-11T07:06:55Z

안녕하세요 도리와 함께한 첫번째 미션 제출합니다. 온보딩 때 확실히 페어와 여러 우여곡절을 겪고나니, 1단계 미션은 오히려 많은 부분이 발전되어서 즐겁게 임할 수 있었습니다.

기능 구현 후 해당 기능에 대한 테스트 코드를 작성했습니다.

이전에 온보딩 과제에서는 코드 구현 이후 테스트 코드를 추후에 작성했는데요, 이번에는 페어와 상의 하에 기능 구현 후 해당 기능에 대한 테스트코드를 바로 작성해서 테스트하는 방식으로 진행했습니다.
그러다보니 예상치 못했던 예외사항들도 즉각 발견할 수 있었고, 구현 속도도 빨라져서 매우 효율적이었습니다.
하지만 테스트코드를 거의 처음 작성하다보니, 좋은 테스트코드인지는 아직 잘 모르겠습니다

(고민) 모든 이벤트 (이름 입력, 레이싱 횟수 입력, 다시 시작하기 버튼 클릭)을 한번에 #app 엘리먼트에서 받도록 구현하기 위해서 이름 입력, 레이싱 횟수 입력을 form submit 이벤트가 아니라 button click 이벤트로 구현하게 되었는데요, 아예 이벤트를 여러군데에서 받더라도 각각 form으로 수정한 후 submit 이벤트를받는게 좋을지 고민입니다. 클릭 이벤트로 구현하다 보니 Enter 키를 누르는 경우도 또 따로 이벤트를 바인딩 시켜서 처리해줘야했습니다.

감사합니다 :)

- prettier, eslint 설정 - cypress 초기 설정

youngbeomrhee

안녕하세요 호프님 리뷰를 맡은 YB입니다
먼저 구현하느라 고생하셨습니다
Form에 대해 고민하신 부분에 대해서는 작성하신 코드에 코멘트로 조언을 남겼습니다
요약하자면 불필요한 Form내의 요소들의 작동에 의해 의도하지 않은 Submit이 발생하지 않도록 제어하고 (예: button의 default type이 submit이므로 type을 button으로 명시)의도한 경우에만 Submit이 발생하도록 구현하시면 좋을 것 같습니다
form과 관련된 내용은 나의 첫 HTML 폼을 참고해 보시면 좋을 것 같습니다
테스트 케이스에 대한 보강과 addEventListener 리팩토링, 이름 입력 후 "입력" 버튼 클릭시 미작동 되는 오류에 대한 수정요청 드립니다
아무래도 텍스트의 한계가 있어서 의미가 충분히 전달되지 않거나 잘못 전달될 수 있습니다
코멘트 드린 부분에 대해서 이해 안되시는 부분이나 다른 생각이 있으시면 언제든 편하게 말씀주세요

youngbeomrhee · 2022-02-12T06:02:32Z

.eslintrc.json

+    "array-callback-return":"off",
+
+}
+}


lint와 prettier는 실무에서 협업할 때 꼭 필요한 도구인데 잘 적용해 주셨네요

youngbeomrhee · 2022-02-12T06:21:46Z

package.json

+    "doc": "docs"
+  },
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1",


사용하지 않는 명령어는 깔끔하게 정리하시는 것을 권해 드립니다

수정했습니다 !

youngbeomrhee · 2022-02-12T06:37:16Z

index.html

+        </div>
+    </section>
+    <section id="racing-status" class="racing-status">
+      <!--


사용하지 않게 된 코드가 쌓이면 추후에 레파지토리가 많이 지저분해지게 됩니다
기존 로직을 참조해야 되는 상황이라면 git에서 변경사항을 확인할 수 있으므로 삭제하시는 것을 권해 드립니다

수정했습니다 !

youngbeomrhee · 2022-02-12T06:38:34Z

src/constants.js

@@ -0,0 +1,27 @@
+const ID = {


전역으로 관리할 영역을 잘 분리하신 것 같습니다

youngbeomrhee · 2022-02-12T07:01:09Z

cypress/integration/1-getting-started/racingcar.spec.js

+  });
+
+  it('레이싱 횟수를 입력받는다', () => {
+    inputRacingCount(2);


너무 작은 값만 테스트하고 있는 것 같습니다
충분히 큰 값으로도 테스트해서 사용성에 문제가 없는지 테스트 해본다면 보다 좋은 사용자 경험을 제공할 수 있을 것 같습니다

감사합니다! 이부분 반영해서, 최댓값 지정하여 테스트하도록 변경했습니다 !

youngbeomrhee · 2022-02-12T08:28:55Z

src/view.js

@@ -0,0 +1,16 @@
+const userStatusView = ({ name, racingCount }) => {
+  return `<div id="user-status" class="user-status" data-name=${name}>


바로 리턴하는 경우라 return 문 제거해도 될 것 같습니다

넵 수정했습니다 :)

youngbeomrhee · 2022-02-12T15:23:32Z

cypress/integration/1-getting-started/racingcar.spec.js

+    inputRacingCount(0);
+    checkAlertMessage(MESSAGE.WRONG_COUNT);
+  });
+});


모든 기능 요구사항을 테스트하기는 힘들겠지만 입력값만을 테스트 하신 것 같아 조금 아쉽습니다
기능요구사항 관련된 많은 테스트 케이스를 작성하다보면 분명 예상하지 못했던 기능상의 오류를 발견할 수 있을 것 같습니다
테스트 케이스를 조금 더 보강해 보시면 좋을 것 같습니다

youngbeomrhee · 2022-02-12T15:33:29Z

src/index.js

+
+  bindEvents() {
+    getElement(ID.INPUT_FORMS).addEventListener('keyup', event => {
+      if (event.key !== KEY.ENTER) {


Form 전체에 keyup 이벤트를 거는건 예상하지 못한 동작을 발생시킬 수 있을 것 같습니다
input 별로 event listener를 세분화하고 의도치 않은 submit이 발생되지 않도록 조정이 필요해 보입니다

이 부분 반영해서 각 input 에 keyup 이벤트를 걸도록 수정했습니다 :)

youngbeomrhee · 2022-02-12T15:37:54Z

src/index.js

+    });
+
+    getElement(ID.APP).addEventListener('click', ({ target }) => {
+      const buttonIds = {


App 전체에 Click event listener를 거는건 불필요하게 많은 이벤트(무의미한 화면 클릭 등)를 listen하게 되어 권장하기 어렵습니다
버튼 별로 event를 분리하는게 목적이라면 selector를 해당 영역의 button 으로 한정하기만 해도 불필요한 리소스를 줄일 수 있을 것 같습니다

나름대로 이벤트 위임을 시키고자 App 전체에 이벤트를 위임시키고자 했었습니다. 하지만 말씀해주신대로 불필요한 이벤트 역시 받아온다고 판단되어 이벤트를 모두 분리해주었습니다 !! :)

youngbeomrhee · 2022-02-12T15:39:15Z

src/index.js

+    if (!validateRacingCount(count)) {
+      return alert(MESSAGE.WRONG_COUNT);
+    }
+    moveCars(this.cars, count);


아래 3줄은 의미상 새로운 함수로 분리하면 좀 더 의미가 명확해질 것 같습니다

아래와 같이 수정했습니다 :)
추가된 resetCars 함수는 사용자가 레이싱 횟수를 재입력할 시, 기존에 남아있던 레이싱횟수를 0으로 초기화하는 함수로, 새로 추가했습니다!

startGame(count) { resetCars(this.cars); moveCars(this.cars, count); getElement(ID.RACING_STATUS).innerHTML = this.cars .map(car => userStatusView(car)) .join(''); getElement(ID.RACING_WINNERS).innerHTML = winnersView(this.getWinner()); }

- div element -> form element 로 수정 - form 과 버튼 별로 각각 submit, click 이벤트를 받도록 수정

- 이름 재입력시, 자동차 리스트가 업데이트 되도록 수정 - 레이싱횟수 재입력시 입력된 횟수만큼 다시 게임을 진행하도록 수정

- innerHTML -> insertAdjacentHTML 로 수정 - clearHTML 함수 구현 - resetInputValue 함수 구현

- 두개의 분리된 폼-> 하나의 폼으로 수정 - 각 input Element에 keyup 이벤트 추가하여 Enter 처리하도록 구현 - 각 button Element 에 click 이벤트 추가

- Enter 키 입력에 대한 테스트 추가 - 불필요한 util 제거

moonheekim0118 · 2022-02-13T16:24:25Z

안녕하세요 YB님! 좋은 리뷰 정말 감사드립니다. 리뷰 최대한 반영하여 리뷰 재요청 드립니다. form의 경우 처음에는 두개의 form으로 각각 분리해서 submit 이벤트를 따로 받았었지만 결국에는 하나의 form으로 수정하고 버튼에 따라 click 이벤트, input에 따라 keyup이벤트를 바인딩하도록 수정했습니다. 말씀주신 기능 에러 역시 수정했고, 테스트코드도 최대한 보강해보았습니다! 부족한 부분이 있다면 더 알려주시면 감사하겠습니다 :)

youngbeomrhee

호프님 안녕하세요 구현하느라 고생하셨습니다
피드백 드린 부분에 대해 너무나 잘 반영해 주신 것 같습니다
특히나 테스트 케이스를 많이 보강해주셔서 한결 완성도가 올라간 것 같습니다
바로 Approve 해드리고 싶었는데 아쉽게도 필수요구사항 중에 random 값이 0부터 9가 아니라 8까지 나오고 있어서 이 부분 하나만 수정해 주시면 바로 Approve 하도록 하겠습니다
(이 부분은 코멘트를 입력했는데 오류가 있는지 저장이 안되서 번거롭게 해드렸네요 양해 부탁 드립니다)

youngbeomrhee · 2022-02-12T16:22:03Z

src/utils/index.js

+};
+
+const generateRandomNumber = () => {
+  return Math.floor(Math.random() * 9);


기능 요구사항은 0 부터 9까지의 무작위 값이 조건인데 이렇게 구현할 경우 0 부터 8까지의 값이 나오게 됩니다
이렇게 공통으로 사용되는 코드는 특히 더 많이 테스트 하시는게 좋습니다

지금과 같은 경우라면 반복문을 활용하여 충분히 큰 횟수로 테스트 했을 때 값이 적절히 분포되어 있는지 확인해서 테스트 해볼 수 있을 것 같습니다
이 테스트는 필수는 아니고 선택적으로 해보시면 될 것 같습니다

엇..구현할 때 놓쳤던 부분이네요. 알려주셔서 감사합니다! 수정했습니다. 말씀해주신 테스트는 현재 E2E테스트보다 유닛테스트에 더 가까운 것 같은데, cypress로 테스트 해도 괜찮을까요 ? 괜찮다면 다음 스텝에서 구현해보고싶습니다!

이 부분은 E2E 테스트로 하기에 적합한 대상이 아닐 것 같고요
그냥 VanillaJS나 Mocha, Chai 등을 사용해서 Unit 테스트 해보시면 좋을 것 같습니다

youngbeomrhee · 2022-02-14T09:57:04Z

src/utils/dom.js

+
+const setHTML = (element, html) => {
+  clearHTML(element);
+  element.insertAdjacentHTML('afterbegin', html);


단순히 innerHTML을 사용하지 않고 용도에 맞게 insertAdjacentHTML을 사용하셨네요

youngbeomrhee · 2022-02-14T09:57:40Z

src/utils/index.js

+const validateRacingCount = count =>
+  count >= RACING_COUNT.MIN && count <= RACING_COUNT.MAX;
+
+const generateRandomNumber = () => Math.floor(Math.random() * 9);


이렇게 구현할 경우에 랜덤값은 0부터 8까지만 나오게 됩니다
0부터 9까지의 랜덤값을 가질 수 있는게 필수요구사항인만큼 수정 요청 드립니다

youngbeomrhee · 2022-02-14T13:36:59Z

src/utils/racingGame.js

@@ -0,0 +1,27 @@
+const parseCarName = names => names.split(',').map(name => name.trim());
+
+const generateRandomNumber = () => Math.floor(Math.random() * 9);


이 부분은 util과 중복되네요 하나를 정리하시는게 좋을 것 같습니다

제가 파일구조를 변경해서 중복으로 보이는 것 같습니다 ! 현재는 하나로 정리했습니다 :)

youngbeomrhee · 2022-02-14T13:46:47Z

cypress/integration/racingcar.spec.js

@@ -0,0 +1,126 @@
+import { MESSAGE, ID, RACING_COUNT } from '../../src/constants.js';


테스트 케이스가 많이 보강됐네요 고생하셨습니다
다만 기능요구사항과 작성해주신 테스트에 대한 설명이 달라서 어떤 부분이 테스트 된건지 한 눈에 안들어오네요
step2에서 테스트 케이스를 작성하실 때는 기능요구사항을 대제목으로 만들어 주시면 커뮤니케이션 비용을 아낄 수 있을 것 같습니다 :)

테스트는 바꿔놓고 페어와 함께 작성해놓은 기능 구현 사항을 수정하지 않았네요 ㅠ ㅠ 죄송합니다. 다음번에는 꼭 챙기겠습니다 !

moonheekim0118 · 2022-02-14T14:19:37Z

늦은시간까지 꼼꼼한 리뷰 감사합니다 YB님 :) 말씀하신 사항 수정했습니다. 다음번에는 꼭 테스트와 기능 목록을 맞추겠습니다 😢 😢 😢 혹시 또 수정할 사항이 있으면 말씀해주시면 감사하겠습니다 !!

youngbeomrhee

호프님 안녕하세요 수정해주신 random 함수 확인했습니다
늦은 시간까지 구현하느라 고생하셨습니다
Step2에서 뵙겠습니다 :)

youngbeomrhee · 2022-02-14T14:29:36Z

src/utils/racingGame.js

@@ -0,0 +1,28 @@
+const parseCarName = names => names.split(',').map(name => name.trim());
+
+const generateRandomNumber = (min, max) =>


좀 더 범용적으로 잘 변환하셨네요 기능도 문제 없는 것 확인했습니다 :)

moonheekim0118 added 22 commits February 10, 2022 14:44

Co-authored-by: SunHo Park <prefer2@users.noreply.github.com>

82411f2

docs: 기능 목록 작성

0223d17

chore: 프로젝트 초기 설정

d19304c

- prettier, eslint 설정 - cypress 초기 설정

docs: 기능 목록 수정

25ac205

feat: UI 마크업

c2125de

feat: n대의 자동차 이름 입력 구현

c00c42c

docs: 기능목록 수정

0480292

docs: 기능 목록 추가

fea2fa2

feat: 자동차 객체 생성

a5190a9

feat: 레이싱 횟수 입력 구현

7e649ce

feat:자동차 전진 횟수 결정

f658656

feat: 레이싱 게임 우승자 출력 구현

33c1d78

docs: 기능 목록 추가

697e04b

feat: 다시 시작하기 구현

c99adaa

docs: 예외 사항 추가

cd21981

feat: 중복된 이름 입력 불가능 구현

9730f56

feat: 이름 입력 후 재입력 불가능 구현

1281f2e

feat: 레이싱 횟수 입력 후 재입력 불가능 구현

db63d0e

feat:ENTER시 입력 가능 구현

d35069d

refactor: 상수 분리

8ed1030

refactor: 함수 분리

5f369f6

chore: gitignore 수정

8ae3727

pocojang requested a review from youngbeomrhee February 11, 2022 07:23

pocojang assigned youngbeomrhee Feb 11, 2022

youngbeomrhee suggested changes Feb 12, 2022

View reviewed changes

moonheekim0118 added 5 commits February 13, 2022 16:51

chore: 불필요한 명령어 제거

baf2e08

chore: 주석 제거

b2672e1

refactor: 불필요한 return 제거

0871483

refactor: 이벤트 바인딩 분리

4ddfba1

- div element -> form element 로 수정 - form 과 버튼 별로 각각 submit, click 이벤트를 받도록 수정

fix: 이름 입력 에러 수정

daa01bb

- 이름 재입력시, 자동차 리스트가 업데이트 되도록 수정 - 레이싱횟수 재입력시 입력된 횟수만큼 다시 게임을 진행하도록 수정

moonheekim0118 added 10 commits February 13, 2022 18:50

refactor: dom관련 함수 유틸화

774f74a

- innerHTML -> insertAdjacentHTML 로 수정 - clearHTML 함수 구현 - resetInputValue 함수 구현

refactor: form 수정

24d750c

- 두개의 분리된 폼-> 하나의 폼으로 수정 - 각 input Element에 keyup 이벤트 추가하여 Enter 처리하도록 구현 - 각 button Element 에 click 이벤트 추가

refactor: 불필요한 상수 제거

f2ce27d

feat: 레이싱 최대횟수 지정

7e264a0

chore: gitignore 수정

f977fbe

test: 테스트코드 보강

b99ff4d

refactor: 최소 최댓값 상수화

556581f

docs: 기능 목록 수정

0c590f1

chore: selector 추가

b79fc3d

test: 테스트 코드 수정

3519044

- Enter 키 입력에 대한 테스트 추가 - 불필요한 util 제거

moonheekim0118 added 7 commits February 14, 2022 16:28

refactor: 불필요한 상수 제거

44edd42

feat: 소수 입력 처리 추가

b73027d

refactor: validator utils 분리

2a281e1

refactor: 파일명 변경 및 상수화

549a5f3

refactor: 일반함수를 cypress command로 수정

dbbf13c

refactor: alert Message와 상수값 연결

bdfa661

refactor: 불필요한 import 제거

0856e16

youngbeomrhee suggested changes Feb 14, 2022

View reviewed changes

moonheekim0118 added 2 commits February 14, 2022 23:09

fix: 랜덤 함수 수정

3f2a024

refactor: 랜덤 함수 수정

399f5c9

youngbeomrhee approved these changes Feb 14, 2022

View reviewed changes

youngbeomrhee merged commit f19b86d into woowacourse:moonheekim0118 Feb 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1단계 - 자동차 경주 미션] 호프(김문희) 미션 제출합니다. #61

[1단계 - 자동차 경주 미션] 호프(김문희) 미션 제출합니다. #61

moonheekim0118 commented Feb 11, 2022 •

edited

Loading

youngbeomrhee left a comment

youngbeomrhee Feb 12, 2022

youngbeomrhee Feb 12, 2022

moonheekim0118 Feb 13, 2022

youngbeomrhee Feb 12, 2022

moonheekim0118 Feb 13, 2022

youngbeomrhee Feb 12, 2022

youngbeomrhee Feb 12, 2022

moonheekim0118 Feb 13, 2022

youngbeomrhee Feb 12, 2022

moonheekim0118 Feb 13, 2022

youngbeomrhee Feb 12, 2022 •

edited

Loading

youngbeomrhee Feb 12, 2022

moonheekim0118 Feb 13, 2022

youngbeomrhee Feb 12, 2022

moonheekim0118 Feb 13, 2022

youngbeomrhee Feb 12, 2022

moonheekim0118 Feb 13, 2022

moonheekim0118 commented Feb 13, 2022 •

edited

Loading

youngbeomrhee left a comment •

edited

Loading

youngbeomrhee Feb 12, 2022

youngbeomrhee Feb 14, 2022 •

edited

Loading

moonheekim0118 Feb 14, 2022

youngbeomrhee Feb 14, 2022

youngbeomrhee Feb 14, 2022

youngbeomrhee Feb 14, 2022

youngbeomrhee Feb 14, 2022 •

edited

Loading

moonheekim0118 Feb 14, 2022

youngbeomrhee Feb 14, 2022

moonheekim0118 Feb 14, 2022

moonheekim0118 commented Feb 14, 2022

youngbeomrhee left a comment

youngbeomrhee Feb 14, 2022

		@@ -0,0 +1,16 @@
		const userStatusView = ({ name, racingCount }) => {
		return `<div id="user-status" class="user-status" data-name=${name}>

		@@ -0,0 +1,27 @@
		const parseCarName = names => names.split(',').map(name => name.trim());

		const generateRandomNumber = () => Math.floor(Math.random() * 9);

		@@ -0,0 +1,126 @@
		import { MESSAGE, ID, RACING_COUNT } from '../../src/constants.js';

		@@ -0,0 +1,28 @@
		const parseCarName = names => names.split(',').map(name => name.trim());

		const generateRandomNumber = (min, max) =>

[1단계 - 자동차 경주 미션] 호프(김문희) 미션 제출합니다. #61

[1단계 - 자동차 경주 미션] 호프(김문희) 미션 제출합니다. #61

Conversation

moonheekim0118 commented Feb 11, 2022 • edited Loading

youngbeomrhee 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

youngbeomrhee Feb 12, 2022 • 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

moonheekim0118 commented Feb 13, 2022 • edited Loading

youngbeomrhee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngbeomrhee Feb 14, 2022 • 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

youngbeomrhee Feb 14, 2022 • 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

moonheekim0118 commented Feb 14, 2022

youngbeomrhee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moonheekim0118 commented Feb 11, 2022 •

edited

Loading

youngbeomrhee Feb 12, 2022 •

edited

Loading

moonheekim0118 commented Feb 13, 2022 •

edited

Loading

youngbeomrhee left a comment •

edited

Loading

youngbeomrhee Feb 14, 2022 •

edited

Loading

youngbeomrhee Feb 14, 2022 •

edited

Loading