-
Notifications
You must be signed in to change notification settings - Fork 191
[로또] 김예안 미션 제출합니다. #159
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
base: main
Are you sure you want to change the base?
[로또] 김예안 미션 제출합니다. #159
Conversation
올바른 입력값을 받을 때까지 반복해서 입력값을 요청하는 기능 포함
- 파사드 패턴을 활용하여InputValidator를 공통 입력값 검증 클래스로 추상화 - 구입 금액 검증 클래스 추가
파일의 주요한 기능이 입력값 검증이라는 것을 고려하여 파일명 구체화 Validator -> InputValidator
검증 함수를 정적 프로퍼티에 추가하여 validate 실행 시 각 검증함수를 순회하며 검증하도록 개선
- 보너스 번호 검증 클래스 추가 - 입력값 검증 관련 상수 추가 - 보너스 번호 테스트 코드 추가
Lotto 클래스 내부적으로 getNumbers 메서드를 통해 값을 반환할 수 있게 되어 로또 번호를 Map의 key로 갖게 되는 것이 불필요하다고 판단. 이에 따라 issuedLottos의 데이터 구조를 Map -> 리스트 형태로 변경
기존 App.run 메서드는 게임 실행의 모든 세부 단계를 모두 포함하고 있어 가독성이 낮고 단일 책임 원칙을 위배 이에 따라 각 단계를 명확한 책임을 가진 헬퍼 메서드로 분리 run 메서드는 전체적인 흐름만 보여주는 '컨트롤러' 역할을 하도록 구조 개선
manNomi
left a comment
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.
코드가 깔끔하고 기풍이 느껴지십니다!
실력에서 느껴지는 가독성이 보기가 너무 좋았고요 덕분에 많이 배웠습니다!
|
|
||
| ### 엣지 케이스 | ||
|
|
||
| - [x] 당첨되지 않았을 때 (2개 이하로 번호 일치) |
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.
체크리스트를 만들고 꼼꼼히 작성하셨네요!!
꼼꼼하신 성격이 코드에도 느껴집니다!
|
|
||
| class App { | ||
| async run() {} | ||
| #issuedLottos = []; |
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.
필드로 구현하신 이유가있을까요 ?
run 내부에서 메소드 흐름이 그대로 가서 파라미터로 넘겨주어도 직관적으로 보였을것 같아서요 !
| ); | ||
|
|
||
| StatisticsView.printResult(rankCounts, purchaseAmount); | ||
| } |
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.
함수의 선언 순서도 중요할수 있다고 들었던것 같습니다..
run 함수가 전반적인 코드의 시작점이라 가장 상단에 존재해야 아무래도 보기 좋을것 같긴합니다 !!
흐름을 읽고 내부 구현을 들여다 보기 마련인데 최하단까지 내려가야하니까요!
| for (let i = PRIZE_INFO.length - 1; i >= 0; i--) { | ||
| const { description, prize } = PRIZE_INFO[i]; | ||
| const count = rankCounts[i]; | ||
| const prizeString = prize.toLocaleString('ko-KR'); |
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.
오 자바스크립트 내장 api로 처리하셨군요!
| @@ -0,0 +1,61 @@ | |||
| import { Console } from '@woowacourse/mission-utils'; | |||
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.
View라는 이름의 클래스인데 로직이 다소 깊을 수 있겠다는 생각이 들었습니다
View가 model의 데이터 형태를 알아야 할 정도로 로직이 깊게 박혀있어서 모듈별로 테스트 하기에 다소 힘들 수
있겠다는 생각이 들었는데 작성자분은 어떤 의도가 있으신걸까요!!
| { description: '5개 일치', prize: 1_500_000 }, | ||
| { description: '4개 일치', prize: 50_000 }, | ||
| { description: '3개 일치', prize: 5_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.
이건 constant로 분리 안하신 이유가 있을까요?
| static validate(value) { | ||
| this.#validators.forEach((validator) => validator.call(this, value)); | ||
| } | ||
| } |
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.
한개 파일에 여러 클래스가 있어서 다소 읽기 불편할 수 도있을것 같습니다!
InputValidate/
~~.js
~~.js
이렇게 나누는것은 어떨까요 !!
https://flaming.codes/ko/posts/barrel-files-in-javascript
이런것도 참고하시면 좋을것 같아요!
Antoliny0919
left a comment
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.
3주차 고생하셨습니다 ⭐
몇 가지 코멘트를 남겼어요 :)
| /** | ||
| * 구입 가능한 로또 티켓 수 계산 | ||
| */ | ||
| static #determineLottoNumber(purchaseAmount) { |
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.
determineLottoNumber보단 calculateLottoCount가 더 나을거 같습니다.
LottoNumber라고 했을때는 로또번호와 관련된 로직이 예상됩니다.
갯수를 반환하기 때문에 Count가 더 적절한거 같습니다.
| static #issueLottoTickets(purchaseCount) { | ||
| const issuedLottos = []; | ||
|
|
||
| Array.from({ length: purchaseCount }).forEach(() => { | ||
| const lottoNumbers = this.#generateLottoNumbers(); | ||
| const lotto = new Lotto(lottoNumbers); | ||
|
|
||
| issuedLottos.push(lotto); | ||
| }); | ||
|
|
||
| return issuedLottos; | ||
| } |
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 #issueLottoTickets(purchaseCount) { | |
| const issuedLottos = []; | |
| Array.from({ length: purchaseCount }).forEach(() => { | |
| const lottoNumbers = this.#generateLottoNumbers(); | |
| const lotto = new Lotto(lottoNumbers); | |
| issuedLottos.push(lotto); | |
| }); | |
| return issuedLottos; | |
| } | |
| static #issueLottoTickets(purchaseCount) { | |
| const lottos = Array.from( | |
| { length: purchaseCount }, | |
| () => { | |
| const lottoNumbers = this.#generateLottoNumbers(); | |
| const lotto = new Lotto(lottoNumbers); | |
| return lotto; | |
| } | |
| ); | |
| return lottos; | |
| } |
forEach 호출이 불필요한거 같습니다.
그리고 로또번호를 생성하고 로또를 생성하는 과정 또한 분리되었다면 더 좋을거 같습니다.
| }).toThrow('[ERROR]'); | ||
| }); | ||
|
|
||
| // TODO: 추가 기능 구현에 따른 테스트 코드 작성 |
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.
index 9abfb89..622caa7 100644
--- a/__tests__/LottoTest.js
+++ b/__tests__/LottoTest.js
@@ -1,6 +1,8 @@
import Lotto from '../src/model/Lotto';
describe('로또 클래스 테스트', () => {
+ const lotto = new Lotto([1, 2, 3, 4, 5, 6]);
+
test('로또 번호의 개수가 6개가 넘어가면 예외가 발생한다.', () => {
expect(() => {
new Lotto([1, 2, 3, 4, 5, 6, 7]);
@@ -12,4 +14,13 @@ describe('로또 클래스 테스트', () => {
new Lotto([1, 2, 3, 4, 5, 5]);
}).toThrow('[ERROR]');
});
+
+ test('로또 번호와 당첨 번호를 비교하여 일치하는 숫자 갯수를 반환합니다.', () => {
+ expect(lotto.calculateMatchCount([4, 5, 6, 7, 8, 9])).toBe(3);
+ });
+
+ test('로또 번호에 보너스 번호가 포함되었는지 확인합니다.', () => {
+ expect(lotto.hasBonusNumber(8)).toBe(false);
+ expect(lotto.hasBonusNumber(3)).toBe(true);
+ });
});테스트코드가 더 있다면 좋았을거 같습니다.
예시로 calculateMatchCount, hasBonusNumber 메서드 유닛 테스트를 작성해봤습니다 :)
geongyu09
left a comment
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.
늦은 리뷰이지만.. 코드 잘 봤습니다!!
3주차도 정말 수고 많으셨습니다!
"외부에서 호출되지 않는다"는 이유만으로 private을 사용하는 것이 맞는지, 혹은 "캡슐화를 위해 반드시 숨겨야 하는 핵심 로직"에만 사용하는 것이 맞는지 등, private을 적용하는 더 명확한 기준이나 모범 사례가 있는지 리뷰어분들의 의견을 듣고 싶습니다.
저도 객체지향언어를 사용한 경험이 없다보니 객체지향적으로 매우 앝은 지식만 가지고 있습니다.
그래서인지, "외부에서 사용하지 않는 내부의 값" 정도로 생각하고 있고, private 사용 기준 또한 외부에서 호출을 원하지 않는다면 사용하면 된다 정도로 생각합니다!
추가적으로 이번에 코드를 작성하면서 private 메서드는 테스트가 안되다보니까 '잘 동작하겠지?' 라는 믿음으로 사용해야하는 문제점도 있는 것 같다고 느꼈습니다!
private 사용에 대한 또 다른 기준을 둔다면, 이를 테스트를 해야하는지, 안해도 되는지로 나누는 것도 좋지 않을까 싶습니다!
| static run(purchaseAmount) { | ||
| const purchaseCount = this.#determineLottoNumber(purchaseAmount); | ||
| Console.print(IO_MESSAGE.PURCHASE_COUNT_OUTPUT(purchaseCount)); | ||
|
|
||
| return this.#issueLottoTickets(purchaseCount); | ||
| } | ||
| } |
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.
해당 메서드는 구매 개수를 출력하고, 로또를 반환해주는 동작을 하는 것 같습니다
하나의 함수에서 여러 책임을 가지는 것에서 테스트 하기 어렵지 않을까 싶습니다!
이에 더해서, run이라는 메서드 명이 해당 메서드의 동작을 완전히 설명해주지는 못하는 것 같습니다
저도 좋은 메서드명이 떠오르지는 않는데, 이는 아마 메서드에서 많은 동작을 하는 것이 그 원인이 되지 않을까 싶기도 해요!
출력하는 로직은 해당 클래스가 아니라 다른 곳에서 관리하는 것은 어떠신가요?!
| /** | ||
| * 4. 당첨 번호와 보너스 번호를 입력받고 파싱 | ||
| */ | ||
| async getWinningNumbers() { |
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.
getWinningNumbers라는 메서드 이름에서 당첨 번호 배열을 반환하는 것으로 이해를 하고 읽어 내려갔습니다..!
실제로 리턴하는 값은 winningNumber와 bonusNumber 두 값을 가지는 객체를 반환하는 것 같습니다..!
getWinningNumbersAndBonus()와 같이 조금 더 명확한 이름으로 변경해보는 것은 어떠신가요?!
폴더 구조
주요 기능 및 실행 흐름
App.js가 컨트롤러(Controller) 역할을 맡아, model과 view를 조율하여 전체 흐름을 다음과 같이 실행합니다.
[입력] Input 뷰를 호출해 구입 금액을 입력받습니다.
[발행] LottoMachine 모델에 구입 금액을 전달해 Lotto 객체 배열을 생성하고, 발행된 로또 목록을 출력합니다.
[입력] 다시 Input 뷰를 호출해 당첨 번호와 보너스 번호를 입력받습니다.
[계산] 입력받은 당첨 번호와 보너스 번호를 발행된 로또 목록과 함께 RankCalculator 모델에 전달하여 최종 당첨 통계(rankCounts)를 받습니다.
[출력] StatisticsView에 이 통계와 총 구입 금액을 전달하여, 최종 결과 및 수익률 출력을 위임합니다.
리뷰어 분들께
private의 사용 기준이 궁금합니다.2주 차 피드백을 바탕으로 "외부에서 호출되지 않는 내부 헬퍼 메서드"들을 private으로 선언했습니다.
하지만 이렇게 메서드를 캡슐화하여 사용하는 것이 익숙하지 않아 적절하게 사용했는지에 대한 의문점이 남았습니다.
"외부에서 호출되지 않는다"는 이유만으로 private을 사용하는 것이 맞는지,
혹은 "캡슐화를 위해 반드시 숨겨야 하는 핵심 로직"에만 사용하는 것이 맞는지 등,
private을 적용하는 더 명확한 기준이나 모범 사례가 있는지 리뷰어분들의 의견을 듣고 싶습니다.
물론 그 외 로직, 테스트 케이스, 네이밍 등 다른 어떤 피드백이라도 편하게 남겨주시면 감사하겠습니다!