-
Notifications
You must be signed in to change notification settings - Fork 214
[자동차 경주] 장예훈 미션 제출합니다. #211
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
base: main
Are you sure you want to change the base?
Conversation
|
커밋 메시지에 테스트 코드 추가는 test 머릿말을 사용하는게 더 좋을 것 같아요! |
|
전체적인 코드에서 역할 분리가 명확했으면 좋았을 것 같아요 |
|
앗 나름 분리한다고 해봤는데 부족했나봅니다! MVC패턴 참고해서 작성해보겠습니다! |
pangil5634
left a comment
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주차도 고생 많으셨습니다.
전반적인 코드를 보면서 가장 크게 느꼈던 점은 MVC 패턴과 같이 디자인 패턴을 적용해보시는 것은 어떨까 하는 바램이 듭니다.
또한, constants 폴더를 만들고 필요한 상수를 따로 분리시키는 것도 유지보수성에 있어서 효과가 있을 것 같아서, 이러한 점을 고려하여 3주차를 진행하시면 더욱 좋을 것 같습니다.
수고 많으셨습니다.
| const CAR_NAMES = await GameInput.getCarNames(); | ||
| const ROUND_COUNT = await GameInput.getRoundCount(); | ||
|
|
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.
입력 받는 부분을 하나의 함수로 분리해서, 자동차 이름 입력과 라운드 입력을 받는 하나의 입력 단계로 App.js에 구성하는 것은 어떨까요?
const [carNames, roundCount] = await initRacingGame();
async function initRacingGame() {
const carNames = await GameInput.getCarNames();
const roundCount = await GameInput.getRoundCount();
return [carNames, roundCount];
}
| const WINNERS = RACE_GAME.getWinners(); | ||
| Console.print(`최종 우승자 : ${WINNERS.join(", ")}`); |
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.
이후 로직에서 배열을 다시 사용하지 않기에, join하는 부분을 getWinners()와 함께 사용하는 것은 어떨까요?
1안
Console.print(`최종 우승자 : ${RACE_GAME.getWinners().join(", ")}`);
2안
const WINNERS = RACE_GAME.getWinners().join(", ");
Console.print(`최종 우승자 : ${WINNERS}`);
| playRound(RandomNumber) { | ||
| this.cars.forEach((car) => { | ||
| const randomNumber = makeRandomNumber(); | ||
| car.move(randomNumber); | ||
| }); | ||
| } |
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.
인자를 제거하는 것은 어떨까요?
현재 RandomNumber 매개변수를 받지만, 실제로 사용하고 있지 않는 코드로 보입니다.
이것은 혼동을 줄 수 있는 죽은 파라미터와 같아서, 제거하는 것이 깔끔해 보입니다!
혹시 인자를 추가하신 이유가 있으실까요?
| const NAME_ARRAY = carNames.split(","); | ||
|
|
||
| return NAME_ARRAY.map((name) => new CarInfo(name.trim())); |
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.
대문자 네이밍은 상수(불변, 전역 수준)에 사용하는 것이 원칙으로 알고 있습니다!
여기서는 지역 변수이므로, 소문자로 변경하는 것이 좋을듯 합니다!
No description provided.