-
Notifications
You must be signed in to change notification settings - Fork 455
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
[자동차 경주] 최재영 미션 제출합니다. #264
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
학습 목표에 있었던 큰 함수를 단일 역할을 하는 작은 함수로 분리하기 부분을 정말 많이 신경쓰신 것이 느껴지는 코드여서 인상 깊었습니다!
2주차도 수고 많으셨습니다!!
import ERROR from '../constants/errors.js'; | ||
|
||
const validateCarNames = (carNames) => { | ||
if (!Array.isArray(carNames) || carNames.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자동차 이름에서 예외 처리할 수 있는 것들이 있을 것 같습니다.
이름이 중복되는 경우 등이 있을 것 같습니다..!
import CustomError from '../constants/CustomError.js'; | ||
import ERROR from '../constants/errors.js'; | ||
|
||
const validateRoundCount = (roundCount) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
시도 횟수도 예외 처리할 수 있는 것들이 조금 더 있을 같습니다.
시도 횟수가 소수일 경우 등이 있을 것 같습니다..!
import ERROR from '../constants/errors.js'; | ||
|
||
const validateRoundCount = (roundCount) => { | ||
if (roundCount === undefined || roundCount === null || roundCount === '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같이 줄여서 표현할 수 있을 것 같습니다!
if (roundCount === undefined || roundCount === null || roundCount === '') { // before
if (!roundCount) { // after
|
||
function executeRaceRounds(race) { | ||
OutputView.printStart(); | ||
for (let i = 0; i < race.getRounds(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javascript style guide에서 for문보다 고급 함수를 사용하라고 하는 글이 있어서 혹시 몰라 첨부드립니다..!
하지만, 저를 리뷰해주신 분 중에 성능면에서 떨어질 수 있다고 피드백해주셔서 이 부분은 한 번 확인해보는 것이 좋을 듯합니다.🥹
이터레이터 스타일 가이드 보러가기
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 iterable 객체의 item을 순회하거나, 수정하는 사항이 아니고, 재영님 코드와 같이 반복하는 횟수가 명확히 정해진 경우에는 for문이 더 바람직하다고 생각합니다!
for문보다 고급함수를 더 우선적으로 고려해서 사용할 이유는 없다고 생각해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이부분은 현재터럼 for문으로 순회하는게 더맞아보이네요. 배열의 아이템을 순회하고 있는게 아니라서 이런식으로 순회하느게 더 적합하지 않나 싶어요.
class RacingCar extends Car { | ||
move() { | ||
const randomValue = generateRandomNumber(); | ||
if (randomValue >= 4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 피드백 받은 사항 중 하나였습니다.
4라는 숫자가 프로젝트 사이즈가 커질수록 어디서 나온 숫자인지 알 수가 없는 매직 넘버 이슈가 생길 수도 있을 것 같다는 피드백을 받았습니다!ㅎㅎ
const MOVE_THRESHOLD = 4;
if (randomValue >= MOVE_THRESHOLD) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조금더, 관심사를 분리하고, 코드를 모듈화 했으면 좋겠습니다. 또한, 피드백을 보시고, 시간이 나시면 그것을 이번주차 코드에 적용해보세요! 2주차도 수고하셨습니다.
mockQuestions(inputs); | ||
mockRandoms([4, 3, 4, 4]); | ||
|
||
const mockRace = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 깊이가 3이 넘지 않게 분리 해보는것이 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드 작성이 익숙하지 않아, 실수가 많았네요..!
더 공부하고, 깊이가 3 이상이 되지 않도록 개선해보겠습니다:)
ERROR.NAN_ROUND_COUNT.name, | ||
); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 depth를 줄이는 방식을 조금더 고려해보았으면 해요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
흠.... 마빈님 코드를 보면 인자를 하나로 줄여서 depth를 줄이셨는데, 그렇게 작성하는 것이 컨벤션을 더 준수할 수 있는 방식이겠네요! 감사합니다🙇🏻🙇🏻
|
||
const getRoundCount = async () => { | ||
const countInput = await InputView.readInput('시도할 횟수는 몇 회인가요?'); | ||
const roundCount = parseInt(countInput, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1000000000000000000000000를 입력하면 어떻게 될까요? 확인해보세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
너무 긴 라운드가 진행되네요!
시간제한을 적용하거나, 최대 숫자를 지정했으면 더 좋았을 것 같네요😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
재영님 2주차 과제도 너무 고생 많으셨습니다! 저번 주차보다 훨씬 더 코드가 잘 분리되어 있다는 느낌을 많이 받았습니다! 피드백에 대한 빠른 수용 멋집니당 👍
이미 다른분들이 좋은 피드백을 남겨주셔서 저는 많은 피드백을 드리지는 못했고 간단하게 몇가지 부분에 대해서만 제 생각을 공유해 보았습니다!
다음 주차 과제에선 커밋 메세지도 조금 더 구체적으로 적어보면 어떨까요? 지금의 커밋 메세지는 재영님이 어떤 작업을 하셨는지 파악하기엔 조금 부족하다고 생각이 들어요 재영님이 작성해주신 코드가 어떤 의도로 어떻게 작성되었는지, 수정되었다면 어느 부분이 수정되었는지 커밋 메세지에 구체적으로 남겨주시면 리뷰어 분들이나 미래의 재영님 스스로에게도 많은 도움이 될 거라 생각해요
2주차도 고생 많으셨습니다!
EMPTY_CAR_NAMES: { | ||
name: 'EmptyCarNamesError', | ||
message: `${ERROR_TAG} 최소 하나 이상의 자동차 이름을 입력해야 합니다.`, | ||
}, | ||
INVALID_CAR_NAMES: { | ||
name: 'InvalidCarNamesError', | ||
message: `${ERROR_TAG} 자동차 이름은 5자 이하의 문자열이어야 합니다.`, | ||
}, | ||
|
||
EMPTY_ROUND_COUNT: { | ||
name: 'EmptyRoundCountError', | ||
message: `${ERROR_TAG} 시도 횟수를 입력해야 합니다.`, | ||
}, | ||
NAN_ROUND_COUNT: { | ||
name: 'NaNRoundCountError', | ||
message: `${ERROR_TAG} 시도 횟수는 숫자여야 합니다.`, | ||
}, | ||
INVALID_ROUND_COUNT: { | ||
name: 'InvalidRoundCountError', | ||
message: `${ERROR_TAG} 시도 횟수는 1 이상의 정수여야 합니다.`, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다양한 상황을 상정해서 ERROR 메세지를 관리하시는 건 좋은 방법이라고 생각해요 👍
이름이 조금 더 명확해지면 무슨 에러인지 이해하기 쉽고 유지 보수하기도 좋을 것 같아요!
예를들어 INVALID_CAR_NAMES
만 보면 다양한 이유가 생각날 것 같아요 이름에 들어가면 안되는 특수문자라던가, 중복되는 닉네임일 수도 있을 것 같아요 지금 INVALID_CAR_NAMES
는 자동차 이름 길이에 대한 에러 메세지이니 INVALID_CAR_NAME_LENGTH
같은 이름은 어떨까요?
import Race from './Race.js'; | ||
|
||
class RacingGame extends Race { | ||
runRound() { | ||
this.cars.forEach((car) => car.move()); | ||
} | ||
} | ||
|
||
export default RacingGame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race 클래스를 상속받아 새로운 클래스를 만들고 확장하는 방법은 좋은 선택이 될 수 있다고 생각합니다!
다만 조금 아쉬운 부분은 RacingGame이 Race를 상속받아서 차별화 된 로직이 추가되거나 메서드가 바뀌는 점이 크게 보이지 않는다는 점이에요 😢
지금의 경우는 직접 race에 구현하는 것도 괜찮은 방법이라고 생각되지만 race 클래스의 확장까지 고려하고 있다면 RacingGame이 어떤 차별점을 가지고 있어야할지도 고려해보는게 좋을 것 같아용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race는 경주를 위해 필요한 차와 라운드 수를 받아오고, RacingGame에서는 실제 라운드를 진행하도록 나누면 좋겠다고 생각을 했는데, 제가 객체지향적으로 코드를 작성하는데 익숙하지 않고, 단일 책임의 원칙을 잘못 이해하여 응집성이 높은 코드를 작성하지 못한 것 같습니다!
작성해주신 리뷰 새겨듣고, 잘 개선해보도록 하겠습니다!
const carNames = carInput | ||
.split(',') | ||
.map((name) => name.trim()) | ||
.filter((name) => name.length > 0); | ||
|
||
validateCarNames(carNames); | ||
return carNames; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만약에 유저가 단 한명의 플레이어만 입력했다던가 공란을 입력했다 하면 어떻게 처리될까요? 또 다른 에러가 발생할 수 있는 지점은 어떤게 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요, Jay님!
다른 분들이 이미 리뷰를 많이 남겨주셔서 제가 남겨드릴 리뷰가 딱히 없는 것 같네요.. 그래서 커밋 메시지까지 확인해보다가 fix: 함수명으로 파일명 변경 메시지를 봤는데 fix 타입은 버그, 에러 수정에 해당해서 저는 chore가 더 적합하다 생각하는데, 재영님은 이 부분에 대해 어떻게 생각하시는지 궁금하네요!
2주차도 수고 많으셨습니다! 3주차도 화이팅이에요!
|
||
function executeRaceRounds(race) { | ||
OutputView.printStart(); | ||
for (let i = 0; i < race.getRounds(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ 기능을 사용하시고 싶은 거라면 i++
보다 i += 1
을 사용하라고 Airbnb JavaScript Style Guide에 포함되어 있어요! 관련 문서는 링크로 남겨두겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안그래도 다른 분들 코드에서는 다 i += 1 코드를 사용하더라구요! 새겨두겠습니다:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2주차 수고하셨습니다! 클래스의 상속을 잘 활용한 코드 인상적이었습니다!
시간 괜찮으시면 제 PR 코드리뷰 도 부탁드리겠습니다!
}); | ||
}); | ||
|
||
describe('주요 기능', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
describe 을 좀 더 세분화하면 좋을 것 같아요!
예외 테스트라던가 함수별로 테스트를 해본다던가 하는 식으로요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test 코드 작성이 처음인데, 다른 분들 테스트 코드에서 정말 많이 배울 수 있었습니다!
3주차 테스트 코드에서는 많이 적용해보도록 하겠습니다 :)
async run() {} | ||
async run() { | ||
try { | ||
const { carNames, rounds } = await initializeRace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여러 개의 입력을 받을 경우에 이렇게 시작하는 거 가독성도 너무 좋네요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 파일을 constants
폴더에 위치시킨 이유가 궁금하네요!
@@ -0,0 +1,27 @@ | |||
const ERROR_TAG = '[ERROR]'; | |||
|
|||
const ERROR = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수인만큼 Object.freeze()
를 사용해주면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번에 코드 리뷰를 하면서 여러 개발자분들이 사용하신 함수였는데, 3주차부터는 해당 메서드를 공부해서 사용해보려고 합니다! 감사합니다:)
|
||
function executeRaceRounds(race) { | ||
OutputView.printStart(); | ||
for (let i = 0; i < race.getRounds(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 iterable 객체의 item을 순회하거나, 수정하는 사항이 아니고, 재영님 코드와 같이 반복하는 횟수가 명확히 정해진 경우에는 for문이 더 바람직하다고 생각합니다!
for문보다 고급함수를 더 우선적으로 고려해서 사용할 이유는 없다고 생각해요!
} | ||
|
||
getCars() { | ||
return this.cars; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.cars
라는 필드가 private으로 캡슐화된 필드가 아닌데 왜 getCars()
메서드를 구현하셨는지 이유가 궁금하네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 재영님, 우아한테크코스 6기 코레아 팀의 FE 초코입니다. 🍫
서비스 이용해 주셔서 감사합니다 :>
이미 다른 분들이 리뷰를 많이 남겨주셔서 가볍게 살펴보았습니다!
다만, 저는 코드리뷰를 진행할 때 커밋 내역을 따라 작업 내용을 살펴보는데요, 기능적으로 다른 부분을 구현하고 있음에도 커밋 메시지가 동일하게 작성되어 있는 부분이 있어 약간의 혼란을 주었어요. 각 커밋이 담고 있는 의도가 더 명확히 드러나도록 메시지를 구분해주시면 좋을 것 같습니다! 🙂
다음 미션들도 화이팅하세요!! 👍 👍
+) 코드리뷰를 요청할 때는 원하는 포인트들을 PR 본문에 언급해보세요. 더 유의미한 피드백을 받을 수 있을거에요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 코드를 보는 입장에서 테스트 코드만으로도 그 기능을 파악할 수 있어야 한다고 생각하는 편이에요. 테스트 케이스를 Given-When-Then 형식으로 다음과 같이 더 명확하게 작성하는 것은 어떤가요?
ex.
test('쉼표로 구분된 자동차 이름들이 주어졌다면, 유효한 이름 배열을 반환한다'
test('자동차 이름이 5자를 초과하면, 에러를 발생시킨다'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 요구사항은 단순히 랜덤 값에 따라 전진하는 기능만 필요해요. 별다른 요구사항이 존재하지 않는 상황에서 클래스가 분리되어 있으니 여러 파일을 봐야만 한다는 복잡성이 증가할 수 도 있을 것 같아요!
Car 클래스 안에 RacingCar 클래스의 move 메서드를 포함하는 방법은 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 마찬가지로 Race 클래스와 RacingGame 클래스를 통합해볼 수 있을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 재영님!:) 이리저리추상화를 고민하시면서 코드들을 진행하신것 같아보여서 재미있는 코드였어요. Controller가 딱히 내부상태를 지니지 않아서 메소드들을 독립된 파읻들로 나눠주신 것으로 보았는데, 이게 맞다면, 각 파일을 controllers/index.js 에서 병합해서 Controller객체로 내보내주시면 더 좋지 않았을까 하는생각이 조금 있습니다!:)
(물론 취향차이지만요!)
2주차 과제도 고생하셨구 3주차도 화이팅이에요!:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이 파일은 src/errors 로 움직이는게 더 좋지 않을까 싶어요!
function createRace(carNames, rounds) { | ||
const cars = carNames.map((name) => new RacingCar(name)); | ||
return new RacingGame(cars, rounds); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메소드가 팩토리 패턴같이 보여서 이런 RaceGame.create() 이렇게 추상화 한다거나 혹은 RaaceFactory.create() 와 같이 추상화를 통해서 명확하게 어떤 패턴인지 드러내주면 좋겠다는 생각이 들어요! 😄
|
||
function executeRaceRounds(race) { | ||
OutputView.printStart(); | ||
for (let i = 0; i < race.getRounds(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이부분은 현재터럼 for문으로 순회하는게 더맞아보이네요. 배열의 아이템을 순회하고 있는게 아니라서 이런식으로 순회하느게 더 적합하지 않나 싶어요.
import validateCarNames from './validateCarNames.js'; | ||
|
||
const getCarNames = async () => { | ||
const carInput = await InputView.readInput( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util함수가 Inputview에 의존해도 괜찮을까요? 재영님 생각은 어떠신지 궁금해요!
@@ -0,0 +1,5 @@ | |||
import { Random } from '@woowacourse/mission-utils'; | |||
|
|||
const generateRandomNumber = () => Random.pickNumberInRange(0, 9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
랜덤넘버의 생성법을함수로 잘 랲핑해서 가려주신 것 같아요! 다만 여기 등장하는 0과 9는 어떤 의미를 지닌 숫자일지 고려하시구 상수로 분리하시면 더 좋지 않을까 싶단 생각이 들어요!:)
} | ||
|
||
carNames.forEach((name) => { | ||
if (name.length > 5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서도 마찬가지로 5라는 숫자가 어떤 걸 의미하는지 좀더 고려하셔서 상수를분리해주셨다면 좋았을것 같아요!:)
); | ||
} | ||
|
||
if (roundCount < 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기의 1도 상수로 분리해낼 수 있지않을까요?!:)
@@ -0,0 +1,16 @@ | |||
class Race { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 Race클래스를 RacingGame의 인터페이스이자, 클래스로 사용하신것 같아보여요.하지만, RacingGame의 runRound는 RacingGame이라는 구상클래스에서 만 필요한 메소드일까요? 아니면 Race라는 추상클래스가 지니고 자식클래스들이 오버라이딩 해서 쓰는게 맞을지 고민해보시면 좋을것 같아요. 재영님은 어떻게 생각하시나요?:)
미션 - 자동차 경주
🪐 구현 기능
1. 자동차 이름 입력 기능
,
)로 구분하여 자동차 이름을 추출합니다."pobi,woni,jun"
➔["pobi", "woni", "jun"]
2. 시도할 횟수 입력 기능
3. 자동차 전진 여부 결정 기능
4. 라운드 결과 출력 기능
-
기호로 표시하여 출력합니다.5. 최종 우승자 출력 기능
에러 처리 케이스
EMPTY_CAR_NAMES: 최소 하나 이상의 자동차 이름이 필요합니다.
INVALID_CAR_NAMES: 자동차 이름은 5자 이하의 문자열이어야 합니다.
EMPTY_ROUND_COUNT: 시도 횟수를 입력하지 않은 경우 발생합니다.
NAN_ROUND_COUNT: 시도 횟수가 숫자가 아닐 때 발생합니다.
INVALID_ROUND_COUNT: 시도 횟수가 1 이상의 정수가 아닐 경우 발생합니다.