Conversation
pangil5634
left a comment
There was a problem hiding this comment.
3주차도 고생 많으셨습니다!
전반적으로 고생 많이 하신 모습이 보이네요!
OOP를 더 고려해보시면서, 디자인 패턴까지 신경 써보시면 더 좋아질 것 같습니다! 고생 많으셨습니다.
| async run() { | ||
| const purchaseAmount = await this.#inputHandler.readPurchaseAmount(); | ||
| const ticketCount = purchaseAmount / LOTTO_PRICE; | ||
| printNewLine(); | ||
|
|
||
| const tickets = this.#lottoService.publishLottos(ticketCount); | ||
| this.#lottoService.printTickets(tickets); | ||
|
|
||
| const winningNumbers = await this.#inputHandler.readWinningNumbers(); | ||
| const bonusNumber = await this.#inputHandler.readBonusNumber( | ||
| winningNumbers | ||
| ); | ||
|
|
||
| const stats = this.#lottoService.calculateStats( | ||
| tickets, | ||
| winningNumbers, | ||
| bonusNumber | ||
| ); | ||
| this.#lottoService.printStats(stats, purchaseAmount); | ||
| } |
There was a problem hiding this comment.
run() 안에 있는 흐름을 메소드로 분리시켜서 아래와 같이 해보면 어떨까요?
async run() {
const purchaseAmount = await this.#readPurchaseAmount();
const tickets = this.#publishTickets(purchaseAmount);
const { winningNumbers, bonusNumber } = await this.#readWinningInputs();
const stats = this.#calculateStatistics(tickets, winningNumbers, bonusNumber);
this.#printStatistics(stats, purchaseAmount);
}
async #readPurchaseAmount() {
const purchaseAmount = await this.#inputHandler.readPurchaseAmount();
printNewLine();
return purchaseAmount;
}
#publishTickets(purchaseAmount) {
const ticketCount = purchaseAmount / LOTTO_PRICE;
const tickets = this.#lottoService.publishLottos(ticketCount);
this.#lottoService.printTickets(tickets);
return tickets;
}
async #readWinningInputs() {
const winningNumbers = await this.#inputHandler.readWinningNumbers();
const bonusNumber = await this.#inputHandler.readBonusNumber(winningNumbers);
return { winningNumbers, bonusNumber };
}
#calculateStatistics(tickets, winningNumbers, bonusNumber) {
return this.#lottoService.calculateStats(tickets, winningNumbers, bonusNumber);
}
#printStatistics(stats, purchaseAmount) {
this.#lottoService.printStats(stats, purchaseAmount);
}
| if (amount < LOTTO_PRICE) { | ||
| throw new Error(ERROR_MESSAGES.PURCHASE_AMOUNT_TOO_LOW); | ||
| } | ||
| if (amount % LOTTO_PRICE !== 0) { | ||
| throw new Error(ERROR_MESSAGES.PURCHASE_AMOUNT_NOT_DIVISIBLE); | ||
| } |
There was a problem hiding this comment.
이곳 말고도 에러 메시지와 로직 결합 완화가 필요해 보입니다!
각 검증 함수에서 에러 메시지를 직접 지정하고 있다. 메시지 상수는 좋지만, 파라미터를 통해 주입하는 대신 내부에서 하드코딩하는 부분이 많은듯 해요.
에러 유형별로 ValidationError 클래스를 따로 두거나, 메시지 맵을 한 곳에서 관리하면 코드 일관성이 더 좋아질 것 같습니다!
| #parseWinningNumbers(input) { | ||
| const trimmedInput = this.#trimInput(input); | ||
| const parts = trimmedInput.split(",").map((value) => value.trim()); | ||
| if (parts.length !== LOTTO_COUNT) { | ||
| throw new Error(ERROR_MESSAGES.WINNING_NUMBERS_COUNT_INVALID); | ||
| } | ||
|
|
||
| const numbers = parts.map((numberString) => | ||
| this.#parseNumber(numberString) | ||
| ); | ||
| this.#validateLottoNumbers(numbers); | ||
|
|
||
| const unique = new Set(numbers); | ||
| if (unique.size !== LOTTO_COUNT) { | ||
| throw new Error(ERROR_MESSAGES.WINNING_NUMBERS_DUPLICATE); | ||
| } | ||
| return numbers; | ||
| } |
There was a problem hiding this comment.
#parseWinningNumbers가 입력 다듬기, 분리, 변환, 검증까지 모두 맡고 있는 것 같아요.
#splitAndTrimInput(문자열 처리)과 #validateLottoNumbers(검증)를 분리해 단일 책임 원칙을 적용하면 각 함수의 목적이 더 명확해질듯 합니다!
| calculateStats(tickets, winningNumbers, bonusNumber) { | ||
| const stats = { | ||
| three: 0, | ||
| four: 0, | ||
| five: 0, | ||
| fiveBonus: 0, | ||
| six: 0, | ||
| }; | ||
|
|
||
| for (const ticket of tickets) { | ||
| const matchCount = this.#countMatch(ticket, winningNumbers); | ||
| const hasBonus = ticket.includes(bonusNumber); | ||
| this.#applyStat(stats, matchCount, hasBonus); | ||
| } | ||
| return stats; | ||
| } |
There was a problem hiding this comment.
stats 객체의 구조가 명시적이라 이해하기 쉽지만, key 이름(three, four, fiveBonus 등)이 하드코딩되어 있어 유지보수 시 오류가 생기기 쉬울 것 같아요.
PRIZE_TABLE의 key를 그대로 사용하는 형태로 일관성을 맞추면 어떨까요?
const stats = Object.keys(PRIZE_TABLE).reduce((acc, key) => {
acc[key] = 0;
return acc;
}, {});
| #countMatch(ticket, winningNumbers) { | ||
| let matchedCount = 0; | ||
| for (const number of ticket) { | ||
| if (winningNumbers.includes(number)) { | ||
| matchedCount += 1; | ||
| } | ||
| } | ||
| return matchedCount; | ||
| } |
There was a problem hiding this comment.
단순한 루프지만, 자바스크립트 내장 메서드로 더 간결하게 표현 가능할 것 같아요.
return ticket.filter((num) => winningNumbers.includes(num)).length;
이렇게 하면 반복문과 임시 변수 없이 의도가 더 명확해질듯 합니다!
manNomi
left a comment
There was a problem hiding this comment.
코드가 정말 훌룡하다고 느꼈습니다
이번주차 코드가 정말 가장 최고인것 같네요
실력이 엄청 느신게 느껴져서 위축되는 기분이네요
피드백 드리기보다는 의견을 여쭙기만 해서 도움이 될지 모르겠지만 의견에 답을 하시는 과정에서 도움을 받으시길 바랍니다!
| ) | ||
| ); | ||
| expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("당첨 통계")); | ||
| }); |
| PURCHASE_AMOUNT: "구입금액을 입력해 주세요.\n", | ||
| WINNING_NUMBERS: "당첨 번호를 입력해 주세요.\n", | ||
| BONUS_NUMBER: "보너스 번호를 입력해 주세요.\n", | ||
| }; |
There was a problem hiding this comment.
저도 까먹는데 object.freez !!
평소 코드짤때도 잘 적용안하는데 우테코 하면서는 object.freez 에 대한 이야기를 많이 들었던것 같네요
| count: number | ||
| ): number[]; | ||
| }; | ||
| } |
| async run() { | ||
| const purchaseAmount = await this.#inputHandler.readPurchaseAmount(); | ||
| const ticketCount = purchaseAmount / LOTTO_PRICE; | ||
| printNewLine(); |
There was a problem hiding this comment.
printNewLine 도 좋지만
아예 OutputHandler를 만들어서 공통화 해보는것은 어떨까요 ??
매번 쓸필요도 없을것 같다는 느낌이 들어서요 !
| if (uniqueCount !== LOTTO_COUNT) { | ||
| throw new Error(ERROR_MESSAGES.LOTTO_NUMBERS_DUPLICATE); | ||
| } | ||
| } |
There was a problem hiding this comment.
validate 로직이 다소 가독성이 떨어지는것 같긴합니다!
증복된 validate도 있을수 있으니 util한 validate와 도메인에
결합되는 validate를 분리해보면 어떨까요!
|
|
||
| const profitRate = this.#calculateProfitRate(stats, purchaseAmount); | ||
| Console.print(`총 수익률은 ${profitRate}%입니다.`); | ||
| } |
There was a problem hiding this comment.
이런 print로직은 inputHandler처럼 outputHandler로 만들어서 처리해보면 어떨까요 ??
지금도 충분히 잘 구조화 되어있어서 좋은데 프린트 로직이 서비스보다는 뷰라는 개념으로 접근해봐도 좋을것 같아서요!
kojesung
left a comment
There was a problem hiding this comment.
늦었지만 리뷰 작성해봤습니다. 프리코스도 막바지를 향해 가고 있는데 마지막까지 화이팅입니다 ㅎㅎ 고생 많으셨어요!
| async readPurchaseAmount() { | ||
| while (true) { | ||
| const input = await Console.readLineAsync(INPUT_MESSAGES.PURCHASE_AMOUNT); | ||
| try { | ||
| return this.#parsePurchaseAmount(input); | ||
| } catch (error) { | ||
| Console.print(error.message); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
App.js에서 readPurchaseAmount()를 사용할 때 인스턴스 변수로 저장하여 사용하지만 readPurchaseAmount()는 매번 같은 인스턴스를 사용하는 형태로 보이고 cocnstructor가 불필요해보이네요! static 메서드로 인스턴스 변수 저장 없이 InputHandler.readPurchaseAmount() 형식으로 상용할 수 있을 것 같아요!
There was a problem hiding this comment.
this.#numbers = numbers;처럼 그냥 할당하는 방식은 외부에서 전달받은 배열을 직접 참조하는 방식이기 때문에 외부에서 수정이 가능한 상태라 의도치 않은 side effect를 유발할 수 있다고 합니다.
저도 다른 분 코드리뷰 하면서 배우게 된 내용인데
#62
[...numbers] 와 같은 형태로 복사하여 저장하는 방식이 안전하다고 하네요ㅎㅎ
| new Lotto(sorted); | ||
| tickets.push(sorted); |
There was a problem hiding this comment.
Lotto 클래스는 검증만을 위한 클래스인건가요?
Lotto에서 getNumbers 같은 검증된 로또 번호들을 반환하는 메서드를 만들고
lottos.push(new Lotto(numbers));와 같이 Lotto 인스턴스들을 로또 번호로 관리하는게 더욱 일관성있고 안전한 방식일 것 같아요!
| LOTTO_MAX, | ||
| LOTTO_COUNT | ||
| ); | ||
| const sorted = [...numbers].sort((first, second) => first - second); |
| #countMatch(ticket, winningNumbers) { | ||
| let matchedCount = 0; | ||
| for (const number of ticket) { | ||
| if (winningNumbers.includes(number)) { | ||
| matchedCount += 1; | ||
| } | ||
| } | ||
| return matchedCount; | ||
| } |
There was a problem hiding this comment.
uniqueCount 찾을 때처럼 Set 사용하는 방식을 사용하면서 코드 일관성과 연산 속도까지(미비하겠지만..) 챙길 수도 있겠네요!
🎰 구현 내용
1. 입력 및 검증
Console.readLineAsync()로 사용자 입력 수신[ERROR]예외 발생#trimInput()메서드로null,undefined입력도 안전하게 처리2. 게임 로직
LottoService클래스publishLottos(): 구입 금액에 따른 로또 발행Random.pickUniqueNumbersInRange()로 중복 없는 6개 번호 생성Lotto클래스#numbers필드만 유지하여 제약사항 준수3. 출력
N개를 구매했습니다.형식[1, 2, 3, 4, 5, 6]형식으로 출력"당첨 통계") 출력4. 아키텍처
InputHandler: 입력 처리 및 검증 로직 담당LottoService: 로또 발행 및 통계 계산 로직 담당App: 전체 흐름 조율constants/constants.js: 로또 상수 및 상금 테이블constants/messages.js: 에러 메시지 및 입력 프롬프트utils/output.js: 출력 관련 유틸리티 함수#parseNumber(): 숫자 파싱 로직 통합#trimInput(): 입력 trim 로직 통합NUMBER_REGEX: 정규식 패턴 상수화ERROR_MESSAGES) 적용5. 테스트
6. 프로그래밍 요구사항
else미사용process.exit()미사용.js확장자 명시