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

[로또 2단계] 모찌 미션 제출합니다. #150

Open
wants to merge 32 commits into
base: wondroid-world
Choose a base branch
from

Conversation

wondroid-world
Copy link

셀프 체크리스트

  • 프로그램이 정상적으로 작동하는가?
  • 모든 테스트가 통과하는가?
  • 이전에 받은 피드백을 모두 반영하였는가?
  • 코딩 스타일 가이드를 준수하였는가?
    • IDE 코드 자동 정렬을 적용하였는가?
    • 린트 검사를 통과하였는가?
  • 프로그래밍 요구 사항을 준수하였는가?
  • README.md에 기능 목록을 정리하고 명확히 기술하였는가?

어떤 부분에 집중하여 리뷰해야 할까요?

안녕하세요! 두루.
저는 이번에 객체지향적인 코드를 작성하는 데, 많은 시간을 투자했습니다. 그리고 모든 원시값과 문자열을 포장할려고 노력했습니다. 예외 처리는 아직 미흡하여 추가적으로 보충한 뒤, 다시 pr 하도록 하겠습니다! 그리고 두루가 말한대로, LottoTicketGenerator interface를 만들어서 수동 로또 생성기와 자동 로또 생성기를 구현했습니다.

@Gyuil-Hwnag Gyuil-Hwnag self-requested a review February 28, 2025 02:13
Copy link
Member

@Gyuil-Hwnag Gyuil-Hwnag left a comment

Choose a reason for hiding this comment

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

모찌 안녕하세요!
해당 미션 구현 고생 많으셨습니다!

이번 단계를 진행하면서 많이 어려움이 있으셨나 보군요..🥲
구현한 내용들을 보면서, 이번 미션을 진행하면서 어려움이 있으셨는지 구현에 통일성이 없다는 생각이 들었어요
어떤 부분에서 어려움이 있었는지 같이 의논해가면서 해결해나가면 되는거라서, 의논해보고 싶은 내용있다면 언제든 말씀 주세요~
고민해보시면 좋을만한 내용들 코멘트를 달았으니 확인해주시면 감사하겠습니다.

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 🙏
화이팅입니다💪

.map(::LottoNumber)
return LottoTicket(LottoIssueType.AUTO, lottoNumbers)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

다음 설정 옵션을 활성화하여 EOF 경고를 없애주세요🥲
File > Preferences > Editor > General > Ensure every saved file ends with a line break

이전에는 없다가 새로 생긴걸 보니, Android Studio가 재설치 되었거나 했나 보군요~

Copy link
Author

Choose a reason for hiding this comment

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

어?! 저는 Android Studio를 쓰고 있지 않았는 데, 음... 제가 수동으로 merge하다가 괄호를 지운거 같습니다. 😭

Comment on lines 7 to 8
class LottoTest {
@Test
Copy link
Member

Choose a reason for hiding this comment

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

추가로 미션 제출전에 다음 설정으로 문법에 안 맞는 내용은 없는지 확인해 보시는 것도 좋을것 같습니다~😆
./gradlew clean ktlintCheck

스크린샷 2025-02-28 오전 11 16 24

Comment on lines 10 to 19
return kotlin.runCatching {
val possibleToLottoTicketCount = meetLottoStoreCashier()
val lottoTickets = getLottoTickets(possibleToLottoTicketCount)
val winningLotto = getWinningLotto()
getResult(lottoTickets, winningLotto)
}.map {
Result.success(Unit)
}.getOrElse {
e -> Result.failure(Exception("(로또 실행 중 오류 발생) : ${e.message}"))
}
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
Author

Choose a reason for hiding this comment

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

사실, 이 구조가 나온것은 논리적 오류와 비논리적 오류에 대해서 잘 알지 못했습니다. 그래서, 비지니스 오류에서 나오는 것을 모두 출력하고 프로그램을 종료하고자 위와 같이 구현했습니다.

지금까지 정리한 생각

  • 논리적 오류 : 비지니스 로직 상의 오류
  • 비논리적 오류 : 네크워크 문제와 같이 외부적인 문제 오류
    라고 생각합니다.

그래서 현재 구현한 로직은 사용자가 입력을 잘못하거나 비지니스 상 오류가 생겼을 때, 입력을 다시 받을 수 있도록 구현했습니다. 그래서 사용자가 저의 프로그램을 끝까지 사용할 수 있도록 했습니다.

Comment on lines 5 to 15
class AutoLottoTicketGenerator: LottoTicketGenerator {
override val type: LottoIssueType = LottoIssueType.AUTO

override fun generateLottoTicket(): LottoTicket {
val lottoNumbers = (LottoRuleConstants.MINIMUM_NUMBER.value..LottoRuleConstants.MAXIMUM_NUMBER.value)
.shuffled()
.take(LottoRuleConstants.LOTTO_PICK_COUNT.value)
.sorted()
.map(::LottoNumber)
return LottoTicket(LottoIssueType.AUTO, lottoNumbers)
}
Copy link
Member

Choose a reason for hiding this comment

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

자동 로또 생성기 정말 좋네요~👍
다만, type이 꼭 있어야 하는 이유가 있을까요?
현재 LottoTicket 에서는 전혀 사용을 안하고 있는 것 같은데, 추가를 하신 이유가 궁급합니다🤔

Copy link
Author

Choose a reason for hiding this comment

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

현실에서 로또를 발행했을 때, 번호 옆에 자동, 수동이 표시가 되었습니다. 현실 로또 모양을 반영하고 싶어서 타입을 추가했습니다. 그래서 로또를 출력했을 때 아래와 같이 보이게 구현했습니다.

image

Comment on lines 6 to 10
fun issueLottoTickets(
customerWantToBuyAutoLottoTicketCount: LottoTicketCount,
manualLottoNumbers: List<List<Int>>,
): List<LottoTicket> =
purchaseManualLottoTickets(manualLottoNumbers) + purchaseAutoLottoTickets(customerWantToBuyAutoLottoTicketCount)
Copy link
Member

Choose a reason for hiding this comment

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

issue 라는 네이밍이 뭔가 와닿지를 않기는 했는데, create, make 등등이 있었을 것 같긴 한데 모찌는 어떠신가요..?🤔

Copy link
Author

Choose a reason for hiding this comment

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

저는 로또 머신이 로또를 발행하는 주체라고 생각했습니다. '로또티켓을 발행한다'라는 의미에서 issue를 썼는데.. 음…. 의미가 잘 안 와닿았나 보네요. 이 부분은 조금 더 고민해보겠습니다!

Comment on lines -15 to +27
fun getNumbers() = numbers
fun matchNumbersSize(lottoTicket: LottoTicket) = numbers.intersect(lottoTicket.getNumbers()).size
Copy link
Member

Choose a reason for hiding this comment

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

로또가 몇개가 맞는지에 대한 로직이 LottoTicket 내로 이동을 한 이유가 궁금합니다~🤔

Copy link
Author

Choose a reason for hiding this comment

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

로또 번호가 몇 개 맞는지에 대한 로직이 LottoTicket으로 이동한 이유는 책임의 분리(SRP) 원칙을 따르기 위해서입니다.

WinningLotto 클래스는 "당첨된 로또 티켓과 다른 로또 티켓들을 비교"하는 책임을 가집니다. 이 클래스는 당첨 결과를 확인하는 역할에 집중해야 하며, 실제로 번호를 맞추는 로직은 LottoTicket 객체가 처리해야 할 일이라고 생각했습니다.

LottoTicket 클래스는 로또 번호를 담고 있는 객체로, 자신에게 맞는 번호가 몇 개인지 확인하는 역할을 맡는 것이 자연스럽다고 생각했습니다.

WinningLotto의 관심사는 "당첨된 로또 티켓과 비교하여 몇 등인지 확인하는 것"이지, 어떻게 번호가 맞는지를 구현하는 것은 LottoTicket의 책임이라고 생각햅니다. 각 객체는 자신의 책임에만 집중해야 하기 위해서, LottoTicket 내로 함수를 이동시켰습니다.

Copy link
Member

Choose a reason for hiding this comment

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

WinningLotto는 로또의 당첨 번호만을 관리하고 LottoTicket 에서 내 번호가 당첨됐는지를 판별하도록 하셨군요~
모찌의 생각을 잘 알 수 있어서 좋네요!👍

Comment on lines 14 to 17
init {
require(customerWantBuyLottoTicketCount >= customerWantToBuyManualLottoTicketCount) { "전체 발행 가능한 로또 개수보다 수동로또 발행 개수가 더 많습니다." }
require(customerWantToBuyManualLottoTicketCount == manualLottoNumbers.size) { LOTTO_COUNT_NOT_MATCH_LOTTO_NUMBERS }
}
Copy link
Member

Choose a reason for hiding this comment

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

하드코딩된 내용들을 이전 단계처럼 상수화를 해보면 좋을 것 같습니다~

Comment on lines 3 to 6
interface LottoTicketGenerator {
val type: LottoIssueType
fun generateLottoTicket() : LottoTicket
}
Copy link
Member

Choose a reason for hiding this comment

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

인터페이스의 생성 정말 좋네요ㅎㅎ👍

Comment on lines 22 to 23
fun getCustomerWantToBuyManualLottoTicketCount(): Int = customerWantToBuyManualLottoTicketCount.toInt()

Copy link
Member

Choose a reason for hiding this comment

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

오호 함수명이 어마무시하게 길군요..!🫣
무조건 짧아야 한다는 아니지만, 엄청 긴데 무슨 기능을 하는지 한눈에도 안들어오는 느낌이 살짝 드는군요~🤔
LottoTicketIssueManager전반적인 코멘트 입니다

Copy link
Author

Choose a reason for hiding this comment

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

하하하.... 저는 무조건 자세한 변수명이 좋은 건줄 알았습니다.
두루말처럼 간결한 변수명으로 변경해봤습니다!

eba7611

Comment on lines 26 to 27
OutputView.printMessage("\n거스름 돈은 ${change}입니다.")
}
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
Author

Choose a reason for hiding this comment

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

두루! 두루말처럼 한번 해봤습니다. 이러니, 정말 문자열들이 한눈에 들어오네요! 나중에 관리하기 편하겠어요! 좋은 꿀팁 감사합니다!

d2499d4

@wondroid-world
Copy link
Author

두루,
한 객체는 한 acting만해야한다는 생각에 너무많은 객체를 생성한거 같아요. 그런데, 주석을 하나하나 달아보면 다 그만한 역할이 있고 또 이를 합치는 것은 한 객체가 너무 많은 역할을 하는 거 같아요. 나름의 기준을 잡아가는 과정이 꾀 어렵네요.

두루 덕분에 네이밍에 대해서 많은 고민을 할 수 있어서 좋았습니다. 너무 자세한 네이밍을 하기 위해서 변수 이름이 길어지는 건 지양해야겠습니다. ㅎㅎㅎ

@Gyuil-Hwnag Gyuil-Hwnag self-requested a review March 3, 2025 14:11
Copy link
Member

@Gyuil-Hwnag Gyuil-Hwnag left a comment

Choose a reason for hiding this comment

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

모찌 안녕하세요!

해당 미션 리뷰 반영 고생 많으셨습니다
리뷰 반영하면서 구현한 내용 수정 너무 잘 해주셨습니다 💯

고민해보시면 좋을만한 내용들 코멘트를 달았으니 확인해주시면 감사하겠습니다.
다음 미션인 블랙잭 미션이 로직적으로 많은 어려움이 있어서, 모든 코멘트를 반영하기 보다는 UserInterface (Input / Output) / 테스트 코드 부분만 이라도 우선적으로 살펴보고 나머지 코멘트는 살펴보고 고민후 추후 적용을 해봐도 좋을 것 같다는 생각이 듭니다!

위에 부분은 블랙잭 미션전에 한번 정리를 하고 가면 좋을 것 같다는 생각이 듭니다~
다만 다음 미션에서 시간이 많이 소요될 것 같아서, 반영에 시간이 오래걸리실 것 같다면 코멘트에 대한 의견이라도 남겨주신다음에 다시 요청 주셔도 좋습니다

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요! 🙏
화이팅입니다💪

Comment on lines -15 to +27
fun getNumbers() = numbers
fun matchNumbersSize(lottoTicket: LottoTicket) = numbers.intersect(lottoTicket.getNumbers()).size
Copy link
Member

Choose a reason for hiding this comment

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

WinningLotto는 로또의 당첨 번호만을 관리하고 LottoTicket 에서 내 번호가 당첨됐는지를 판별하도록 하셨군요~
모찌의 생각을 잘 알 수 있어서 좋네요!👍

Comment on lines +5 to +7
class AutoLottoTicketGenerator : LottoTicketGenerator {
override val type: LottoIssueType = LottoIssueType.AUTO

Copy link
Member

Choose a reason for hiding this comment

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

Generator는 생성자인데, model 에 있는것이 좋을까 싶긴 합니다!
다른 패키지로 옮긴다면 어떤 패키지로 옮겨볼 수 있을까요?🤔

Comment on lines +11 to +23
// 자동 로또를 발행
private fun purchaseAutoLottoTickets(customerWantToBuyAutoLottoTicketCount: LottoTicketCount): List<LottoTicket> {
val autoLottoGenerator = AutoLottoTicketGenerator()
val autoLottoTickets =
List(customerWantToBuyAutoLottoTicketCount.toInt()) { autoLottoGenerator.generateLottoTicket() }
return autoLottoTickets
}

// 수동 로또를 발행
private fun purchaseManualLottoTickets(manualLottoNumbers: List<List<Int>>): List<LottoTicket> {
val manualLottoTickets = manualLottoNumbers.map { ManualLottoTicketGenerator(it).generateLottoTicket() }
return manualLottoTickets
}
Copy link
Member

Choose a reason for hiding this comment

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

타입별 로또 티켓에 대한 생성자 좋습니다~👍

Comment on lines 4 to +5
class LottoMachine {
fun purchase(count: Int): List<LottoTicket> = List(count) { LottoTicket() }
// 로또 티켓을 발행한다.
Copy link
Member

Choose a reason for hiding this comment

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

class외에 object라는 키워드가 있는데, 이둘은 어떤 차이점이 있을까요?🤔
모찌의 생각을 남겨주세요!
각 두개의 키워드는 어떤 경우에 사용하면 좋을까요?

Comment on lines +11 to +28
// 로또의 가격은 천원이다. 로또를 구매할려면 천원이상 넣어야한다.
init {
require(customerMoney >= LottoRuleConstants.LOTTO_AMOUNT.value) { INSUFFICIENT_MONEY_FOR_LOTTO_PURCHASE }
}

// 고객이 준 돈에서 몇장을 살 수 있는 지 계산한다.
fun calculatePossibleToBuyLottoTicketCount(): Int = customerMoney / LottoRuleConstants.LOTTO_AMOUNT.value

// 고객이 몇장을 사고 싶다고 이야기를 하면 가능한지 계산한다.
fun isPossibleToBuy(customerWantBuyLottoTicketCount: Int): Boolean = possibleToLottoTicketCount >= customerWantBuyLottoTicketCount

// 고객에게 전달할 잔돈을 계산한다.
fun calculateChange(customerWantBuyLottoTicketCount: Int): Int =
customerMoney - (LottoRuleConstants.LOTTO_AMOUNT.value * possibleToLottoTicketCount)

companion object {
private const val INSUFFICIENT_MONEY_FOR_LOTTO_PURCHASE = "로또는 천원이상 넣어야지 구매 가능합니다."
}
Copy link
Member

Choose a reason for hiding this comment

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

이전 코멘트
해당 코멘트를 통해서 주석을 남겨주신 이유에 대해서는 너무 잘 이해를 했습니다!

다만, 한가지 고민을 해보면 좋을 것 같아서 코멘트를 남겨두었습니다~
복잡한 로직에 대해서는 주석을 남겨두면 어떤 로직인지에 대한 이해가 잘되도록 돕습니다.
그러나, 함수명 or 구현된 코드를 보고도 유추할 수 있는 모든 내용들에 주석이 존재한다면 오히려 구현을 이해하는데 혼동을 줄 수도 있기는 해서 추후 개발을 하면서 어떤 부분에 남겨야할지 모찌만의 기준을 잘 잡으셨으면 좋겠습니다💪

Comment on lines 11 to 13
class LottoController(
private val userInterface: UserInterface = UserInterface(),
) {
Copy link
Member

Choose a reason for hiding this comment

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

45 이상의 숫자를 입력하면 앱이 죽도록 되어 있군요~
이부분은 요구사항에 없긴 한데, 한번 고민해보면 좋을 것 같은 내용인 것 같아서 코멘트 남겨두었습니다!
재 입력을 받도록 해볼 수도 있겠다는 생각도 드는군요!💪

(수정이 이루어져야 하는 것은 아니고 어떻게 하면 해당 기능을 구현할 수 있을지 모찌의 의견을 남겨주셔도 좋을 것 같습니다)

class LottoMachine {
fun purchase(count: Int): List<LottoTicket> = List(count) { LottoTicket() }
Copy link
Member

Choose a reason for hiding this comment

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

LottoMachine을 통해서 로또를 생성하는군요!
LottoMachine을 통해서 수동 로또 / 자동 로또가 재대로 생성되는지에 대한 테스트를 해볼 수 있겠군요💪

Comment on lines 3 to 7
object OutputView {
fun printMessage(msg: Any) = println(msg)
fun printlnMessage(msg: Any) = println(msg)

fun printMessage(msg: Any) = print(msg)
}
Copy link
Member

Choose a reason for hiding this comment

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

OutputView와 InputView에서 너무 담당을 하는 것이 없다는 생각이 드네요..😅
UserInterface에서 입출력에 대해서 모든 기능을 담당하고 있으므로, 해당 InputView / OutputView의 내용을 UserInterface의 하나의 함수로 만들어 보는 것이 나을 수도 있겠다는 생각이 듭니다!

Comment on lines 8 to 10
class UserInterface(
private val inputValidator: InputValidator = InputValidator(),
) {
Copy link
Member

Choose a reason for hiding this comment

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

Input / Output을 담당하는 UserInterface가 생겼으니, 로또의 InputOutput을 담당한다는 기능에 맞게 네이밍을 수정해보면 좋을 것 같다는 생각이 듭니다!💪

Comment on lines 7 to 8
class LottoTest {
@Test
Copy link
Member

Choose a reason for hiding this comment

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

테스트 코드에 대한 전반적인 코멘트 입니다!
https://developer.android.com/studio/test/test-in-android-studio#view_test_coverage
테스트코드를 어느 정도로 작성했는지 확인하는 방법으로 테스트 커버리지 라는 개념이 있습니다
IDEA에서 기능을 지원해 주고 있습니다

무조건 커버리지가 100을 달성해야 한다는 말 보다는 각 객체들이 담당하고 있는 기능들에 대해서는 어느정도의 테스트는 있어야 할 것 같다는 생각이 듭니다!💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants