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

[Step4] 블랙잭(베팅) 구현 #816

Open
wants to merge 5 commits into
base: seokho-ham
Choose a base branch
from

Conversation

Seokho-Ham
Copy link

안녕하세요 수현님!
마지막 4단계 구현 후 리뷰 요청드립니다🙇🏻‍♂️

이전 [단계 피드백] (https://github.com/next-step/kotlin-blackjack/pull/809#pullrequestreview-2504302728)에 대한 의견들은 댓글로 남겨 두었습니다!

잘부탁드립니다!

Copy link
Member

@wisemuji wisemuji left a comment

Choose a reason for hiding this comment

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

안녕하세요 석호님!

마지막 단계 역시 구현을 잘 해주셨습니다 👍
마지막 단계인 만큼 여러 가지 고민과 함께 편하게 도전해 보시면 좋겠습니다.

궁금한 점이나 논의하고 싶은 내용이 있으면 언제든 DM 주세요!

Copy link
Member

Choose a reason for hiding this comment

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

from: #809 (comment)

저는 사실 테스트코드만을 위한 코드 변경은 해본적이 없어서 그런지 굳이 필요할까라고 생각이 들기는 합니다!
혹시 수현님은 이렇게 변경했을때 경험하신 이점들이 따로 있으셨을까요?

예를 들어 Cards의 add 함수가 바뀐다거나, Cards의 테스트 과정에서 많은 카드 더미가 있어야 하는 전제 조건이 있는 경우 이점을 얻을 수 있을 것 같아요.

저도 테스트 코드만을 위한 코드 변경은 최대한 보수적으로 접근하려고 하는데요, (예를 들어 테스트코드만을 위해 private -> public으로 전환한다던지?) 예외적으로 생성자 파라미터의 경우 객체를 초기화하는 전제 조건 주입 목적으로는 허용하고 있습니다! 외부에서 변경 가능하지 않도록 설계할 수 있기도 하구요!

이 부분은 재성님이 수업에서도 언급하셔서 제안드렸으나 개발자마다 주관적인 의견이 있을 것이라고 생각합니다 🙂

Comment on lines +6 to +13
class DealerResult(playerGameResults: List<PlayerGameResult>) : CalculateEarnAmount {
private var earnAmount: Int =
playerGameResults
.map { it.getEarnAmount() }
.fold(0) { total, amount -> total + amount }.toInt()

init {
playerGameResults.forEach {
when (it.result) {
PlayerWinLoseResult.WIN -> loseCount++
PlayerWinLoseResult.LOSE -> winCount++
PlayerWinLoseResult.PUSH -> pushCount++
}
}
override fun getEarnAmount(): Int {
return -earnAmount
Copy link
Member

Choose a reason for hiding this comment

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

earnAmount가 variable이 될 필요가 있을까요~?

package blackjack.domain

fun interface CalculateEarnAmount {
fun getEarnAmount(): Int
Copy link
Member

Choose a reason for hiding this comment

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

어떤 의도로 만드신 인터페이스인지 이해했습니다!

다만 현재는 인터페이스의 Calculate~ 워딩이 함수처럼 느껴질 수도 있을 것 같아서 더 적절한 이름으로 바꿔볼 수 있을 것 같아요!

import blackjack.domain.Players
import blackjack.view.PlayerInfo

class BlackJackGame private constructor(
Copy link
Member

Choose a reason for hiding this comment

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

이전 단계에서 딜러와 플레이어의 중복 코드에 대한 고려 + 차이가 나는 로직에 대해 적절히 분리하는 작업이 선행되어서 이번 단계에서는 큰 변경사항이 없었네요!

이 부분이 사실 블랙잭 미션의 핵심 중 하나인데요, 딜러와 플레이어는 언뜻 보면 같은 참가자로 묶였을 때 더 직관적인 것처럼 보이나, 사실 분리했을 때 오히려 구조가 더 깔끔해집니다.

4단계를 진행하면서 이 구조의 이점이 잘 와닿으셨으면 좋겠네요!

Copy link
Member

Choose a reason for hiding this comment

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

혹시 어제의 수업을 듣고 상태 패턴을 적용해볼 계획도 있으신가요?
마지막 단계인 만큼 편하게 도전해보세요!

https://seonghoonc.tistory.com/14

Copy link
Author

Choose a reason for hiding this comment

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

마지막이니까 한번 도전해보고 다시 리뷰 요청 드리겠습니다:)

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