-
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
[1단계 - 블랙잭] 몽이(황재웅) 미션 제출합니다. #770
Conversation
- 요구사항의 딜러는 첫번째 카드만 출력한다.
- 16점 이하라면, true를 반환한다. - 17점 이상이라면, false를 반환한다.
-refactor: 예외 처리를 커스텀하여 관리하라 -refactor: 덱 셔플을 생성과 분리하라
-refactor(Deck): final을 보장하라 -chore: DealerRoster에 대한 설명을 추가하라 -chore: Converter를 패키지로 관리하라
-refactor: 도메인을 text로 바꾸는 컨버터를 분리하라
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다시 만나 반가워요! 앤지😎
앞선 피드백을 토대로,
이번 리펙토링에서 신경쓰였던 부분들 추가로 표시해보았어요!
긴 글 읽으며, 좋은 리뷰 남겨주셔서 감사해요~🙌
public class BlackjackGame { | ||
|
||
private static final int NUMBER_OF_DECK = 1; | ||
|
||
private final Participants participants; | ||
private final Deck deck; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컴포지션 활용
현재 서비스가 크지 않기에, 이를 총괄하는 객체를 토대로 컴포지션을 활용했습니다.
컨트롤러가 보다 객체를 활용하도록 수정했습니다.
이 과정에서 캡슐화에 좀 더 가치를 두었기에, 잘 반영되었는지 궁금합니다!
Setter와 같이 객체의 상태를 조작하는 것은 항상 BlackjackGame에게 시켰습니다.
단순히 조회 혹 상태 저장과 무관한 계산은
getter를 통해 작업을 할 객체를 반환하도록 구성하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순한 조회여도 getter를 많이 사용하면 참고 - 디미터 법칙 위반 같은 문제점이 생길 수 있어요. 디미터 법칙을 엄격하게 지켜야 하는가
는 고민 포인트가 될 수도 있겠지만 아래 코드는 수정하면 좋을 것 같아요 😄
BlackjackController.outputDealerInitialDealResult
코드에 리뷰를 남겼으니 확인해보시겠어요? 😄
public abstract boolean isHit(); | ||
|
||
public abstract boolean isDealer(); | ||
|
||
public abstract String getName(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추상 클래스 활용
Participant가 name을 필드로 가지지 않는 대신,
getName()를 추상 메서드로 선언하였습니다.
이때 isDealer()의 사용으로, 추상클래스의 장점을 살리지 못한 것 같아 아쉽습니다.
다형성을 활용하지 않는 부분에 isDealer()를 통하여 객체를 구분하게 됩니다.
이와 같이 자식을 알고 구별하기 위한 메서드가 추상 클래스에 존재해도 괜찮은가?
오히려 추상 클래스의 장점을 해치는 것이 아닌가 걱정됩니다.
isDealer()를 없애자니 getName을 통해 구별하든, InstanceOf로 구별하든
결과적으로 List내에서 Dealer와 Player를 구별하는 작업이 요구되는 상황입니다.
차라리 isDealer()가 가장 직관적이라 생각하여 추상 메서드로 관리했습니다.
이렇게 타협하고 넘어가도 괜찮을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isDealer() 메서드를 사용하면 Dealer와 Player를 구별할 수 있지만 타입을 직접 체크하는 방식이므로 다형성을 완전히 활용하지 못하는 것은 맞아요. 그러나 상속을 사용하는 환경에서 Dealer와 Player를 반드시 구별해야 한다면, isDealer()를 활용하는 것도 현실적인 타협이 될 수 있습니다.
다만, isDealer()의 사용 빈도가 점점 많아진다면 다른 방법은 없을지 고민해보면 좋을 것 같아요.
또한, isDealer()메서드가 딜러는 한 명이어야 하고, 플레이어는 여러 명이 될 수 있다는 가정을 검증 할 때 주로 사용되는데 그렇다면 Dealer와 Player를 하나의 리스트에서 함께 관리하는 것이 적절한지도 다시 생각해보는 것도 좋겠습니다.
몽이만의 정답을 찾아보세요 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 몽이~
드린 리뷰에 대해 굉장히 많은 고민 학습을 해주셨군요. 👍 고맙습니다 😄
주신 질문들에 대한 답변은 코멘트로 남겼으니 확인해주세요!
2단계도 파이팅입니다 😄
private void outputDealerInitialDealResult(final BlackjackGame blackjack) { | ||
final var dealer = blackjack.getDealer(); | ||
final Hand hand = dealer.getHand(); | ||
final TrumpCard firstCard = hand.getCards().getFirst(); | ||
outputView.printDealerHitResult(converter.cardToText(firstCard)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드의 TrumpCard firstCard
는 결국 아래와 같이 getter를 많이 사용해서 값을 가져오네요.
final TrumpCard firstCard = blackjack.getDealer().getHand().getCards().getFirst();
딜러의 첫번째 카드는 공개한다.
는 블랙잭 게임의 룰 같은데 의미있는 메서드명으로 나타내보면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
디미터 법칙에 대해 잘못 이해하면서, 제가 정한 기준으로 생긴 문제입니다.
캡슐화를 잘 지킨 객체를 만들기 위해, 디미터 법칙이 생기고 그 단점으로 deepth가 깊어질 수 있다.
이와 같이 이해하며, 중간에 getter로 끊어 사용하면 단점을 해결할 수 있겠다 생각했습니다.
저는 그 중간에 끊기는 지점을 Player와 Dealer로 두었지요.
이 기준이 위와 같이 컨트롤러로 도메인 로직이 넘쳐버린 원인이라 생각합니다.
BlackjackGame 내부에서 충분히 처리할 수 있는 것인데, 이를 외부로 보내어 캡슐화가 깨진 것이죠.
디미터 법칙의 단점만 보고, 더 중요한 캡슐화에 대한 이점을 망각했던 것 같습니다.
때문에 BlackjackGame의 응집도를 높히고, 이 과정에서 불필요해진 컨트롤러를 제거하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전에는 그러면 너무 엄격하게 디미터 법칙을 지키는 것인가? 고민했습니다.
그러나 1차적으로 객체의 품질이 우선으로 두는 것이 디미터 법칙의 핵심이라 깨달았습니다.
객체의 품질을 지키면서, 단점들을 개선하는 것 요구되었던 거죠..!
정말 엄격한지 고민하기 위해서, 당장의 코드 품질을 좀 더 향상시키는 것이 중요한 것 같습니다.🫡
public class BlackjackGame { | ||
|
||
private static final int NUMBER_OF_DECK = 1; | ||
|
||
private final Participants participants; | ||
private final Deck deck; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순한 조회여도 getter를 많이 사용하면 참고 - 디미터 법칙 위반 같은 문제점이 생길 수 있어요. 디미터 법칙을 엄격하게 지켜야 하는가
는 고민 포인트가 될 수도 있겠지만 아래 코드는 수정하면 좋을 것 같아요 😄
BlackjackController.outputDealerInitialDealResult
코드에 리뷰를 남겼으니 확인해보시겠어요? 😄
public abstract boolean isHit(); | ||
|
||
public abstract boolean isDealer(); | ||
|
||
public abstract String getName(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isDealer() 메서드를 사용하면 Dealer와 Player를 구별할 수 있지만 타입을 직접 체크하는 방식이므로 다형성을 완전히 활용하지 못하는 것은 맞아요. 그러나 상속을 사용하는 환경에서 Dealer와 Player를 반드시 구별해야 한다면, isDealer()를 활용하는 것도 현실적인 타협이 될 수 있습니다.
다만, isDealer()의 사용 빈도가 점점 많아진다면 다른 방법은 없을지 고민해보면 좋을 것 같아요.
또한, isDealer()메서드가 딜러는 한 명이어야 하고, 플레이어는 여러 명이 될 수 있다는 가정을 검증 할 때 주로 사용되는데 그렇다면 Dealer와 Player를 하나의 리스트에서 함께 관리하는 것이 적절한지도 다시 생각해보는 것도 좋겠습니다.
몽이만의 정답을 찾아보세요 😄
src/test/java/domain/DeckTest.java
Outdated
class pickCard { | ||
@DisplayName("카드를 올바르게 뽑아온다.") | ||
@Test | ||
public void pickCard() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
학습과 고민 많이 해주셨네요. 😄
src/main/java/domain/Player.java
Outdated
public boolean isPickCard() { | ||
return participant.calculateAllScore() <= MAX_SCORE; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오! 전반적으로 다 수정해주셨군요! 아주 좋습니다. 👍 이제 도메인들을 보면 누가봐도 블랙잭 게임인줄 알겠군요!
public void pickCard(final Deck deck) { | ||
final Card card = deck.pickCard(); | ||
hand.add(card); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
죄송할필요 없습니다~! 저도 늘 100퍼센트인 정답을 찾고 싶은데 그런 경우가 잘 없어 곤란하더라구요. ㅎㅎ. 의견 물어봐줘서 고마워요! 👍
src/main/java/domain/Dealer.java
Outdated
public class Dealer { | ||
private final Participant participant; | ||
private int winCount = 0; | ||
private int loseCount = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호~ 다전제까지 생각해주셨군요. 그렇다면 winCount, loseCount를 필드로 가지고 있던게 이해가 가네요. 👍
src/main/java/domain/Player.java
Outdated
public Participant getParticipant() { | ||
return participant; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 맞아요! 엄격하게 디미터의 법칙을 지키려고 하면 오히려 더 복잡해질 수 있어요. 상황에 따라 적절히 비교해보고 판단해서 사용하는게 좋을 것 같아요 😄
학습목표
페어 간 개인 목표
-> 그리고 리펙터링 과정에서 테스트를 추가한다.
(혹 Green케이스 커밋에 나뉜 작은 테스트를 함께 첨부한다.)
체크 리스트
test
를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
1. Dealer와 Player의 중복 코드를 어떻게 관리할 것인가?
현재 컴포지션을 통해 중복 코드를 관리하고 있습니다.
다음은 컴포지션을 선택한 의사 결정 과정입니다.
그런데, 이 방식에서는 도메인에서 중복 코드가 많지 않지만,
컨트롤러 단에서 각 도메인이 사용될 때 중복 코드가 많았습니다.
때문에, 컴포지션을 선택한 것과 활용법에 대해 피드백 받고 싶습니다.
2. 추상 클래스에 대한 접근
저는 추상 클래스를 엄연히 객체로 보아 다형성을 꼭 활용해야 한다 보았습니다.
물론, 비즈니스 로직도 적은 Nickname 정도는 그냥 추상화 하면 편한데...
그냥 편하니까 하나로 선택하지 않기 위해서, 차선책과 컴포지션 적용을 고민하였습니다.
*차선책으로 떠올렸던 것은
Player에 Nickname을 상태로 관리하고, Dealer는 딜러 네임을 상수로 가지며
추상 클래스의 GetName 메소드를 각각 오버라이딩하는 것 입니다.
(인터페이스처럼 활용하기)
이 또한, 추상 클래스에 대해 잘못 접근하는 부분일지 궁금합니다.
학습 목표와 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
이번 미션에서 추가된 요구사항을 잘 수행하지 못했기에, 2점을 주었습니다.
1. 모든 엔티티를 작게 유지한다.
모든 엔티티를 작게 유지한다는 것은, 곧 도메인의 책임이 잘 분리되었는 지를 의미한다 생각합니다.
현재 미션 자체의 비즈니스 로직이 적어 엔티티가 크지 않다곤 느껴지지만,
설계의 방향성을 바꾸면 더 작은 엔티티를 가질 수 있지 않을까 생각합니다.
현재 구조와 다르게, 완전히 블랙잭 게임과 룰을 담당하는 객체를 만들고,
플레이어와 딜러로부터 블랙잭 룰을 빼면 어떻게 달라질까?
(블랙잭 딜러/플레이어가 아닌, 그냥 딜러/플레이어로 접근해보기)
이와 같은 고민이 있지만, 양 측면을 비교하지 못해 정답을 구하지 못했으니,
설계적으로 부족한 코드(확신이 없는 코드)라 생각합니다.
2. 3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
Player가 3개의 인스턴스 변수를 가지게 되었습니다.
승리 여부를 저장하기 위한 필드를 외부로 분리하고, 별도의 승패 기록을 관리하는 클래스가 필요하다 느꼈습니다.
그러나 개발 과정에서, 이를 완전히 설계해내지 못하여, 아쉬운 부분이라 생각합니다.
3. 딜러와 플레이어에서 발생하는 중복 코드를 제거해야 한다.
컴포지션을 활용했지만, 코드 중복 제거가 다소 부족하다 느껴집니다.