Skip to content
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

[로또] 김민경 미션 제출합니다. #641

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Kim-Mingyeong
Copy link

No description provided.

Comment on lines +53 to +63
while (true) {
try {
this.#amount = Number(await Console.readLineAsync(GAME.INPUT.AMOUNT));
this.validateAmount(this.#amount);
const count = this.#amount / 1000;
this.generateLottosList(count);
break;
} catch (error) {
throw new Error(ERROR.INPUT);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 throw error아니면
throw new Error(error.message) 해보셨나요??

#amount;
#lottos;

constructor() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

필드 선언시에

#amount =0;
#lottos=[];

으로 값을 설정하는게 더 좋을 것 같아요

inputPurchaseAmount = async () => {
while (true) {
try {
this.#amount = Number(await Console.readLineAsync(GAME.INPUT.AMOUNT));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

구매 금액에 대한 유효성 검사 후에 검사를 통과한 구매 금액은 this.#amount 의 값으로 넣어주는게 더 나을 것 같아요

this.generateLottosList(count);
break;
} catch (error) {
throw new Error(ERROR.INPUT);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validateAmount에서 오류가 나면 throw를 하기 때문에 throw문이 필요없어요. 그런데 오류 메세지를 출력하는 코드가 보이지 않네요.

generateLottosList = (count) => {
Console.print(`${count}개를 구입했습니다.`);

for (let i = 0; i < count; i += 1) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for 문 보다 Array.from({lenght:count})을 사용해서 count만큼의 배열을 생성하는 방법도 있어요.

winningNumbers.includes(number)
).length;

if (matchedCount === 6) matched[4] += 1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else문을 지양하라는 요구사항이 있어서 switch문을 사용해 보시는 거는 어떨까요?

@@ -0,0 +1,17 @@
export const GAME = Object.freeze({
INPUT: {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상수로 쓰인 변수의 속성은 대문자로 사용하지 않으셔도 돼요

@BadaHertz52
Copy link

App의 메서드들을 여러 클래스나 객체로 분리하는 것, 게임 룰에 쓰이는 매직넘버들도 상수로 관리해보는 것을 추천드려요.

@Kim-Mingyeong
Copy link
Author

Kim-Mingyeong commented Nov 20, 2023

App의 메서드들을 여러 클래스나 객체로 분리하는 것, 게임 룰에 쓰이는 매직넘버들도 상수로 관리해보는 것을 추천드려요.

와...정성 가득하게 코드리뷰 해주셔서 감사합니다🥺 리뷰 남겨주신것을 바탕으로 코드 리팩토링 진행해봐야겠어요. 이것 저것 배워갑니다.😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants