-
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
[ 자동차 경주 ] 이진 미션 제출합니다. #455
base: main
Are you sure you want to change the base?
Conversation
과제의 요구사항을 분석한 README를 추가했습니다. `시작하기`, `입력받기`, `출력하기`, `게임하기`, `입력값 예외 처리하기` 의 단계로 나누어 각 단계 별로 구현해야할 기능을 명시하였습니다.
입출력 메시지 관련 상수, 에러 메시지 관련 상수를 추가했습니다.
- Console API를 사용하여 한 줄의 입력을 받는 함수를 구현했습니다. - Console API를 사용하여 입력받은 값을 출력하는 함수를 구현했습니다.
- 이전: `input` 변경 `readUserInput` 입력의 의미보다는 사용자의 입력을 읽는 함수이기 때문에 함수의 기능을 명확하게 표현하기 위해 변경했습니다. - 이전: 'output' 변경: `printOutput` 단순한 출력의 의미보다는 결괏값을 Console에 print 하는 함수이기 때문에 함수의 기능을 명확하게 표현하기 위해 변경했습니다.
- `readUserInput` 함수를 사용하여 사용자로부터 자동차 이름을 입력받는 기능을 구현했습니다. - `MESSAGE.PROMPT_NAME_USER_INPUT` 상수를 통해 입력을 안내했습니다. - 입력받은 값을 `input` 변수에 저장하여 이후 로직에서 활용 가능하도록 구현했습니다.
- `readUserInput` 함수를 사용하여 사용자로부터 시도 횟수를 입력받는 기능을 구현했습니다. - `MESSAGE.PROMPT_COUNT_USER_INPUT` 상수를 통해 입력을 안내했습니다. - 입력받은 값을 attemptCount 변수에 저장하여 이후 로직에서 활용 가능하도록 구현했습니다.
- `splitByComma` 함수를 통해 문자열을 쉼표로 구분하여 배열로 반환하였습니다.
- `MissionUtils.Random.pickNumberInRange` 메서드를 사용하였습니다. - `startRange` 와 `endRange` 를 통해 숫자 범위를 입력받아 해당 범위 내에서 랜덤한 숫자를 생성하였습니다.
- `pickRandomNumberInRange` 함수의 기능을 검증하는 테스트 코드를 작성했습니다. - `Random.pickNumberInRange` 메서드를 `jest.spyOn`으로 모의 처리하여 테스트의 일관성을 유지하였습니다. - 0-9 사이의 범위에서 반환되는 값이 지정된 범위 내에 있는지 테스트하였습니다 - mock을 사용하여 설정한 중간값이 반환되는지 확인하는 테스트를 추가하였습니다. - 테스트 전후로 mock을 설정 및 복원하여 다른 테스트에 영향을 주지 않도록 처리하였습니다.
- `Simulator` 클래스 생성자로 이름 배열을 받아 초기화하도록 구현하였습니다. - `names` 배열을 사용하여 각 이름을 키로 하고 값을 0으로 초기화한 객체 배열 생성하였습니다. - `state` 의 key는 자동차의 이름을 의미하고, value는 해당 자동차의 전진 척도를 의미합니다.
- 함수 이름과 파일 이름을 통일하여 가독성 및 코드 컨벤션을 준수하였습니다.
- 클래스 내에서만 사용 가능한 `#canMoveForward()` 메서드를 추가하였습니다. - `#`으로 private 메서드로 정의하여 캡슐화하고 외부에서 접근 불가하도록 설계하여 내부 로직을 보호했습니다. - `pickRandomNumberInRange` 함수를 사용해 0-9 범위의 랜덤 값이 4보다 큰 지를 확인하여 전진 가능 여부를 판단했습니다.
- `simulate` 메서드를 구현하여 각 자동차의 전진 여부를 결정하고 상태를 업데이트 하였습니다. - 전진 시 `count` 를 증가시키도록 설정하였습니다. - `attemptCount`만큼 시뮬레이션을 반복 실행하는 로직을 추가하여, 사용자가 입력한 횟수만큼 시뮬레이션을 진행하도록 하였습니다.
- `printSimulate` 함수 구현하여 자동차 이름과 전진 상태를 '-'로 시각적으로 출력하였습니다. - 기존의 `simulate` 메서드에서 각 자동차의 전진 여부를 결정한 후, `printSimulate` 함수를 호출해 현재 상태를 출력하는 부분을 추가하였습니다. - 현재 상태를 모두 출력한 후 공백을 출력하여 과제의 출력 요구 사항을 충족시켰습니다.
- `Simulator` 클래스의 인스턴스를 생성할 때 `new` 키워드를 사용하도록 수정했습니다.
- `Simulator` 클래스의 기능을 검증하는 테스트 코드를 작성했습니다. - name, count 초기화 확인하기 - `names` 배열을 통해 초기 상태가 설정되고, 실행 메시지가 출력되는지 검증하였습니다. - `#canMoveForward` 가 true일 때 상태 업데이트를 확인하기 - `pickRandomNumberInRange`를 mock 처리하여 전진 조건을 만족하는 경우를 검증하였습니다. - 전진 시 `count` 가 증가하고, `printOutput`으로 올바른 결과가 출력되는지 확인하였습니다. - `#canMoveForward` 가 false일 때 상태 업데이트를 확인하기 - `pickRandomNumberInRange`를 mock 처리하여 전진 조건을 만족하지 않는 경우를 검증하였습니다. - 전진하지 않으면 `count`가 유지되고, `printOutput`으로 올바른 결과가 출력되는지 확인하였습니다. - 자동차가 여러 개일 때 상태 업데이트 확인하기 - `mockReturnValueOnce`를 사용해 각 자동차에 대해 다른 전진 조건을 설정하고, 결과를 검증하였습니다. - 여러 자동차의 상태를 확인하여 시뮬레이션 결과가 일관되게 출력되는지 검증하였습니다.
- expect의 `toEqual`을 `toStrictEqual`로 변경했습니다. - 객체의 구조와 값뿐만 아니라 객체의 형까지 엄격하게 비교하도록 수정하였습니다. - 테스트의 신뢰성을 높이고, 객체의 구조적 일관성을 더 정확하게 검증하기 위해 변경했습니다.
- 이전: `{ [name]: count }` - 변경: `{ name, count }` - `simulate` 메서드와 `test` 코드의 상태 반환 형식을 맞추었습니다.
- `Simulator` 클래스의 `state`를 private 필드 `#state` 로 변경했습니다. - 외부에서 직접 `state`에 접근할 수 없도록 하여 캡슐화를 강화했습니다.
- `printWinner` 메서드를 통해 자동차 경주 종료 시 가장 많이 이동한 우승자들을 출력하였습니다. - 최대 이동 횟수를 계산하고, 해당 이동 횟수를 가진 자동차들을 필터링하여 우승자 이름만 추출하였습니다. - 우승자들의 이름을 쉼표`.` 로 구분하여 출력 형식에 맞췄습니다.
- `Simulator` 클래스의 `printWinner` 메서드에 대한 테스트를 추가했습니다. - 우승자가 한 명일 경우, 여러 명일 경우, 아무도 전진하지 않았을 경우에 대한 시나리오별로 테스트를 작성했습니다. - `describe`로 테스트를 그룹화하여 `simulate`와 `printWinner` 테스트를 분리하였습니다. - `state`를 `private` 필드로 변경함에 따라, 외부에서 직접 `state`에 접근하는 테스트 코드를 삭제했습니다. - 상태 변경 검증은 `printOutput` 호출 여부를 통해 간접적으로 확인할 수 있습니다.
- `pickRandomNumberInRange` 호출 시 하드코딩된 숫자(0, 9)를 `RANGE` 상수로 대체했습니다. - `RANGE.START` , `RANGE.END` , `RANGE.VALID` 상수를 사용하여 코드 가독성 및 유지보수성을 향상 시켰습니다.
- validate 메서드를 통해 입력값 검증 기능을 추가할 예정입니다.
- `Validator` 클래스에 시도 횟수가 양수가 아닐 경우 오류를 던지는 로직을 추가했습니다. - `#validatePositiveNumbers` 메서드에서 시도 횟수가 0 이하일 경우 Error를 던지도록 구현했습니다. - `validate` 메서드를 수정하여 외부에서 인스턴스를 생성하지 않고도 private 필드를 사용할 수 있도록 하였습니다.
- `#validateNoDuplicates` 메서드를 추가해 배열 내 중복 요소를 검사하도록 구현했습니다. - Set을 사용해 중복을 확인하고, 중복이 있을 경우 Error를 던지도록 처리했습니다. - `validate` 메서드에서 배열 타입의 값에 대해 #validateNoDuplicates 메서드를 호출하여 중복 검사를 수행했습니다.
- `#validateMaxLength `메서드를 추가해 배열 내 각 문자열의 길이를 검사하도록 구현했습니다. - 각 문자열의 길이가 `MAX_INPUT_LENGTH` 를 초과할 경우 Error를 던지도록 처리했습니다. - `MAX_INPUT_LENGTH` 상수를 5로 설정하여 최대 글자 수 제한을 정의하였습니다.
- #validateNotEmpty 메서드를 추가해 입력값이 비어 있는지 검사하도록 구현했습니다.
- `__test__` 폴더 내에 다른 테스트 파일명들과 일관성을 유지하기 위해 변경했습니다.
- `App` 클래스의 `run` 메서드에서 발생 가능한 다양한 예외 상황에 대한 테스트 코드를 추가했습니다. - `test.each` 를 사용해 빈 입력값, 중복된 이름, 이름 길이 초과, 시도 횟수의 유효성 검사 등의 케이스를 검증했습니다. - `Validator` 의 예외 메시지와 일치하는지 확인하여 입력값에 대한 유효성 검증 로직이 제대로 동작하는지 확인하였습니다.
- 기존: `input` 을 `split` 한 부분만 유효성 검사 - 변경: `input`유효성 검사 후 `split` - `splitByComma` 로 자동차 이름을 나누기 전에, 입력값 자체가 유효한지 검증이 필요했습니다. - 그렇지 않으면, `split` 메서드 사용에서 에러가 발생했습니다.
- 이전: object 일 때만 빈 값 검증 - 변경: type에 상관없이 빈 값 검증 - validate 메서드에서 `#validateNotEmpty()` 를 모든 타입의 값에 대해 우선적으로 검증하도록 수정했습니다. - 빈 값 검증을 가장 먼저 수행하여 이후의 유효성 검사 로직이 실행되기 전에 입력값의 존재를 확인할 수 있게 됐습니다. - 숫자 0이 빈 값으로 검증되지 않게 하기 위해 조건문을 추가했습니다.
- 시도 횟수에 따른 시뮬레이션 반복 로직을 `App.run()`에서 `Simulator`의 `simulate` 메서드로 이동시켰습니다. - 시뮬레이션 횟수와 상태 업데이트가 동일한 관심사라고 판단했기 때문입니다. - 코드의 응집도를 높이고, `App.run()`의 책임을 줄여 가독성 및 유지보수성을 개선시켰습니다.
- `#canMoveForward` 메서드를 간결하게 리팩토링했습니다. - 불필요한 if-else 구조를 제거하고, 조건문을 직접 반환하는 방식으로 코드를 간소화했습니다.
- `this.#state` 초기화 시 `reduce` 대신 `map`을 사용하여 간결하게 수정했습니다.
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.
저와 다르게 클래스형으로 구성하셔서 왜 클래스로 만들었을까를 생각해보며 읽으니 흥미로웠습니다! 테스트 코드를 유연하게 필요한 곳에서 작성하신 것도 좋았고, 전체적으로 코드를 너무 과하게 쪼갠것도 아니고 그렇다고 종속성이 너무 심한 느낌도 안들고 분리를 적절히 하신 것 같아요 :) 덕분에 배워갑니다. 고생하셨어요~!
|
||
// then | ||
await expect(app.run()).rejects.toThrow("[ERROR]"); | ||
await expect(app.run()).rejects.toThrow(expectedError); |
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.
저는 예외 테스트 시 던지는 오류는 커스텀하지 않았는데 여기서도 오류에 맞는 메시지를 띄울 수 있게 설정하신게 좋은 아이디어 같아요! 다만, 어떤 테스트케이스인지 쉽게 알 수가 없어서 .each()
내부에 각각 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.
좋은 피드백 감사합니다! 다음번에는 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.
아 이렇게 ApplicationTest 등 따로 파일을 만들어서 테스트를 할 수도 있군요..!
저는 몰랐는데 이번 과제부터 이렇게 적용해봐야겠어요! 🔥
const endRange = 8; | ||
const result = pickRandomNumberInRange(startRange, endRange); | ||
expect(result).toBe(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.
어떤 경우에도 일관적인 테스트 결과를 유지하기 위해 spyon
사용해서 pickRandomNumberInRange
에서 중간값이 뽑히도록 설정해두었기 때문입니다!
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.
아 넵 이해하였습니다! 답변 감사합니다 :)
expect(printOutput).toHaveBeenCalledWith('Car2 : '); | ||
expect(printOutput).toHaveBeenCalledWith(''); | ||
}); | ||
}); |
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.
좋은 영향을 드렸다니 너무 뿌듯하네요 :)
} else if (typeof validator.#value === 'object') { | ||
validator.#validateMaxLength(); | ||
validator.#validateNoDuplicates(); | ||
} |
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문보다 switch문을 사용하면 조금 더 가독성이 좋을 것 같아요! typeof validator.#value
가 중복되기도 하구요!
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 input = await readUserInput(MESSAGE.PROMPT_NAME_USER_INPUT); | ||
Validator.validate(input); | ||
const cars = splitByComma(input); | ||
Validator.validate(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.
유효성 검사를 위한 메소드를 input
입력 이후 한 번, cars
입력 이후에 또 한 번 해서 두 입력에 대해 각각 호출하고 있는데, 한 번만 호출하도록 로직을 구성하는 것이 좀 더 좋을 것 같아요:)
아예 처음 input
에 대한 유효성 검사와 cars
에 대한 유효성 검사를 분리하는 것도 좋아보이는데, 클래스를 사용하신 걸로 보아 개인적으로는 Validator.js
내에서 validate()
메소드 자체를 분리시키는 건 어떨까 의견 내봅니다!
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.
바로 밑에 attempCount
에 대한 유효성 검사도 포함해야겠네요
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.
한번에 유효성 검사를 할 수도 있지만, 저는 input
에 대해 유효성 검사가 통과되지 못하면 다음단계로 넘어가지 못하도록 분기처리 역할을 만들어주고 싶어서 이렇게 프로그래밍하게 되었습니다!
input
값이 undefined가 아닌 적절한 값이 매핑되었다는 것을 먼저 확인해준 후 splitByComma
와 같은 기능을 수행하고 싶었거든요!
그렇지 않다면, splitByComma
에서 인자로 받은 값의 type을 확인해줘야하는 번거로움이 있는데, 이를 방지하기 위해 매 값마다 validate
를 호출하는 형식을 채택했습니다!
이에 대해 어떻게 생각하시는지 의견이 궁금하네요!!
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.
우선 자세히 생각을 알려주셔서 감사해요😊 어떤 의도로 이렇게 하셨는지 이해했습니다!
.map((car) => car.name); | ||
printOutput(`${MESSAGE.FINAL_OUTPUT} : ${winner.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.
우승자 출력하는 부분은 클래스를 아예 따로 분리하는건 어떨까요!
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.
우승자를 출력하는 역할인 printWinner에서, 우승자를 결정하는 로직까지 포함하고 있는 것 같아요!
분리하는 것도 좋아보입니다 ㅎㅎ!
만약 그렇게 된다면 printWinner는 단순히 우승자를 print해주는 역할이 될텐데요!
이럴 경우에 과연 Simulator에서 관리할 메서드인지에 대해 고민해보면 좋을 것 같네요.
Console.print(message); | ||
} | ||
|
||
export { readUserInput, printOutput }; |
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 { printOutput } from './io'; | ||
|
||
function printSimulate(name, count) { | ||
const result = '-'.repeat(count); |
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.
맞습니다! 좋은 피드백 감사합니다 :)
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주차도 파이팅입니다!!
import printSimulate from './util/printSimulate'; | ||
|
||
class Simulator { | ||
#state; |
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.
이 #state가 의미하는게 무엇인가요??
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.
자동차 이름별로 전진한 횟수입니다!
|
||
#canMoveForward() { | ||
return pickRandomNumberInRange(RANGE.START, RANGE.END) >= RANGE.VALID; | ||
} |
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.
메서드와 프로퍼티를 private로 분리할때 기준이 있으신가요?!
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.
외부에서 사용되지 않을 경우 (내부에서만 사용될 경우) private 으로 처리하여 캡슐화를 강화시켜주었습니다!
if (hasDuplicates) { | ||
throw new Error(ERROR_MESSAGE.DUPLICATE_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.
Set을 사용한 중복 검사 너무 좋네요!
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.
와 MAX_INPUT_LENGH 도 따로 constant로 빼서 상수화하셔서 쓴 부분 너무 좋은 것 같아요
이진님 코드 보면서 상수를 쓰는 방법에 대해 다시 한 번 더 생각해보게 되네요!
} | ||
} | ||
|
||
export default Validator; |
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.
validatior를 class로 구현할 생각을 못했는데 class로 구현하니 가독성도 좋고 코드가 깔끔한거 같네요!!
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주도 같이 화이팅합시다~
## 📄 폴더구조 | ||
``` | ||
📦 __tests__ | ||
┗━ 📜ApplicationTest.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.
리드미에 폴더 구조가 있는 것 좋은 것 같아요! 이쁘네요~
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 Simulator from '../src/Simulator'; // Simulator 클래스가 정의된 경로로 수정하세요. | ||
import { MESSAGE } from '../src/constant'; |
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.
잘못 들어간 불필요한 주석이네요!
const attemptCount = await readUserInput(MESSAGE.PROMPT_COUNT_USER_INPUT); | ||
Validator.validate(+attemptCount); |
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.
+연산자로 형변환을 하셨군요! 그런데 최근에 보니까 22.3 이런 컨벤션이 있더라구요! +로 형변환하는 것보다 Number()를 하는 것이 조금 더 의도가 명확하다는 것 같아요~ 확인해보시면 좋을 것 같아요!
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.
맞습니다! 노티 감사드립니다 앞으로는 Number()
를 활용해야겠어요
const simulator = new Simulator(cars); | ||
simulator.simulate(attemptCount); | ||
simulator.printWinner(); |
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.
오호.. Simulator라.. 너무 괜찮은 클래스명인데요? 저도 다음에 한 번 써먹어보겠습니다🤤
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.
넹 !! 좋은 영향을 드려 기쁘네요!
#state; | ||
|
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.
state라는 상태명은 조금 모호한 것 같은데 어떻게 생각하시나요? 처음 봤을 때 무엇인지 알기 힘들다는 느낌을 받았어요😅
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.
자동차의 전진상태를 나타내려고 한 건데 변수명을 너무 추상적으로 지은 것 같네요!
progressState
와 같은 이름을 정할 걸 그랬어요! 알려주셔서 감사합니다 :)
#canMoveForward() { | ||
return pickRandomNumberInRange(RANGE.START, RANGE.END) >= RANGE.VALID; | ||
} |
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.
네! 좋습니당 ㅎㅎ
function printSimulate(name, count) { | ||
const result = '-'.repeat(count); |
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.
아주 사소하지만 print + simulate 둘 다 동사인 점에서 어색함이 느껴질 수도 있을 것 같아요🙂
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.
맞습니다! 변수명 네이밍에 좀 더 심혈을 기울여야겠네용
function readUserInput(message) { | ||
return Console.readLineAsync(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.
이 부분에서 async/await 해주지 않았는데 코드 동작에 이상은 없으셨나요?🫨
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.
바로 await 을 return해주어야할 경우에 async/await 둘 다 생략 가능합니다!
참조할 아티클 첨부해놓을게용 ~!
return await vs. no return await
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주차 수고하셨습니다
function splitByComma(input) { | ||
return input.split(','); | ||
} | ||
|
||
export default splitByComma; |
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.
input에서 바로 사용해도 될 정도의 로직이라고 생각하는데 따로 빼신 이유가 있나요 ?!
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.
기능별로 함수를 분리하는 데에 집중했기 때문입니당
printSimulate(name, count); | ||
return { name, count }; | ||
}); | ||
printOutput(''); |
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.
\n 로 개행이라는 것을 명확하게 표현해주면 좋을 것 같아요
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.
근데 아마 \n으로 줄바꿈하면 두 줄이 띄워질 수도 있슴다!
이미 Console.print에서 줄바꿈이 포함되어 있어서용
constructor(value) { | ||
this.#value = value; | ||
} |
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.
각 메서드의 인자로 넣지 않고 constructor로 value를 받으시는 이유가 있으실까요?
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.
클래스 내에서 모든 메서드가 해당 value를 공유하는데, 매번 인자로 넘겨주기보다 클래스의 속성을 활용하고 싶었습니다!
validator.#validateNotEmpty(); | ||
if (typeof validator.#value === 'number') { | ||
validator.#validatePositiveNumber(); | ||
} else if (typeof validator.#value === 'object') { |
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.
null도 object로 판별되어 예상치 못한 동작이 될 것 같기도 한데, 이에 대해 어떻게 생각하시나요??
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.
조건문을 들어가기 전에 notEmpty 메서드로 빈값인지 확인하기 때문에 해당 사이드이펙트는 없을 거라고 예상됩니다!
#validateNotEmpty() { | ||
if (this.#value !== 0 && !this.#value) { | ||
throw new Error(ERROR_MESSAGE.EMPTY_INPUT); | ||
} | ||
} |
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.
프라이빗을 어디에 붙여야할지 기준이 확고하신 것 같네요!
저는 아직 잘 모르겠어서...
진님 처럼 프라이빗 쓰면 되겠다는 생각이 들었습니다.
이렇게 한다면 외부에서 static만 가져다쓰면 되니까!
배워갑니다 ㅎㅎ!
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.#state = this.#state.map(({ name, count }) => { | ||
if (this.#canMoveForward()) count++; | ||
printSimulate(name, count); | ||
return { name, count }; |
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문 안에 있는 친구들을 다른 메서드로 분리한다면 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.
인정합니다! 좋은 피드백 감사합니다 :)
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
}); |
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.
오 모든 모킹을 afterEach가 아니라 beforeEach에서 해주셨네요?
이런 구현방식을 택하신 이유가 있으실까요?
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.
beforeEach에서 jest.clearAllMocks()를 사용하는 이유는 각 테스트가 실행되기 전에 모킹 상태를 초기화하여 테스트 간에 영향을 미치지 않도록 보장하기 위함입니다!
이렇게 하면 각 테스트가 독립적으로 실행되며 이전 테스트의 영향을 받지 않습니다.
반면에 afterEach에서 jest.clearAllMocks()를 사용하는 경우, 현재 테스트가 완료된 후에 모킹이 초기화되기 때문에 다음 테스트의 상태가 이전 테스트의 영향을 받을 가능성이 있습니다.
따라서, 테스트가 시작되기 전에 항상 초기 상태를 유지하려는 의도라면 beforeEach에서 초기화하는 것이 더 적합하다고 생각합니다 :)
pickRandomNumberInRange.mockReturnValueOnce(5); | ||
pickRandomNumberInRange.mockReturnValue(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.
pickRandomNumberInRange.mockReturnValueOnce(5).mockReturnValue(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.
체크리스트에 . (메서드 연쇄) 를 사용하지 않았는가 를 체크하는 부분이 있습니다
메서드 연쇄를 사용하면 코드가 간결해진다는 장점이 있지만, 그만큼 코드 한 줄이 의미하는 복잡성이 증가하게 되고 이는 가독성 저하로 이어집니다
코드가 간결한 것도 중요하지만, 저에게 이번 우테코 프리코스에서 가장 중요하게 생각하는 것은 가독성이었기에 이렇게 작성하였습니다!
VALID: 4, | ||
}); | ||
|
||
export const MAX_INPUT_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.
여기에는 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.
객체가 아니고 primitive type 이기 때문에 동결시킬 필요가 없다고 판단했기 때문입니다
export const RANGE = Object.freeze({ | ||
START: 0, | ||
END: 9, | ||
VALID: 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.
더 구체적인 변수명이면 어떨까요!
어떤 것에 대한 RANGE인지 붙인다면 더 가독성이 좋아질 것 같슴다!
START나 END는 괜찮은데,
특히 이 부분이 쓰이는 곳에서
#canMoveForward() {
return pickRandomNumberInRange(RANGE.START, RANGE.END) >= RANGE.VALID;
}
RANGE.VALID는 모호하다는 생각이 들어요.
뭐가 유효하다는거지? 라는 생각이 들 수 있을 것 같습니다.
RACE_ENABLED와 같은 더 직관적인 네이밍이면 어떨까 생각이드네용
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.
에러 메세지를 상수화 할 때 Object.freeze를 사용한다는 것은 처음 접해봐서 이진님 코드를 보며 Object.freeze()에 대해 공부하게 되었어요!
저도 다음 과제 할 때 Object.freeze()를 사용해봐야겠습니다 감사합니다
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.
리뷰어와 리뷰이가 토론한 코멘트를 통해서 많이 배우고, 생각할 수 있었어요.
미션 종료 직후가 아닌 시간 차를 두고 코드리뷰를 하니 많은 분들의 생각을 엿볼 수 있어 좋네요 😋
고생하셨습니다! 👍
if (typeof validator.#value === 'number') { | ||
validator.#validatePositiveNumber(); | ||
} else if (typeof validator.#value === 'object') { |
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.
해당 조건문에서 else if
가 꼭 필요한 자리는 아닌 것 같아요.
제가 이렇게 생각한 이유는 number
이면 object
가 아니고, object
이면 number
가 아니기 때문입니다!
코드를 단편적으로 보아 제가 오해할 수도 있어서 어떤 의도가 있었는지 여쭙습니다! ㅎㅎ
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.
이진님 코드 보면서 많은 부분 배워가는 것 같습니다!
저도 다른 분들처럼 이진님께 더 도움되는 리뷰 남기고 싶었는데, 제 실력 부족으로 아직 도움되는 리뷰를 남겨드리기엔 부족하네요ㅜㅜ
그래도 덕분에 많이 성장한 것 같습니다! 저도 이진님께 좋은 인사이트를 드릴 수 있을 만큼 성장해보도록 하겠습니다 감사합니다!
|
||
// then | ||
await expect(app.run()).rejects.toThrow("[ERROR]"); | ||
await expect(app.run()).rejects.toThrow(expectedError); |
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.
아 이렇게 ApplicationTest 등 따로 파일을 만들어서 테스트를 할 수도 있군요..!
저는 몰랐는데 이번 과제부터 이렇게 적용해봐야겠어요! 🔥
if (hasDuplicates) { | ||
throw new Error(ERROR_MESSAGE.DUPLICATE_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.
와 MAX_INPUT_LENGH 도 따로 constant로 빼서 상수화하셔서 쓴 부분 너무 좋은 것 같아요
이진님 코드 보면서 상수를 쓰는 방법에 대해 다시 한 번 더 생각해보게 되네요!
export const RANGE = Object.freeze({ | ||
START: 0, | ||
END: 9, | ||
VALID: 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.
에러 메세지를 상수화 할 때 Object.freeze를 사용한다는 것은 처음 접해봐서 이진님 코드를 보며 Object.freeze()에 대해 공부하게 되었어요!
저도 다음 과제 할 때 Object.freeze()를 사용해봐야겠습니다 감사합니다
javascript-racingcar-precourse
🚀 요구 사항 분석
시작하기
README
constant
입력받기
input
출력하기
output
게임하기
game
입력값 예외 처리하기
error
🚨 체크리스트
📄 폴더구조