-
Notifications
You must be signed in to change notification settings - Fork 622
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
[로또 게임] 김유선 미션 제출합니다. #624
base: main
Are you sure you want to change the base?
Conversation
-ex) 1, 2,3, 5, 6 -> 1,2,3,4,5,6
-string 형식에서 공백을 없애고, 콤마를 기준으로 각 요소가 숫자인 배열로 변환한다
- 발행된 각 티켓이 오름차순으로 정렬되었는지 확인한다.
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.
정말 좋은 README네요,, 배우고 갑니다. 저도 마지막주차는 더 상세한 기능 목록 작성해봐야겠어요!
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.#tickets = await this.#getLottoTicketList(); | ||
this.#displayLottoTicket(this.#tickets); |
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.
저는 그냥 입력과 관련된 변수명을 지었는데 이렇게 프로그램과 맞는(ticket)변수명을 사용하는게 더 좋아보이네요! 좋은 코드 감사합니당
#validateWinningNumbers(inputWinningNumbers) { | ||
if (!inputWinningNumbers.includes(SYMBOL.separator)) { | ||
throw new Error(WINNING_NUMBER.withoutComma); | ||
} | ||
} |
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.
해당 validate는 Controller에 작성하신 이유가 있나용?
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.
// TODO: 이 테스트가 통과할 수 있게 구현 코드 작성
test('로또 번호에 중복된 숫자가 있으면 예외가 발생한다.', () => {
expect(() => {
new Lotto([1, 2, 3, 4, 5, 5]);
}).toThrow('[ERROR]');
});
로또 번호 입력을 받을 시 무조건 string으로 들어오는데 이 테스트 코드에서 Lotto는 매개변수로 배열을 받는 것을 확인할 수 있습니다.
원래라면 Lotto에서 같이 검증을 했을 것 같은데 LottoTest와 같이 Lotto의 매개변수를 배열로 넘겨받고자 이 검증을 여기서 처리를 하게 되었습니다.
저도 만들면서 더 좋은 방법이 없을까 고민하고 있던 부분이라 궁금해 하실 것 같았어요 😂
혹시 더 좋은 방법이 있다거나 개선 방향을 추천해주셔도 좋아요 🤩👍
async #getMatchStatus(tickets, winningNumbers, bonusNumber) { | ||
return new Matcher(tickets, winningNumbers, bonusNumber).matchStatus; | ||
} | ||
|
||
async #getRateOfReturn(purchaseAmount, matchStatus) { | ||
return new Calculator(purchaseAmount, matchStatus).rateOfReturn; | ||
} |
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.
말씀대로 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.
잘 짜여진 코드라고 생각합니다.
코드 보면서 많이 배우고갑니다~~
await method.call(this); | ||
} catch (error) { | ||
OutputView.printMessage(error.message); | ||
await this.#handlerErrorAndProceed(method); |
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.
메소드 15줄 제한으로 분리에 대해 고민을 하다보니 이렇게 처리를 해볼 수 있겠더라고요! 좋게 봐주셔서 감사합니다 😊
@@ -0,0 +1,5 @@ | |||
import { Random } from '@woowacourse/mission-utils'; | |||
|
|||
const RandomNumberGenerator = () => Random.pickUniqueNumbersInRange(1, 45, 6); |
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.
함수명이 랜덤 정수를 생성한다인데 실제로는 로또 번호에 사용할 1~45 사이의 6자리수를 생성하는거니,
이름을 더 명확히 표현하는것도 좋을 것 같습니다!
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 issuedLottoList = []; | ||
while (issuedLottoList.length < this.#lottoCount) { | ||
const sortedRandomNumbers = RandomNumberGenerator().sort((a, b) => a - b); | ||
issuedLottoList.push(sortedRandomNumbers); |
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.
여기서 각 LottoList들은 결국 각 Lotto 도메인 로직을 가지고 있는데
단순히 문자열을 저장하기보다, 반환받은 문자열로 Lotto인스턴스들을 생성후 반환하도록 하는 것도 좋을 것 같아요
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 matchedNumberCount = ticket.filter((number) => winningNumbers.includes(number)).length; | ||
const isBonusMatch = ticket.includes(bonusNumber); | ||
const keyOfPrizeTable = | ||
isBonusMatch && matchedNumberCount === 5 ? '5+bonus' : matchedNumberCount; |
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.
5+bonus
부분은 상수로 분리해도 괜찮아 보이네요!
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.
이 부분을 상수로 분리한다는 것이 .. 놓친 것 같아요 😂
#generate() { | ||
const issuedLottoList = []; | ||
while (issuedLottoList.length < this.#lottoCount) { | ||
const sortedRandomNumbers = RandomNumberGenerator().sort((a, b) => a - b); |
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.
저도 놓쳤던 부분인데 a b대신 좀더 구체적인 네이밍이 좋을 것 같아요!
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.
말씀대로 a, b 보다 구체적인 네이밍이 좋을 것 같네요!!
@@ -0,0 +1,19 @@ | |||
const PURCHASE_AMOUNT = Object.freeze({ | |||
notNumber: '[ERROR] 구매 금액은 숫자여야 합니다.', |
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.
[ERROR]부분은 반복되는 부분이라 따로 상수화해줘도 좋을 것 같아요!
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.
ERROR 부분을 따로 상수로 분리하는 것도 좋은 방법인 것 같아요 👍
const inputValue = await Console.readLineAsync(INFO.inputBonusNumber); | ||
return inputValue; | ||
}, | ||
}; |
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.
저는 반복되는 부분이라 생각하고 하나의 함수로 통일했는데, 나눠 놔도 한 눈에 보기 좋네요
💫 javascript-lotto
미션 - 로또
🎮 프로그램 동작 순서
✨ 기능구현목록
🔨 리팩토링목표
🧪 테스트구현목록
🗂 디렉토리구조
💌 리뷰어분들에게
로또 미션도 다들 고생많으셨어요! 소중한 리뷰 정말 감사드립니다 ✨🙇♂️ 남겨주신 리뷰 덕분에 코드에 대해 더 많이 생각하고 고민하면서 성장할 수 있는 것 같아요. 마지막까지 함께 으쌰으쌰해봐요 파이탱!!!!!!! 🔥🔥🔥💪