Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[클린코드 2기 정현수] 자동차 경주 미션 STEP2 #76

Merged
merged 56 commits into from Apr 10, 2022

Conversation

junghyeonsu
Copy link

@junghyeonsu junghyeonsu commented Apr 9, 2022

안녕하세요 리뷰어님!
STEP2 기능을 완료해서 리뷰요청드립니다!

데모 링크

🎯 2단계 요구사항

  • 자동차 경주 게임을 완료한 후 누가 우승했는지를 알려준다. 우승자는 한 명 이상일 수 있다.
  • 우승자가 여러명일 경우 ,를 이용하여 구분한다.
  • 재시작 버튼 클릭 시 게임이 재시작된다.

바뀐부분에 대해서 코멘트를 남겨놓겠습니다!
참고해서 리뷰해주시면 감사하겠습니다!

- cypress-watch-and-reload 설치
- cypress 설치
- cypress 실행 스크립트 추가
- 자동차 이름 입력창과 버튼을 볼 수 있는지 실패하는 테스트 작성
- 시도 횟수 폼은 자동차 이름 입력 후 렌더링 할 예정
- 5글자 이하인지 확인
- 자동차 이름이 입력됐는지 확인
- 알맞은 자동차 이름들을 넣었을 때 시도 횟수 템플릿을 보여주도록 구현
- 1보다 작을 경우 경고창 띄우기 구현
- 처음 render하는 template에 section tag들을 미리 작성
- 만약 시도 횟수가 잘 입력되었다면 게임 보드 위에 플레이어 수 만큼 children을 잘 가지고 있는지 확인
- Car 객체를 만들어서 이름을 부여
- 0에서 9까지 숫자를 랜덤으로 뽑는 함수 구현
- 3이 넘으면 전진
- 각 자동차마다 gameResult 배열로 stop인지 advance인지 담음
- 애플리케이션 전체 렌더링 함수 모듈화
- view/TryCount 파일에서 tryCountFormView로 모듈화
- RacingCarGame 컨트롤러를 생성해서 모든 게임 관련 로직들을 해당 컨트롤러에 삽입
- 게임 모델, 자동차 모델에 필요한 데이터들은 model 폴더에 따로 정의
- index.js -> number.js로 변경
- pickRandomNumberBetweenZeroToNine에서 조금 더 유틸스럽게 pickNumberInRange으로 변경하고 랜덤 시작 range, 끝 range를 지정하도록 변경
- i에서 조금 더 직관적인 carName으로 변경
- 검증에 통과하지 못하면 다시 input에 focus하도록 구현
- 엔터를 눌렀을 때, 버튼을 눌렀을 때 둘 다 submit 되도록 구현
- 없어도 똑같이 동작함
- visit가 reload와 똑같게 동작함
- constructor로 이동
- try-catch를 validation안에서 하는 것이 아니라 밖에서 진행
- alert & focus를 validation 함수안에서 진행하는 것이 아니라 밖에서 진행
- CAR_NAME_SPLITTER으로 생성
- ',' 기준으로 나누고 그 뒤에 자동차 객체 생성전에 trim()으로 공백 제거
- View를 인스턴스화 시켜서 focus, 또는 render 같은 역할을 하도록 분리
- index 파일에서 View suffix를 붙이도록 하고 파일명 자체는 suffix를 삭제
- util쪽에 가깝다고 생각해서 util, 상수와 같은 라인에 배치
- RacingCarGameResult에서 RacingCarGameProgressSection으로 변경
- 자동차들을 돌면서 advance의 수를 확인해 제일 많이 나간 자동차들의 이름을 뽑는 식으로 구현
- 모든 view에 reset 메서드를 만들어서 innerHTML을 초기화
Copy link
Author

@junghyeonsu junghyeonsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review🔥

Comment on lines 105 to +109
this.progressRacingResult();
this.racingCarGameResultView.renderRacingGameResultTemplate(this.racingCarGameModel.cars);
this.racingCarGameProgressSectionView.renderRacingGameResultTemplate(
this.racingCarGameModel.cars,
);
this.racingCarGameEndView.renderEndSection(this.racingCarGameModel.winnerCars);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

시도 횟수 입력시 racingCarGameEndView에서 renderEndSection메서드로 우승자를 결정해서 렌더링하게 됩니다.

Comment on lines +128 to 134
restartGame() {
this.racingCarGameEndView.reset();
this.racingCarGameProgressSectionView.reset();
this.tryCountFormView.reset();
this.racingCarGameView.reset();
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

게임 재시작 버튼 클릭 시 호출되는 메서드입니다.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 모델쪽은 다시 리셋할 필요가 없나요?

Comment on lines +12 to +14
get advanceCount() {
return this.gameResult.filter(oneRoundResult => oneRoundResult === GAME.ADVANCE).length;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Car 인스턴스에 gameResult 가 ['advance', 'stop', 'advance'] 이런식으로 담겨져있습니다. (tryCount가 3일 때)
그래서 advanceCount 메서드를 호출하면 advance가 몇 개 인지 확인할 수 있도록 했습니다.

Comment on lines +10 to +13
get winnerCars() {
const maxAdvance = Math.max(...this.cars.map(car => car.advanceCount));
return this.cars.filter(car => car.advanceCount === maxAdvance).map(car => car.name);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RacingCarGame 인스턴스에서 winnerCars 메서드를 호출하면
각각의 Car 인스턴스에서 advanceCount 가져와서 가장 높은 advanceCount를 판단하고
가장 높은 advanceCount를 가진 Car 인스턴스를 반환하는 메서드입니다.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반환하는 이름에 맞게면 winnerCarNames가 적절하겠네요

@@ -0,0 +1,34 @@
import { DOM, GAME } from '../constants.js';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RacingCarGameResultView에서 RacingCarGameProgressSectionView으로 이름을 바꿨습니다.

Comment on lines +8 to +11
reset() {
this.$target.innerHTML = null;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

재시작 버튼 클릭 시 호출되는 메서드입니다.

Comment on lines +23 to +26
reset() {
this.$target.innerHTML = null;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

재시작 버튼 클릭 시 호출되는 메서드입니다.

Comment on lines +23 to +27
reset() {
this.$carNamesInput.value = '';
this.$carNamesInput.focus();
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

재시작 버튼 클릭 시 호출되는 메서드입니다.

Comment on lines +8 to +11
reset() {
this.$target.innerHTML = null;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

재시작 버튼 클릭 시 호출되는 메서드입니다.

@@ -0,0 +1,34 @@
import { DOM } from '../constants.js';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RacingCarGameEndSectionView 자동차 게임의 끝 부분이라서 이렇게 이름을 지었습니다.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요쪽이 result라 네이밍되어도 괜찮을 것 같긴하네요

Copy link

@jho2301 jho2301 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 현수님
적절하게 잘 적용된 것으로 보여요! 크게 이견 있는 부분이 없었어요
스텝1에서의 리뷰도 잘 적용된 것으로 보입니다
적용해주지 않은 부분도 있었는데 왜 적용하지 않았는지 코멘트를 남겨주셨으면 그걸로 얘기나눠볼 것도 있지 않을까 생각이 드네요 (스텝1에 비해 볼륨자체가 작아서 ㅎㅎ)
스텝1에서 이야기 된 것들을 제하니 마이너한 코멘트들만 남겼어요
가볍게 확인해주시고
다음 과제 진행해주시면 될 것 같습니다!
고생하셨습니다

@@ -45,6 +56,13 @@ class RacingCarGame {
}
}

onClickRacingGame(target) {
if (target === $(`#${DOM.GAME_RESTART_BUTTON_ID}`)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

querySelect를 하지않고 target.id 프로퍼티로 검증하거나,
핸들러를 직접 등록해준 것이기때문에 굳이 검증이 필요할까 싶어요

@@ -0,0 +1,34 @@
import { DOM } from '../constants.js';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요쪽이 result라 네이밍되어도 괜찮을 것 같긴하네요

Comment on lines +10 to +13
get winnerCars() {
const maxAdvance = Math.max(...this.cars.map(car => car.advanceCount));
return this.cars.filter(car => car.advanceCount === maxAdvance).map(car => car.name);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반환하는 이름에 맞게면 winnerCarNames가 적절하겠네요

Comment on lines +128 to 134
restartGame() {
this.racingCarGameEndView.reset();
this.racingCarGameProgressSectionView.reset();
this.tryCountFormView.reset();
this.racingCarGameView.reset();
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 모델쪽은 다시 리셋할 필요가 없나요?

@jho2301 jho2301 merged commit 5b8be4d into next-step:junghyeonsu Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants