-
Notifications
You must be signed in to change notification settings - Fork 192
[클린코드 8기 조세민] 로또 미션 STEP1 #328
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
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.
안녕하세요 세민님!
미션 진행 너무 잘 진행해주셔서 감동이에요
고생 많으셨습니다!
함수형 프로그래밍 위주로 접근하신 말씀을 보고 읽어내려갔는데 제게도 익숙한 방식이어서 코드가 명료해 보였습니다. 다만, 몇몇 부분은 개선할 포인트 그리고 좀 더 나은 방식이 있을 수도 있어서 관점을 제시해 드렸으니 참고해 주시면 좋겠어요.
또한, 함수형/객체지향 모두 최신/낡은, 우월한/열등한 패러다임 구도로 바라보는 것이 아니라, 각기 적절한 쓰임새에 따라서 작성한다는 점을 앞으로도 양지하시고 코드를 작성해 주시면 보다 클린한 코드를 작성하실 수 있을 거예요 ! 이에 관해서는 9/9 피드백 강의에서 다루도록 하겠습니다.
Node.js 환경에서 도메인 로직을 어떻게 작성하는지를 훈련하는 거라, 콘솔로 입출력을 잘하고 있을지에 관한 걱정은 크게 하지 않으셔도 괜찮습니다! 이미 충분히 잘하셨다고 생각하고요. 또한, 단위 테스트만을 중점적으로 다루고 있고 STEP3(웹 브라우저 출력)부터 E2E 테스트로 진행하시면 되는 부분이라서 Jest로는 지금처럼 단위 테스트에만 집중해 주시면 좋을 거 같아요!
그럼, 코멘트 검토 부탁 드리며, 피드백 개선 시 다시 뵙겠습니다 👋🏻
|
|
||
| describe('getTicket 함수 테스트', () => { | ||
| it('1,000원을 입력하면 1장을 발급한다', () => { | ||
| const money = 1000 |
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.
lottoGame.test.js와 일관성을 맞추시면 좋겠어요!
천 단위 구분 기호를 사용해 맞춰 주세요 😊
| const money = 1000 | |
| const money = 1_000 |
| it('1,000원 미만은 에러를 발생시킨다', () => { | ||
| const money = 900 | ||
|
|
||
| expect(() => getTicket(money)).toThrow(LottoError.TicketPriceTooLow()) |
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.
LottoError객체에서 예외 메시지를 생성하는 함수를 호출해서 검증한다는 접근법이 틀렸다고 말씀드리고 싶지 않습니다!
그렇지만, 조금 제가 익숙지 않은 코드 스타일이기도 해서요. 오히려 아래와 같이 커스텀 클래스로 관리해 보시는 건 어떠신가요? 그게 좀 더 컨벤션(명사)에도 부합하는 접근법일 것 같아요!
class TicketPriceTooLowError extends Error {
constructor() {
super('구매 금액은 최소 1,000원 이상이어야 합니다.')
this.name = 'TicketPriceTooLowError'
}
}expect(() => getTicket(money)).toThrow(TicketPriceTooLowError)(혹시)... 함수형 프로그래밍으로 작성하고 싶으셔서 이런 접근법을 취하신 거라면.... 저는 둘 다 적재적소에 맞게 사용하는 방법을 익히시는 편이 좋겠다는 생각이 듭니다. 자바스크립트는 멀티 패러다임 프로그래밍 언어이기도 하고요. 이에 관해서는 9/9 피드백 강의에서 다룰 예정이니 많관부탁드립니다 🙇♂️
| @@ -0,0 +1,7 @@ | |||
| function ticketPriceTooLow() { | |||
| return new Error('구매 금액은 최소 1,000원 이상이어야 합니다.') | |||
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.
어떤 의도로 만드신 것인지는 이해가 되었지만, 함수의 이름은 동사로 시작해야 하는데 ticketPriceTooLow라는 함수에서 ticket은 동사의 의미로 쓰인 것 같지는 않습니다. 테스트 코드에서 피드백을 드렸던 것처럼 커스텀 오류 클래스를 만들고 관리하시는 접근법으로 방향을 전환하신다면 조금 더 자연스러워질 것 같아요!
| let winningNumbers | ||
| let winningBonusNumber | ||
|
|
||
| beforeEach(() => { | ||
| winningNumbers = [1, 2, 3, 4, 5, 6] | ||
| winningBonusNumber = 7 | ||
| }) |
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.
winningNumbers를 beforeEach를 돌 때마다 초기화하도록 선언하신 이유가 있을까요?
어림짐작하는 의도로는, 동일한 검증 값이기도 하고 재사용 목적으로 두신 것 같기도 합니다. 충분합니다 👍🏻. 그렇지만 입체적인 관점을 제시해 드리기 위해 아래의 코멘트도 남겨 드려요! (틀린 게 아닙니다! 하나의 관점으로만 해석하세요)
물론, 클린 코드 관점에서 테스트 코드도 코드이기 때문에 중복되는 코드는 최대한 간결하게 유지해야 한다는 부분도 타당합니다. 하지만, 테스트 코드의 경우 검증 값을 확인해야 한다는 특성상 제 생각에는 반복되는 부분이 있다고 하더라도 하나의 테스트 케이스에 같이 선언해 주는 것이 보다 타당하다는 생각이 들었어요.
그래서 저의 경우, beforeEach는 테스트를 수행하기 이전에 초기화되어야 할 작업 내용들을 담을 때 쓰고 있어요.
이러한 내용은 E2E Cypress 테스트에서 익숙해지실 거예요👍🏻 (예를 들어, 웹 페이지 초기화를 위해 검증 페이지로 이동하는 작업)
그러면 저의 호(好)에 관해 예시를 포함해서 설명해 드릴게요!
저는 검증 값을 모두 상수 선언을 해두면서 명시적으로 확인할 것 같아요 :)
그게 BDD 관점에서 보다 가깝다고 생각했기 때문입니다.
it("...", () => {
// given
const winningNumbers = [1, 2, 3, 4, 5, 6]
const winningBonusNumber = 7
const 상금 = 5_000
// when
const ticketNumbers = [1, 2, 3, 14, 15, 16] // 검증 대상
const rank = getRank(ticketNumbers, winningNumbers, winningBonusNumber) // 검증될 rank 객체
// then
expect(rank.prize).toBe(상금)
});| export const LOTTO_PRICE = 1000 | ||
| const LOTTO_NUMBERS_LENGTH = 6 | ||
|
|
||
| export function getTicket(money) { |
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.
getTicket이 반환하는 건 티켓의 배열이라고 이해했습니다!
그렇다면 getTickets가 되어야 하지 않을까요?
return ([
[1,2,3,4,5,6],
[2,3,4,5,6,7],
]) // 로또 배열| let winningNumbers = await readLineAsync('당첨 번호를 입력해 주세요. ') | ||
| winningNumbers = winningNumbers.split(',').map(num => Number(num.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.
사용자가 입력한 값과, 사용자의 입력 값을 토대로 winingNumber를 추출한 값을 따로 따로 선언해 준다면 보다 분명해지고 let 키워드의 사용 빈도도 줄어들 수 있을 거예요 :)
다만, 도메인이 복잡하지 않기도 하고 살려뒀다고 해서 고쳐야 하는 코드라고 생각지는 않습니다!
| let winningNumbers = await readLineAsync('당첨 번호를 입력해 주세요. ') | |
| winningNumbers = winningNumbers.split(',').map(num => Number(num.trim())) | |
| const inputWinningNumbers = await readLineAsync('당첨 번호를 입력해 주세요. ') | |
| const winningNumbers = winningNumbers.split(',').map(num => Number(num.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.
제가 다른 리뷰이분께 드린 피드백과 유사한 질문을 드리겠습니다!
비슷한 현상이 나타나고 있습니다.
이에 관해서 테스트 케이스도 함께 작성해 두시면 좋겠죠!
질문: Number객체로 변환했을 때 어떤 우려사항이 있을까요? 이 우려사항을 해소하기 위해서는 어떻게 개선해 볼 수 있을까요?
또는 의도적으로 Number로 변환하신 것인지 제가 생각한 아래의 우려사항을 검토하셔서 의견을 남겨 주세요!
우려사항 Type 1
- 금액 입력란에 Infinity, -Infinity 입력한다면? 배열 에러가 발생하네요!
- 금액 입력란에 1999원을 입력한다면? 1장이 구매됩니다.
우려사항 Type 2
- 사용자가 당첨번호/보너스 번호에
Infinity를 입력한다면? - 사용자가 당첨번호/보너스 번호에 유리수(소숫점이 있는 숫자)를 입력한다면?
- 사용자가 당첨번호/보너스 번호에 음의 정수를 입력한다면?
- 사용자가 당첨번호/보너스 번호에 지수(1e2)를 입력한다면?
- 사용자가 당첨번호/보너스 번호에 보너스 번호에 아무것도 입력하지 않는다면?
우려사항 Type 3
- 사용자가 당첨번호에
,,,,,를 입력한다면?
|
그동안 고생 많으셨습니다 🙂 리뷰 기한이 만료되어 PR을 닫겠습니다. |
안녕하세요. 리뷰어님. '1단계 - 콘솔 기반 로또 게임' 미션 제출합니다.
이번 미션은 함수형 프로그래밍 방식으로 구현해 보았습니다.
평소에는 클래스 기반으로만 작성하다 보니, 제가 올바른 방향으로 접근했는지 확신이 없네요. 피드백 많이 부탁드립니다!
유닛 테스트는 어느 정도 감을 잡았지만, 콘솔 입출력 로직을 어떻게 다뤄야 할지 고민이 됩니다.
기능 위주의 유닛 테스트만 진행하는 게 맞을지, 아니면 E2E 테스트처럼 입출력 검증까지 포함해야 할지도 의견을 듣고 싶습니다.
감사합니다.