[1단계 - 콘솔 기반 로또 게임] 에프이(박철민) 미션 제출합니다.#293
Conversation
liswktjs
left a comment
There was a problem hiding this comment.
안녕하세요 에프이
이번에 리뷰를 맡게 된 샐리라고 합니다 👋
커밋 로그를 살펴보았을 때 테스트코드와 기능을 같이 커밋 해주셔서 이해가 빨랐습니다 👍
tdd를 진행하면서 많은 고민을 해주신 것 같아요
고민을 많이 해주신 만큼 읽기 좋은 구조가 만들어 졌다고 생각합니다
이와 관련해서 red-green-refactor를 생각해주시면 좋을 것 같아요
(아마 수업시간에 배우신 것 같긴 하지만 혹시 몰라 남겨봅니다!)
그럼 남은 미션도 파이팅입니다 💪
| async readPurchaseAmount() { | ||
| try { | ||
| const purchaseAmountInput = await Private.readPurchaseAmount(); | ||
| const purchaseAmount = purchaseAmountInput.trim(); | ||
| purchaseAmountValidator.validate(purchaseAmount); | ||
| return purchaseAmount; | ||
| } catch (error) { | ||
| OutputView.print(error.message); | ||
| return this.readPurchaseAmount(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
input에 대한 유효성 검사를 이곳에서 해주신 것 관련해서 질문을 주셨는데요
input에서 유효성 검사를 해서 넘겨주는 지금 방식이 현재 작성해준 코드 구조상 더 깔끔하다고 느껴집니다!
지금은 TDD개발 방법론을 하고 있다 보니 우선 요구사항에 맞는 함수를 개발한 뒤에 이걸 어떻게 넣을지 구조를 고민하는 단 계를 리팩토링 단계에 넣는 방식으로 진행하면 좋을 것 같아요
추가로 경험치가 쌓일 수록 구조를 파악하거나 짜는 능력이 늘어난다고 생각해요
조급하게 생각하실 필요 없이 천천히 경험치를 쌓으면 좋을 것 같습니다 😊
TDD관련 도움이 되는 글이 하나 있어서 공유드립니다!
There was a problem hiding this comment.
저도 개발 과정에서 모든 input들을 단일 객체로 만들고 있지 않기 때문에 InputView에서 유효성 검사를 하는 편이 조금 더 합리적이라고 생각이 들었어요. input이 저마다 단일 객체가 되었던 지난 미션에서는 해당 객체의 생성자에서 유효성 검사를 하는 것이 더 합리적이라 생각했었어요. 결국 이것도 코드 구조마다 다를 것 같은데요, 구조에 더 적합한 유효성 검사의 위치를 판단하는 저만의 기준을 갖는 방향으로 공부를 하는 것이 좋을까요? 샐리의 의견이 궁금합니다!
좋은 글 감사합니다😊
There was a problem hiding this comment.
구조를 정해서 생각하는 것 보다 어떤 요구 조건, 어떤 기능을 구현하는지에 따라 달라질 것 같아요!
기준을 지금 당장 정하기 보다는 우테코 생활을 하면서 좀 더 경험을 쌓으면 좋을 것 같아요 👍
src/views/InputView.js
Outdated
| async readWinningNumbers() { | ||
| try { | ||
| const winningNumbersInput = await Private.readWinningNumbers(); | ||
| const winningNumbers = winningNumbersInput.split(CONFIG.SEPARATOR).map(number => parseInt(number.trim(), 10)); |
There was a problem hiding this comment.
이 한 라인에 연산이 여러번 일어나고 있는데요
10라인을 넘어가면 안 된다는 컨벤션에 따라서 한 번 함수로 분리해주는 것도 좋을 것 같습니다
There was a problem hiding this comment.
async readWinningNumbers() {
try {
const winningNumbersInput = await Private.readWinningNumbers();
const winningNumbers = winningNumbersInput.split(CONFIG.SEPARATOR).map(number => parseInt(number.trim(), 10));
winningNumbersValidator.validate(winningNumbers);
return winningNumbers;
} catch (error) {
OutputView.print(error.message);
return this.readWinningNumbers();
}
},해당 메서드 전체입니다. 궁금한 부분이 있습니다.
-
메서드의 길이를 따질 때 저는 가장 바깥 중괄호를 제외한 내부의 body 라인만 세는 것이 맞다고 생각해요. 그래서 44번 줄이 두 줄로 분리가 되어도 총 10줄로 컨벤션을 만족하고 있다고 생각했는데요, 어떻게 생각하시는지 궁금해요.
-
연산이 여러 번 일어나고 있는 것이 dot notation을 2번 사용해서 그런 것인지, 혹은 split과 map 이외에도 콜백 함수로 parseInt, trim이 포함되어서 그렇게 보신 건지 궁금합니다! 개인적으로 2번의 메서드 체이닝 정도는 괜찮다고 생각하는데요, 이 부분에서도 혹시 샐리는 어떻게 생각하시는지 궁금해요.
유효성 검사와 데이터를 가공하는 순서에 오류가 있어서 이를 고치면서 자연스럽게 라인이 2개로 나뉘어졌습니다. 😊
There was a problem hiding this comment.
- 기존에 구현해주신 코드들은 10줄이긴 맞지만 로직들 사이에 빈 라인이 없어서 가독성이 떨어진다고 생각했습니다!
const winningNumbers = winningNumbersInput.split(CONFIG.SEPARATOR).map(number => parseInt(number.trim(), 10));
- 경우에 따라 다르겠지만 현재 winningNumbers의 경우에는 분리가 필요하다고 생각합니다
- 왜냐하면 기획에 따라 winningNumbers를 만드는 기준이 바뀔 수 있는데 그럴때 대응하기가 힘들어집니다
| const lotto = []; | ||
| while (lotto.length < CONFIG.LOTTO_RANK_LENGTH) { | ||
| const randomNumber = Math.ceil(Math.random() * CONFIG.MAX_LOTTO_NUMBER); | ||
| if (lotto.includes(randomNumber)) continue; | ||
| lotto.push(randomNumber); | ||
| } |
There was a problem hiding this comment.
while문을 사용하면 자칫하면 무한 루프에 빠질 수 있기 때문에 프로덕트 코드에 사용하기에는 위험한 측면이 있습니다
지금 부터 while보다는 array method를 사용해서 개발하는 습관을 들이면 좋을 것 같아요!
There was a problem hiding this comment.
array method라는 힌트를 주셔서 프리코스에서 사용했던 util 함수를 다시 살펴볼 수 있었어요! random 값을 이용해서 1~45가 들어있는 배열을 무작위 정렬한 뒤 처음 6개의 값을 slice해서 중복되지 않는 6개의 숫자를 뽑을 수 있을 것 같습니다. 다만 매개변수 개수 요구 사항 때문에 숫자 범위의 시작과 끝, 그리고 뽑을 숫자의 개수를 모두 인자로 받을 수 없어서 util 함수로 분리하지는 못했습니다.
There was a problem hiding this comment.
while문이 충분히 개발자의 실수로, 혹은 개발 과정에서 불완전한 코드로 인해 무한 루프가 발생할 수 있을 것 같네요. 현업에서는 while문을 최대한 사용하지 않는 방향으로 개발하는지 궁금합니다!
There was a problem hiding this comment.
현업에서 while문을 최대한 사용하지 않습니다!
왜냐하면 프론트가 사용하는 값은 서버에 따라서도 변동적이고 유저가 입력하는 값에 따라서도 달라지기 때문에 무한루프에 빠질 위험도 있습니다
추가로, 함수형프로그래밍에 대해서 더 공부해보시면 좋을 것 같습니다
__tests__/lotteryMachine.test.js
Outdated
| import LotteryMachine from '../src/domain/services/LotteryMachine'; | ||
|
|
||
| describe('로또 발행 테스트', () => { | ||
| test('구입 금액에 해당하는 만큼 로또를 발행한다.', () => { |
There was a problem hiding this comment.
기능 명세를 작성한다는 접근으로
테스트 명세를 더 자세히 적어주시면 좋을 것 같아요
1000원 단위로 나누어 떨어지면서 로또를 발행해주는 것이 핵심 로직이니 이 설명이 들어가면 좋을 것 같습니다
…validateWinningNumber로 분리
|
안녕하세요 샐리! 피드백 내용을 바탕으로 리팩토링 및 버그 수정 과정을 거쳤습니다 :) 수정한 부분LottoController의 run 함수 분리
while문 대체
상수의 관심사 별 분리
Message를 더 상세하게
InputView 리팩토링
test 문구 보완
유효성 검사 보완
앞서 코멘트 주신 부분에 추가적으로 궁금한 부분을 댓글로 달았습니다. 확인 부탁드려요! |
liswktjs
left a comment
There was a problem hiding this comment.
안녕하세요! 에프이
좋은 주말보내셨나요?
pr 코멘트 남겨 주신거 확인했습니다!
추가적으로 코멘트 남겼으니 2단계 반영하면서 같이 반영해주세요 👍
로또 미션 2단계도 파이팅입니다 🎱
| this.#processLottoResult(matchedResultList); | ||
| this.#processProfit(matchedResultList); |
There was a problem hiding this comment.
process라는 네이밍으로는 함수가 어떤 역할을 하는지 잘 와닿지 않는 것 같아요!
한번 수정해보시면 어떨까요?
There was a problem hiding this comment.
process 함수에는 각각 값을 구하고, 출력하는 로직이 들어가 있습니다. 그 처리하는 과정에 초점을 맞추다 보니 process라는 이름을 앞에 붙이게 되었습니다. showPurchaseResult 함수처럼 일관성 있게 showLottoResult, showProfit으로 설정하는 것도 괜찮을 것 같아서 show로 수정했습니다.
p.s. show는 hide와 반대되는 이름으로 많이 쓰인다고 하는데, 콘솔 기반의 step1에서는 이를 고려하지 않아도 될 것 같습니다. 어떻게 생각하시나요?
There was a problem hiding this comment.
현재 1단계가 콘솔 기반의 구현이긴 하지만
process~관련 함수들이 현재 OutputView 로직들이 구현되어 있기 때문에 show도 적절하다고 생각합니다 👍
| #pickUniqueLottoNumbers() { | ||
| const numberList = Array.from({ length: CONFIG_LOTTO.MAX_LOTTO_NUMBER }, (_, i) => i + 1); | ||
| numberList.sort(() => Math.random() - 0.5); | ||
| return numberList.slice(0, CONFIG_LOTTO.LOTTO_LENGTH); |
There was a problem hiding this comment.
로직을 상수로 어떻게 분리할 수 있는지 잘 모르겠습니다..! 힌트를 구할 수 있을까요?
There was a problem hiding this comment.
아하 sort 쪽을 한 번 const 변수로 할당해주고 진행하면 좋을 것 같다는 의견이였습니다!
현재 numberList에 접근해서 매번 바꿔주고 있는데 이를 한 번 변수 할당을 통해 끊어주고 진행하면 좋을 것 같습니다
| let winningNumbers = winningNumbersInput.split(CONFIG_FORMAT.SEPARATOR); | ||
| winningNumbersValidator.validate(winningNumbers); | ||
| winningNumbers = winningNumbers.map(number => parseInt(number.trim(), 10)); | ||
| return winningNumbers; |
There was a problem hiding this comment.
배열을 let으로 선언할 필요가 있을까요?
재할당 할 필요 없이 바로 return해도 무방할 것 같습니다!
There was a problem hiding this comment.
배열을 const로 선언하고, map()를 적용한 결과를 바로 return하는 것이 더 깔끔할 것 같네요!
이 부분은 수정하겠습니다.
안녕하세요! 에프이입니다 :)
앱 실행 방법
구현 사항
설계 방법
프로그램 구조
LotteryMachine
Lotto
index
LottoController
LottoService
프로그램 종료 이후 재시작 여부를 입력받을 때, 대소문자 모두 인식하도록 toLowerCase()를 사용했습니다.
고민했던 점
예를 들어, 0으로 끝나는 소수 부분을 표현하기에 float보다 string이 적합하다고 생각했습니다. (parseFloat의 경우 62.500 -> 62.5를 반환하기 때문)
이번 미션은 스스로도 부족한 부분이 많다고 생각합니다. 감사합니다😊