-
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
[자동차 경주] 배지아 미션 제출합니다. #353
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주차 고생하셨고 잘 하셨습니다.
전체적으로 코드가 깔끔하고 읽기 쉽게 된 것 같아요.
예외 테스트 할 때 Jest도 사용하는 것을 추천합니다. Jest 사용법도 생각보다 크게 어렵지 않고 배우면 유용하다고 생각합니다.
import { | ||
getCarNames, | ||
checkForDuplicates, | ||
hasInvalidCarNameLength, | ||
getTryCount, | ||
isPositiveInteger, | ||
startRace, | ||
printWinners, | ||
} from "./utils/index.js"; |
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
말고 index.js
로 따로 분리해서 선언했는 데 더 좋은 이점이 있는지 궁금합니다.
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.
utils/index.js에 유틸리티 함수들을 모아 두면, 새로운 유틸리티 파일을 추가하거나 기존 유틸리티 파일을 수정하는 좀 더 용이할 것 같아 그렇게 하였습니다!
export const getCarNames = async () => { | ||
const carName = await Console.readLineAsync( | ||
"경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)\n" | ||
); | ||
return carName.split(",").map((carName) => carName.trim()); | ||
}; | ||
|
||
export const getTryCount = async () => { | ||
const tryCount = await Console.readLineAsync("시도할 횟수는 몇 회인가요?\n"); | ||
return Number(tryCount); | ||
}; |
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.
다음 과제부터는 출력 메시지도 따로 상수로 빼놓아 보아야겠어요🤗
export const CARNAME_MIN_LENGTH = 1; | ||
export const CARNAME_MAX_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.
자동차 이름의 길이 제한을 상수화해서 따로 분리하건 건 추후 유지보수 측면에서 좋을 것 같아요!
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주차 미션 하시느라 고생 많으셨습니다!
고차함수의 처리나 export 처리 등 생각을 많이 하시면서 하셨다는게 느껴지는 코드였어요!
따로 패턴을 사용하시진 않으셨는데 혹시 이유가 있으신가요? (저도 사용하진 않았습니다..ㅎㅎ)
시간있으실때 맞리뷰 해주시면 감사하겠습니다!!
리뷰 링크
export function startRace(cars, tryCount) { | ||
for (let i = 0; i < tryCount; i++) { | ||
cars.forEach((car) => car.move()); | ||
printRaceStatus(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.
저도 이번에 조언을 받은 내용인데 utils라는 파일에는 유틸함수들이 들어가는데 유틸함수를 정의하는 조건이 다음과 같다고 해요.
- 순수 함수여야 한다 (같은 입력에 항상 같은 출력)
- 외부 상태에 의존하지 않는다
- 특정 도메인에 종속되지 않고 범용적으로 사용 가능하다
정의 해주신 startRace를 보면 같은 입력(cars, tryCount)이어도 car.move()의 랜덤 결과에 따라 다른 결과가 나오는 것을 알 수 있습니다. 이는 1번 조건에 위반 되기 때문에 적합하지 않다 하더라구요.
App이 흐름을 관리하기 때문에 매서드가 아니라 유틸함수로 빼신것 같은데 한 번 utils에 파일을 넣기전 생각해보시면 좋을 것 같아요!
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.
그러네요.. 제가 알지 못한 부분 집어주셔서 감사합니다! 다음 과제에는 더 유의해서 작성해볼게요.
유틸함수 조건에 대해 찾아보고 작성한 함수가 Utils 파일에 들어갈 수 있는지 자세히 들여다 봐야 할 것 같습니다😊
for (let i = 0; i < tryCount; i++) { | ||
cars.forEach((car) => car.move()); | ||
printRaceStatus(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.
대부분 고차함수로 잘 작성해주신 것 같은데 다음 부분도 다음과 같이 적을 수 있을 것 같아요!
(저도 상수를 이렇게 쓸 수 있는지 몰랐습니다..ㅎㅎ)
Array.from({ length: tryCount }).forEach(() => {
cars.forEach((car) => car.move());
printRaceStatus(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.
그러네요 이 부분을 살펴보지 못했네요 알려주셔서 감사합니다!!
move() { | ||
const randomValue = Random.pickNumberInRange(0, 9); | ||
if (randomValue >= 4) this.position += 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.
사실 미션을 수행하는데 문제가 되는 부분은 아닌데 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.
넵 매직 넘버로 대체하는게 코드가 더 일관성 있어 보일 것 같네요!!
리뷰해주셔서 감사합니다! 저도 맞리뷰하러 가겠습니다😊 |
리뷰 달아주셔서 감사합니다!! Jest 사용법도 한번 공부해볼게요 😁😁 |
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주 차 미션 수행하느라 고생 많으셨습니다!
3주 차도 파이팅입니다!! 🔥🔥🔥
제 PR도 한 번 봐주시면 감사하겠습니다~
1. [x] 자동차 이름 입력받기 | ||
|
||
- [x] 쉼표 기준으로 나누기 | ||
- [x] 쉼표 기준으로 앞, 뒤 공백 제거 | ||
|
||
2. [x] 자동차 이름 예외 처리 | ||
|
||
- [x] 자동차 이름 중복 예외 처리 | ||
- [x] 자동차 이름 1자 이상 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.
기능 구현 목록과 예외 처리 목록을 분리하는 건 어떨까요?
2주 차 미션은 기능이 많지 않아 분간하는 데 어렵지 않으나, 항목이 많아질수록 어려울 것 같아서요!
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.
네.. 항목이 많아질 수록 기능을 파악하기가 어려울 것 같네요.
기능목록이랑 예외사항, 제약사항 등을 따로 구분해놔야겠어요
const carName = await Console.readLineAsync( | ||
"경주할 자동차 이름을 입력하세요.(이름은 쉼표(,) 기준으로 구분)\n" | ||
); | ||
return carName.split(",").map((carName) => carName.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.
trim()
메서드를 사용해서 pobi, woni, jun
과 같이 입력되었을 때,
['pobi','woni','jun']
으로 알맞게 배열에 할당되도록 한 것이죠?!
JS API를 잘 사용하셔서 코드가 가독성이 좋네요 👍
export function printRaceStatus(cars) { | ||
cars.forEach((car) => { | ||
Console.print(`${car.name} : ${"-".repeat(car.position)}`); | ||
}); | ||
Console.print(""); | ||
} |
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.
-
미션에 대한 맥락을 알면 해당
Console.print("");
코드가 의미하는 바를 단번에 알 수 있습니다.
그러나 미션 출력 요구사항을 완벽하게(?) 알지 못 하고, 코드를 볼 경우 의아할 수 있다고 생각해요. 그래서 주관적인 판단으로는..
Console.print("");
를 새로운 메서드로 분리하여 네이밍을 부여하거나, 주석을 추가하여 어떤 이유로 공백을 출력하는 것이 있는지 설명하면 더 좋을 것 같아요. -
${"-".repeat(car.position)}
해당 부분에서 "-" 또한, Constants로 분리되면 더 좋을 것 같습니다!
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로 분리하는 것이 더 맞는 표현인 것 같습니다. 지적해주셔서 감사해요😊
import { printRaceStatus } from "./console.js"; | ||
|
||
export function startRace(cars, tryCount) { | ||
for (let i = 0; i < tryCount; 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.
Airbnb JS Convention에 따르면 i++ 보다 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.
자바스크립트로 처음 코드를 작성하다 보니 세세한 부분들을 놓칠 때가 많네요..😭
이 부분도 고차 함수로 작성했으면 나았을텐데 싶기도 해요
링크까지 첨부해주셔서 감사합니다..!
const createError = (message) => { | ||
throw new Error(message); | ||
}; | ||
|
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.
createError()
로 묶여 있으니 가독성이 훨씬 더 좋네요 👍👍
|
||
// 자동차 이름 길이 수 체크 | ||
export const isValidLength = (target, min, max) => { | ||
return min <= target.length && target.length <= max; |
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.
이 경우는 함수가 무척 짧아서 그냥 넘어가도 괜찮지만,
논리연산자가 들어가면 코드가 직관적으로 해석되지 않을 때가 있어서
헬퍼 함수를 쓰는 것도 좋은 방법일 것 같습니다!
const inRange = (value, min, max) => min <= valud && value <= max;
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.
코드의 가독성 측면에서 민정님께서 제시해준 방안이 더 좋네요!
const inRange = (value, min, max) => min <= value && value <= max;
export const isValidLength = (target, min, max) => {
return inRange(target.length, min, max);
};
제안대로 수정한 코드가 이전 코드보다 의미 전달도 확실하고 잘 읽히네요 🤗
@@ -0,0 +1,29 @@ | |||
import { Console } from "@woowacourse/mission-utils"; |
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주차 과제 정말 수고 많으셨습니다.
파일 분리도 잘하시고, 설계도 잘하셔서 정말 많이 배워갑니다.
특히 이번 3주차 과제에서 엄청 난항을 겪고있는데
이렇게 연계되는 코드를 보니 참고가 정말 많이 되네요.
이번 과제도 응원하겠습니다!👍👍
넵! 커밋 메시지도 통일성 있게 맞춰보겠습니다! 상세한 피드백 남겨주셔서 감사합니다. 다음 미션도 파이팅해보아요!! 👍👍 |
리뷰 달아주셔서 감사합니다! 저도 맞리뷰 달았습니다! 다음 주차 과제도 파이팅해보아요~😁 |
No description provided.