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

[로또] 박지섭 미션 제출합니다. #2

Open
wants to merge 40 commits into
base: HarrySeop
Choose a base branch
from

Conversation

HarrySeop
Copy link

로또 게임 미션 제출

궁금한 점

  • 제공된 Lotto클래스를 기반으로 다른 기능들도 비슷하게 구현했습니다. 아직 클래스를 잘 이해하지 못한 것 같아서 확인 부탁드립니다..!
  • jest를 처음 사용해봤습니다! 기본적인 문법만 사용해서 구현해봤고, 틀리게 사용한 jest 코드가 있는지 확인 부탁드립니다!
  • 코드 관련 궁금한 점은 아래 구현 실패한 부분에 작성해 뒀습니다!

구현 실패한 부분

  • ApplicationTest 테스트를 통과하지 못했습니다.
 RUNS  __tests__/ApplicationTest.js
F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Purchase.js:67
        throw new Error(_Message.ERROR_MESSAGE.non_divisible_money);
              ^

Error: [ERROR] 구입금액은 1,000원 단위로 입력해야 합니다.
    at Purchase.checkNotMultipleOfThousand (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Purchase.js:67:15)
    at Purchase.validate (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Purchase.js:40:12)
    at new Purchase (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Purchase.js:30:10)
    at Game._callee2$ (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Game.js:75:31)
    at tryCatch (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Game.js:17:1062)
    at Generator.<anonymous> (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Game.js:17:3012)
    at Generator.next (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Game.js:17:1699)
    at asyncGeneratorStep (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Game.js:18:103)
    at _next (F:\MyGithub\GDSC_Pair_Programming\javascript-lotto\src\Game.js:19:194)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

테스트 코드를 분석해봤지만 1000j를 입력받았을 때 에러를 반환하는게 맞는 코드인 것 같지만, 테스트코드 통과된 에러문이 아닌 것 같아서 확인 부탁드립니다..

  • 수익률을 출력하는 문에서 세자리마다 ,를 나오게 하면 소수점이 사라지고, 소수점을 출력하게 하면 세자리마다 , 가 사라지는걸 고치지 못했습니다.
    (정규식을 아직 이해하지 못해서 사용하지 않고 구현해보려고 노력했습니다.)

느낀 점

내장함수, 고급함수 등을 너무 몰라서 구현하다가 막힌 부분이 많았습니다.(리액트를 사용한 개발보다.. 자바스크립트 공부를 우선시 해야할 것 같습니다..) 그래서 페어끼리 로또 당첨 결과 반환까지만 같이 구현 한 뒤, 시간이 부족해서 이후 당첨 통계 출력문, 총 수익률 출력은 각자 구현해서 제출합니다. 그래도 포기하지않고 같이 해준 페어분들 감사합니다.

HarrySeop and others added 30 commits May 7, 2024 23:41

- [ ] ,가 맨 앞에 오는 경우
- [ ] ,가 맨 뒤에 오는 경우
- [ ] ,가 중복되서 입력 되는 경우
Copy link
Author

Choose a reason for hiding this comment

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

해당 유효성 검사도 구현하려고 했으나 실패했었고 우선순위에서 밀린다고 생각해서 포기하고 해당 미션을 먼저 수행했습니다. jest를 통한 검사가 아닌 node를 통해 실행해서 검사하는 과정에서 해당 조건을 입력하면 [ERROR] 로또 번호는 1부터 45 사이의 숫자여야 합니다. 를 출력하여 본의아니게(?) 검사과정에서 걸러졌습니다.
이런 경우에 추가적으로 유효성 검사를 추가하는 것이 좋은지 아니면 검사할 필요가 없는 조건인지 확인부탁드립니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

지섭님의 말씀대로 해당 조건과 다른 구현사항을 비교해보았을 때, 우선순위에서 밀리는 것에 저도 동의합니다!
다만 요렇게 꼼꼼하게 입력에 대해 검증하는 것도 필요해요!

테스트 한다면 InputView로 넘어오는 입력값을 검증하면 되겠죠?!


showProfit() {
const statistics = new Statistics(this.lottos, this.winningNumbers, this.bonusNumber);
const rankResults = statistics.getRankResults();
Copy link
Author

Choose a reason for hiding this comment

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

showStatistics함수와 showProfit함수에 statistics를 두번 불러서 사용합니다. Statistics의 다른 함수를 사용하니까 이렇게 구현하는 것이 맞는지 아니면 다른 방법으로 구현하는게 좋은지 확인 부탁드립니다..!

Copy link
Collaborator

Choose a reason for hiding this comment

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

같은 인스턴스를 두번 생성하고 있는데 전 지금방법도 현재의 상황에선, 괜찮을 거라고 생각해요!
중복적인 인스턴스를 성능에 무리가 갈 정도로 생성하는 것도 아니라, 지금방식도 좋습니다 👍

this.checkNunNumber(numbers);
this.checkLottoRange(numbers);
this.checkDuplicateNumber(numbers);
// this.checkEmpty(numbers);
Copy link
Author

Choose a reason for hiding this comment

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

해당 주석이 ,를 기준으로 공백일때를 검사하려했다가 실패해서 주석한 함수입니다..!

Copy link
Collaborator

Choose a reason for hiding this comment

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

this.checkEmpty(numbers)의 코드를 올려주세욤 👍 👍

const formattedResults = this.formatResults();
const resultsWithHeader = `\n당첨통계\n---\n${formattedResults}`;
return resultsWithHeader;
}
Copy link
Author

@HarrySeop HarrySeop May 13, 2024

Choose a reason for hiding this comment

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

해당 부분을 원래는 Message.js에서 statistics를 매개변수로 받아서 구현하려고했습니다.

formatResults() {
    const resultStrings = [];
    Object.entries(LOTTO_RESULT).forEach(([rank, { match, bonus, prize }]) => {
      const count = this.#results[rank];
      const message = `${match}개 일치${bonus ? ', 보너스 볼 일치' : ''} (${prize.toLocaleString()}원) - ${count}개`;
      resultStrings.push(message);
    });
    return resultStrings.join('\n');
  }

  getResults() {
    return OUTPUT_MESSAGE.winning_statistics(this.formatResults());
  }
export const OUTPUT_MESSAGE = Object.freeze({
  winning_statistics: (statistics) => `\n당첨통계\n---\n${statistics}`,
});

다만 이렇게 구현했을 시,

당첨통계
---

해당 문구가 두번 출력되는 문자가 생겨서 해당 코드로 수정했습니다. 수정 후 정상적으로 출력이 되지만, 지금 올린 코드로 구현했을 때 어느 부분에 의해서 당첨 통계 문구가 중복 출력됐는지 궁금합니다..!

Copy link
Collaborator

Choose a reason for hiding this comment

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

OutputView파일의 showStatistics를 다음과 같이 바꾸면 됩니당!
매개변수로 넘어오는 statistics를 잘 추적해보시면 알 수 있을 거에요!

static showStatistics(statistics) {
   Console.print(statistics);
}

image

Copy link
Collaborator

@khj0426 khj0426 left a comment

Choose a reason for hiding this comment

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

테스트 코드를 분석해봤지만 1000j를 입력받았을 때 에러를 반환하는게 맞는 코드인 것 같지만, 테스트코드 통과된 에러문이 아닌 것 같아서 확인 부탁드립니다..

<- 먼저 에러를 던지고 어디서 잡을지 정리하는 것이 필요해 보입니다.
Pruchase에서 에러를 던지고 이를 잡는 측이 어딘지 정리가 필요해요!

이건 개발을 할 때도 마찬가지인데, 에러를 던지는 측이 있으면 에러를 받아서 적절히 처리하는 곳이 어딘지도 꼭 정리하고 가셔야 합니다!

예를 들어 Purchase를 사용하는 game에서 에러가 전파되어서 App으로 전파될 때 에러를 잡겠다 <- 이런식으로 꼭 에러를 잡는 곳을 정하고 가셔야 합니당!!

async start() {
    try {
      await this.getPurchaseAmount();
      this.showLottoNumbers();
      await this.getWinningNumbers();
      await this.getBonusNumber();
      this.showStatistics();
      this.showProfit();
    } catch (e) {
      throw e;
    }
  }

이런식으로 던지게 되면 App에서 전파된 에러를 받을 수 있겠죠??

async play() {
    try {
      const game = new Game();
      await game.start();
    } catch (e) {
      throw e;
    }
  }

그리고 App.js에서 async키워드를 생략하였는데 붙여주심 좋을 거 같아윰!

 async play() {
    const game = new Game();
    await game.start();
  }

const formattedResults = this.formatResults();
const resultsWithHeader = `\n당첨통계\n---\n${formattedResults}`;
return resultsWithHeader;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

OutputView파일의 showStatistics를 다음과 같이 바꾸면 됩니당!
매개변수로 넘어오는 statistics를 잘 추적해보시면 알 수 있을 거에요!

static showStatistics(statistics) {
   Console.print(statistics);
}

image


showProfit() {
const statistics = new Statistics(this.lottos, this.winningNumbers, this.bonusNumber);
const rankResults = statistics.getRankResults();
Copy link
Collaborator

Choose a reason for hiding this comment

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

같은 인스턴스를 두번 생성하고 있는데 전 지금방법도 현재의 상황에선, 괜찮을 거라고 생각해요!
중복적인 인스턴스를 성능에 무리가 갈 정도로 생성하는 것도 아니라, 지금방식도 좋습니다 👍


- [ ] ,가 맨 앞에 오는 경우
- [ ] ,가 맨 뒤에 오는 경우
- [ ] ,가 중복되서 입력 되는 경우
Copy link
Collaborator

Choose a reason for hiding this comment

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

지섭님의 말씀대로 해당 조건과 다른 구현사항을 비교해보았을 때, 우선순위에서 밀리는 것에 저도 동의합니다!
다만 요렇게 꼼꼼하게 입력에 대해 검증하는 것도 필요해요!

테스트 한다면 InputView로 넘어오는 입력값을 검증하면 되겠죠?!

this.checkNunNumber(numbers);
this.checkLottoRange(numbers);
this.checkDuplicateNumber(numbers);
// this.checkEmpty(numbers);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this.checkEmpty(numbers)의 코드를 올려주세욤 👍 👍

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.

4 participants