[부나] 1단계 로또 제출합니다. #22

tmdgh1592 · 2023-02-16T08:54:56Z

안녕하세요 제리님!
제리님께 리뷰받게되어 영광입니다 :)
우선 리뷰요청 버튼이 의도치않게 3번 눌려 푸시가 여러번 날아갔을텐데 죄송하다는 말씀드립니다.

최대한 TDD + 페어프로그래밍으로 미션을 수행하려고 하다보니 시간에 쫓겨 코드에 미숙함이 많이 묻어나오고 있습니다. 😥
step2를 진행하면서 리뷰어님께서 주시는 리뷰를 바탕으로 개선해보기 위해 노력하겠습니다!

이번 미션에서는 일급 컬렉션, 불변, 원시값을 포장한 클래스를 적용해보고자 신경썼습니다.
그럼에도 놓친 부분이 많은 것 같아 궁금한 부분에 대해 질문을 드리고 싶습니다.

Rank(당첨 등수)을 Key로 갖고, 해당 Rank를 몇개 맞추었는지에 대한 정수값을 Value로 하는 Map을 사용했습니다.
+ 구매한 로또 객체를 List로 반환하는 로직이 있습니다.
이 경우에도 모두 일급컬렉션으로 감싸주는 것이 좋은지 궁금합니다!
특별한 비즈니스 로직을 가지고 있지 않은 원시값, 컬렉션의 경우에도 모두 포장해줄 필요가 있는지 궁금합니다!
테스트하기 위한 다양한 값을 주입해주기 위해 @MethodSource를 자주 사용하였는데, 가독성이 많이 떨어지는 느낌을 받았습니다.
@MethodSource가 필요하지 않은 경우에는 @valuesource 또는 @CsvSource를 사용하였는데, List와 같은 인자를 받기 위해서는 제가 알고 있는 선에서는 + 구글링해본 결과 @MethodSource밖에 떠오르지 않아 더 나은 개선책이 있는지 궁금합니다!
도메인 모델에서 public으로 외부에서 접근할 수 있는 메서드는 모두 테스트 코드 작성이 필요한지 궁금합니다!
Controller가 도메인 모델을 멤버 변수로 가지고 있는 것이 바람직한 형식인지 궁금합니다!
찾아보는 글마다 내용이 달라 혼란이 생겨 질문드립니다..!

긴 질문글 읽어주셔서 감사합니다🙂

- lotto 패키지 내 Number 관련 클래스는 lotto.number 패키지로 이동 - Rank 클래스는 Rank 패키지로 분리

vagabond95 · 2023-02-16T14:56:29Z

일급 컬렉션, 원시값으로 굳이 감싸는 이유는 무엇일까요? 이전에 감싸주셨던 이유가 해당 케이스들에도 포함된다면 같이 감싸주시면 될 거 같아요 :)
저도 얘기주신 방법에서 크게 벗어나지는 않고 있는데요, 추가로 괜찮은 방법이 떠오른다면 공유드리겠습니다 :)
테스트 코드를 작성하는 이유, 테스트 코드를 통해 무엇을 하고자하는 것인지에 대해 고민해보시면 좋겠어요.

컨트롤러의 역할을 어떻게 이해하고 있는지,
MVC 에서 Model 에 도메인 모델도 포함되는지 고민해보시면 될 것 같아요. 아키텍처에 대한 세부 디테일은 개인차가 있기 때문에 해당 아키텍처가 추구하는 바가 무엇인지를 인지하는 것이 우선이라고 생각됩니다.

vagabond95

전반적으로 가독성이 뛰어났다는 느낌을 많이 받았어요.
그만큼 세세하게 각 클래스 함수의 역할과 네이밍, 구조등을 신경써주신 것 같습니다 👍

질문 주신 내용을 비롯하여 구현 내용에 대하여 몇가지 코멘트를 남겼어요.
확인 후 리뷰요청 부탁드릴게요!

vagabond95 · 2023-02-16T14:44:53Z

.editorconfig

@@ -0,0 +1,97 @@
+root = true


이러한 파일들은 .gitignore 로 관리 부탁드려요~

외부에 드러나면 좋지 않은 파일에 대해 .gitignore 파일에 추가해두었습니다!

vagabond95 · 2023-02-16T14:46:56Z

src/main/kotlin/util/common/Extensions.kt

+
+fun IntRange.generateDistinctRandomNumbers(count: Int): List<Int> = shuffled().take(count)
+
+fun Int.divideByThousand(): Int {


이 부분은 money 에 대한 원시값을 포장하고, 해당 클래스 내부에서 처리되는 것이 더 적절하지 않을까요?

divideByThousand 라는 네이밍은 비교적 common 한데,
오류 메세지는 '입력값' 이라는 내용을 내포하고 있는 부분 자체가 common 한 케이스를 다루지 않는 증거가 되는 것 같아요.

함수를 작성하면서 약간 모호하다는 생각을 하였는데 말씀해주신 것처럼 common에서 다루기에는 '입력값'이라는 오류 메세지는 적합하지 않은 것 같습니다!
해당 로직은 별도의 Money라는 래퍼 클래스로 옮겼습니다!

vagabond95 · 2023-02-16T14:49:05Z

src/main/kotlin/domain/rank/Rank.kt

+ }
+
+ private fun judgeEitherSecondOrThird(matchBonus: Boolean): Rank {
+ if (matchBonus) return SECOND


특정 등수에 대해 조건문을 두지 않고, 일관된 방식으로 등수를 구하도록 리펙토링 해볼 수 있을까요?
현재와 같은 구조는 보너스번호를 가지는 등수가 추가될 때 마다 if 문이 계속 추가되어야하는 구조가 될 거 같아요.

enum의 values() 메서드를 활용하여 등수를 구하는 로직을 리팩터링하였습니다!

vagabond95 · 2023-02-16T14:49:14Z

src/main/kotlin/domain/rank/Rank.kt

+package domain.rank
+
+enum class Rank(val countOfMatch: Int, val winningMoney: Int) {
+ FIRST(6, 2_000_000_000),


언더스코어로 가독성 확보 👍

vagabond95 · 2023-02-16T14:51:57Z

src/main/kotlin/util/validator/InputValidator.kt

+ fun validateNumeric(value: String): Int =
+ value.toIntOrNull() ?: throw IllegalArgumentException(NOT_NUMERIC_EXCEPTION_MESSAGE)
+
+ fun validateWinningNumbersIsNumeric(value: List<String>): List<Int> {


이렇게 domain 의 요구사항에 가까운 검증 함수는 해당 클래스에 선언되는게 더 적절하지 않을까요?

제시해주신 피드백과 기능 목록 문서를 다시 살펴보면서, domain 요구사항과 관련된 검증 함수를 ui 레이어에서 다루는 것은 적합하지 않아보임을 깨달았습니다!
숫자인지에 대한 여부를 각 domain 모델에서 다루도록 리팩터링하였습니다.

Co-authored-by: rhkrwngud445 <[email protected]>

- LottoController에서 멤버 변수를 사용하지 않는 구조로 변경 - initWinningLottoNumbers()를 호출되지 않은 상태에서 matchLottos()를 호출할 수 있도록 변경

- LottoGameController 메서드를 분리하면서 LottoGame에서 LottoMachine 객체를 생성자로 받지 않도록 변경

tmdgh1592 · 2023-02-17T16:34:52Z

안녕하세요 제리님 : )
제리님께서 제공해주신 피드백과 페어의 리뷰어분의 피드백을 기반으로 리팩터링을 마치고, 2차 피드백을 요청드리게 되었습니다!

감사드리게도 질문드린 내용에 대한 답변과 더불어 피드백을 통해 궁금한 점이 많이 해결되었습니다!
다만 4번 질문에 대해서는 4. 라고만 작성되어 있어서 제가 잘 이해하지 못했습니다..!
혹시 스스로 생각해봤으면 좋겠다는 의미에서 공백으로 남겨주신건지 글이 지워진건지 여쭤봐도 될까요..?! 🙂

vagabond95 · 2023-02-18T02:32:12Z

앗 포메팅이 제대로 되지 않았나 봅니다. 4. 하위에 적은 내용이 4번 질문에 대한 답변이었어요.
아키텍처에 대해 제 생각을 조금 더 남겨보자면, MVC, MVP, MVVM 등 여러 형태의 아키텍처가 등장하곤하는데 결국 궁극적으로 이러한 아키텍처들이 출현하게된 이유는 무엇인지 / 공통점은 무엇인지를 고민하시다보면 MVC 각각에 속하는 구현에 대한 감이 잡히실 수 있을거 같습니다 :)

vagabond95

코멘트 드렸던 부분 모두 잘 반영해주셨네요 👍
TDD 방식으로 요구사항을 진행해주신 부분도 좋았습니다 ㅎㅎ

1단계는 크게 피드백 드릴 부분이 없어 머지하도록 하겠습니다. 남긴 코멘트는 다음 단계 진행 시 참고 부탁드려요~!

vagabond95 · 2023-02-18T02:29:48Z

src/main/kotlin/domain/rank/Rank.kt

- FIFTH.countOfMatch -> FIFTH
- else -> MISS
- }
+ fun valueOf(countOfMatch: Int, matchBonus: Boolean): Rank {


rank 의 각 element 가 보너스 번호 검사 여부를 알고 있다면, 아래 조건문도 제거할 수 있지 않을까요? :)

그 부분에 대해서 페어와 함께 이야기를 나눈적이 있습니다!
그러다가 지금의 방식으로 구현하게 된 이유가 만약, 5등에 당첨된 경우에도 matchBonus가 true이거나 false인 2가지 경우가 있을 수 있는데, 이러면 각 등수별로 element를 하나씩 더 추가해야만 하는 문제가 있다고 생각했습다.

이런 경우에도 valueOf를 위해 element들이 추가되는 것이 더 나은지 궁금합니다!

vagabond95 · 2023-02-18T02:30:16Z

src/main/kotlin/domain/money/Money.kt

+
+import util.common.constant.ERROR_PREFIX
+
+class Money(val amount: Int) {


Money 에 대한 비즈니스 로직을 스스로 처리할 수 있게 되었네요 👍

rhkrwngud445 added 30 commits February 14, 2023 17:04

docs: 기능목록 문서 작성

191e5cd

chore: 불필요한 파일 .gitkeep 제거

98d3d7f

feat: 로또 번호가 1이상 45이하인지 체크하는 기능 구현

ea9203a

docs: 로또를 winning lotto와 purchased lotto로 분리하여 기능 목록 작성

8698c00

refactor: LottoNumber 클래스를 domain의 lotto 패키지로 이동

395fbd6

refactor: 테스트명 띄어쓰기 변경

bce5bd8

feat: 로또 생성 기능 구현

b4286f1

feat: 로또 생성시 로또 번호가 6개인지 검증하는 로직 구현

5ce0145

chore: 에디터 설정 추가

b4b729b

feat: 로또 중복 검증 기능 구현

41cce16

refactor: Lotto 클래스가 numbers 인자를 위임 받는 형태로 변경

26ecd11

feat: Bonus number가 WinningLotto에 포함 여부 검증 기능 구현

02a7750

docs: 기능 목록 중복 제거

7a6a1e1

refactor: WinningLottoTest에 중복 테스트 코드 제거

6f01406

feat: PurchasedLotto에서 정렬된 로또를 반환하는 기능 구현

75a5bc7

refactor: LottoNumber의 equals 메소드 구현

67bd142

refactor: BonusNumber의 LottoNumber 상속을 위한 open class 적용

ff070ab

refactor: WinningLotto와 BonusNumber 분리

7f42ab9

refactor: LottoNumber의 equals 메소드 구현

2969fe6

refactor: WinningLotto에서 BonusNumber 분리

4c26e79

refactor: BonusNumber 포함여부 검증을 WinningLotto 생성시 하도록 변경

9e45c62

refactor: BonusNumber 포함여부 검증을 WinningLotto 생성시 하도록 변경

01ea82d

feat: Rank 클래스에서 맞춘 숫자 개수와 보너스 번호 일치여부가 주어졌을 때 등수를 반환하는 기능 구현

d6a6451

feat: PurchasedLotto에서 winningLotto와 bonusNumber로 당첨 등수를 반환하는 기능 구현

7be89dd

refactor: lotto 패키지내 number, rank 패키지 분리

46627ab

- lotto 패키지 내 Number 관련 클래스는 lotto.number 패키지로 이동 - Rank 클래스는 Rank 패키지로 분리

feat: 중복되지 않는 랜덤 번호 리스트 생성 기능 구현

1c5b3fb

feat: 정수를 1000으로 나누는 기능 구현

a95e3bf

refactor: 로또 범위와 로또 개수에 대해 접근제어자 public으로 변경

aded188

feat: 주어진 금액만큼 PurchasedLotto 리스트를 반환하는 기능 구현

87560f0

refactor: LottoGame -> LottoMachine으로 클래스명 변경

55a0f21

rhkrwngud445 added 4 commits February 16, 2023 17:47

refactor: BonusNumber 클래스를 제거 및 LottoNumber로 대체

72dab84

refactor: LottoNumber 클래스를 value 클래스로 변경

3c7001d

feat: LottoGame 클래스 기능 구현

333bb52

feat: LottoGameController 기능 구현

a155e6a

vagabond95 requested changes Feb 16, 2023

View reviewed changes

tmdgh1592 and others added 2 commits February 17, 2023 14:47

chore: git 커밋 메세지 템플릿 추가

2935501

Co-authored-by: rhkrwngud445 <[email protected]>

chore: .gitignore에 .editorconfig, .gitmessage.txt 추가

53f065a

Co-authored-by: rhkrwngud445 <[email protected]>

tmdgh1592 force-pushed the step1 branch from c1e9664 to 53f065a Compare February 17, 2023 06:10

tmdgh1592 and others added 17 commits February 17, 2023 15:11

chore: .editorconfig 삭제

55561f9

chore: .gitmessage.txt 삭제

fabffb1

docs: 피드백 기반으로 리팩터링 내용 추가

0ec3096

Co-authored-by: rhkrwngud445 <[email protected]>

feat: Money 클래스 0이상 검증 기능 구현

a0998b8

refactor: 로또 구입 금액을 Int로 사용하는 코드를 Money 객체로 변경

84338c5

feat: Money 클래스에서 현재 금액을 1000원으로 나눈 몫을 반환하는 기능 구현

be0af43

feat: divideByThousand 메서드 제거

54ec80a

refactor: Rank의 valueOf() 메서드에서 when-else를 사용하지 않도록 변경

c692a80

refactor: RankTest에서 로또 숫자 일치 상수값을 Rank의 enum값으로 변경

e2620fe

refactor: InputValidator 클래스 제거

b494d0d

refactor: Lotto 사이즈 에러 메시지에 LOTTO_SIZE 변수를 함께 출력하도록 변경

36537b8

refactor: 로또 게임 결과 수익을 출력을 Rank의 values() 메서드를 이용하여 출력하도록 변경

1c64961

refactor: 당첨 통계를 출력하는 메서드명 변경 및 printIncomeStatics() 매서드 로직 분리

a7bccc2

refactor: LottoGameController 메서드 분리

3dd75f4

- LottoGameController 메서드를 분리하면서 LottoGame에서 LottoMachine 객체를 생성자로 받지 않도록 변경

refactor: Extensions에서 divideByThousand 함수 제거

a37b292

refactor: 공통된 Error Prefix를 상수화

6539ebe

vagabond95 approved these changes Feb 18, 2023

View reviewed changes

vagabond95 merged commit 24ae24c into woowacourse:tmdgh1592 Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[부나] 1단계 로또 제출합니다. #22

[부나] 1단계 로또 제출합니다. #22

tmdgh1592 commented Feb 16, 2023 •

edited

Loading

vagabond95 commented Feb 16, 2023 •

edited

Loading

vagabond95 left a comment

vagabond95 Feb 16, 2023

tmdgh1592 Feb 17, 2023

vagabond95 Feb 16, 2023

vagabond95 Feb 16, 2023

tmdgh1592 Feb 17, 2023

vagabond95 Feb 16, 2023

tmdgh1592 Feb 17, 2023

vagabond95 Feb 16, 2023

vagabond95 Feb 16, 2023

tmdgh1592 Feb 17, 2023

tmdgh1592 commented Feb 17, 2023 •

edited

Loading

vagabond95 commented Feb 18, 2023

vagabond95 left a comment

vagabond95 Feb 18, 2023

tmdgh1592 Feb 18, 2023

vagabond95 Feb 18, 2023


		fun IntRange.generateDistinctRandomNumbers(count: Int): List<Int> = shuffled().take(count)

		fun Int.divideByThousand(): Int {


		import util.common.constant.ERROR_PREFIX

		class Money(val amount: Int) {

[부나] 1단계 로또 제출합니다. #22

[부나] 1단계 로또 제출합니다. #22

Conversation

tmdgh1592 commented Feb 16, 2023 • edited Loading

vagabond95 commented Feb 16, 2023 • edited Loading

vagabond95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmdgh1592 commented Feb 17, 2023 • edited Loading

vagabond95 commented Feb 18, 2023

vagabond95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmdgh1592 commented Feb 16, 2023 •

edited

Loading

vagabond95 commented Feb 16, 2023 •

edited

Loading

tmdgh1592 commented Feb 17, 2023 •

edited

Loading