[조수연] 로또 미션 Step1 #5
Conversation
- accord: 비교할 두 배열을 받아서 일치하는 개수를 반환하는 함수 - generateRandomNumberArray: 배열의 개수를 받아서 그 개수만큼 랜덤 번호를 생성하는 함수
- LottoRule 의 RANK_BY_ACCORD 프로퍼티가 잘못 작성되어 있던 문제 - Lotto 가 LottoRule 을 상속받지 않도록 변경
- Lotto 의 static 이 아니라 LottoRule 의 static 을 참조하도록 변경
There was a problem hiding this comment.
코드 잘 읽었습니다!
뭔가 여러가지 시도를 해보는게 보이는군요 ㅎㅎ
유효성 검사 테스트 코드를 중복작성 해주는 편이 좋은가요? 예를 들어 다음과 같은 코드가 있을 때 setData 에서 유효성 검사에 대한 테스트 케이스도 작성하고, getError 에 대한 테스트 케이스도 따로 작성해야 하나요?
getError에서 통과되면 setData에서 중복해서 테스트할 필요는 없을 것 같아요!
제가 본 책에서 코드도 부채라고 생각하면 좋다고 하더라구요!
utils 함수 내부의 파일 이름을 Array, Number, String... 과 같이 매개변수의 타입이 어떤 것이 들어오는지에 따라 작성했는데 이 방법이 그렇게 좋은 방법은 아니라는 생각이 들어요. 보통 util 함수 내부의 파일 이름은 어떻게 관리해야 직관적인가요?
흠... 저는 그냥 유틸 함수명으로 관리하는게 직관적인거 같아요!
| const set = new Set(); | ||
|
|
||
| while (set.size < amount) { | ||
| set.add(generateRandomNumber(min, max)); | ||
| } | ||
|
|
||
| return [...set].sort((a, b) => a - b); |
There was a problem hiding this comment.
중복 검사랑 정렬이라는 두가지 일을 하고있다는 생각이 듭니다!
There was a problem hiding this comment.
함수 자체는 랜덤한 번호를 반환하는 거고, 정렬이 된 채 반환되면 좋을 것 같다고 생각했어요. 정렬을 외부에서 해주는 것이 좋을까요? 🤔
There was a problem hiding this comment.
유틸에 속해 있어서 최소한의 일만하면 좋다고 생각했어요! 그래서 정렬은 외부에서 해주면 좋지않을까 ㅎㅎ
| set(lotto) { | ||
| const lottoError = Lotto.getLottoError(lotto); | ||
|
|
||
| if (lottoError !== undefined) { | ||
| throw new Error(lottoError); | ||
| } | ||
|
|
||
| this._lotto = lotto; | ||
| } |
There was a problem hiding this comment.
생성자 함수에서 해줘도 괜찮지 않을까요..? 그리고 비공개로 안한 이유는 뭔가요??
There was a problem hiding this comment.
- set메서드를 재사용하고 싶어서 입니다!
- 유효성 검사가 있으니 외부에서 함부로 변화시킬 수 없다고 생각해서 입니다!
| static RANK_BY_ACCORD = Object.freeze({ | ||
| 3: { bonus: 5, base: 5 }, | ||
| 4: { bonus: 4, base: 4 }, | ||
| 5: { bonus: 2, base: 3 }, | ||
| 6: { bonus: 1, base: 1 }, | ||
| }); | ||
| static REWARD = Object.freeze({ | ||
| 1: 200000000, | ||
| 2: 30000000, | ||
| 3: 1500000, | ||
| 4: 50000, | ||
| 5: 5000, | ||
| }); | ||
| static ACCROD_STRING_BY_RANK = Object.freeze({ | ||
| 1: `6개 일치 ${changeStandardMoneyString(LottoRule.REWARD[1])}`, | ||
| 2: `5개 일치, 보너스 볼 일치 ${changeStandardMoneyString(LottoRule.REWARD[2])}`, | ||
| 3: `5개 일치 ${changeStandardMoneyString(LottoRule.REWARD[3])}`, | ||
| 4: `4개 일치 ${changeStandardMoneyString(LottoRule.REWARD[4])}`, | ||
| 5: `3개 일치 ${changeStandardMoneyString(LottoRule.REWARD[5])}`, | ||
| }); |
| static get MAX_NUMBER() { | ||
| return 45; | ||
| } | ||
|
|
||
| static get MIN_NUMBER() { | ||
| return 1; | ||
| } | ||
|
|
||
| static get MONEY() { | ||
| return 1000; | ||
| } | ||
|
|
||
| static get DEFAULT_LENGTH() { | ||
| return 6; | ||
| } |
There was a problem hiding this comment.
넵 맞습니다...! 인스턴스에 종속되지도 않고 값만 가져다 쓰는 거면 이런식으로 써볼 수 있을 것 같았어요...!
|
|
||
| get() { | ||
| return [...this._winLotto]; | ||
| } | ||
|
|
||
| checkRank(compareLotto) { | ||
| const compareLottoError = Lotto.getLottoError(compareLotto); | ||
| const winLottoError = WinLotto.getLottoError(this._winLotto); | ||
|
|
||
| if (compareLottoError !== undefined || winLottoError !== undefined) { | ||
| return compareLottoError || winLottoError; | ||
| } | ||
|
|
||
| const accordCount = accord( | ||
| this.get().slice(0, LottoRule.DEFAULT_LENGTH), |
There was a problem hiding this comment.
get보다는 명확한 이름으로 바꾸는게 좋을 것 같아요!
읽다보니 이 get이 어떤걸 가져오지라는 의문이 들었거든요 ㅎㅎ 제가 길게 기억을 못해서 그런것도 있지만..
There was a problem hiding this comment.
getLotto나 getNumber 등으로 명시하려고 노력해야겠네요!!
| const winLotto = new WinLotto([...winPureLotto, bonusNumber]); | ||
| const rankInfo = { | ||
| 1: 0, | ||
| 2: 0, | ||
| 3: 0, | ||
| 4: 0, | ||
| 5: 0, | ||
| }; | ||
| lottos.forEach((lotto) => { | ||
| rankInfo[winLotto.checkRank(lotto.get())] += 1; | ||
| }); |
There was a problem hiding this comment.
안 그래도 이번 main.js파일에서 코드를 모두 분리해보려고 했는데 별로 좋은 시도는 아니었던 것 같아요. 적절히 메서드로 분리해보겠습니다!
| test('get 메서드는 현재 lotto 의 값을 가져올 수 있다.', () => { | ||
| const lotto = makeLottoMockingData(initialLotto); | ||
|
|
||
| expect(lotto.get()).toStrictEqual(initialLotto); |
There was a problem hiding this comment.
.toStrictEqual(initialLotto)에서 initialLotto 부분은 하드코딩하는게 책에서 읽기 좋다고 하더라구요!
There was a problem hiding this comment.
하드코딩이라는 게 테스트 내부에 바로 배열을 전달해 주는 건가요?!
There was a problem hiding this comment.
그런 것 같네요...! 아무래도 다른 테스트에서 값을 변경시킬 수 있을 위험이 있을 것 같아요.
There was a problem hiding this comment.
직접 값을 적어주는거죠! 예를 들어, [1,2,3,4,5,6] 이렇게요!
hyoribogo
left a comment
hyoribogo
left a comment
There was a problem hiding this comment.
수연님 안녕하세요! 😸
이번 로또 과제에서 로직을 분리하고, 또 어떻게 그걸 결합해야 할지 너무 어려워서 며칠동안 헤매다 리뷰 먼저 하러 왔는데 .. 수연님도 같은 고민을 하고 계셨네요 ..!
그럼에도 완성시킨 게 정말 대단합니다.. 많이 참고할게요 !!
방금 코드를 다 읽었는데, 수연님이 고민하신 과정이 저와 너무 똑같아서 읽는 내내 공감이 되었어요 ....ㅋㅋㅋㅜㅜ 그리고 많은 해결 방향을 얻어가요. 감사합니다!
수연님의 궁금한 점은 동시에 제 궁금한 점이기 때문에 .. 답변은 하지 않겠습니다. 왜냐면 저도 궁금하기 때문입니다 😂 다른 분들 답변 참고하러 오겠습니다!
전체적으로 일관성 있게 작성되어 있어서 수연님의 코드 스타일을 단번에 파악하기 쉬웠어요! 이것 또한 나름대로 수연님의 클린 코드라고 볼 수 있겠네요 !! 너무너무 고생하셨습니다!
| static getPostProcessFnError(postProcessFn) { | ||
| if (postProcessFn !== undefined && typeof postProcessFn !== 'function') { | ||
| return '후처리 함수(postProcessFn)는 함수 타입이어야 합니다.'; | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| static getErrorCheckFnError(errorCheckFn) { | ||
| if (errorCheckFn !== undefined && typeof errorCheckFn !== 'function') { | ||
| return '에러 검사 함수(errorCheckFn)는 함수 타입이어야 합니다.'; | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
혹시 외부에서 재사용할 메서드가 아니라면 #를 붙여줘도 좋을 것 같아요 !
There was a problem hiding this comment.
이전에 도메인의 유효성 검사 함수를 외부에 꺼내 쓴 적이 있던 것 같아서 private은 적용시키지 않았어요...! 그런데 이렇게 할 바엔 아예 validator폴더를 만드는 방법이 좋을 것 같아서 4주차 코드엔 아예 따로 빼버렸답니다. ㅎㅎ..!
| async inputPurchaseLotto( | ||
| options = { | ||
| postProcessFn: undefined, | ||
| errorCheckFn: undefined, | ||
| }, | ||
| ) { | ||
| return this.input('> 구입금액을 입력해 주세요:', options); | ||
| } | ||
|
|
||
| async inputWinLotto( | ||
| options = { | ||
| postProcessFn: undefined, | ||
| errorCheckFn: undefined, | ||
| }, | ||
| ) { | ||
| return this.input('> 당첨 번호를 콤마(,)로 구분해 입력해 주세요:', options); | ||
| } | ||
|
|
||
| async inputBonusNumber( | ||
| options = { | ||
| postProcessFn: undefined, | ||
| errorCheckFn: undefined, | ||
| }, | ||
| ) { | ||
| return this.input('> 보너스 번호를 입력해 주세요: ', options); | ||
| } |
There was a problem hiding this comment.
options가 undefined일 때의 처리를 하느냐 or 기본값을 넣어주느냐...! 를 고민했을 때 후자는 options에 어떤 값이 들어가는지 유추할 수 있다고 생각해서 기본값을 넣어주게 되었어요!
There was a problem hiding this comment.
로또에 관련된 View니까 LottoView라고 명시하면 좋을 것 같아요!
There was a problem hiding this comment.
그러게요...!! 컨벤션 오류인것 같아요 수정하겠습니다!! 이름도 명시하기! 다음에 적용해볼게요!!
| test('get 메서드는 현재 lotto 의 값을 가져올 수 있다.', () => { | ||
| const lotto = makeLottoMockingData(initialLotto); | ||
|
|
||
| expect(lotto.get()).toStrictEqual(initialLotto); |
There was a problem hiding this comment.
하드코딩이라는 게 테스트 내부에 바로 배열을 전달해 주는 건가요?!
| ])( | ||
| 'getLottoError 메서드는 인수($lotto)가 배열이 아니면 undefined 를 반환하지 않는다.', | ||
| ({ lotto }) => { | ||
| expect(Lotto.getLottoError(lotto)).toBeDefined(); |
There was a problem hiding this comment.
헤헤..이건 제가 노션에 간단히 정리한 건데 도움이 될까 싶어 올려봐요!!
expect
assertion을 만드는 함수이다.
import { expect } from 'vitest'
const input = Math.sqrt(4)
expect(input).to.equal(2) // chai API
expect(input).toBe(2) // jest APIsoft
expect와 유시하지만 테스트가 실패했을 때 터미널을 끝내지 않고 계속해서 테스트 실패를 체크해나간다. 이렇게 실패한 테스트 케이스는 테스트가 완료된 후 집계되어 나온다.
import { expect, test } from 'vitest'
test('expect.soft test', () => {
expect.soft(1 + 1).toBe(3)
expect.soft(1 + 2).toBe(4)
})만일 soft를 사용하지 않은 expect가 있다면 해당 expect 구문에서 터미널이 종료되고, 이전까지의 실패개수들이 나타난다.
not
assertion을 부정하는 함수이다.
import { expect, test } from 'vitest'
const input = Math.sqrt(16)
expect(input).toBe(4)
expect(input).not.toBe(2) toBe
assertion의 원시값이 동일하거나 객체가 같은 참조값을 가지는 경우 사용한다. 참조값이 같지 않지만 같은 구조를 가지고 있는지 판별하기 위해서는 toEqual 을 사용한다.
import { expect, test } from 'vitest'
const stock = {
type: 'apples',
count: 13,
}
test('stock has 13 apples', () => {
expect(stock.type).toBe('apples')
expect(stock.count).toBe(13)
})
test('stocks are the same', () => {
const refStock = stock // 같은 참조값을 지닌다
expect(stock).toBe(refStock)
})toBeCloseTo
toBeCloseTo 는 float 형 숫자를 비교하기 위해 사용한다. numDigits 옵션은 소수 점 뒤 자릿수를 제거한다.
import { expect, test } from 'vitest'
test.fails('0.2 + 0.1 은 자바스크립트에서 0.3 이 아니다...!!', () => {
expect(0.2 + 0.1).toBe(0.3) // 0.2 + 0.1 = 0.30000000000000004
})
test('소수점 5번째까지만 비교한다!', () => {
// 0.2 + 0.1 is 0.30000
expect(0.2 + 0.1).toBeCloseTo(0.3, 5)
// 50으로 주면 제거 되는 게 없다 0.30000000000000004
expect(0.2 + 0.1).not.toBeCloseTo(0.3, 50)
})toBeDefined
toBeDefined 는 undefined 가 아닌지 판별한다. 함수가 undefined 가 아닌 특정 값을 반환하는지 판별할 때 유용하다.
import { expect, test } from 'vitest'
function getApples() {
return 3
}
test('함수가 어떤 값을 반환합니다.', () => {
expect(getApples()).toBeDefined()
})toBeUndefined
toBeDefined 와 반대되는 함수이다. 값이 undefined 라고 판별한다.
import { expect, test } from 'vitest'
function getApplesFromStock(stock: string) {
if (stock === 'Bill')
return 13
}
test('mary 에게 남은 재고는 없어요...', () => {
expect(getApplesFromStock('Mary')).toBeUndefined();
})toBeTruthy
값을 true 로 추정한다.
toBeFalsy
값을 false 로 추정한다.
toBeNull
값을 null 로 추정한다.
toBeNaN
isNaN 이 true, 즉 숫자가 아닌 값으로 추정한다.
toBeTypeOf
반환받은 값의 타입이 인자로 받은 타입과 일치한다고 추정한다.
import { expect, test } from 'vitest'
const actual = 'stock'
test('stock is type of string', () => {
expect(actual).toBeTypeOf('string')
})toBeInstanceOf
생성자로 생성한 인스턴스가 해당 클래스의 인스턴스인지 확인한다.
import { expect, test } from 'vitest'
import { Stocks } from './stocks.js'
const stocks = new Stocks()
test('stocks are instance of Stocks', () => {
expect(stocks).toBeInstanceOf(Stocks)
})toBeGreaterThan | toBeGreaterThanOrEqual
값이 인자로 받은 값보다 크다고/크거나 같다고 추정한다.
import { expect, test } from 'vitest'
import { getApples } from './stocks.js'
test('have more then 10 apples', () => {
expect(getApples()).toBeGreaterThan(10)
})toBeLessThan | toBeLessThanOrEqual
값이 인자로 받은 값보다 작다고/작거나 같다고 추정한다.
import { expect, test } from 'vitest'
import { getApples } from './stocks.js'
test('have less then 20 apples', () => {
expect(getApples()).toBeLessThan(20)
})toEqual
수신된 값의 참조값이 같거나, 실제 구조가 같은 경우(재귀적으로 호출하여 판단)를 증명한다.
import { expect, test } from 'vitest'
const stockBill = {
type: 'apples',
count: 13,
}
const stockMary = {
type: 'apples',
count: 13,
}
test('stocks have the same properties', () => {
expect(stockBill).toEqual(stockMary)
})
test('stocks are not the same', () => {
expect(stockBill).not.toBe(stockMary)
})toStrictEqual
toEqual 과 비슷하지만 다음 부분에서 다르다.
undefined키에 대해서도 검사를 진행한다..toStrictEqual를 사용하면{a: undefined, b: 2}의 경우{b: 2}와 다르다.[, 1]과[undefined, 1]도 다르다.
- 객체 유형에 대해 검사한다. 예를 들어 a, b 프로퍼티를 가진 객체와 a, b 프로퍼티를 가진 클래스는 다르다.
import { expect, test } from 'vitest'
class Stock {
constructor(type) {
this.type = type
}
}
test('구조적으론 같으나 의미적으로 다르다.', () => {
expect(new Stock('apples')).toEqual({ type: 'apples' })
expect(new Stock('apples')).not.toStrictEqual({ type: 'apples' })
})toContain
값이 배열 안에 있는지, 어떤 하위 문자열이 특정 문자열 안에 있는지 판별할 때 쓰인다. Vitest 1.0부터는 브라우저와 유사한 환경에서 테스트하는 경우 클래스가 ClassList 안에 포함돼 있는지, 혹은 엘리먼트가 다른 엘리먼트 안에 있는지를 판별할 수도 있다.
import { expect, test } from 'vitest'
import { getAllFruits } from './stocks.js'
test('the fruit list contains orange', () => {
expect(getAllFruits()).toContain('orange')
const element = document.querySelector('#el')
expect(element.classList).toContain('flex')
expect(document.querySelector('#wrapper')).toContain(element)
})| }).toThrow(); | ||
| }); | ||
|
|
||
| test('winLotto 가 위 조건을 모두 통과하면 에러를 반환하지 않는다.', () => { |
There was a problem hiding this comment.
저는 개인적으로는 "위 조건"이라는 표현 말고 명시적으로 작성해줬으면 좋겠어요!
갠취인 것 같지만.. 여기서 갑자기 테스트 통과를 하지 못하면, 위 내용을 전부 확인해줘야 하니까요!!
There was a problem hiding this comment.
조건이 많은 경우엔 어떻게 작성해야할지 고민인데, 다음엔 다 적어봐야겠습니다!
| ({ lotto, rank }) => { | ||
| const winLotto = makeWinLottoMockingData(initialLotto); | ||
|
|
||
| expect(winLotto.checkRank(lotto)).toBe(rank); |
There was a problem hiding this comment.
헉 당첨 로또를 어떤 도메인에서 관리해야 할지 어려웠는데... 영감 받아갑니다 ❤️
| static get MAX_NUMBER() { | ||
| return 45; | ||
| } | ||
|
|
||
| static get MIN_NUMBER() { | ||
| return 1; | ||
| } | ||
|
|
||
| static get MONEY() { | ||
| return 1000; | ||
| } | ||
|
|
||
| static get DEFAULT_LENGTH() { | ||
| return 6; | ||
| } |
There was a problem hiding this comment.
Lotto 클래스를 상속하고 있는 상태인데 set, get이 중복되이는 게 보이네요!
여기 코드 보니까 뭔가 더 간결하게 작성할 수 있을 것 같은 게
_lotto와 _winLotto를 numbers로 표현하면 어떨까요?
Lotto.numbers WinLotto.numbers 이렇게 나타낼 수 있게요 !
그리고 validation은 constructor에서 검사해주면 될 것 같아요.
constructor(lotto) {
super(lotto);
WinLotto.getLottoError(lotto);
}물론 Lotto와 WinLotto에서 각각 다른 길이로 에러 검사를 하고 있는데, 길이도 매개변수로 받아서 처리하면 괜찮을 것 같기도..합니당.
이렇게 작성 하면 !!!!
WinLotto에서 get이랑 set 작성 부분을 지울 수 있을 것 같아요.
그냥 주저리주저리 의견이었습니다 ... 😊
There was a problem hiding this comment.
저도 이 부분에 대해 고민을 했어요...! 원래는 #lotto와 같이 private 변수로 선언을 해준 상태였거든요...! 그리고 둘다 constructor에서 setLotto를 통해 #lotto에 접근하고 있는데, WinLotto에서 super()를 호출하면 Lotto의 #lotto에 접근하게 되는 에러가 발생하더라구요. 그래서 private으로 사용할 경우엔 서로 다른 이름을 지어줘야하는 경우도 있는 것 같아요...! set과 get의 경우는 일부러 만들어둔 것도 있지만 중복되는 부분을 줄이는 방법에 대해선 더 생각해볼게요!
| export function changeStandardMoneyString(money) { | ||
| const reversedMoney = String(money).split('').reverse(); | ||
| const stack = []; | ||
|
|
||
| reversedMoney.forEach((num, index) => { | ||
| if (reversedMoney[index + 1] !== undefined && index % 3 === 0) { | ||
| stack.push(','); | ||
| } | ||
| stack.push(num); | ||
| }); | ||
|
|
||
| return stack.reverse().join(''); | ||
| } |
There was a problem hiding this comment.
오!! 이거 제가 참석 못했을 때 해결법은 이미 나왔던 것 같은데 그래도 답변 달아봅니다..!
new Intl.NumberFormat().format(money)이렇게 한 줄로 표현할 수 있어용 ! :)
jgjgill
left a comment
jgjgill
left a comment
There was a problem hiding this comment.
수고 많으셨습니다..! 🙇♂️
테스트 코드에서 들었던 생각은 toBeDefined이 많이 쓰이고 있더라구요.
개인적인 생각으로 저같은 경우 테스트 코드에서 궁금한 부분은 어떻게 쓰일까? 값이 뭐가 오는거지? 어떤 경우에서 에러가 발생할까? 와 같은 경우인 것 같아요..!
근데 undefined 를 반환하지 않는다.는 크게 정보를 얻기에는 도움이 되지 않는다는 생각을 했습니다..! 🙇♂️
utils 함수 내부의 파일 이름, 매개변수의 타입
으음 사용하는 용도에 따라 다를 것 같긴 합니다..!
예를 들어 유틸 함수를 관리하는 라이브러리 경우 비슷한 폴더 구조를 띄는 것 같아요..!
근데 지금은 로또가 주 관심사라는 생각이 들어서 이에 맞게 적절한 폴더명으로 바뀌지 않을까 싶네요..!
| }).not.toThrow(); | ||
| }); | ||
|
|
||
| test('get 메서드는 현재 lotto 의 값을 가져올 수 있다.', () => { |
There was a problem hiding this comment.
'현재'라는 표현이 있으면 과거의 로또 값도 가져올 수 있는 걸까요??
There was a problem hiding this comment.
종길님 리뷰를 보고 나니 헷갈릴 수 있을만한 문구를 적지 않게 조심해야겠다는 생각이 드네요 👀
| while (set.size < amount) { | ||
| set.add(generateRandomNumber(min, max)); | ||
| } |
There was a problem hiding this comment.
ㅠㅠㅠㅠㅠㅠㅠㅠㅠ 제한을 걸어두어야겠습니다 (최고의 딴지)
|
AI가 리뷰해줬어요! 역할: 1년차 프론트엔드 개발자를 위한 코드리뷰 멘토 주요 고려사항:
응답 형식:
추가 지침:
1차 리뷰코드 분석
강점
개선점
구체적 제안
학습 포인트
2차 리뷰
기존 요구사항에 새로운 요구사항이 추가되거나 변경될 경우 문제가 될 수 있는 부분은 다음과 같습니다: 예시 요구사항 추가 및 변경
각 변경 사항에 대한 피드백 및 대응 방안1. 로또 구매 한도 추가문제가 될 수 있는 부분:
대응 방안:
async function inputPurchaseLotto() {
const money = await getValidInput(view.inputPurchaseLotto, LottoRule.getMoneyError);
const quantity = LottoRule.calculateQuantity(money);
if (quantity > 10) {
throw new Error('최대 10개까지 구매할 수 있습니다.');
}
return quantity;
}2. 로또 번호 유효성 검사 강화문제가 될 수 있는 부분:
대응 방안:
class Lotto {
constructor(numbers) {
this.numbers = this.validateNumbers(numbers);
}
validateNumbers(numbers) {
if (numbers.length !== 6 || new Set(numbers).size !== 6) {
throw new Error('로또 번호는 6개의 서로 다른 숫자로 구성되어야 합니다.');
}
numbers.forEach(num => {
if (num < 1 || num > 45) {
throw new Error('로또 번호는 1부터 45 사이의 숫자여야 합니다.');
}
});
return numbers;
}
}3. 다양한 로또 게임 모드 추가문제가 될 수 있는 부분:
대응 방안:
async function selectGameMode() {
const mode = await getValidInput(view.inputGameMode, LottoRule.getGameModeError);
return mode;
}
function generateLottoNumbers(mode) {
switch (mode) {
case '자동':
return LottoRule.generateLottoNumber();
case '반자동':
return LottoRule.generateSemiAutoLottoNumber();
case '수동':
return LottoRule.generateManualLottoNumber();
default:
throw new Error('잘못된 게임 모드입니다.');
}
}총평 및 학습 포인트총평기존 코드베이스는 모듈화와 유효성 검사, 에러 처리 등이 잘 이루어져 있어 유지보수성과 확장성이 높은 편입니다. 그러나 새로운 요구사항이 추가될 때, 코드의 일부를 수정하거나 확장해야 하는 경우가 있습니다. 이러한 경우를 대비하여 함수의 역할을 명확히 분리하고, 유연한 구조를 유지하는 것이 중요합니다. 학습 포인트
|
|
@coderabbitai review please |
|
@JunilHwang, thank you for the detailed context and the previous review insights. Based on the information provided, here's my review: Code Flexibility and ScalabilityThe current implementation shows a good level of modularization which aids in maintaining flexibility. However, as noted, certain areas could be improved to better handle potential requirement changes:
Compliance with Programming RequirementsThe adherence to frontend best practices such as error handling and input validation is commendable. However, the depth of function nesting and the number of parameters in some functions could be reduced to enhance readability and maintainability. Single Responsibility Principle (SRP)The separation of concerns appears to be well managed with distinct functionalities encapsulated within dedicated classes and functions. However, considering the Single Responsibility Principle, the Module Reuse and ReusabilityThe current structure facilitates reuse to some extent, especially with functions like
Specific Suggestions
Learning Points
Overall, the codebase exhibits a solid foundation with good practices in modularization and error handling. With a few refinements, particularly in managing function complexity and enhancing modularity, the application will be well-positioned for future expansion and maintenance. Actions performedReview triggered.
|
WalkthroughThe recent updates introduce configuration files for ESLint and Prettier, enhancing code formatting and linting. New classes and methods for handling lotto operations and validations were added, along with extensive unit testing to ensure code robustness. Utility functions for array operations, number generation, string formatting, and asynchronous input reading were also introduced. These changes centralize lottery-related logic, enhance code quality, and streamline user interactions. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range, codebase verification and nitpick comments (2)
src/__tests__/LottoRule.test.js (2)
4-12: Confirm range validation ingenerateLottoNumbermethod.The test checks if the generated numbers are within the 1 to 45 range, which is a good practice. However, it might be beneficial to also confirm the length of the array to ensure exactly six numbers are generated, as expected for a Lotto draw.
lotto.forEach((num) => expect(num).greaterThanOrEqual(1).lessThanOrEqual(45), ); + expect(lotto).toHaveLength(6);
24-34: Improve test description clarity for type validation ingetMoneyError.The test checks for the return of
undefinedwhenmoneyis not of type number. The description "returns undefined when not a number type" could be clearer to explicitly state the expected behavior when invalid types are passed.- 'getMoneyError 는 money($money)가 숫자 타입이 아닌 경우 undefined 를 반환하지 않는다.', + 'getMoneyError should not return undefined when money is not of type number.',
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (16)
- .eslintrc (1 hunks)
- .prettierrc (1 hunks)
- eslint.config.js (1 hunks)
- package.json (2 hunks)
- src/View/View.js (1 hunks)
- src/tests/Lotto.test.js (1 hunks)
- src/tests/LottoRule.test.js (1 hunks)
- src/tests/WinLotto.test.js (1 hunks)
- src/domain/Lotto.js (1 hunks)
- src/domain/LottoRule.js (1 hunks)
- src/domain/WinLotto.js (1 hunks)
- src/main.js (1 hunks)
- src/utils/Array.js (1 hunks)
- src/utils/Number.js (1 hunks)
- src/utils/String.js (1 hunks)
- src/utils/readline.js (1 hunks)
Files not reviewed due to server errors (2)
- src/tests/Lotto.test.js
- src/tests/WinLotto.test.js
Files skipped from review due to trivial changes (2)
- .eslintrc
- .prettierrc
Additional comments not posted (23)
src/utils/Number.js (1)
1-3: Well-implemented utility function for generating random numbers.The function
generateRandomNumbercorrectly calculates a random number between the specified minimum and maximum values. It's a clean and efficient implementation for a commonly needed utility.eslint.config.js (1)
1-9: Proper configuration for ESLint and Prettier integration.The setup correctly integrates ESLint with Prettier, ensuring that linting and formatting are handled efficiently. The combination of global settings for both browser and node environments is a thoughtful addition, catering to cross-environment compatibility.
src/utils/Array.js (1)
3-10: Efficient implementation for calculating array intersections.The function
accorduses a Set to efficiently calculate how many elements from thecomparearray are present in thebasearray. This is a clean and efficient approach.package.json (2)
16-21: Addition of ESLint and Prettier dependencies approved.Adding these dependencies supports better code quality and consistency, which is beneficial for the project.
6-6: Query regarding the unchangedtypefield.The
typefield is updated from"module"to"module", which seems redundant. Could you clarify if this change was intentional or necessary?src/domain/WinLotto.js (3)
18-25: Error Handling insetMethodThe method correctly checks for errors before setting the lotto numbers, which is good practice. However, consider improving the clarity of the error message in
WinLotto.getLottoErrorto make it more specific about what went wrong.
50-64: Validation Logic ingetLottoErrorThe method correctly checks for common errors and specific conditions for the winning lotto numbers. However, consider adding more detailed error messages to help developers understand the validation failures better.
66-78: Detailed Validation forgetBonusNumberErrorThis method provides thorough validation for the bonus number, which is crucial for the integrity of the game logic. The detailed checks for number type and range are well-implemented.
src/View/View.js (6)
68-71: Good implementation ofprintPurchaseLotto.The method is straightforward and effectively communicates the number of lottos purchased to the user.
73-77: Effective implementation ofprintAllLotto.The method correctly iterates through the lottos and logs each one, providing clear output to the user.
93-95: Correct implementation ofprintRate.The method accurately displays the rate of return, providing clear and concise information to the user.
97-111: Proper validation in static methodsgetPostProcessFnErrorandgetErrorCheckFnError.These methods correctly ensure that the provided functions are of the correct type, which is crucial for the robustness of input handling.
50-56: Review default parameter usage ininputWinLotto.As with
inputPurchaseLotto, ensure that the default parameters are handled correctly in theinputmethod to prevent any unintended behavior.Verification successful
Default parameters in
inputWinLottoare handled correctly.The
inputmethod properly extracts and utilizes the default parameters from theoptionsobject, ensuring no unintended behavior.
inputWinLottomethod definition and usage are consistent.inputmethod handles default parameters as expected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the handling of default parameters in `inputWinLotto`. # Test: Search for usage of `inputWinLotto`. Expect: Proper handling of default parameters. rg --type python -A 5 $'inputWinLotto'Length of output: 68
Script:
#!/bin/bash # Description: Check the handling of default parameters in `inputWinLotto`. # Test: Search for the definition and usage of `inputWinLotto`. rg --type js -A 5 'inputWinLotto' # Test: Search for the definition of the `input` method to understand how it handles default parameters. rg --type js -A 10 'input('Length of output: 707
Script:
#!/bin/bash # Description: Check the handling of default parameters in `inputWinLotto`. # Test: Search for the definition and usage of `inputWinLotto`. rg --type js -A 5 'inputWinLotto' # Test: Search for the definition of the `input` method to understand how it handles default parameters. rg --type js -A 10 'input\('Length of output: 2253
59-65: Check default parameter handling ininputBonusNumber.This method, like the others, uses undefined defaults which may lead to issues if not properly managed. Ensure robust handling in the
inputmethod.src/domain/LottoRule.js (6)
5-17: Well-defined constants for lotto ranks and rewards.Using
Object.freezeensures that these objects are immutable, which is appropriate for constant values.
18-24: Effective use of utility functions for formatting strings.The
changeStandardMoneyStringfunction is used correctly to format monetary values, which enhances the readability of the constants.
26-40: Proper encapsulation of lotto configuration values.The use of static getters for configuration values like
MAX_NUMBER,MIN_NUMBER, etc., is a good practice as it encapsulates these values and allows for easy adjustments.
42-48: Correct implementation of lotto number generation.The method
generateLottoNumbereffectively uses thegenerateRandomNumberArrayutility with appropriate parameters to generate lotto numbers within the specified range.
50-57: Well-implemented method for calculating lotto ticket quantity.The method
calculateQuantitycorrectly calculates the number of tickets that can be bought and includes robust error handling to ensure that the input is valid.
60-89: Clear and correct implementation of validation methods.The methods
getMoneyErrorandgetCommonLottoErrorprovide comprehensive validation checks and return informative error messages, which enhances the robustness of the input handling.src/__tests__/LottoRule.test.js (3)
13-22: Ensure clarity in test descriptions forcalculateQuantity.The test description uses template literals to dynamically insert values, which is good for readability. However, the description could be clearer about what the test is verifying, specifically that it's checking the division of money by a constant to determine the number of lottos purchasable.
36-48: Validate error handling for boundary values ingetMoneyError.The tests for boundary values (less than the minimum and exactly at the minimum) are well-implemented. It ensures that the function correctly identifies invalid monetary inputs, which is crucial for preventing incorrect lotto purchases.
64-76: Verify range and duplication checks ingetCommonLottoError.The tests for checking the range and duplication of lotto numbers are correctly implemented. It's crucial that these checks are thorough, as they prevent the acceptance of invalid lotto tickets.
| constructor(lotto) { | ||
| super(lotto); | ||
| this.set(lotto); | ||
| } |
There was a problem hiding this comment.
Constructor Overhead and Redundancy in Initialization
The constructor calls super(lotto) and then immediately sets this.set(lotto). This duplicated handling of lotto might be unnecessary if the parent class Lotto already sets the lotto numbers correctly.
- super(lotto);
- this.set(lotto);
+ super(lotto);Consider removing the this.set(lotto) if the parent class's constructor already performs the necessary initialization.
Committable suggestion was skipped due to low confidence.
| get() { | ||
| return [...this._winLotto]; | ||
| } |
There was a problem hiding this comment.
Ambiguous Getter Method Name
The get method name is too generic. It should be more descriptive, such as getNumbers or getWinLottoNumbers, to clarify what it retrieves.
- get() {
+ getWinLottoNumbers() {Renaming the method would enhance code readability and maintainability.
Committable suggestion was skipped due to low confidence.
| export function readLineAsync(query) { | ||
| return new Promise((resolve, reject) => { | ||
| if (arguments.length !== 1) { | ||
| reject(new Error('arguments must be 1')); | ||
| } | ||
|
|
||
| if (typeof query !== 'string') { | ||
| reject(new Error('query must be string')); | ||
| } | ||
|
|
||
| const rl = readline.createInterface({ | ||
| input: process.stdin, | ||
| output: process.stdout, | ||
| }); | ||
|
|
||
| rl.question(query, (input) => { | ||
| rl.close(); | ||
| resolve(input); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Well-implemented asynchronous read function with robust error handling.
The function correctly handles errors related to the number of arguments and the type of the query. Using promises for asynchronous operations is a good practice.
Suggestion: Simplify argument count check.
Instead of using arguments.length, you could use default parameters or rest parameters to handle unexpected numbers of arguments more gracefully.
- if (arguments.length !== 1) {
+ if (!query) { // Assuming `query` is always required and must be a stringCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function readLineAsync(query) { | |
| return new Promise((resolve, reject) => { | |
| if (arguments.length !== 1) { | |
| reject(new Error('arguments must be 1')); | |
| } | |
| if (typeof query !== 'string') { | |
| reject(new Error('query must be string')); | |
| } | |
| const rl = readline.createInterface({ | |
| input: process.stdin, | |
| output: process.stdout, | |
| }); | |
| rl.question(query, (input) => { | |
| rl.close(); | |
| resolve(input); | |
| }); | |
| }); | |
| } | |
| export function readLineAsync(query) { | |
| return new Promise((resolve, reject) => { | |
| if (!query) { // Assuming `query` is always required and must be a string | |
| reject(new Error('arguments must be 1')); | |
| } | |
| if (typeof query !== 'string') { | |
| reject(new Error('query must be string')); | |
| } | |
| const rl = readline.createInterface({ | |
| input: process.stdin, | |
| output: process.stdout, | |
| }); | |
| rl.question(query, (input) => { | |
| rl.close(); | |
| resolve(input); | |
| }); | |
| }); | |
| } |
| export function changeStandardMoneyString(money) { | ||
| const reversedMoney = String(money).split('').reverse(); | ||
| const stack = []; | ||
|
|
||
| reversedMoney.forEach((num, index) => { | ||
| if (reversedMoney[index + 1] !== undefined && index % 3 === 0) { | ||
| stack.push(','); | ||
| } | ||
| stack.push(num); | ||
| }); | ||
|
|
||
| return stack.reverse().join(''); | ||
| } |
There was a problem hiding this comment.
Consider using built-in JavaScript functions for formatting.
The function changeStandardMoneyString manually formats the number, which can be simplified using Intl.NumberFormat as suggested in a previous comment. This approach is more efficient and readable.
- const reversedMoney = String(money).split('').reverse();
- const stack = [];
- reversedMoney.forEach((num, index) => {
- if (reversedMoney[index + 1] !== undefined && index % 3 === 0) {
- stack.push(',');
- }
- stack.push(num);
- });
- return stack.reverse().join('');
+ return new Intl.NumberFormat().format(money);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function changeStandardMoneyString(money) { | |
| const reversedMoney = String(money).split('').reverse(); | |
| const stack = []; | |
| reversedMoney.forEach((num, index) => { | |
| if (reversedMoney[index + 1] !== undefined && index % 3 === 0) { | |
| stack.push(','); | |
| } | |
| stack.push(num); | |
| }); | |
| return stack.reverse().join(''); | |
| } | |
| export function changeStandardMoneyString(money) { | |
| return new Intl.NumberFormat().format(money); | |
| } |
| checkRank(compareLotto) { | ||
| const compareLottoError = Lotto.getLottoError(compareLotto); | ||
| const winLottoError = WinLotto.getLottoError(this._winLotto); | ||
|
|
||
| if (compareLottoError !== undefined || winLottoError !== undefined) { | ||
| return compareLottoError || winLottoError; | ||
| } | ||
|
|
||
| const accordCount = accord( | ||
| this.get().slice(0, LottoRule.DEFAULT_LENGTH), | ||
| compareLotto, | ||
| ); | ||
| const isBonusCorrect = compareLotto.includes(this._winLotto.at(-1)); | ||
|
|
||
| return isBonusCorrect | ||
| ? LottoRule.RANK_BY_ACCORD[accordCount].bonus | ||
| : LottoRule.RANK_BY_ACCORD[accordCount].base; | ||
| } |
There was a problem hiding this comment.
Complexity in checkRank Method
This method is doing too much: it validates both lotto sets and calculates the rank. Consider splitting this into separate methods for single-responsibility and clearer error handling.
+ validateLottos(compareLotto) {
+ const errors = [
+ Lotto.getLottoError(compareLotto),
+ WinLotto.getLottoError(this._winLotto)
+ ].filter(error => error !== undefined);
+ if (errors.length > 0) return errors[0];
+ }
+ calculateRank(compareLotto) {
+ const accordCount = accord(this.getWinLottoNumbers().slice(0, LottoRule.DEFAULT_LENGTH), compareLotto);
+ const isBonusCorrect = compareLotto.includes(this._winLotto.at(-1));
+ return isBonusCorrect ? LottoRule.RANK_BY_ACCORD[accordCount].bonus : LottoRule.RANK_BY_ACCORD[accordCount].base;
+ }
- checkRank(compareLotto) {
- const compareLottoError = Lotto.getLottoError(compareLotto);
- const winLottoError = WinLotto.getLottoError(this._winLotto);
- if (compareLottoError !== undefined || winLottoError !== undefined) {
- return compareLottoError || winLottoError;
- }
- const accordCount = accord(this.get().slice(0, LottoRule.DEFAULT_LENGTH), compareLotto);
- const isBonusCorrect = compareLotto.includes(this._winLotto.at(-1));
- return isBonusCorrect ? LottoRule.RANK_BY_ACCORD[accordCount].bonus : LottoRule.RANK_BY_ACCORD[accordCount].base;
- }Splitting the method improves clarity and separation of concerns.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| checkRank(compareLotto) { | |
| const compareLottoError = Lotto.getLottoError(compareLotto); | |
| const winLottoError = WinLotto.getLottoError(this._winLotto); | |
| if (compareLottoError !== undefined || winLottoError !== undefined) { | |
| return compareLottoError || winLottoError; | |
| } | |
| const accordCount = accord( | |
| this.get().slice(0, LottoRule.DEFAULT_LENGTH), | |
| compareLotto, | |
| ); | |
| const isBonusCorrect = compareLotto.includes(this._winLotto.at(-1)); | |
| return isBonusCorrect | |
| ? LottoRule.RANK_BY_ACCORD[accordCount].bonus | |
| : LottoRule.RANK_BY_ACCORD[accordCount].base; | |
| } | |
| validateLottos(compareLotto) { | |
| const errors = [ | |
| Lotto.getLottoError(compareLotto), | |
| WinLotto.getLottoError(this._winLotto) | |
| ].filter(error => error !== undefined); | |
| if (errors.length > 0) return errors[0]; | |
| } | |
| calculateRank(compareLotto) { | |
| const accordCount = accord(this.getWinLottoNumbers().slice(0, LottoRule.DEFAULT_LENGTH), compareLotto); | |
| const isBonusCorrect = compareLotto.includes(this._winLotto.at(-1)); | |
| return isBonusCorrect ? LottoRule.RANK_BY_ACCORD[accordCount].bonus : LottoRule.RANK_BY_ACCORD[accordCount].base; | |
| } | |
| checkRank(compareLotto) { | |
| const compareLottoError = Lotto.getLottoError(compareLotto); | |
| const winLottoError = WinLotto.getLottoError(this._winLotto); | |
| if (compareLottoError !== undefined || winLottoError !== undefined) { | |
| return compareLottoError || winLottoError; | |
| } | |
| const accordCount = accord(this.get().slice(0, LottoRule.DEFAULT_LENGTH), compareLotto); | |
| const isBonusCorrect = compareLotto.includes(this._winLotto.at(-1)); | |
| return isBonusCorrect ? LottoRule.RANK_BY_ACCORD[accordCount].bonus : LottoRule.RANK_BY_ACCORD[accordCount].base; | |
| } |
| async function main() { | ||
| const view = new View(); | ||
| const money = await view.inputPurchaseLotto({ | ||
| postProcessFn: (input) => Number(input), | ||
| errorCheckFn: (input) => LottoRule.getMoneyError(input), | ||
| }); | ||
| const quantity = LottoRule.calculateQuantity(money); | ||
| const lottos = [...new Array(quantity)].map( | ||
| () => new Lotto(LottoRule.generateLottoNumber()), | ||
| ); | ||
|
|
||
| view.printPurchaseLotto(quantity); | ||
| view.printAllLotto(lottos.map((lotto) => lotto.get())); | ||
|
|
||
| const winPureLotto = await view.inputWinLotto({ | ||
| postProcessFn: (input) => input.split(',').map((el) => Number(el.trim())), | ||
| errorCheckFn: (input) => Lotto.getLottoError(input), | ||
| }); | ||
| const bonusNumber = await view.inputBonusNumber({ | ||
| postProcessFn: (input) => Number(input), | ||
| errorCheckFn: (input) => WinLotto.getBonusNumberError(input), | ||
| }); | ||
| const winLotto = new WinLotto([...winPureLotto, bonusNumber]); | ||
| const rankInfo = { | ||
| 1: 0, | ||
| 2: 0, | ||
| 3: 0, | ||
| 4: 0, | ||
| 5: 0, | ||
| }; | ||
| lottos.forEach((lotto) => { | ||
| rankInfo[winLotto.checkRank(lotto.get())] += 1; | ||
| }); | ||
| const totalMoney = Object.keys(rankInfo).reduce( | ||
| (money, rank) => money + rankInfo[rank] * LottoRule.REWARD[rank], | ||
| 0, | ||
| ); | ||
|
|
||
| view.printAccordByRank(rankInfo); | ||
| view.printRate(totalMoney / money); |
There was a problem hiding this comment.
Good use of async functions and modular interaction patterns in main function.
The function effectively uses asynchronous operations and interacts with domain classes in a modular way. This enhances readability and maintainability.
Suggestion: Further modularize the main function.
Consider breaking down the main function into smaller, more focused functions to further enhance readability and maintainability.
- async function main() {
+ async function setupLottoPurchase(view) {
+ // code for setting up lotto purchase
+ }
+ async function processWinningLotto(view) {
+ // code for processing winning lotto
+ }
+ async function main() {
+ const view = new View();
+ await setupLottoPurchase(view);
+ await processWinningLotto(view);
+ }Committable suggestion was skipped due to low confidence.
| async input( | ||
| message, | ||
| options = { | ||
| postProcessFn: undefined, | ||
| errorCheckFn: undefined, | ||
| }, | ||
| ) { | ||
| const { postProcessFn, errorCheckFn } = options; | ||
| const postProcessFnError = View.getPostProcessFnError(postProcessFn); | ||
| const errorCheckFnError = View.getErrorCheckFnError(errorCheckFn); | ||
|
|
||
| if (postProcessFnError !== undefined || errorCheckFnError !== undefined) { | ||
| throw new Error(postProcessFnError || errorCheckFnError); | ||
| } | ||
|
|
||
| while (true) { | ||
| const inputValue = await this.rl(message); | ||
| const postInputValue = postProcessFn | ||
| ? postProcessFn(inputValue) | ||
| : inputValue; | ||
| const error = errorCheckFn | ||
| ? errorCheckFn(postInputValue) | ||
| : postInputValue; | ||
|
|
||
| if (error === undefined) { | ||
| return postInputValue; | ||
| } | ||
|
|
||
| console.log(error); | ||
| } |
There was a problem hiding this comment.
Consider refactoring the input method for clarity and efficiency.
The input method handles both input processing and error handling, which could be separated for better readability and adherence to SRP. Additionally, the loop control could be made more explicit to avoid potential infinite loops in edge cases.
| async inputPurchaseLotto( | ||
| options = { | ||
| postProcessFn: undefined, | ||
| errorCheckFn: undefined, | ||
| }, | ||
| ) { | ||
| return this.input('> 구입금액을 입력해 주세요:', options); |
There was a problem hiding this comment.
Ensure consistent handling of default parameters in inputPurchaseLotto.
The method uses default parameters that are undefined, which might lead to unexpected behaviors if not handled properly in the input method. Consider providing meaningful defaults or ensuring that the input method can handle undefined values gracefully.
| printAccordByRank(rankInfo) { | ||
| const ranks = Object.keys(rankInfo); | ||
|
|
||
| console.log('당첨 통계'); | ||
| console.log('--------------------'); | ||
|
|
||
| ranks.forEach((rank) => { | ||
| console.log( | ||
| `${LottoRule.ACCROD_STRING_BY_RANK[rank]} - ${rankInfo[rank]}개`, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Well-implemented printAccordByRank, consider enhancing rank handling.
The method effectively displays rank information. Consider using a more dynamic way to handle ranks to accommodate possible changes in ranking rules without modifying the code.
| test.each([ | ||
| { lotto: 1 }, | ||
| { lotto: '1' }, | ||
| { lotto: {} }, | ||
| { lotto: undefined }, | ||
| { lotto: null }, | ||
| { lotto: function () {} }, | ||
| ])( | ||
| 'getCommonLottoError는 인자 lotto ($lotto) 가 배열 타입이 아닌 경우 undefined 를 반환하지 않는다.', | ||
| ({ lotto }) => { | ||
| expect(LottoRule.getCommonLottoError(lotto)).toBeDefined(); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Enhance test descriptions for getCommonLottoError.
The test checks if the getCommonLottoError method correctly identifies non-array inputs. It's important to also verify the behavior when the input is an empty array or an array of incorrect length, which are common edge cases in lotto number validations.
- 'getCommonLottoError는 인자 lotto ($lotto) 가 배열 타입이 아닌 경우 undefined 를 반환하지 않는다.',
+ 'getCommonLottoError should not return undefined when lotto is not a valid array type or is improperly structured.',Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.each([ | |
| { lotto: 1 }, | |
| { lotto: '1' }, | |
| { lotto: {} }, | |
| { lotto: undefined }, | |
| { lotto: null }, | |
| { lotto: function () {} }, | |
| ])( | |
| 'getCommonLottoError는 인자 lotto ($lotto) 가 배열 타입이 아닌 경우 undefined 를 반환하지 않는다.', | |
| ({ lotto }) => { | |
| expect(LottoRule.getCommonLottoError(lotto)).toBeDefined(); | |
| }, | |
| ); | |
| test.each([ | |
| { lotto: 1 }, | |
| { lotto: '1' }, | |
| { lotto: {} }, | |
| { lotto: undefined }, | |
| { lotto: null }, | |
| { lotto: function () {} }, | |
| ])( | |
| 'getCommonLottoError should not return undefined when lotto is not a valid array type or is improperly structured.', | |
| ({ lotto }) => { | |
| expect(LottoRule.getCommonLottoError(lotto)).toBeDefined(); | |
| }, | |
| ); |
5 participants
미션을 수행하며 어려웠던 점
1. 도메인 역할 분리
Lotto: 일반 로또 클래스, set 과 get 가능WinLotto: 당첨 로또 클래스, set 과 get 가능, 일반 로또와 비교 가능LottoRule: 로또의 최소 번호, 최대 번호, 기본 금액, 환전(?) 등등 로또를 구입할 때 기본 원칙이 되는 기능 & 상수 모음2. 도메인을 관리하는 코드관리
main파일에서 도메인과 뷰을 제어해주는 역할을 하고 있습니다.궁금한 점
input 값을 받아올 때 에러처리와 후처리에 대한 부분을 함수로 넘겨 View 클래스에서 처리하도록 시도해봤습니다. View 가 담당하는 역할을 늘린 것 같기도 한데 이전엔
main폴더에서 while 이 수없이 많아지던 걸 View 로 옮겨버려서 깔끔해졌다는 생각도 들어요. 좋은 시도였을까요?utils 함수 내부의 파일 이름을 Array, Number, String... 과 같이 매개변수의 타입이 어떤 것이 들어오는지에 따라 작성했는데 이 방법이 그렇게 좋은 방법은 아니라는 생각이 들어요. 보통 util 함수 내부의 파일 이름은 어떻게 관리해야 직관적인가요?
리뷰받고 싶은 점
궁금한 점과 미션 수행을 하며 어려웠던 점에 대부분 다 작성해놓은 것 같습니다...! 이외에도 눈에 띄는 점이 있다면 리뷰 남겨주세요! 🙇♀️
Summary by CodeRabbit
New Features
Viewclass for handling user interactions in the lotto application.Lotto,LottoRule, andWinLottoclasses to encapsulate lotto game logic and validation.Tests
Lotto,LottoRule, andWinLottoclasses to ensure functionality and error handling.Chores
package.json.