Skip to content
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

refactor : 로또 도메인 리팩토링 진행 #22

Merged

Conversation

this-is-spear
Copy link
Member

✏️ 구현 내용

값 객체가 아닌 경우를 제외하고 단방향으로 의존 할 수 있도록 코드를 리팩토링 합니다.

LottoApp에서 LottoGame의 의존도를 없애고 LottoTickets, LottoTicket에 의존하도록 설정해 응집력을 조금 높였습니다.

LottoTicket은 최소 하나 이상 다섯 개 이하까지 포함될 수 있도록 검증했습니다.

LottoTicket에 올바른 값이 포함되도록 테스트 코드를 작성했습니다.

LottoGame은 이제부터 클래스 타입으로 비교하게 됩니다.

LottoGame 하위 클래스를 생성해 인스턴스 타입으로 비교할 수 있도록 변경했습니다. 로또 타입은 행위의 결과로 프로퍼티에 존재해도 됐지만 변경될 가능성이 적고 각 유형마다 추가될 행위들이 다르다고 판단했습니다. 각 클래스마다 generate 메서드를 만들면 어떨까요 ㅎㅎ

🤔 논의하고 싶은 내용 / 질문 등

단방향으로 의존 할 수 있도록 코드를 리팩토링 하고 싶었지만 LottoTickets <- LottoTickt <- LottoGame 이였지만 방법을 찾지 못했습니다. 어떻게 하면 좋을까용?

@this-is-spear this-is-spear requested a review from a team as a code owner November 6, 2023 14:24
Copy link

github-actions bot commented Nov 6, 2023

👋 @this-is-spear 님, 진행하시느라 고생하셨어요!
조금만 기다려주시면 리뷰가 시작될거에요. 😄
PR 이 리뷰를 위한 정보를 충분히 담고 있는지 마지막으로 살펴봐주시는건 어떨까요? 🤔

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (88ffaed) 73.33% compared to head (426c3a1) 72.46%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main      #22      +/-   ##
============================================
- Coverage     73.33%   72.46%   -0.87%     
+ Complexity       23       22       -1     
============================================
  Files            11       11              
  Lines            75       69       -6     
  Branches          9       10       +1     
============================================
- Hits             55       50       -5     
+ Misses           20       19       -1     
Files Coverage Δ
src/main/kotlin/model/lotto/LottoGame.kt 100.00% <100.00%> (ø)
src/main/kotlin/model/lotto/LottoTicket.kt 83.33% <100.00%> (+83.33%) ⬆️
src/main/kotlin/model/lotto/LottoTickets.kt 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@doxxx93 doxxx93 self-assigned this Nov 8, 2023
Comment on lines +3 to +5
sealed class LottoGame(val numbers: LottoNumbers) {
class Manual(manualNumbers: LottoNumbers) : LottoGame(manualNumbers)
class Auto(autoNumbers: LottoNumbers) : LottoGame(autoNumbers)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

패턴 매칭 😀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 방법을 패턴 매칭이라 하는군요!! 😲

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 새로운거 알아갑니다~

Copy link
Contributor

@doxxx93 doxxx93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

패턴 매칭을 이용해서 기존의 코드들이 상당히 깔끔하게 정리됐네요..!

Comment on lines +9 to +11
val manualGames = manualNumbers.map { LottoGame.Manual(it) }.toList()
val autoGames = autoNumbers.map { LottoGame.Auto(it) }.toList()
val tickets = (manualGames + autoGames).chunked(5).map { LottoTicket(it) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아름답습니다...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이게 언어지...

Copy link
Member

@songkg7 songkg7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그동안 코틀린 스타일로 짜기 위해 나름 노력해왔다고 생각했는데, 건창님이 한 단계 더 높은 경지를 보여주신 것 같네요 👍

코드 스타일로는 제가 피드백 드릴 부분이 따로 없는 것 같아요 😂

의존성 방향에 관해서는 여러 생각이 드네요~ 내일 오전 시간에 간단하게 함께 이야기해보고 정리해보시죠~

@songkg7
Copy link
Member

songkg7 commented Nov 10, 2023

이 PR 을 마지막으로 마무리할게요~ 1달여남짓 진행하셨는데 수고하셨습니다 👏 merge 처리는 추후 제가 해놓을게요~!

@songkg7 songkg7 merged commit 2148c01 into Learning-Is-Vital-In-Development:main Nov 17, 2023
4 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants