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

moonheekim0118 · 2022-02-17T09:39:18Z

데모페이지

안녕하세요 ! YB님 ~ 이전 코드리뷰에서 많은걸 얻어갔습니다 감사합니다. 이번 step2 진행하면서 기존의 코드도 많이 리팩토링해보았습니다! 변수명이나 함수명을 유의미하게 수정하려고 많이 노력했는데,,어떨지 모르겠네요!! 그럼 이번에도 잘부탁드립니다 😃

파일 구조

📦
├─ .eslintrc.json
├─ .gitignore
├─ .prettierrc
├─ LICENSE
├─ README.md
├─ cypress.json
├─ cypress
│  ├─ integration
│  │  └─ racingcar.spec.js
│  └─ support
│     ├─ commands.js
│     └─ index.js
├─ docs
│  └─ README.md
├─ index.html
├─ package-lock.json
├─ package.json
├─ src
│  ├─ class
│  │  ├─ Car.js
│  │  ├─ RacingCarGame.js
│  │  └─ View.js
│  ├─ constants.js
│  ├─ index.js
│  └─ utils
│     ├─ dom.js
│     ├─ general.js
│     ├─ racingGame.js
│     └─ validations.js
└─ styles.css

Step 2 필수 구현 목록

자동차 경주 게임의 턴이 진행 될 때마다 1초의 텀(progressive 재생)을 두고 진행한다.
1초의 텀동안 로딩 애니메이션을 보여준다.
정상적으로 게임의 턴이 다 동작된 후에는 결과를 보여주고, 2초 후에 축하 메시지를 확인 할 수 있다.

추가된 기능들

자동차 이름이 입력되기 전 까지는, 레이싱 횟수 입력 폼을 볼 수 없다.
자동차 이름 입력 폼과, 레이싱 횟수 입력 폼 모두 입력 완료되면 disable 상태로 변경된다.
경주가 완료된 경우에만 다시 시작하기 버튼을 볼 수 있다.

리팩토링

1단계 코드에서는 RacingCarGame 클래스에서 dom 조작 및 view 렌더링을 모두 담당했는데요, 너무 복잡해지기도 하고, 역할을 확실히 분리하고자 View라는 클래스를 만들어서, RacingCarGame 클래스에서는 View 클래스의 객체를 멤버변수로 지정해주었습니다.
테스트코드는 given, when, then 을 구분하여 재작성했습니다.
클래스 멤버변수들을 private 변수로 수정했습니다. 외부에서 직접적으로 접근하면 안된다고 판단되었기 때문입니다.

궁금한 점

현재 따로 디자인 패턴을 채택하지 않고 필요 시에 나누고 있는데요, 현재 RacingCarGame 클래스나 View 클래스에서 더 나눌 부분이 있을지 조언을 얻고싶습니다.
Step2 기능 사항에 대한 테스트코드는 정상적으로 게임의 턴이 다 동작된 후에는 결과를 보여주고, 2초 후에 축하 메시지를 확인 할 수 있다. 만 구현한 상태입니다. 1초의 텀을 두고 진행상황을 출력하는 기능에 대한 테스트는 위의 최종 우승자 출력에서 이미 검증되었다고 생각했기 때문입니다. Step2 기능 사항에 대한 테스트코드는 이정도면 충분할지 궁금합니다.
step1에서는 Car 클래스의 Move 메서드의 역할이, '랜덤함수를 실행한 후, 전진여부를 결정하고 전진한다'였는데요, 이번에는 전진여부는 외부 함수에서 결정한 후에 Move 메서드는 전진만 하도록 구현해보았습니다. 그런데 이게 올바른 방법인지 궁금합니다. 아예 전진 여부 결정도 클래스 내부 메서드에서 해주는게 맞을까요? 아니면 이렇게 외부로 분리해도 괜찮을까요?

질문이 좀 많네요..ㅠㅠ,,늘 감사합니다 : )

- dom 조작 기능은 View 클래스에 위임

- getElementById 대신 querySelector, querySelectorAll 사용 - ID 대신 SELECTOR 상수화

- 기존의 Move 메서드에서 t/f 반환 제거 - 해당 함수에서 직접 확인하도록 수정

- 자동차 이름 입력 후 자동차 이름 폼 비활성화 - 레이싱 횟수 입력 후 레이싱 횟수 폼 비활성화

…script-racingcar into step2-moonheekim0118

- getInputValue 에서 element로 받은 input value의 값을 바로 리턴하도록 수정 - 쿼리 셀렉터에 target 값으로 APP 추가

youngbeomrhee

호프님 안녕하세요 피드백 드린 내용을 충실히 잘 구현해 주셨네요 구현하느라 고생하셨습니다
추가적으로 더 나은 코드를 위해 많은 고민을 해주셨네요
아직 미션일정 여유가 있고 고민하시는 부분이 있으니 피드백과 함께 수정요청 드립니다

질문주신 5가지 사항에 대해서 개인적인 일정상 먼저 3가지에 대해서 답변 드립니다 나머지는 다음 피드백 때 마저 답변 드리도록 할께요 양해 부탁드려요

질문에 대한 답변

저는 도메인 단위로 분리하는걸 선호하는 편인데요
이 프로그램은 크게 봤을 때 입력, 경주, 우승자 표시 이렇게 3개의 도메인으로 구성되어 있다고 봅니다
그런 관점에서 봤을 때 RacingCarGame이 다루고 있는 도메인은 입력 form과 경주화면 2가지를 모두 담고 있는데 이 2개는 분리가 가능할 것 같습니다
그리고 View는 화면단의 모든 영역을 핸들링 하고 있는데 이 부분도 도메인 단위로 분리가 가능한 영역으로 보입니다
테스트코드를 상세히 잘 구현해 주셨지만 "1초의 텀을 두고 진행상황을 출력하는 기능에 대한 테스트"는 누락된 것 같습니다
하지만 사실 이 부분은 테스트하기 굉장히 까다로운 부분이라고 생각하는데요
이유는 Spinner가 무조건 표시되거나 안되거나 할 수 있다면 테스트 할 수 있지만 랜덤으로 표시되기 때문에 "게임시작 전에 spinner가 표시되지 않았다"만 정확하게 테스트 가능하기 때문입니다
물론 레이서와 게임횟수가 충분히 늘어난다면 각 단계별로 최소 1개 이상의 Spinner가 존재하고 게임이 끝났을 때도 최소 1개 이상의 Spinner가 존재한다고 할 수 있지만 100% 확실한건 아니라서 과연 테스트 대상으로서의 의미가 있는가에 대해서는 의문입니다
만약 게임시작이라는 상태와 횟수별 레이싱의 시작을 상태로 관리한다면 해당 상태가 바뀌었는지의 여부를 테스트 하는 정도면 충분할 것 같습니다
하나의 메서드가 하나의 기능만 한다는 관점에서 이번에 작업하신 "전진여부는 외부 함수에서 결정한 후에 Move 메서드는 전진만 하도록" 분리하신게 더 나은 방식이라고 봅니다

youngbeomrhee · 2022-02-18T13:45:17Z

.eslintrc.json

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


lint까지 세분화 하셨군요 박수를 보냅니다 신입때의 제 모습과 너무 대조되서 반성하게 되네요

youngbeomrhee · 2022-02-18T13:58:25Z

cypress/support/commands.js

@@ -9,32 +9,33 @@ Cypress.Commands.add('checkAlertMessage', expectedMessage => {

 // CarName 을 입력하는 커맨드
 Cypress.Commands.add('inputCarNames', names => {
-  cy.get(`#${ID.CAR_NAMES_INPUT}`).type(names);
+  cy.get(SELECTOR.CAR_NAMES_INPUT).type(names);


#를 변수에 포함시켜서 기존 로직보다 명료한 것 같습니다

youngbeomrhee · 2022-02-18T13:59:46Z

docs/README.md


-## [✅] UI 마크업
- figma 시안을 기준으로 구현한다.
+## [✅] 자동차 이름 입력


요구사항에 구현한 기능을 나열하라고 했는데 제가 놓치고 피드백을 드리지 못했던 부분인데 셀프로 잘 반영해 주셨네요

youngbeomrhee · 2022-02-18T14:19:40Z

src/class/Car.js

@@ -0,0 +1,20 @@
+export default class Car {
+  #racingCount = 0;


접근제한과 캡슐화에 대해서도 고민하셨군요 👍

youngbeomrhee · 2022-02-18T14:43:20Z

cypress/integration/racingcar.spec.js

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


테스트 케이스를 상당히 잘게 쪼개서 다양한 경우에 대해서 테스트 해주셨군요 👍

youngbeomrhee · 2022-02-18T14:48:39Z

cypress/integration/racingcar.spec.js

+    cy.tick(RACING_COUNT.MAX * milliseconds + delayTime);
+    cy.tick(END_MESSAGE_DELAY);
+    // then
+    cy.checkAlertMessage(MESSAGE.GAME_END);


이렇게만 테스트하면 2초 후에 축하 메시지가 출력됐다는 사실은 테스트 되지만 그 이전에 해당 메시지가 출력되지 않았다는 사실은 테스트 되지 않을 것 같네요
보다 정확히 테스트 하기위해 2초 전에는 축하 메시지가 출력되지 않았다는 점도 추가적으로 테스트되면 좋을 것 같습니다

생각지도 못했던 부분입니다..ㅎㅎ..맞네요. 2초가 지난 후에 테스트하는건 2초 전에 alert이 떴는지 아닌지를 검증못해주네요..감사합니다 테스트코드 추가했습니다 :)

youngbeomrhee · 2022-02-19T03:03:07Z

cypress/integration/racingcar.spec.js

-    cy.submitRacingCount(RACING_COUNT.MIN).then(() => {
-      cy.get(`#${ID.WINNERS}`).should('have.text', winners);
+  it('자동차 이름은 쉼표(,)를 기준으로 구분한다.', () => {
+    // given


given, when, then으로 의미적인 분리를 하니 보다 구조가 명확해 졌네요!

youngbeomrhee · 2022-02-19T03:06:31Z

index.html

    <h1>🏎️ 자동차경주 게임🏁</h1>
-    <form id="racing-game-form">
-      <div class="input-section">
+    <form id="racing-game-form" class="racing-game-form">


id와 class의 이름을 동일하게 주신건 안티패턴일 것 같습니다
id는 전체 document에서 유일하게 1개만 존재해야 되므로 보다 구체적인 이름을, class는 복수로 존재하는걸 가정하기 때문에 보다 일반적인 이름을 붙이는게 일반적입니다
네이밍에 대해서 조금 더 고민해 보시면 좋을 것 같습니다

단순히 같은 엘리먼트를 가리키니까, CSS와 JS에서 가리키는 엘리먼트가 모두 같은 이름이면 개발자가 인식하기 편할 것 이라고 생각했는데, 생각이 짧았네요..이 부분 더 고민해서 고쳐보도록 하겠습니다. :)

youngbeomrhee · 2022-02-19T03:07:07Z

src/class/RacingCarGame.js

@@ -0,0 +1,116 @@
+import Car from './Car.js';


전체 코멘트 참고해서 분리해 보시면 좋을 것 같습니다

youngbeomrhee · 2022-02-19T03:07:37Z

src/class/View.js

@@ -0,0 +1,132 @@
+import {


뷰 영역도 전체 코멘트 참고해서 개선해 보시면 좋을 것 같습니다

moonheekim0118 · 2022-02-19T12:07:43Z

안녕하세요 YB님 남겨주신 리뷰와 질문에 대한 답변 정말 감사드립니다 🙇 많은 도움이 되었습니다. 그리고, 처음에 너무 많고 모호한 질문을 드린 것 같아서 죄송합니다... 나머지 두 질문은 저 스스로 답을 찾는게 맞는 것 같아서 삭제했습니다.. 번거롭게 해드려 죄송합니다. 😢 YB님의 리뷰로 리팩토링한 결과는 아래와 같습니다.

테스트코드 추가 작성

"1초의 텀을 두고 진행상황을 출력하는 기능에 대한 테스트"를 추가했습니다.
위에서 '스피너가 확실히 화면에 그려질지 아닐지 확실하지 않다'라고 하셨는데요, 제가 상세된 기능을 이해한 바로는 모든 자동차는 전진 전에 스피너가 렌더링 되어야 한다고 이해했습니다. 그래서 이 부분에 대해 게임이 시작되고 1초 후에 스피너가 화면에 존재하는가?로 테스트 할 수 있었습니다.
2초가 지나기 전에 alert가 뜨지 않음을 보장하는 테스트를 추가 했습니다.

class, id 네이밍

말씀해주신 대로 class는 조금 더 범용적인 이름을 붙이고자 했으나..현재 규모에서는 엘리먼트끼리 CSS를 공유하는 일이 적기때문에 일단 id와 변수명만 다르게 변경했습니다.
또한, 굳이 필요없는 class는 삭제했습니다.

구조 변경

이 부분도 제가 너무 깊게 생각하지 않고 무턱대고 질문한 것 같아서 죄송합니다 😢 말씀해주신 도메인 별 분리에서 인사이트를 얻어서 아래와 같이 분리해보았습니다.

- Components
   - RacingCarGame
   - RacingCarGameForm
- View 
   - GameView
   - FormView

Components / RacingCarGame
- 경주 할 자동차 상태 와 우승자 상태 를 갖습니다.
- 재시작 버튼과 경주 결과 출력을 관리합니다.
Components / RacingCarGameForm
- 해당 폼에 대한 이벤트 처리를 관리합니다.

조금 찝찝한 점은, RacingCarGame 컴포넌트에서 FormView 까지 제어를 한다는 것 입니다. ( 자동차 결과가 출력 될 때와 게임이 끝날 때를 RacingCarGameForm 컴포넌트에서 감지 할 수 없기 때문입니다.)
하지만 이는 전역 상태를 도입하지 않으면 해결되지 않을 문제라고 판단했습니다. 그리고 현재 규모에서는 전역상태가 굳이 필요하지 않을 것 같아서 이와 같이 구현했습니다..!!

주말인데,,정말 좋은 답변과 리뷰 감사드립니다 ~ 혹시 더 수정해야할 부분이 있다면 말씀해주시면 감사하겠습니다 :)

youngbeomrhee

호프님 안녕하세요 구현하느라 고생하셨습니다
제안드린 리팩토링 너무나 잘 수행해 주셔서 승인드리려고 합니다
질문주신 5개 중에 아직 질문 못드린 영역에 답변 드리려고 보니 질문을 삭제하셨네요 ㅠ
절대 과한 질문은 아니었고 차차 답변드리려고 했던 부분인데 히스토리가 안남아서 답변을 드릴수가 없게 되었네요
해결해 보시다가 잘 안되시면 언제든 편하게 다시 질문 부탁 드려요
그리고 말씀주신 Spinner 관련된 테스트는 "1초의 텀을 두고 진행상황을 출력하는 기능에 대한 테스트"라고 기재해 주셔서 전진여부(↓)를 테스트 하시고자 하신걸로 오해하고 피드백 드린 부분이고 (�피드백 중에도 ↓를 제가 Spinner로 잘못 말씀 드렸네요 이 부분은 혼선을 드려 죄송합니다) Spinner의 출력여부만 테스트 하고자 한다면 Spinner는 전진여부와 상관없이 매 게임마다 표시되므로 테스트 가능한게 맞습니다
미션 완료하느라 고생 많으셨습니다 진행중에 끊임없이 고민하고 개선해 주신 점들이 인상 깊었습니다 남은 나머지 과정도 화이팅하세요 :)

moonheekim0118 added 30 commits February 15, 2022 15:30

merge step1

e990926

fix: html lang 수정

cca965a

chore: EOL 제거

e834805

docs: 기능 목록 수정

09b6c25

test: 기능 목록에 맞게 테스트코드 수정

9a605ff

refactor: map->forEach로 수정

f42ea60

refactor: 자동차 경주 관련 숫자 상수화

6351ebb

docs: 기능목록 수정

35e8261

docs: 기능 목록 수정

930f81f

refactor: View 클래스 생성하여 역할 위임

6f21f14

- dom 조작 기능은 View 클래스에 위임

refactor: class명 수정

d4b18aa

chore: lint,prettier 수정

f20b592

chore: lint rules 추가

6f5ba0c

refactor: 클래스명 수정

fa822c4

refactor: CSS, HTML 수정

202c74f

feat: 자동차 이름 입력 시, 레이싱 횟수 입력 폼이 보이도록 구현

121292b

feat: spinner 애니메이션 구현

5abafc9

feat: 자동차 경주 진행상황 출력 구현

be9ffb1

refactor: dom 관련 유틸 수정

696618b

- getElementById 대신 querySelector, querySelectorAll 사용 - ID 대신 SELECTOR 상수화

fix: 스피너 및 화살표 스타일 수정

b8d9e99

style: background-color 수정

0fdac0c

refactor: 자동차 움직임 여부 확인 로직 수정

832c109

- 기존의 Move 메서드에서 t/f 반환 제거 - 해당 함수에서 직접 확인하도록 수정

insertAdjcentHTML 중복 제거

ebb4678

refactor: dataset 상수화

f52fbf4

refactor: 함수 분리

8546b78

refactor: 불필요한 메서드 삭제

e761131

refactor: 변수명 및 함수명 수정

1e4fcd0

docs: 기능 목록 수정

27dd672

feat: Form 비활성화 기능 구현

a9c8673

- 자동차 이름 입력 후 자동차 이름 폼 비활성화 - 레이싱 횟수 입력 후 레이싱 횟수 폼 비활성화

refactor: 재사용되는 dom element 저장하도록 수정

4df1eb6

moonheekim0118 and others added 14 commits February 17, 2022 10:43

refactor: 불필요한 메서드 제거

3977d70

chore: lint rules 수정

1a5fb13

refactor: div->section으로 태그 수정

04fe1ec

style: button disable 스타일 추가

7200cd0

refactor: 파일 구조 수정

5a022b2

Merge branch 'moonheekim0118' into step2-moonheekim0118

1e7f738

chore: 기능 목록과 테스트코드 description 일치하도록 수정

584a443

Merge branch 'step2-moonheekim0118' of github.com:moonheekim0118/java…

08ceefe

…script-racingcar into step2-moonheekim0118

refactor: 불필요한 파일 삭제

0328f86

refactor: 불필요한 구문 삭제

88b35ee

refactor: getMaxCount 함수 map 사용하여 수정

2b57f2b

refactor: dom 구조에 의존적인 구문 수정

16d8af2

- getInputValue 에서 element로 받은 input value의 값을 바로 리턴하도록 수정 - 쿼리 셀렉터에 target 값으로 APP 추가

refactor: View 클래스에 app Element를 인자로 넘겨주도록 수정

7f65840

chore: 중복되는 파일 제거

b880de3

pocojang requested a review from youngbeomrhee February 18, 2022 07:28

pocojang assigned youngbeomrhee Feb 18, 2022

moonheekim0118 added 5 commits February 18, 2022 17:16

chore: lint rules 수정

c30e252

refactor: RacingCarGame 클래스 내 멤버변수들 프라이빗 변수로 수정

86a55bc

refactor: View 클래스 내 멤버변수 private 변수로 수정

6455d98

refactor: Car 클래스 내 멤버변수 private 변수로 수정

97549ec

chore: console.log 제거

70b42d5

youngbeomrhee suggested changes Feb 19, 2022

View reviewed changes

moonheekim0118 added 5 commits February 19, 2022 19:08

chore: lint rules 수정

1db3b3b

refactor: 2초가 지나기 전 축하메시지를 볼수 없음을 보장하는 테스트 추가

a1d41b7

test: progressive 재생에 대한 테스트 추가

cccdb11

refactor: class와 id 변수명 수정

5adfc87

refactor: 컴포넌트 / 뷰 분리

316f90e

youngbeomrhee approved these changes Feb 20, 2022

View reviewed changes

youngbeomrhee merged commit 9763f7c into woowacourse:moonheekim0118 Feb 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

moonheekim0118 commented Feb 17, 2022 •

edited

Loading

youngbeomrhee left a comment

youngbeomrhee Feb 18, 2022

youngbeomrhee Feb 18, 2022

youngbeomrhee Feb 18, 2022

youngbeomrhee Feb 18, 2022

youngbeomrhee Feb 18, 2022

youngbeomrhee Feb 18, 2022

moonheekim0118 Feb 19, 2022

youngbeomrhee Feb 19, 2022

youngbeomrhee Feb 19, 2022

moonheekim0118 Feb 19, 2022 •

edited

Loading

youngbeomrhee Feb 19, 2022

youngbeomrhee Feb 19, 2022

moonheekim0118 commented Feb 19, 2022

youngbeomrhee left a comment

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

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

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

Conversation

moonheekim0118 commented Feb 17, 2022 • edited Loading

파일 구조

Step 2 필수 구현 목록

추가된 기능들

리팩토링

궁금한 점

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

moonheekim0118 Feb 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moonheekim0118 commented Feb 19, 2022

테스트코드 추가 작성

class, id 네이밍

구조 변경

youngbeomrhee left a comment

Choose a reason for hiding this comment

moonheekim0118 commented Feb 17, 2022 •

edited

Loading

moonheekim0118 Feb 19, 2022 •

edited

Loading