[1단계 - 콘솔 기반 로또 게임] 기린(정유정) 미션 제출합니다. by jeongyou · Pull Request #356 · woowacourse/javascript-lotto

jeongyou · 2025-02-20T08:35:49Z

학습 목표

이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.

UI와 도메인 영역을 분리할 수 있는 설계를 고민해보고, 목적에 맞게 객체와 함수를 활용
TDD 방식으로 개발하며, 단위 테스트 기반으로 점진적인 리팩터링

제출 전 체크 리스트

기능 요구 사항을 모두 구현했고, 정상적으로 동작하는지 확인했나요?
기본적인 프로그래밍 요구 사항을 준수하고 있는지 확인했나요?
테스트 코드는 모두 정상적으로 실행되나요?

리뷰 요청 & 논의하고 싶은 내용

1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점

TDD 방식을 이해하는데 오래 걸렸고 TDD 사이클을 지키면서 코드를 작성하는게 가장 어려웠습니다.

2) 이번 리뷰를 통해 논의하고 싶은 부분

TDD로 기능을 개발 한 후 에러가 없다고 생각했는데 앱을 실행했을 때는 TDD만으로 알 수 없는 오류들이 많이 발생했습니다. 이렇게 되면 테스트를 먼저 작성하고 통과시키고, 앱을 실행시켜 오류를 확인하고 오류가 있다면 다시 테스트를 작성하는 과정을 반복하게 되는데 조금 번거롭다는 생각이 듭니다. 원래 이런 방식으로 구현하는건가요?
LottoController의 start() 함수가 길어져 기능별로 함수를 나눴습니다. 하지만 이렇게 분리한 함수가 하나의 기능만 하는지 궁금합니다. 예를 들어 #processWinningResult()를 보면 당첨 통계에 관련한 함수라는 점에서 기능별로 분리가 된 것 같다고도 생각이 들고, 계산하는 로직과 출력하는 로직이 하나의 함수에 들어있어 두개의 기능을하는 것처럼 보여지기도 합니다. 제가 나눈 방법은 함수가 하나의 기능을 하는것이 맞는지 궁금합니다.
상수화를 할 때 view에서 사용하는 상수는 view 폴더에 넣고, domain과 test에서 사용하는 상수는 공통적으로 사용되기 때문에 constants 폴더에 common.js에서 정의하고 domain과 test에서 사용하였습니다. 여기서 궁금한 점은 원래 view, domain, controller, test 폴더에 각각따로 두는것과 공통 상수 파일을 하나 두고 거기서 사용하는것이 좋은지 궁금합니다. 그리고 하나 더 궁금한 것은 test에서도 상수를 사용하는게 맞는지 궁금합니다. 굳이 production 코드가 아닌 test에서까지 굳이 상수를 사용해야 하는지 모르겠습니다.

✅ 리뷰어 체크 포인트

### 1단계 - [ ] TDD를 활용해 기능을 구현하는 과정에서 적절한 테스트 우선 접근 방식을 적용했는가? 단위 테스트의 커버리지는 충분한가? - [ ] 도메인과 UI의 관심사를 분리하여 적절한 모듈화가 이루어졌는가? 하나의 객체나 모듈이 너무 많은 책임을 가지고 있지는 않은가? - [ ] 객체의 프로퍼티를 직접 조작하기보다 메시지를 던지고 있는가? - [ ] 불필요한 클래스를 사용하지 않고, 함수를 적극적으로 활용하여 JavaScript다운 방식으로 로직을 구현했는가?

2단계

도메인 로직에 불필요한 영향을 주지 않고 UI 변경에 대응했는가?
DOM 조작과 이벤트 활용을 JavaScript의 개념에 맞게 이해하고 적절하게 적용했는가?
웹 표준을 준수하는 마크업을 활용하며, 스타일 작성에 일관성이 있는가?

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

- 에러 메세지 출력 - 생성된 로또 출력 Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

- buyLotto : 로또 구매 - getWinningNumbers : 당첨 번호, 보너스 번호 입력 - processWinningResult : 당첨 통계 계산 후 출력 - processPrizeResult : 수익률 계산 후 출력 Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

JUDONGHYEOK

안녕하세요! 기린 이번 로또게임에서 리뷰를 맡게된 동키콩입니다~ 잘부탁드려요!! 기린이 경험한 TDD는 어떠셨나요? PR Description을 보니 인내심을 갖고 차근차근 적용해주신 것 같아요😎 TDD에 대한 다양한 생각을 가질 수 있게 된 만큼 좋은 경험이 되었을거라 생각됩니다!

코드를 보면서 테스트 코드를 꼼꼼하게 짜려고 노력하셨구나라는 것을 많이 느꼈어요. TDD를 수행하셔서 커버리지가 높은 것은 어찌보면 TDD의 장점이라고도 할 수 있겠죠? 그만큼 테스트 코드들을 도메인별로 분리하여 짜주신것 같아요. 그런데 지금은 객체지향적인 코드를 조금 놓치지않았나 생각이 들기도 해요.

또 도메인 측면에서 행동으로 정의되는 것에 대해서 추가적으로 테스트를 작성할 수 있을 것 같기도 하구요. 아마 첫번째 질문으로 남겨주신 앱을 실행했을 때는 TDD만으로 알 수 없는 오류들이 많이 발생했습니다가 이런 측면에서 발생했을 수도 있을 것 같다고 생각이 되네요. TDD라는게 단순히 테스트코드만 먼저 작성하는 것이 아니라 비즈니스 로직에 대한 테스트 신뢰도가 충분히 쌓일 때까지 과정을 반복하는 것이 중요한데 비즈니스로직 자체에 대한 테스트 신뢰도가 충분하지 않았다는 생각이 들어요. 객체의 작업단위의 종료점에 대해서 다시한번 생각해보시고 어떤 케이스가 추가될 수 있을지 고민해보면 좋을 것 같다는 생각이 듭니다!

구조에서도 도메인과 UI를 분리하고자하는 모습이 코드에 잘 나타난것 같아요. 한가지 아쉬운점은 현재 Controller가 너무 비대하다는 것이에요. 컨트롤러의 역할에 대해서 기린이 다시한번 직접 생각해보면 더 좋을 것 같아서 조금 러프하게 코멘트를 달아놓았는데요. 다음 리뷰에서 기린이 생각한 구조에 대해서 더 설명을 들으면 좋을 것 같네요!

논의하고 싶은 부분

TDD 관련

앞서말한 것과 더불어서 제 생각을 말씀드리자면 TDD는 뚜렷한 장점이 있다고 생각해요. 요구사항중심으로 생각하게 해주고 질좋은 코드와 테스트 코드설계에도 도움이 되구요. 하지만 TDD는 이를 배우고 활용하기까지 많은 노력과 시간이 필요하다고도 생각합니다. 당연히 지금껏 해온 프로세스와 반대로 수행하기 때문에 번거롭고 어렵게 느껴질 수 있다고도 생각이 됩니다.

다른 리뷰에도 남겼듯이 TDD는 복잡한 도메인이나 중요 비즈니스에서 큰역할을 할 수 있어요. 쉽게 놓칠 수 있는 요구사항들을 먼저 정의함으로써 기능중심 개발을 가능하게 하죠. 지금 어렵고 미숙한 이유는 실제로 처음 TDD를 접하고 간단한 로직에 적용했기 때문이라고 생각이 되요. 2단계에서 조금 더 TDD의 효과를 느낄 수 있지 않을까도 생각이되네요!

함수의 단일책임

해당 함수 자체는 책임이 명확하다고 생각합니다. 다만 앞서 말씀드린대로 Controller자체가 너무 많은 역할을 담당하다보니 짧은 코드임에도 마음에 들지 않게 되었다고 생각해요. 객체와 계층의 책임에 따라 분리를 해주시는 것은 어떨까요?

추가로 함수가 하나의 역할을 하는지 체크하기 위해서 아래와 같은 사항들을 고민해보면 좋을 것 같아요!

함수가 한 가지 명확한 작업만 수행하는가?
함수의 이름이 수행하는 작업을 명확히 설명하는가?
함수의 추상화 수준이 일관되는가?

상수분리

constant의 위치에 대해서 질문을 주신 것 같은데요. constant같은 경우에는 사용되는 파일 근처에 위치하는 경우가 가장 좋다고 생각하구요. 여러 폴더에 걸쳐 사용된다면 constant가 어떤 성격을 지니는 지도 위치에 영향을 끼칠 것 같습니다. 예를들어 Lotto의 발행숫자 같은 경우는 비즈니스 로직을 포함하고 있기 때문에 domain에 위치할 수도 있구요. 메세지와 같은 경우에는 view의 성격이 강하니 view폴더에 위치할수 있구요. 복합적인 경우 지금과 같이 constants폴더로 위치시켜도 좋다고 생각합니다.

테스트코드에서 상수의 활용에 대해서도 질문을 주셨는데요. 테스트코드도 결국엔 관리되어야할 문서이자 코드입니다. 그렇기 위해서는 당연히 가독성과 유지보수성이 좋아야되구요. 애플리케이션은 보통 계속해서 유지보수가 되기 때문에 언제든지 다양한 요구사항에 대처할 수 있어야되요. 테스트코드도 똑같은 이치라고 생각합니다.

기린! 지금도 괜찮은 구조이고 코드이지만 조금 더 개선할만한 부분들이 보여서 RequestChages로 남겨두도록 하겠습니다! 기린만의 생각들이 잘 녹아들어가 있는 코드인만큼 다음 리뷰가 더 기대가 되네요 수고많으셨습니다 기린!🦒

JUDONGHYEOK · 2025-02-21T12:25:39Z

.eslintrc.json

+    // package import를 제외한 모든 import 구문에 대해 확장자를 사용하도록 강제
+    "import/extensions": ["error", "ignorePackages"],
+    // 기타 사소한 오류 해결
+    "import/prefer-default-export": "off",
+    "class-methods-use-this": "off",
+    "no-plusplus": ["error", { "allowForLoopAfterthoughts": true }],
+    "lines-between-class-members": [
+      "error",
+      "always",
+      { "exceptAfterSingleLine": true }
+    ],
+    // 들여쓰기 깊이 제한
+    "max-depth": ["error", 1],
+    // 함수의 길이 제한
+    "max-lines-per-function": ["error", { "max": 15 }]


오 페어와 함께 린트를 수정하였군요 훌륭합니다! 👍

감사합니닷 페어가 제안해서 한번 해 보았는데 잘한 선택이였군요.

JUDONGHYEOK · 2025-02-21T12:28:36Z

src/Model/LottoMachine.js

+  saveUniqueLottoNumber(lottoNumbers, randomNumber) {
+    if (!lottoNumbers.includes(randomNumber)) {
+      lottoNumbers.push(randomNumber);
+    }
+  }


generateLottoNumbers는 private으로 saveUniqueLottoNumber는 public으로 지정하신 이유가 있으실까요? 그리고 해당 로직은 set으로 간소화할 수 있을것 같아요!

또 최악의 경우 무한루프가 발생할 수도 있어서 다른 로직으로 짜볼 수도 있을 것 같구요

Set을 사용해서 중복을 검사하는 로직을 간소화 하고 테스트 코드도 수정해보았습니다. 6df56d4

❓그런데 무한 루프 관련된 부분에 있어서 getRandomNumberInRange()가 1~45 범위라고 생각했을때는 무한 루프가 발생할 가능성이 적다고 생각해서 일단 현재 코드대로 놔둬도 괜찮다고 생각을 했습니다. 하지만 getRandomNumberInRange()가 갑자기 1~5 범위로 바뀌어 버린다던가 하는 경우를 생각했을 때는 확실히 문제가 될 것 같은데 그런 경우까지 생각해서 무한루프를 발생할 가능성을 0으로 만들어야 하는지 궁금합니다!

제 생각에는 이 정도 함수는 넘어가도 판단해도 괜찮겠지만 미션이기도 하고 무한루프가 발생하지 않도록 로직을 짜는 것이 가능할 것 같은데 무한루프를 발생시킬 수 있는 로직을 사용하고 있는 부분에서 개선할 수 있는 사항이라고 생각했어요.
무한루프는 애플리케이션이 치명적이기도 하고 요구사항은 언제든지 바뀔 수 있기 때문에 개선하는 것이 좋다고 판단했습니다!

JUDONGHYEOK · 2025-02-21T12:37:10Z

src/Model/Winning.js

+  #sumPrize(rank, totalPrize) {
+    if (this.rankHistory[rank]) {
+      totalPrize += PRIZE[rank] * this.rankHistory[rank];
+    }
+    return totalPrize;
+  }


함수를 분리해주신 것은 좋지만 파라미터를 직접변경함으로써 순수함수의 규칙을 위배하게 되는 것 같아요. 또 getTotalPrize로직 자체를 지금보다 더 단순하게 리팩토링할수도 있을 것 같아요

순수함수에 대해서 다시 생각해봐야겠네요...

파라미터를 직접 변경하는 sumPrize()를 아예 제거해버리고 getTotalPrize()에서 reduce()를 활용하는 방식으로 코드를 변경하였습니다. 60e09d1

❓이러한 방식이 적절한가요? 아니면 더 개선할 부분이 있는지 궁금합니다.

오 좋은데요 전 이게 훨씬 더 깔끔하고 가독성이 좋은 것 같습니다!

JUDONGHYEOK · 2025-02-21T12:42:19Z

src/Model/Winning.js

+  calculateRankHistory(boughtLotto) {
+    const matchCount = this.winningNumbers.filter((winningNumber) => boughtLotto.includes(winningNumber)).length;
+    if (matchCount === LOTTO_NUMBER_LENGTH) {
+      this.rankHistory.first += 1;
+    }
+    if (matchCount === 5) {
+      this.updateSecondOrThirdPlace(boughtLotto);
+    }
+    if (matchCount === 4) {
+      this.rankHistory.fourth += 1;
+    }
+    if (matchCount === 3) {
+      this.rankHistory.fifth += 1;
+    }
+  }


calcaulateRankHistory를 나름 분리할 수 있을 것 같아요 또 if문의 로직들을 객체를 활용함으로서 대체할 수 있을 것 같구요!

if 문을 객체로 매핑하는 방식으로 리팩토링을 진행해보았습니다. dfe055c

오 저는 이방식이 더 잘읽히는 것 같은데 기린은 어떻게 생각하세요??

처음에는 객체를 사용하는 것에 익숙하지 않아서 이해가 바로 되는 느낌은 아니였는데 리펙터링 하면서 공부 한 다음 다시 보니까 이 방식이 훨씬 더 잘 읽히는 것 같습니다!

JUDONGHYEOK · 2025-02-21T12:47:00Z

src/Model/Lotto.js

+import { LOTTO_NUMBER_LENGTH } from '../constants/common.js';
+
+class Lotto {
+  #numbers = Array.from({ length: LOTTO_NUMBER_LENGTH }).fill(0);


여기서 LOTTO_NUMBER_LENGTH로 선언된 이유가 궁금해요 생성자를 보면 어차피 파라미터로 들어온 numbers배열의 길이에 의존적인 것 같아서요!

저희는 처음에 로또 번호를 담고있는 배열이라는 의미를 확실하게 보여주기 위해 LOTTO_NUMBER_LENGTH를 사용해서 로또 길이만큼의 배열로 초기화했습니다. 그런데 생각해보니 파라미터로 전달받은 numbers 배열의 길이에 이미 의존하고 있기 때문에, 굳이 LOTTO_NUMBER_LENGTH를 사용해서 초기화를 할 필요가 없을 것 같습니다.

그래서 초기화 하는 부분을 삭제하고 생성자에서 numbers길이를 받아서 초기화 하는 방법으로 고쳐보았습니다. 7942fe6

❓그런데 이부분에서 한가지 궁금한 점은 코드에서 미리 #numbers를 선언하고 생성자에서 할당 받고있는데 굳이 그렇게 안해도 코드는 동작 하잖아요. 하지만 저는 이 클래스에서 이 필드를 갖는다는 것을 명확하게 하기 위해서 선언을 했는데 실제로는 필드를 선언 하고 할당받는 방식을 많이 사용하는지 아니면 그냥 선언 없이 생성자에서 바로 할당 받는 방법을 많이 사용하는지 궁금합니다.

아제가 말을 잘못적었네요. 선언이 아니라 할당이였어요! 보통은 필드에서 할당을 하는 경우는 생성자와는 무관하거나 변하지 않는 필드에 선언하는 것 같은데 constructor의 파라미터에 의존해서 할당하는데 필드선언부에서 할당을 하는 것이 의아해서 코멘트를 남겼었습니다.

필드자체를 선언하는 것은 좋은 것 같습니다. 기린의 말처럼 필드를 선언하면 객체에서 사용되는 필드들을 명확하게 정의함으로써 객체에서 사용되는 필드들을 한눈에 파악하기 수월하다고 생각해요. 혼란을 드려 죄송합니다🥲

아! 이해 했습니다. 필드 선언부에서 기본값을 할당하는 경우와 생성자 파라미터에 의존해서 값을 할당하는 경우가 서로 다르군요!

JUDONGHYEOK · 2025-02-21T12:49:00Z

src/Model/Lotto.js

+  constructor(numbers) {
+    this.#numbers = numbers.sort((a, b) => a - b);
+  }


로직은 무관하게 동작할 것 같지만 sort같은 함수들은 원본배열을 변화시키기 때문에 항상 주의해서 사용해야되는 것 같아요. 지금은 파라미터로 넘겨진 변수를 재활용하는 로직이 없어서 괜찮지만 예상치 못한 데이터 변경을 막기위해 순수함수로 구성하는 습관을 들이는 것도 좋을 것 같아요!

제가 순수함수의 의미를 잘 모르고 있었던 것 같네요 한번 수정해보겠습니다.

이 함수는 외부에서 받은 원본 배열을 변경하기 때문에 순수함수가 아니라고 하신것으로 이해해서 입력받은 numbers를 새로운 배열로 만들어서 정렬해보았습니다. 2fec37a

오 너무 좋습니다! sort같은 애들을 항상 헷갈리는 것 같아요🥲

JUDONGHYEOK · 2025-02-21T12:53:04Z

__tests__/Winning.test.js

+    expect(winning.winningNumbers).toEqual(winningNumbers);
+    expect(winning.bonusNumber).toBe(bonusNumber);


이테스트의 목적은 무엇인지 알 수 있을까요??

입력받은 당첨 번호와 보너스 번호를 winning 이라는 객체가 잘 저장하고 있는지 확인하기 위한 테스트입니다. given에 속하는 코드가 beforeEach에 포함되어 테스트 코드를 간단하게 쓰게 되었습니다.

단순히 객체에 저장하는 것을 테스트하는 것이 의미가 있을지 잘모르겠어요. 🧐

JUDONGHYEOK · 2025-02-21T12:57:40Z

__tests__/Winning.test.js

+    winning.rankHistory.first = 1;
+    winning.rankHistory.second = 1;
+    winning.rankHistory.third = 1;
+    winning.rankHistory.fourth = 1;
+    winning.rankHistory.fifth = 1;


이렇게 rankHistory를 직접조작하는 것은 위험할 수 있다고 생각해요. 실제 사용되는 코드에서도 언제든지 변경될 수 있다는 의미라고 여겨지는데요. 해당 필드 뿐만 아니라 메소드에 대해서도 캡슐화에 대해 조금 더 고민해보면 어떨까요?

rankHistory를 직접조작하지 않도록 Winning 클래스에서 해당 필드를 private으로 변경하여 캡슐화 했고 Winning 클래스 내부에서만 사용되는 메서드들도 private으로 변경하였습니다. 4ea21dd
그리고 그에 따른 테스트 코드도 변경 되었습니다. d0b31e2

❓이 리펙터링을 하면서 두가지 궁금한 점이 생겼는데요.

calculateRankHistory()는 내부에서만 사용되지만, 테스트 때문에 어쩔 수 없이 public으로 놔뒀습니다. 이걸 변경하게 되면 지금까지 작성한 테스트를 새로 작성해야 하지만 이 메서드만 public 으로 놔둔다면 모든 테스트가 가능하기 때문입니다...

궁금한 점은 테스트만을 위해 특정 메서드를 public으로 유지하는 경우도 있나요? 아니면 더 좋은 방법이 있을까요?

캡슐화를 하면서 테스트 코드가 복잡해졌는데, 테스트 코드의 복잡도보다 메서드 캡슐화가 더 중요하다는 생각이 들었습니다.

하지만 캡슐화를 진행하면서 테스트 코드 두개의 로직이 조금 복잡하게 변경되었는데 테스트 코드의 가독성을 높이면서도 캡슐화를 유지할 수 있는 좋은 방법이 있는지 궁금합니다.

private을 테스트하는 것은 참 어렵죠. 좋은 고민이신 것 같습니다. TDD의 목적중 하나가 구현사항 즉 행동 중심에 사고와 테스트를 하기 위함인데요. 테스트를 짜는 것도 행동 중심적으로 이루어져야한다고 생각해요. 때문에 private을 분리해서 테스트하기보다는 공개되어있는 행동중심으로 테스트하는 것은 어떠신가요?

또 이것과는 별개로 만약에 private메소드가 기린이 생각했을 때 핵심적인 비즈니스로직이라고 생각하면 어떻게 테스트하고 분리할 수 있을까요?

테스트의 로직은 제가 느끼기에는 복잡하지 않은 것 같은데 어떤 부분에서 복잡하다고 판단하셨는지 궁금해요!

JUDONGHYEOK · 2025-02-21T12:59:28Z

src/constants/common.js

+export const MAX_LOTTO_NUMBER = 45;
+
+export const PRIZE = {
+  first: 2000000000,


개인적으로 이런 긴 숫자는 numeric separator를 사용하면 더 쉽게 읽히는 것 같아요!

오 numeric separator는 처음 알았네요! 유용하게 사용할 수 있을 것 같습니다!

numeric separators를 사용하여 코드 수정하였습니다. 6f27644

❓한 가지 궁금한 점은 한눈에 알아볼 수 있는 1000이나 5000 같은 작은 단위의 수까지 numeric separators를 적용해야 하는지 의문입니다. 저는 일단 필요 없다고 생각하여 천 단위를 넘는 숫자에만 적용해보았는데 통일성을 위해서 적용해야 하는 것이 더 좋은지 궁금합니다

통일성 있게 적용하는 것이 가독성이 더 좋다고 생각해요. 1000자리 수 자체에서도 1_000와 같은 형식이 개인적으로 더 잘 읽히기도하구요

JUDONGHYEOK · 2025-02-21T13:04:53Z

src/Controller/LottoController.js

+import LottoMachine from '../Model/LottoMachine.js';
+import Winning from '../Model/Winning.js';
+
+class LottoController {


Contorller의 역할에 대해 다시한번 생각할 필요가 있을 것 같아요. 현재는 입력검증, 로직처리, 흐름제어등을 모두 직접적으로 하고 있기 때문에 Controller가 간단한 로직임에도 불구하고 많이 무거워진 상태입니다.

Contorller가 세부로직들을 모두 알 필요가 있을까요? 추상화할 부분들도 보이는 것 같구요. 적당한 객체를 만들어서 관심사를 분리하는 것은 어떨까요? 조금 더 고민이 필요한 부분일 것 같습니다!

Controller가 너무 많은 역할을 하고 있던 문제를 해결하기 위해 입력 검증 로직을 Validate 여러개를 사용하는 것에서 하나의 함수로 묶어 컨트롤러의 검증 로직을 단순화했습니다.

현재는 Controller에서 개별적인 검증을 여러 번 수행하는 대신, Validate의 하나의 검증 함수(validatePrice, validateWinningNumbers 등등)을 호출하는 방식으로 수정했습니다. 5566486

아 그리고 이 변경에 맞춰서 Validate의 테스트 코드도 수정하였습니다. 3508ef4

오 저는 validate를 별도로 분리해주신 것을 좋은 것 같아요! 조금 더 개선해보면 validate의 위치에 대해서도 고민해 볼 수 있을 것 같아요 각각의 validate가 어떤 역할인지 어떤 위치에 위치하면 좋을지 고민해보면 좋을 것 같아요!

JUDONGHYEOK · 2025-02-21T14:34:34Z

src/Controller/LottoController.js

+    } catch (error) {
+      throw error;
+    }


validate에서 error를 throw하는데 또 받아서 throw하는 이유가 있으실까요??

처음엔 예외 처리를 모든 함수에서 직접 해줘야 한다고 생각해서 #validatePrice 내부에서도 try-catch로 잡고 다시 throw했습니다. 그런데 찾아보니 #readPrice에서만 예외를 처리하면 충분하네요! 불필요한 try-catch는 없애보겠습니다.

불필요한 try-catch를 제거하였습니다. 96a3b1d

그리고 이 과정에서 로또 당첨 번호 개수 검증 함수가 빠져있는 것을 발견해서 추가하였습니다. 6106398

오 너무 좋습니다 훨씬더 깔끔해진 느낌이네요

- LottoMachine의 #generateLottoNumbers()에서 Set을 활용하여 중복 검사 로직 제거 - 로또 한 장에서 중복되지 않는 번호가 저장되는지 검증하는 테스트 수정

- 큰 숫자에 `_` 추가

- numbers 배열을 직접 변경하지 않고 복사본([...numbers])을 만들어 정렬

- rankHistory를 private 필드로 변경하여 외부에서 직접 수정할 수 없도록 함 - 내부에서만 사용하는 updateSecondOrThirdPlace()를 private 메서드로 변경 - 내부에서만 사용하는 increaseRankingHistory()를 private 메서드로 변경 - 내부에서만 사용하는 getTotalPrize()를 private 메서드로 변경

- rankHistory의 직접 조작을 제거하고 calculateRankHistory()를 사용하여 당첨 내역을 반영하도록 수정 - 테스트에서 rankHistory 필드 값을 직접 변경하지 않고, 메서드를 통해 조작하도록 변경 - 당첨 금액 합산 테스트에서 calculateRankHistory()를 활용하여 당첨 횟수를 반영하도록 수정 - 수익률 계산 테스트에서 직접 rankHistory를 변경하는 대신, calculateRankHistory()를 활용하여 로직을 검증하도록 개선

- sumPrize() 함수에서 매개변수를 변경하는 방식을 제거하고 getTotalPrize()에서 reduce()를 사용하여 sumPrize()를 대신함

- validate 함수 내부에서 이미 예외를 throw하므로 불필요한 try-catch 블록 제거 - 예외를 잡아서 다시 throw하는 중복 로직을 제거하여 코드 개선

- validateWinningNumbers()에서 checkWinningNumberCount() 추가 - 기존 로직에서 빠져있던 당첨 번호 개수 검증 로직 추가

- LottoController에서 입력 검증 책임을 Validate 모듈로 이동시켜서 역할 분리 - 중복된 검증 로직을 Validate 내부 함수로 통합하여 코드 중복 제거

- Controller에서 Validate 모듈로 책임을 넘김에 따라 테스트 코드 수정 - validatePrice(), validateWinningNumbers(), validateBonusNumber(), checkRestartChar() 함수 테스트 추가

- if-else 대신 객체 매핑을 활용하여 등수별 처리 로직 개선 - getRankHandler()를 추가하여 등수에 따른 동작을 객체로 분리

jeongyou · 2025-02-23T16:01:01Z

안녕하세요 동키콩! 기린입니다~. 꼼꼼한 리뷰 잘 보았습니디. 정말 감사합니다!
각 리뷰 코멘트에 대한 수정 사항을 반영하고, 해당 댓글에 코멘트를 남겨놓았습니다!

아 그리고 동키콩이 생각해보라고 했던 Controller에 역할에 대해서도 생각해보았습니다.
저는 Controller의 역할이 View와 Model을 연결하는 것이라고 생각했습니다.
즉, 사용자 입력을 받고 → Model에 데이터를 전달하거나 연산을 수행하고 → 결과를 View로 출력하는 흐름을 제어하는 역할이라고 보았습니다.
그래서 LottoController에서는 '로또 구매 → 당첨 번호 입력 → 결과 계산 → 다시 시작 여부 결정' 이 흐름을 관리하도록 구성했습니다.

그런데 원래 제 코드에서는LottoController에서 개별적으로 여러 개의 검증 함수를 호출해서 Controller가 수행하지 않아도 되는 검증 역할을 수행했다고 판단했고 Validate 모듈에서 여러개의 검증 함수를 하나의 함수로 묶어 호출하도록 개선하였습니다.

마지막으로 궁금한 점이 3가지가 있는데

제가 생각한 Controller의 역할이 맞는지
Controller의 역할이 기존보다 줄어든게 맞는지, 더 줄일 수 있는지
지금 제가 생각하는 Controller의 흐름이 코드의 구조에 적절하게 반영되었는지
가 궁금합니다!

혹시 추가로 개선할 부분이 있다면 피드백 부탁드립니다. 🙌

JUDONGHYEOK

안녕하세요 기린!!!🦒 피드백을 아주 잘 반영해주신 것 같아요 감사합니다!

컨트롤러 관련 질문들을 많이 주셨는데 처음보다 기린의 생각대로 컨트롤러가 잘 정리가 된 것 같아요! MVC패턴을 적용하다보면 필연적으로 컨트롤러가 비대해질 수 밖에 없기때문에 최대한 컨트롤러 내의 로직은 간단하게 구성할 필요가 있다고 생각해요. 그래서 기린 생각대로 validate를 하나의 도메인으로 보고 묶고 최소한의 정보만 컨트롤러에 노출시킨 것은 좋은 생각이라고 여겨집니다.
조금 더 고민해볼 필요가 있는 부분은 코멘트에서도 남겼듯이 validate의 위치일 것 같아요

리뷰를 꼼꼼하게 모두 반영해주신 것 같아 어프로브 머지 하도록 하겠습니다 더 개선할 수 있는 부분은 2단계에서 계속해서 반영해주세요!! 수고많으셨어요 기린!!

JUDONGHYEOK · 2025-02-24T02:31:11Z

src/constants/common.js

+export const MAX_LOTTO_NUMBER = 45;
+
+export const PRIZE = {
+  first: 2000000000,


통일성 있게 적용하는 것이 가독성이 더 좋다고 생각해요. 1000자리 수 자체에서도 1_000와 같은 형식이 개인적으로 더 잘 읽히기도하구요

JUDONGHYEOK · 2025-02-24T02:31:50Z

src/Controller/LottoController.js

+    } catch (error) {
+      throw error;
+    }


오 너무 좋습니다 훨씬더 깔끔해진 느낌이네요

JUDONGHYEOK · 2025-02-24T02:35:17Z

src/Controller/LottoController.js

+import LottoMachine from '../Model/LottoMachine.js';
+import Winning from '../Model/Winning.js';
+
+class LottoController {


오 저는 validate를 별도로 분리해주신 것을 좋은 것 같아요! 조금 더 개선해보면 validate의 위치에 대해서도 고민해 볼 수 있을 것 같아요 각각의 validate가 어떤 역할인지 어떤 위치에 위치하면 좋을지 고민해보면 좋을 것 같아요!

JUDONGHYEOK · 2025-02-24T02:42:46Z

__tests__/Winning.test.js

+    expect(winning.winningNumbers).toEqual(winningNumbers);
+    expect(winning.bonusNumber).toBe(bonusNumber);


단순히 객체에 저장하는 것을 테스트하는 것이 의미가 있을지 잘모르겠어요. 🧐

JUDONGHYEOK · 2025-02-24T02:49:25Z

__tests__/Winning.test.js

+    winning.rankHistory.first = 1;
+    winning.rankHistory.second = 1;
+    winning.rankHistory.third = 1;
+    winning.rankHistory.fourth = 1;
+    winning.rankHistory.fifth = 1;


private을 테스트하는 것은 참 어렵죠. 좋은 고민이신 것 같습니다. TDD의 목적중 하나가 구현사항 즉 행동 중심에 사고와 테스트를 하기 위함인데요. 테스트를 짜는 것도 행동 중심적으로 이루어져야한다고 생각해요. 때문에 private을 분리해서 테스트하기보다는 공개되어있는 행동중심으로 테스트하는 것은 어떠신가요?

또 이것과는 별개로 만약에 private메소드가 기린이 생각했을 때 핵심적인 비즈니스로직이라고 생각하면 어떻게 테스트하고 분리할 수 있을까요?

테스트의 로직은 제가 느끼기에는 복잡하지 않은 것 같은데 어떤 부분에서 복잡하다고 판단하셨는지 궁금해요!

aydenote and others added 30 commits February 18, 2025 16:44

chore: eslint 및 prettier 설정

e5f93c0

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

docs: 기능 목록 작성

7b0bd41

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 공백 입력 예외처리 기능 추가

bcef526

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 숫자 이외의 입력 예외처리 기능 구현

3ac00c2

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 천원 단위가 아닌 금액 입력 예외처리 기능 추가

91206d8

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 구입 금액 범위 밖 입력에 대한 예외처리 기능 구현

757867e

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 로또 번호 범위 밖 입력에 대한 예외처리 기능 구현

521fb18

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 구입 금액 범위 상수화

61e18cd

feat: 당첨 번호 중복 입력 예외처리 기능 구현

86a9f53

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 당첨 번호와 중복 입력 예외처리 기능 구현

4c525c2

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

docs: 기능 목록 체크리스트 추가

4ff3212

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 랜덤 숫자 생성 함수 구현

db0aa29

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 6자리 숫자로 로또 생성

90945e4

docs: 기능 목록 수정

ca78f7b

feat: 구입 금액 만큼 로또 발행 기능 구현

78fa404

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 구입 금액 만큼 로또 발행 함수 기능 분리

81a6afc

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

docs: 오름차순 정렬 기능 목록 수정

820a9a3

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 로또 번호 오름차순 정렬 기능 구현

4fcfac9

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

docs: 로또 머신 역할 일부 Winning으로 분리

77aa232

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 입력 받은 당첨번호와 보너스 번호를 저장하는 기능 구현

aa4d2a9

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 각 등수별 당첨 여부 확인 기능 구현

31d9a4b

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

docs: 당첨 통계 계산 기능 로또 머신으로 분리

4950b74

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 로또 구매 금액 로직 수정

8e41f15

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

fix: 숫자 이외 입력 검증 오류 수정

bb11e5f

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

fix: 중복 숫자 없이 로또 발행하도록 오류 수정

3a175f2

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

docs: 당첨 번호가 6개가 아닌 입력 예외처리 목록 추가

c23e26c

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 당첨 번호가 6개가 아닌 입력 예외처리 기능 구현

6204cc5

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 입력 유틸 함수 구현

cbe6f9e

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 입력 함수 모듈 생성

1e48209

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

feat: 출력 함수 구현

e54423e

- 에러 메세지 출력 - 생성된 로또 출력 Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

aydenote and others added 12 commits February 20, 2025 13:34

feat: 다시 시작 로직 기능 구현

00f91ec

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 수익률 출력 시 천 단위 콤마 추가를 위해 toLocaleString() 적용

25c7edc

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: buyLotto의 LottoMachine 매개변수를 제거하고 내부에서 생성하도록 수정

b884aac

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 매직 넘버 제거 및 상수화 적용

7723f1e

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

test: 매직 넘버 제거 및 상수화 적용, 변수명 변경

8c9843f

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 입출력 메세지 상수화 적용

caca961

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 당첨 여부 계산 함수 로직 수정

ebec7ef

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 예외 발생 시 재귀함수를 이용하여 재실행 구현

5d18d5f

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 당첨 금액 합계 계산 함수 분리

ca81e5d

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

refactor: 중복되지 않은 로또 넘버 저장 기능 분리

30f75a4

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

test: 중복되지 않는 로또 번호 저장 테스트 추가

a1c74d1

Co-authored-by: Youjeong Jeong <jeongyou@users.noreply.github.com>

JUDONGHYEOK self-requested a review February 20, 2025 11:41

JUDONGHYEOK requested changes Feb 21, 2025

View reviewed changes

JUDONGHYEOK reviewed Feb 21, 2025

View reviewed changes

jeongyou added 12 commits February 23, 2025 02:37

refactor: LottoMachine 로또 번호 생성 로직 개선 및 테스트 추가

6df56d4

- LottoMachine의 #generateLottoNumbers()에서 Set을 활용하여 중복 검사 로직 제거 - 로또 한 장에서 중복되지 않는 번호가 저장되는지 검증하는 테스트 수정

refactor: 숫자 가독성을 위해 Numeric Separators 적용

6f27644

- 큰 숫자에 `_` 추가

refactor: Lotto 클래스에서 불필요한 초기화 제거

7942fe6

refactor: Lotto 생성자에서 원본 배열을 변경하지 않도록 수정

2fec37a

- numbers 배열을 직접 변경하지 않고 복사본([...numbers])을 만들어 정렬

refactor: getTotalPrize() 순수함수 적용 및 로직 단순화

60e09d1

- sumPrize() 함수에서 매개변수를 변경하는 방식을 제거하고 getTotalPrize()에서 reduce()를 사용하여 sumPrize()를 대신함

refactor: 중복된 예외 처리 제거

96a3b1d

- validate 함수 내부에서 이미 예외를 throw하므로 불필요한 try-catch 블록 제거 - 예외를 잡아서 다시 throw하는 중복 로직을 제거하여 코드 개선

fix: 로또 당첨 번호 개수 검증 함수 추가

6106398

- validateWinningNumbers()에서 checkWinningNumberCount() 추가 - 기존 로직에서 빠져있던 당첨 번호 개수 검증 로직 추가

refactor: Controller의 역할 분리 및 Validator 개선

5566486

- LottoController에서 입력 검증 책임을 Validate 모듈로 이동시켜서 역할 분리 - 중복된 검증 로직을 Validate 내부 함수로 통합하여 코드 중복 제거

test: Validate 모듈 리팩토링에 따른 테스트 코드 수정

3508ef4

- Controller에서 Validate 모듈로 책임을 넘김에 따라 테스트 코드 수정 - validatePrice(), validateWinningNumbers(), validateBonusNumber(), checkRestartChar() 함수 테스트 추가

refactor: 등수 판별 로직을 객체를 활용하여 개선

dfe055c

- if-else 대신 객체 매핑을 활용하여 등수별 처리 로직 개선 - getRankHandler()를 추가하여 등수에 따른 동작을 객체로 분리

JUDONGHYEOK approved these changes Feb 24, 2025

View reviewed changes

JUDONGHYEOK merged commit 679dc67 into woowacourse:jeongyou Feb 24, 2025

		expect(winning.winningNumbers).toEqual(winningNumbers);
		expect(winning.bonusNumber).toBe(bonusNumber);

Conversation

jeongyou commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

학습 목표

제출 전 체크 리스트

리뷰 요청 & 논의하고 싶은 내용

1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점

2) 이번 리뷰를 통해 논의하고 싶은 부분

2단계

Uh oh!

JUDONGHYEOK left a comment

Choose a reason for hiding this comment

논의하고 싶은 부분

TDD 관련

함수의 단일책임

상수분리

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongyou Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongyou Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongyou Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongyou Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongyou Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jeongyou commented Feb 20, 2025 •

edited

Loading

jeongyou Feb 22, 2025 •

edited

Loading

jeongyou Feb 23, 2025 •

edited

Loading

jeongyou Feb 22, 2025 •

edited

Loading

jeongyou Feb 22, 2025 •

edited

Loading

jeongyou Feb 22, 2025 •

edited

Loading

jeongyou Feb 22, 2025 •

edited

Loading