-
Notifications
You must be signed in to change notification settings - Fork 467
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단계 - 블랙잭 베팅] 말론(유민재) 미션 제출합니다. #883
Open
minjae8563
wants to merge
66
commits into
woowacourse:minjae8563
Choose a base branch
from
minjae8563:step2
base: minjae8563
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
딜러에게 카드를 받지 않고, 덱에게 카드를 받는 로직으로 수정
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
체크 리스트
test
를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
Player 가 돈, 이름만 들고있긴 하지만 추상 클래스가 Hand 를 가지고 있기에, 필드가 2개인지 3개인지 애매해서 4점으로 하겠습니다.
어떤 부분에 집중하여 리뷰해야 할까요?
안녕하세요 제이미!! 2단계를 구현하면서 설계에 대한 고민을 많이 했습니다. 다른 요구 사항은 만족하기 쉬웠으나 필드의 인스턴스 변수 3개를 유지하라는 사항에서 고민을 많이 했던 것 같습니다. 다음은 제가 했던 고민 및 설계 방향입니다.
추상클래스
1차 미션에서는 조합을 활용하여 중복된 로직을 한 곳에서 관리하였습니다 (당시 CardDeck 클래스). 하지만 이번에 Player 의 필드에 Money 가 들어옴에 따라 Player 는 총 3개의 인스턴스 변수 (Name, Money, Hand) 를 가지게 됩니다.
따라서 더 이상 조합으로는 요구 사항을 만족할 수 없게 되었습니다. 이후 단순히 생각한 건 상속입니다. 부모 - 자식의 관계로 중복되는 로직을 빼고자 하였습니다. 제가 생각한 중복된 로직은 '카드를 추가한다, 카드의 합계를 구한다' 입니다.
하지만 단순히 부모 - 자식 간의 관계를 하기 보다는, 추상 클래스를 도입하여 객체가 구체화할 수 있는 부분은 없을까 고민하였습니다.
그 결과 초기 받은 카드 출력 부분을 추상 메서드로 뺄 수 있을 것 같다는 결론이 나왔습니다. 추상 클래스에서 추상 메서드로 openInitialCards() 를 가지고 있으며, Player 는 초기 2장을 리턴해주고, 딜러는 초기 1장을 리턴해주는 식으로 구체화하고 있습니다.
이로써 Player 필드에는 Money, Name 만을 가지게 되었습니다. 허나, 여기서 고민인 것은 추상 클래스 Hand 가 있다는 것입니다. 자식이 복사해서 필드로 추가하는 것은 아니기에 괜찮을지 여쭤봅니다.
기존의 CardDeck 을 Hand 와 Deck 분리
기존의 CardDeck 은 조합을 위하여 List 와 관련된 기능들을 한 곳에서 모아서 관리했습니다. 위에서 말씀드린 것처럼 추상클래스를 도입했기에 더 이상 이렇게 진행할 필요가 없어졌습니다. 따라서 각각 List 를 가지고 있는 Hand, Deck 을 구현하였습니다. 또한, 딜러가 더 이상 Deck 을 가지고 게임을 진행하지 않습니다. Deck 을 통해 Player 와 Dealer 에게 카드를 주는 식으로 로직을 수정하였습니다. 이는 추상 클래스에서 구현된 메서드를 둘이 오버라이딩 하여 재사용한다는 관점에서 더욱 들어 맞는다고 생각합니다. (부모는 외부에서 카드를 받아와 저장하지만, 딜러는 본인이 직접 뽑은 카드만 저장할 수 있다면 딜러가 부모를 대체할 수 없다고 생각했습니다.)
Money, Name 포장
기존의 Player 가 들고있던 String name 을 포장한 객체인 Name 을 구현하였습니다. Player 의 필드에 많은 요소들이 있기에 Name 과 관련된 일은 Name, Names 가 해야하는 것이 맞다고 생각하여 포장하였습니다. 비슷한 이유로 Money 도 포장하였습니다.