[1단계 - 자동차 경주 구현] 유세지(김용래) 미션 제출합니다. by usageness · Pull Request #94 · woowacourse/javascript-racingcar

usageness · 2022-02-11T09:00:03Z

[1단계 - 자동차 경주 구현] 유세지(김용래) 미션 제출합니다.

마감시간까지 열심히 달려보았지만 기능을 모두 구현하지 못했습니다. 페어간 의견 교환을 하다보니 '더 좋은 구현 방법이 있을텐데...' 하는 욕심이 생겨서 여러번 설계를 변경했던 점이 가장 큰 원인인 것 같습니다. 같은 미션이지만 프리코스 중 서로 다른 방법으로 접근했던 것을 보며 정말 다양한 방법과 생각들이 있다는 점을 느꼈습니다.

페어 프로그래밍과 테스트 코드 작성 뿐 아니라 페어분의 코드를 보면서도 많은걸 배우고 느껴 저에게는 정말 값진 미션이었습니다. 이번 1단계 미션에서 익힌걸 상기하며 남은 2단계도 성공적으로 끝낼 수 있도록 노력하겠습니다. 코치님, 리뷰어님 모두 감사합니다 😭

EungyuCho

안녕하세요. 리뷰어 아사입니다!

세지님 고생하셨습니다 ㅎㅎ

적어주신 PR 코멘트만 봐도 더 좋은 코드를 설계하기 위해서 페어간에 많은 의견 교환과 페어님의 코드를 보시면서 많이 성장하신 것 같아요! 실제로 코드에서도 조금이라도 더 좋을만한 부분을 주석으로 남긴 걸 보면서 정말 많은 생각을 하시면서 코드를 작성하는 걸 느낄 수 있었어요.
분명 고민하시고 노력하신 것 이상으로 세지님이 이번미션에서 많은 걸 얻어가실 수 있을거라 생각해요.

코드단에 못남긴 몇 가지 코멘트를 남겨볼게요!

View의 역할이 명확하게 구분되어있어서 좋았어요
클래스 관련 함수 네이밍이 적절했어요
시간이 부족할때에는 구현에 우선 집중해보는 것도 방법일 것 같아요! 좋은 설계도 중요하지만 보통 실무에서는 데드라인이 있기 때문에 이번 기회를 통해서 익숙해져가면 좋을 것 같아요!

아래 코드에 남겨드린 코멘트도 확인하셔서 개선할 부분에 대해서는 개선하시고 궁금하신점은 코멘트를 남겨주시면 감사하겠습니다! :)

미션 완주까지 화이팅입니다!

EungyuCho · 2022-02-11T18:13:43Z

@@ -0,0 +1,6 @@
+const isAdvance = () => {
+  const randomNumber = Math.random * 9;


원하는 범위의 random을 어떻게 구해볼지 다시 찾아봅시다.

Math.random의 경우 메서드다보니 호출하지 않고 숫자를 곱하게 되면 randomNumber가 NaN이 되는 현상이 있어요!

그리고 만드신 Random값을 얻는 함수를 분리하면 좋을 것 같아요!

Random값을 얻는 함수를 별도의 함수로 분리하고, 메서드를 호출하도록 코드를 수정했습니다!
추후에 매개변수와 함수 내에 들어가는 숫자도 상수화시킬 예정입니다!

const isAdvance = () => { return generateRandomNumber(9) >= 4; }; function generateRandomNumber(maxNumber) { return Math.random() * maxNumber; }

EungyuCho · 2022-02-11T18:31:37Z

+  if (names.indexOf(',') === -1) {
+    return [names];
+  }


이 조건문은 없어도 될 것 같아요.
split을 했을 때 해당 seperator가 없으면 적어주신 return [names] 형태로 반환돼요!

구분자가 없을때의 split 이 배열 형태로 반환되는 줄 몰랐네요...
이 부분은 삭제하였습니다!

EungyuCho · 2022-02-11T18:33:13Z

+  const output = [];
+  nameList.forEach((name) => {
+    output.push(name.trim());
+  });
+
+  return output;


map을 사용해보면 어떨까요?
변수 선언없이 바로 리턴할 수 있어보여요!

아래와 같이 변수 선언 없이 수정했습니다!

function trimNameList(nameList) { return nameList.map((name) => { name.trim(); }); }

EungyuCho · 2022-02-11T18:35:07Z

+  let output = splitCarNameList(userInput);
+  output = trimNameList(output);
+
+  return output;


Suggested change

let output = splitCarNameList(userInput);

output = trimNameList(output);

return output;

const output = splitCarNameList(userInput);

return trimNameList(output);

다시 output에 할당하는 것 보다 이렇게 줄이는게 가독성이 좋아보여요

수정했습니다!
다음부터는 굳이 변수를 만드는 것 보다는 return을 통해 한 번에 반환 가능하는 쪽으로 코드를 짜볼게요!

EungyuCho · 2022-02-11T18:44:23Z

+    carList.forEach((car) => {
+      if (isAdvance() === true) {
+        return true;
+      }
+
+      car.go();
+    });


구현하신 isAdvance는 현재 자동차가 전진해야하는지 여부를 알려주는 함수인데 현재 구현되어 있는 형태는

carList.forEach((car) => { if (isAdvance() === true) {. // 만약 자동차가 전진 할 조건이라면(if isAdvance) return true; // true를 리턴한다. (`forEach`는 리턴 타입이 `void`이기 때문에 리턴하신 true에 의미는 없어요) } car.go(); // 전진할 조건이 아니면 차를 움직인다 });

이런 구조로 흐름이 짜여 있어요.

전진해야하는 상태인지 확인한 후 만약 전진해야하는 상태가 아니라면 바로 return 해준다.

차를 전진시킨다

이런 흐름이 맞지 않을까요?

원래 의도는 if (isAdvance() === false) 으로 전진 할 조건이 아니면, return을 통해 조기에 종료시키고 다음 자동차의 전진 여부를 판단하는 코드였는데, 정신없이 커밋하다보니 착오가 생겼던것 같습니다 😭😭
원래 의도에 맞게 수정하겠습니다!

EungyuCho · 2022-02-11T19:25:55Z

+    $$('.racing-car-container, #result').forEach(($element) => {
+      $element.setAttribute('data-state', 'on');
+    });


오.. 깔끔하게 element를 순회할 수 있네요 👍

EungyuCho · 2022-02-11T19:37:37Z

+    if (carList.length === 0) {
+      throw new Error('참여 자동차가 0대로 설정되어있습니다.');
+    }


이 예외처리가 필요할 지 생각해보면 좋을 것 같아요.
필요하거나 불필요한 이유를 찾고 개선방향이 있다면 해당 내용들을 코멘트로 주시면 좋을 것 같네요! 💡

이 부분은 처음 화면에서 자동차의 이름을 입력하는 폼과 시도할 횟수를 입력하는 폼이 동시에 노출되어 자동차가 입력되기 전 게임이 시작되는 것을 방지하기 위해 만들어진 예외 처리였습니다.

하지만 자동차 이름을 입력한 뒤에 시도할 횟수를 입력하는 폼을 노출시켜준다면 이 예외처리는 필요하지 않은 부분일 것 같습니다. 로직을 개선해서 불필요한 예외처리를 만들지 않는 방향으로 수정하겠습니다!

EungyuCho · 2022-02-11T19:40:21Z

+      this.View.renderAdvanceDiv(name);
+    });
+
+    for (let i = 1; i < this.RacingGame.round; i += 1) {


시도할 횟수 입력창에 3을 입력하면 RacingGame.round의 값이 3으로 설정됩니다.
그러면 게임을 3회 시도해야하는데 올바른 횟수로 실행되려면 조건문을 어떻게 설정해야할까요?

EungyuCho · 2022-02-11T19:40:34Z

+const SELECTOR = Object.freeze({
+  CAR_NAME_FORM: '#car-name-form',
+  CAR_NAME_INPUT: '#car-name-input',
+  CAR_NAME_BUTTON: '#car-name-button',
+  RACE_TIME_FORM: '#race-time-form',
+  RACE_TIME_INPUT: '#race-time-input',
+  RACE_TIME_BUTTON: '#race-time-button',
+  RACE_CONTAINER_DIV: '.racing-car-container',
+});


EungyuCho · 2022-02-11T19:43:58Z

+    event.preventDefault();
+    this.handleWinnerDisplay();
+
+    $(SELECTOR.RACE_CONTAINER_DIV).innerHTML = '';


이 부분은 View로 분리하는게 좋을 것 같아요.
다른 DOM 조작부분은 모두 View에 있어서 코멘트 드려요!

EungyuCho · 2022-02-13T05:31:39Z

완성하지 못한 step1 기능 요구사항들도 마무리 부탁드립니다!

usageness · 2022-02-14T13:37:32Z

아사님, 안녕하세요! 😊

말씀해주셨던 코드 리뷰를 바탕으로 코드의 문제점들을 보완하고, 나머지 기능 구현도 완료하였습니다.
아직 리팩토링이 많이 필요해보이는 코드들이 있지만 다음주까지 주어진 시간 내로 마무리 지을 수 있도록 열심히 하겠습니다!
정성스러운 리뷰덕분에 문제점들을 빠르게 수정할 수 있었던 것 같아요. 정말 감사드립니다 👍👍

1차 코드 리뷰 변경점 요약

명확성과 가독성을 위한 함수 이름 수정
잘못 구현된 자동차 전진 로직 수정
컨트롤러에 있던 DOM 조작 코드 삭제
완성하지 못한 step1 요구 사항 구현

EungyuCho

안녕하세요 세지님!
step1 미션 진행하느라 정말 고생하셨습니다 :)

코멘트 드린부분 잘 반영해주셨고 개선 못하신 부분도 이미 개선할 부분을 인지하고 계신 것 같네요!
구조면에서도 점점 개선되는게 눈에 보였고 이정도면 MVC도 적절하게 분리된 코드인 것 같아요~
그리고 코드 가독성이 눈에 띄게 좋아졌네요!
다른 크루들의 코드도 한번 살펴보면서 어떤 구조로 짰는지 살펴보면 좋을 것 같아요 ㅎㅎ

리뷰 코멘트같은 경우 아직 상수로 변경하지 않은 숫자가 남아있는 부분들의 위치에 코멘트를 남겨드렸어요!
항상 숫자를 상수로 변경할 필요는 없지만 코멘트 남겨드린 부분은 상수로 변경하는 게 좋을 것 같아 코멘트를 남겨드렸어요.

언제 코멘트에 숫자를 상수로 빼면 좋을지 판단하는데 도움이 되실만한 포스팅을 링크로 걸어드렸으니 시간이 괜찮으시다면 한번 읽어봐주시면 좋을 것 같습니다!~

남은 테스트와 요구사항들은 이어서 step2에서 완성해주시면 되겠습니다.
미션 수행하시느라 정말 고생 많으셨고 step2에서 또 뵙겠습니다!!

EungyuCho · 2022-02-14T13:54:09Z

@@ -0,0 +1,9 @@
+const isAdvance = () => {
+  return generateRandomNumber(9) >= 4;


이 숫자도 상수로 빼보면 좋을 것 같아요! 🤔

EungyuCho · 2022-02-14T13:59:02Z

+
+  const names = nameStringToArray(value);
+  console.log(value);
+  if (!hasValidLengthInArray(names, 1, 5)) {


이 부분도 상수로 빼는게 좋겠습니다~
이 글을 한번 참고해주세요!

EungyuCho · 2022-02-14T14:01:27Z

+    cy.get('#car-name-input').type('compy, usage');
+    cy.get('#car-name-button').click();
+
+    // 비활성화 체크
+    cy.get('#car-name-input')
+      .invoke('attr', 'disabled')
+      .should('eq', 'disabled');
+    cy.get('#car-name-button')
+      .invoke('attr', 'disabled')
+      .should('eq', 'disabled');


반복되는 부분을 함수로 분리하는 것 처럼 cypress에서는 보편적으로 필요한 테스트 로직을 Custom commands를 통해 분리하는데요~
개선이 되면 좋을 것 같아 문서를 첨부드립니다 :)
Docs를 참고해주세요!

usageness and others added 11 commits February 10, 2022 21:10

docs: 요구 사항 정리

a26ef9b

chore: eslint, prettier 적용

c714935

feat: 기본 index 마크업, css 적용

68d5f29

chore: gitignore 적용

2cd7059

style: 사용하지 않는 스타일시트 제거, 웹폰트 추가

b2523df

docs: 요구 사항 정리 문서 작성

8da6fd4

feat: test 코드 작성

2f6808a

chore: 프로젝트 세팅

4da9938

feat: index, css 작성

189ce32

feat: 주요 기능 구현

2d55f4f

chore: git ingore 수정

389b35a

pocojang requested a review from EungyuCho February 11, 2022 09:13

pocojang assigned EungyuCho Feb 11, 2022

EungyuCho suggested changes Feb 11, 2022

View reviewed changes

usageness added 3 commits February 12, 2022 16:33

chore: git ignore에 cypress 기본 파일들 추가

9a55baa

fix: random 함수 오류 수정 및 랜덤 부분 별도 함수로 분리

1d8428c

refactor: 자동차 이름 관련 함수들 가독성 개선

5bca6ea

usageness added 10 commits February 14, 2022 10:25

refator: isCarNameIputValidCheck 함수 이름 변경

62b0503

refactor: 시도 횟수 제한 최댓값 검증 추가 및 함수 분리

40aba22

refactor: isLengthLimit 함수 이름 변경 및 내부 함수 분리

f5ac004

fix: 정상적으로 값이 들어가지 않던 오류 수정

99ac5dd

feature: 우승자 출력 기능 구현

b7c4a83

chore: 사용되지 않는 html 태그와 DOM 요소 제어 코드 삭제

5f20533

feat: 자동차 이름을 입력해야 시도할 횟수 입력 가능

5460f3b

feat: 게임 다시하기 기능 구현

f5c3f1b

refactor: 로직 변경으로 사용되지 않는 코드들 삭제

14087a6

docs: 완료된 요구사항 표기 및 리뷰 후 변경점 작성

efd6132

usageness requested a review from EungyuCho February 14, 2022 13:37

EungyuCho approved these changes Feb 14, 2022

View reviewed changes

EungyuCho merged commit 236a741 into woowacourse:usageness Feb 14, 2022

		@@ -0,0 +1,6 @@
		const isAdvance = () => {
		const randomNumber = Math.random * 9;

		@@ -0,0 +1,9 @@
		const isAdvance = () => {
		return generateRandomNumber(9) >= 4;

Conversation

usageness commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EungyuCho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usageness Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usageness Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EungyuCho commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

usageness commented Feb 14, 2022

1차 코드 리뷰 변경점 요약

Uh oh!

EungyuCho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

usageness commented Feb 11, 2022 •

edited

Loading

usageness Feb 12, 2022 •

edited

Loading

usageness Feb 12, 2022 •

edited

Loading

EungyuCho commented Feb 13, 2022 •

edited

Loading