[2단계 - 자동차 경주 구현] 유조(조윤호) 미션 제출합니다. #25

yujo11 · 2021-02-11T14:37:38Z

🎯 step1

주어진 횟수 동안 n대의 자동차는 전진 또는 멈출 수 있다.
자동차에 이름을 부여할 수 있다. 전진하는 자동차를 출력할 때 자동차 이름을 같이 출력한다.
자동차 이름은 쉼표(,)를 기준으로 구분하며 이름은 5자 이하만 가능하다.
사용자는 몇 번의 이동을 할 것인지를 입력할 수 있어야 한다.
전진하는 조건은 0에서 9 사이에서 random 값을 구한 후 random 값이 4 이상일 경우 전진하고, 3 이하의 값이면 멈춘다.
자동차 경주 게임을 완료한 후 누가 우승했는지를 알려준다. 우승자는 한 명 이상일 수 있다.
우승자가 여러명일 경우 ,를 이용하여 구분한다.

🎯🎯 step2

자동차 경주 게임의 턴이 진행 될 때마다 1초의 텀(progressive 재생)을 두고 진행한다.
- 애니메이션 구현을 위해 setInterval, setTimeout, requestAnimationFrame 을 활용한다.
정상적으로 게임의 턴이 다 동작된 후에는 결과를 보여주고, 2초 후에 축하의 alert 메세지를 띄운다.
위 기능들이 정상적으로 동작하는지 Cypress를 이용해 테스트한다.

🌏 데모 사이트

안녕하세요!!

지난 1단계 미션에서 피드백 받았던 내용을 바탕으로 중복되는 코드를 줄이고 함수와 변수, 그리고 테스트의 네이밍에 신경쓰면서 2단계 미션을 진행했습니다!

잘 부탁드립니다!!

VALIDATOR 가 포함하고 있던 상수들을 GAME으로 이동

max-lines 15 -> 20

Vallista · 2021-02-15T13:36:49Z

안녕하세요 유조님! 이번 미션도 잘 부탁 드립니다! 😄

Vallista

안녕하세요 유조님!

저번 코드와 동일하게 소규모 사이즈에 맞게 적절한 파일 분배와 로직을 사용하셔서 구현을 해주셨어요!
지금까지는 절차지향적으로 짜도 상관이 없지만, 추후에 비즈니스가 복잡해지면 협업적인 측면, 가독성 부분에서 객체 단위로 분리하는게 좋아보여요!

자동차 경주 구현이 몇단계까지 있는지 모르겠지만, 다음 단계에서는 MVC 패턴을 사용하셔서 (지금에서 조금만 수정하시면 되어요) 레이어를 단순히 분리하시어, 객체 단위로 관리하는게 조금 더 좋아보입니다!

또한 전반적으로 set-을 많이 사용해주셨어요! set- 보다는 특정 행동을 지칭하는 함수로 만들어서 사용하시는게 더 좋아보입니다!

수고 많으셨습니다! 몇 가지 코맨트만 해결해주시면 감사하겠습니다!

(p.s. 여전히 커밋 히스토리도 잘 관리 해주시네요!! 보기 좋았습니다! 감사합니다!)

Vallista · 2021-02-15T13:38:28Z

cypress/integration/racingGame.spec.js

-      length: GAME.MAX_SCORE - GAME.MIN_SCORE + 1,
-    }).map((v, i) => i);
+    const randomNumbers = [...Array(100)]
+      .map(() => getRandomNumber(GAME.MIN_SCORE, GAME.MAX_SCORE))


HoF를 이용해 chaining을 진행하니 가독성이 많이 좋아졌네요! 굿입니다! 👍

감사합니다! 다른 크루의 리뷰에 달린 코멘트에서 아이디어를 얻었습니다ㅎㅎ

Vallista · 2021-02-15T13:43:58Z

src/js/game/printGameResult.js

 import { getWinners } from './getWinners.js';
 import { restartGame } from './restartGame.js';

+const alertGameResult = (winners) => {
+  alert(`🎉 축하드립니다! 우승자는 ${winners}입니다! 🎉`);


winners의 값이 빈 배열이거나 undefined, null인 경우에 대한 예외처리는 없어도 될까요~?
이러한 parameter에 대한 예외처리를 함수를 작성하실 때 고려하시는게 중요합니다.

함수를 작성하는 순서

먼저 한글로 어떤 행동을 하는 함수인지 정의한다.

const 게임결과에_대한_알럿을_출력하는_함수 = () => {}

해당 함수가 어떤 parameter가 필요한지 정의한다

const 게임결과에_대한_알럿을_출력하는_함수 = (winners) => {}

해당 parameter는 어떠한 예외가 있는지 bad case를 정의하여 return 구문을 작성한다.

const 게임결과에_대한_알럿을_출력하는_함수 = (winners) => { if (!Array.isArray(winners) || winners.length === 0) return alert(`🎉 축하드립니다! 우승자는 ${winners}입니다! 🎉`); }

사실 이 기법은 TDD할 때도 많이 쓰이는 기법입니다. 테스트 케이스의 base case를 정의하기 전, bad case를 정해두시면 언제나 좋습니다!

TDD 및 알고리즘을 작성할때 유용합니다

와 알려주신 순서대로 함수를 정의하는건 한번도 생각해보지 못 했는데 정말 좋은 방법이네요!! 좋은 습관을 들일 수 있게 앞으로 함수를 작성할 때 알려주신 순서를 지키면서 작성 해야겠습니다!

좋은 인사이트 주셔서 감사합니다!!👍👍👍

넴!! 그런데 마지막 4번째 단계를 빼먹었네요 ㅋㅋ 4번째 단계는 함수명 한글을 영어로 변환 하시면 됩니다! ㅋㅋ

Vallista · 2021-02-15T13:44:50Z

src/js/game/printGameResult.js


-  $gameResultText.innerHTML = `🏆 최종 우승자: ${getWinners()} 🏆`;
+  $gameResultText.innerHTML = `🏆 최종 우승자: ${winners} 🏆`;


무언가 위의 alertGameResult의 alert 메시지와 중복해서 써먹을 수 있을 것 같아 보이네요.

[질문]
최종 우승자를 출력하는(보여주는) 텍스트와 alert를 통해 사용자에게 알려주는 텍스트에서 중복되는 부분이 winners 뿐인데 혹시 어떤식으로 중복해서 사용할 수 있을까요? 나름 생각을 해봤지만 어떤 식으로 써먹을 수 있을지 아이디어가 생각나지 않아서 질문을 남깁니다ㅠㅠ

아 요것은 패스하셔도 됩니다!!!

Vallista · 2021-02-15T13:51:19Z

src/js/utils/setAttribute.js

@@ -0,0 +1,17 @@
+export const setVisibility = (target, option) => {


set... 단위로 함수를 사용하는것도 좋지만,
특정한 행동으로 제어를 하는게 조금 더 좋아보입니다!

set이라고 하면 추후 소프트웨어가 커졌을때 이 함수에 true를 넘겼는지, false를 넘겼는지를 비즈니스 로직을 따라 일일히 확인해가면서 추적해야합니다. 그런 상황을 막기 위해서, 해당 함수가 특정한 행동을 하는 네이밍으로 제어하는 것이지요!

그래서

show(){...} hide() {...} disabled() {...} enabled() {...}

형태로 행동을 나누는게 어떨까 싶습니다!

작은 규모의 소프트웨어였는데도 set의 인자를 넘길 때 헷갈리는 순간이 있었습니다😅. 말씀해주신대로 큰 큐모의 소프트웨어라면 set...으로 명명했을 때 로직을 찾느라 더 큰 비용이 발생할 수 있겠네요!

기존 toggle...으로 작성한 함수보다는 좋은 방법이라고 생각해서 사용했습니다. 하지만 네이밍을 통해 행동을 지시하는 방법은 생각해보지 못 했네요! 코드에 반영해보겠습니다! 감사합니다!!!👍👍👍

Vallista · 2021-02-15T13:53:38Z

cypress/integration/racingGame.spec.js

+    typeRacingCountAndClickToSubmitButton();
+
+    // 자동차 경주 진행시간 5000ms + alert 출력 대기시간 2000ms
+    cy.tick(7000);


7000이라는 숫자가 나온 자동차 경주 진행시간인 5000이라는 숫자와 alert 출력 대기시간 2000을 상수화해서 사용하는게 좋아보입니다.

하단에서도 5000을 사용하는 것 같고, 위에서도 중복해서 사용하네요!

최상단에서

const RACE_TIME = 5000 const DELAY_PRINT_ALERT = 2000

요런 형태로 지정하여

cy.tick(RACE_TIME + DELAY_PRINT_ALERT) cy.tick(RACE_TIME)

형태로 사용하는게 더 좋아보입니다!

테스트 코드의 상수 분리에는 신경을 못 썼군요😅😅😅

놓치고 있었는데 꼼꼼하게 봐주셔서 감사합니다!👍👍👍

Vallista · 2021-02-15T13:54:21Z

cypress/integration/racingGame.spec.js

+
+    // 경주 진행시간 5000ms
+    cy.get('.spinner-container').should('be.visible');
+    cy.wait(2000);


cy.get cy.wait

패턴이 반복되네요! 충분히 함수로 뺄 수 있을 것 같습니다!

그리고, 생각보다 중첩되는 로직이 많아지므로,

인치님께 드렸던 피드백 처럼 객체화를 하여 chaining 형태로 해결하는 방법도 있습니다!

오 좋은 방법이네요! 코드에 적용해보겠습니다 감사합니다!!👍👍👍

set -> hide, show, enabled, disabled

yujo11 · 2021-02-15T16:14:54Z

@Vallista

함수의 예외처리 추가 b794b80
set- 단위 함수들 분리 7625e7c
test 반복되는 숫자 상수로 분리 d6e71c1
test 중복되는 코드 함수로 분리 42f041a

안녕하세요 Vallista님! 늦은 시간에도 꼼꼼하게 리뷰 남겨주셔서 감사합니다!!😁😁😁

피드백 주신 내용들 코드에 반영했습니다! 저번 피드백에 이어 이번에도 피드백을 통해 생각하지 못 했던 많은 부분들을 배워갈 수 있었네요! 특히 함수를 작성하는 순서에 대한 부분은 정말 한번도 생각해보지 못 했었는데 알려주신 순서대로 작성하면 함수를 더 잘(?) 만들 수 있을거 같다는 자신감도 생기네요!!

두가지 궁금한 점이 있는데 혹시나 시간 괜찮으시다면 답변해주시면 감사하겠습니다ㅠㅠ

말씀해주신 부분 중 '레이어'와 '객체'에 대한 부분이 조금 헷갈려서 질문 드립니다! '레이어'가 프레젠테이션 레이어, 비즈니스 레이어, 데이터 액세스 레이어로 나눠지는 레이어 패턴을 말씀하신게 맞을까요? 맞다면 각 레이어마다 역할을 담당하는 객체들을 생성하고 해당 객체들을 통해서 전체 소프트웨어를 관리해보라는 뜻으로 이해해도 될까요?
winners가 존재하지 않는 경우는 문제가 생겨 게임이 정상적으로 진행되지 않는 경우 뿐입니다. 그래서 해당 정보를 alert로 사용자에게 알려주고 개발자가 문제가 생긴 부분을 캐치할 수 있도록 trhow new Error를 추가로 넣어줬는데 이런 경우는 로직을 분리하는게 좋을까요?

  if (!winners) {
    alert('❌ Error : 우승자를 찾지 못 했습니다. ❌');
    throw new Error('우승자를 찾지 못 했습니다.');
  }

Vallista님 덕분에 MVC패턴과 레이어 패턴에 대해서도 찾아보게 됐네요! 많은 인사이트 주셔서 감사합니다!! 다음 과제에서는 Vallista님이 말씀해주신 것처럼 레이어를 분리해서 객체 단위로 관리하는 방법을 꼭 시도해보겠습니다!!👍👍👍👍

Vallista

빠르게 반영해주셔서 감사합니다 유조님!

코드도 깔끔해졌고, 함수도 잘 활용하게 되신 것 같아 발전하시는 모습이 기대됩니다!

질문에 대한 답변으로는..

말씀주신 레이어 패턴이 맞습니다! 레이어 패턴이란 레이어를 어떤 특정한 형태로 나누기보다, 개발을 하시면서 어떤 특정한 일을 하는 레이어를 구축하시고, 레이어 단위로 커뮤니케이션을 할 수 있도록 개발하시면 그게 바로 레이어 패턴입니다! MVC도 일종의 레이어 패턴이라고 볼 수 있죠! 👍
throw new Error를 추가하셔도 됩니다 :) 콘솔에서 로그를 빠르게 파악하기 위함이므로 위와 같은 경우랑은 조금 다를 수 있겠다 싶네요! 보통 크리티컬한 에러가 나오지만 어플리케이션은 다운이 되지 않고 계속 실행이 되어야 할 때 try-catch와 더불어 같이 쓰이곤 합니다. 지금 코드를 짜시는 부분이 비즈니스와 연관된 부분이므로, 굳이 로직을 분리를 안하셔도 될 것 같습니다 👍

yujo11

[셀프 리뷰]

한달 전 내 코드에서 잘한 점
- 상수를 분리한 점.
  - 상수를 분리하면서 전체 코드에 매직 넘버가 사라지고 가독성이 높아졌다.
- 재사용 되는 함수들을 util로 관리한 점
  - utils 폴더의 모듈들 덕분에 코드의 중복을 줄일 수 있었다.
한달 전 내 코드에서 아쉬운 점
- 상태관리를 유연하게 하지 못 했다.
  - 상태를 객체 등에 담아서 관리하지 않고 DOM에 삽입한 상태로 필요할 때마다 파싱했는데 그로 인해 불필요한 DOM접근이 많아졌다.
- 구현에 앞서 전체적인 구조를 설계하지 못 했다.
  - 결국 구현을 마친 후 구조도를 그려봤을 때 flow가 굉장히 이상해졌다.
앞으로 어떻게 다르게 할 것인가
- 전체 구조를 먼저 설계하고 코드를 작성한다.
- 상태관리에 대한 다양한 방법을 고민해본다.
한달 전 내 코드를 보고 느낀 점
- 한 달 전의 내가 작성한 코드를 지금 봤을 때 참을 수 없을만큼 보기 힘들줄 알았는데 그렇지 않아서 조금 아쉽다.
- 한 달 전의 내가 뛰어난 실력을 가졌다기보다는 지난 한 달 간 성장이 정체되어 있었던거 같다.
- 다음 한달은 더욱 더 열심히 해서 많이 성장할 수 있도록 노력해야겠다.

yujo11 · 2021-03-09T02:53:48Z

cypress/integration/racingGame.spec.js

@@ -21,6 +18,27 @@ describe('racing-game', () => {
    return cy.get('#racing-count-submit').click();
  };

+  const waitRaceTime = (raceTime = TEST_RACE_TIME) => {
+    cy.clock();


반복되는 cy.clock()은 beforeEach() 구문에 넣었으면 중복을 줄일 수 있었겠다.

yujo11 · 2021-03-09T02:55:10Z

cypress/integration/racingGame.spec.js

    cy.on('window:alert', alertStub);
+
    typeCarNameAndClickToSubmitButton();
    typeRacingCountAndClickToSubmitButton(negativeRacingCount).then(() => {
      expect(alertStub.getCall(0)).to.be.calledWith(


stub()을 쓰는 발상까지는 좋지만 alertStub.getCall(0)처럼 콜스택에 매직넘버로 접근하는 방법보다는 다른 방법을 더 찾아봐야겠다.

yujo11 · 2021-03-09T02:56:03Z

cypress/integration/racingGame.spec.js

-    }).map((v, i) => i);
+    const randomNumbers = [...Array(100)]
+      .map(() => getRandomNumber(GAME.MIN_SCORE, GAME.MAX_SCORE))
+      .filter((num) => GAME.MIN_SCORE <= num && num <= GAME.MAX_SCORE);


이 조건문이 가독성이 떨어지는데 validate함수를 따로 분리했으면 어땠을까

yujo11 · 2021-03-09T02:57:31Z

src/js/game/startGame.js

    if (isForward) {
      $car.dataset.forwardCount = Number($car.dataset.forwardCount) + 1;
-      $car.parentNode.insertAdjacentHTML('beforeend', arrowTemplate());
+      $car.insertAdjacentHTML('afterend', arrowTemplate());


yujo11 · 2021-03-09T02:59:54Z

src/js/game/startGame.js

+  const gameProcess = setInterval(() => {
+    if (racingCount-- === 1) {
+      clearInterval(gameProcess);
+    }


가독성이 좋지 않다. 차라리 다음 처럼 작성했다면 어땠을까

const gameProcess = setInterval(() => { if (racingCount === 상수) { clearInterval(gameProcess); racingCount -= 1; }

아니면 별도의 함수로 분리했어도 좋았겠다.

yujo11 added 22 commits February 9, 2021 22:00

docs: 불필요한 내용 정리

a2e2b84

test: 자동차 경주가 진행될 때 턴마다 1초의 지연시간이 생기는지 테스트 하는 코드 작성

d0f6194

test: 자동차 경주의 지연시간마다 Animation이 출력되는지 테스트하는 코드 작성

dbc866d

test: 경주를 마쳤을 때 2초 후 축하의 alert메세지가 출력되는지 확인하는 테스트 코드 작성

5d8ef99

refactor: 전체 코드의 일관성을 위해 함수 선언식을 함수 표현식으로 변경

4df4349

feat: 자동차 경주가 진행될 때 매 턴마다 1초의 delay를 주는 기능

cb3803d

feat: 경주가 끝난 후 2초 후에 축하의 alert메세지를 띄우는 기능

ec48997

refactor: alert출력 테스트의 대기시간 수정

b868476

refactor: 역할을 확실히 할 수 있도록 constant 모듈 구성 변경

4a58639

VALIDATOR 가 포함하고 있던 상수들을 GAME으로 이동

refactor: 불필요한 값을 return하지 않도록 수정

a3f6ec2

fix: 테스트코드가 애니메이션이 몇개 출력되는지 검증하지 못 하던 오류 수정

b4c1b7a

refactor: Error Message 변수명 직관적이게 변경

fba8912

feat: 경주 진행 턴 대기시간에 애니메이션을 표시하는 기능

cfbc0dc

docs: 구현한 기능들 체크

5d9d3e4

refactor: 불필요한 requestAnimationFrame 제거

2f0fc82

test: Animation 출력 테스트 신뢰도를 높이기 위해 visible메서드로 변경

ff25aac

test: 자동차 경주가 진행되는 테스트들에 지연시간 추가

a272ee3

test: 자동차 경주 진행 중 딜레이 검증 케이스 추가

a19d9e8

style: 가독성을 위한 함수, 변수 이름 변경

4700ef6

chore: eslint config 변경

2c2f7ba

max-lines 15 -> 20

refactor: toggle- 함수들을 set- 함수로 변경

4e0ca6f

refactor: 반복되는 숫자 상수로 분리

0270679

yujo11 changed the base branch from main to yujo42 February 11, 2021 14:44

pocojang requested a review from Vallista February 12, 2021 03:56

pocojang assigned Vallista Feb 14, 2021

Vallista suggested changes Feb 15, 2021

View reviewed changes

yujo11 added 3 commits February 15, 2021 23:57

refactor: winners가 없을 경우 예외처리가 가능하도록 alert 함수 수정

b794b80

refactor: 가독성을 위한 함수 이름 변경 및 분리

7625e7c

set -> hide, show, enabled, disabled

test: 반복되는 숫자 상수로 분리

d6e71c1

yujo11 added 2 commits February 16, 2021 00:20

style: 상수 이름 변경 RACING_TIME -> RACE_TIME

1f5ab2f

test: 반복되는 테스트 내용 함수로 분리

42f041a

Vallista approved these changes Feb 15, 2021

View reviewed changes

Vallista merged commit 7986277 into woowacourse:yujo42 Feb 15, 2021

Vallista mentioned this pull request Feb 15, 2021

[2단계 - 자동차 경주 구현] 지그(송지은) 미션 제출합니다. #36

Merged

2 tasks

yujo11 commented Mar 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 자동차 경주 구현] 유조(조윤호) 미션 제출합니다. #25

[2단계 - 자동차 경주 구현] 유조(조윤호) 미션 제출합니다. #25

yujo11 commented Feb 11, 2021

Vallista commented Feb 15, 2021

Vallista left a comment •

edited

Vallista Feb 15, 2021

yujo11 Feb 15, 2021

Vallista Feb 15, 2021

Vallista Feb 15, 2021

yujo11 Feb 15, 2021

Vallista Feb 15, 2021

Vallista Feb 15, 2021

yujo11 Feb 15, 2021

Vallista Feb 15, 2021

Vallista Feb 15, 2021

yujo11 Feb 15, 2021

Vallista Feb 15, 2021

yujo11 Feb 15, 2021

Vallista Feb 15, 2021

Vallista Feb 15, 2021

yujo11 Feb 15, 2021

yujo11 commented Feb 15, 2021 •

edited

Vallista left a comment •

edited

yujo11 left a comment •

edited

yujo11 Mar 9, 2021

yujo11 Mar 9, 2021

yujo11 Mar 9, 2021

yujo11 Mar 9, 2021

yujo11 Mar 9, 2021

yujo11 Mar 9, 2021


		$gameResultText.innerHTML = `🏆 최종 우승자: ${getWinners()} 🏆`;
		$gameResultText.innerHTML = `🏆 최종 우승자: ${winners} 🏆`;

		@@ -0,0 +1,17 @@
		export const setVisibility = (target, option) => {

[2단계 - 자동차 경주 구현] 유조(조윤호) 미션 제출합니다. #25

[2단계 - 자동차 경주 구현] 유조(조윤호) 미션 제출합니다. #25

Conversation

yujo11 commented Feb 11, 2021

🎯 step1

🎯🎯 step2

Vallista commented Feb 15, 2021

Vallista left a comment • edited

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

yujo11 commented Feb 15, 2021 • edited

Vallista left a comment • edited

Choose a reason for hiding this comment

yujo11 left a comment • edited

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

Vallista left a comment •

edited

yujo11 commented Feb 15, 2021 •

edited

Vallista left a comment •

edited

yujo11 left a comment •

edited