-
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
[3단계 - 블랙잭 재구현] 엠제이(최민준) 미션 제출합니다 #885
base: main
Are you sure you want to change the base?
Conversation
* docs: 기능 요구 사항 작성 * feat: Player, Players 객체 생성 * feat: 문자열을 쉼표로 파싱하는 기능 구현 * feat: 이름 입력받는 기능 구현 * feat: 카드 객체 생성 * refactor: 상수 추출 * feat: 카드 필드 추가 Co-authored-by: Starlight258 <[email protected]> * feat: 방어적 복사 수행 Co-authored-by: Starlight258 <[email protected]> * refactor: 패키지 변경 Co-authored-by: Starlight258 <[email protected]> * feat: 카드 랜덤 생성기 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 카드를 분배하는 CardManager 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 카드 여러장 분배하는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * test: 테스트 패키지 변경 Co-authored-by: Starlight258 <[email protected]> * feat: 딜러 카드 받는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * test: 테스트픽스쳐 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 플레이어 카드 받는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 플레이어들 카드 받는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * test: 카드와 함께 플레이어 만들어주는 코드 추가 Co-authored-by: Starlight258 <[email protected]> * feat: 카드 보여주는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * test: 카드 보여주는 기능에 대한 테스트 작성 Co-authored-by: Starlight258 <[email protected]> * feat: 모든 참가자에게 카드 배분하는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * refactor: 메서드명 변경 Co-authored-by: Starlight258 <[email protected]> * refactor: 상수명 변경 Co-authored-by: Starlight258 <[email protected]> * refactor: 반환 타입 변경 Co-authored-by: Starlight258 <[email protected]> * feat: 분배한 카드 출력 Co-authored-by: Starlight258 <[email protected]> * docs: 기능 요구 사항 업데이트 Co-authored-by: Starlight258 <[email protected]> * feat: 공백 제거 기능 추가 Co-authored-by: Starlight258 <[email protected]> * feat: 카드 분배 가능 여부 판단 기능 구현 Co-authored-by: Starlight258 <[email protected]> * refactor: 메서드명 변경 Co-authored-by: Starlight258 <[email protected]> * feat: 카드를 분배하는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 카드를 분배 여부 입력 기능 구현 Co-authored-by: Starlight258 <[email protected]> * feat: Controller에서 카드 분배 기능 구현 Co-authored-by: Starlight258 <[email protected]> * test: TestFixture 메서드 추가 Co-authored-by: Starlight258 <[email protected]> * feat: 추가로 받은 카드 출력 기능 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 카드 추가 여부, 닉네임 조회 인터페이스로 두어 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 딜러 카드 분배 기능 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 카드 출력 기능 파라미터 인터페이스로 수정 Co-authored-by: Starlight258 <[email protected]> * feat: 딜러에게 카드 나눠주는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * test: TextFixture 메서드 추가 Co-authored-by: Starlight258 <[email protected]> * docs: 기능 요구 사항 업데이트 Co-authored-by: Starlight258 <[email protected]> * refactor: 인터페이스명 변경 Co-authored-by: Starlight258 <[email protected]> * feat: Cards 일급 컬렉션 도입 Co-authored-by: Starlight258 <[email protected]> * feat: Gamer 추상 클래스 도입 및 기존 인터페이스 제거 Co-authored-by: Starlight258 <[email protected]> * feat: 최종 결과 및 카드 합 보여주는 기능 구현 Co-authored-by: Starlight258 <[email protected]> * refactor: 추상 클래스를 이용하여 하나의 메서드로 변경 Co-authored-by: Starlight258 <[email protected]> * feat: 결과 상태 enum 구현 Co-authored-by: Starlight258 <[email protected]> * feat: 승패 결과 계산 기능 구현 Co-authored-by: Starlight258 <[email protected]> * refactor: 중복된 상수 제거 Co-authored-by: Starlight258 <[email protected]> * feat: 승패 결과 enum 도입 Co-authored-by: Starlight258 <[email protected]> * feat: 승패 결과 출력 기능 구현 Co-authored-by: Starlight258 <[email protected]> * test: testFixture 메소드 추가 Co-authored-by: Starlight258 <[email protected]> * refactor: 카드 받는 메소드 추상 클래스의 메소드로 추출 Co-authored-by: Starlight258 <[email protected]> * refactor: 중복 코드 제거 Co-authored-by: Starlight258 <[email protected]> * refactor: depth 1로 수정 Co-authored-by: Starlight258 <[email protected]> * refactor: 변수를 상수로 변경 Co-authored-by: Starlight258 <[email protected]> * feat: Answer 객체 생성 Co-authored-by: Starlight258 <[email protected]> * feat: 잘못된 입력시 재입력 받도록 수정 Co-authored-by: Starlight258 <[email protected]> * feat: 딜러와 플레이어가 모두 버스트가 난 경우 딜러의 승리로 계산하도록 변경 Co-authored-by: Starlight258 <[email protected]> * refactor: 안쓰는 상수 제거 * refactor: Ace를 1로 계산하는 메소드 추출 * refactor: 주석 변경 * test: 딜러에게 카드 한 장 주는 기능 테스트 추가 * refactor: Answer 도메인으로 패키지 변경 * refactor: 승패 결과를 BlackjackGame에서 계산하도록 변경 * refactor: 사용 안되는 메소드 제거 * refactor: 참가자 수 계산 기능을 처음에 나눠줘야할 카드 수 계산하는 기능으로 변경 * refactor: InputView에서 Scanner 주입받도록 변경 * docs: 기능 요구 사항 업데이트 * test: 테스트 메소드명 변경 * refactor: Gamer와 Cards를 외부에서 찾아서 전달하도록 변경 * refactor: Players 정적 팩토리 메서드 패턴 적용 * refactor: 승패 결과 계산을 BlackjackGame에서 하도록 수정 * refactor: 안쓰는 메서드에 대한 테스트 제거 * refactor: 개행 1줄로 수정 * refactor: index 대신 player 객체를 전달하도록 수정 --------- Co-authored-by: Starlight258 <[email protected]>
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.
엠제이 안녕하세요~
제로베이스부터 다시 설계하여 개발해주셨네요
어떤 점을 느끼셨는지도 궁금하네요 ㅎㅎ
이전코드와 지금 코드를 개발할 때 어떤점이 달랐나요?
추가로 몇 가지 코멘트 남겼습니다
확인부탁드릴게요 :)
} | ||
|
||
private static void betPlayer(final Blackjack blackjack, final Player player) { | ||
final InputView inputView = new BettingMoneyInput(player.getNickName()); |
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.
new InputView(), new ResultView()와 같이 직접 생성하는 코드가 많네요.
뷰 생성이 분산되어 있어서 유지보수나 테스트가 힘들 것 같아요
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.
사실 애초에 View는 테스트 대상이 아니었어요.
그리고 이미 다 만들어진 기능들을 순서에 맞게 호출해주는 메인도 테스트 대상이 아니라고 생각해서 개인적인 생각으로는 문제가 안될 것 같습니다...!
혹시 이에 대해서는 어떻게 생각하시나요?
public class Application { | ||
public static void main(String[] args) { | ||
final Blackjack blackjack = new Blackjack(); | ||
Dealer dealer = Dealer.getDealer(new CardRandomMachine()); | ||
dealer.initCardMachine(); | ||
Players players = makePlayers(); | ||
|
||
betPlayers(blackjack, players); | ||
|
||
spreadInitCards(blackjack, dealer, players); | ||
|
||
printInitialCards(dealer, players); | ||
|
||
final boolean isPush = blackjack.isPush(dealer, players); | ||
|
||
if (!isPush) { | ||
spreadExtraCards(players, blackjack, dealer); | ||
} | ||
|
||
printCardsSum(dealer, players); | ||
|
||
printBettingResult(blackjack, isPush, dealer, players); | ||
} |
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.
Application 클래스가 많은 로직을 직접 수행하고있네요.
Application이 게임의 흐름을 제어하도록 설계하신 것 같아요
- 단순히 흐름만 제어하는 것이 아니라 예외 처리 및 반복 로직까지 담당하는 것 같아요. 이 부분을 보완해볼까요?
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.
제로가 이해하신 게 맞습니다.
Application에서 게임의 흐름을 제어하고 있어요.
그리고 blackjack 객체와 Application의 책임을 부여하는 과정에서,
잘못된 입력에 대한 에러핸들링을 누가 해줘야할까? 고민했었습니다.
초기에는 blackjack의 역할이라고 생각했어요.
입력을 잘못했으니, 이에 대한 피드백도 blackjack이 해줘야한다고 생각했는데,
이렇게 되면 blackjack 내부에서 inputView의 구현 사항을 알게 되더라구요.
저는 이 부분이 잘못 됐다고 생각했습니다.
blackjack이라는 객체가 inputView의 내부 구현을 알고 있다는 점이 결합도를 높인다고 생각했거든요.
그래서 view와 관련되어 있는 blackjack 객체의 로직은 모두 Application에서 다뤄줬어요.
이러면 blackjack이 view들의 내부 구현을 아예 모르게 돼서 이점이 있었어요.
private static boolean readIfHit(final Player player) { | ||
final InputView inputView = new HitOrStandView(player.getNickName()); | ||
try { | ||
final String answer = inputView.read(); | ||
final UserAnswer userAnswer = UserAnswer.of(answer); | ||
return userAnswer.isYes(); | ||
} catch (IllegalArgumentException e) { | ||
inputView.printErrorMessage(e); | ||
} | ||
return readIfHit(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.
재귀호출 하는 방식으로 진행되네요
- 재귀 호출의 단점은 무엇일까요?
- 개선할 수 있을까요?
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.
제가 알기론, 자바에서 메서드를 호출할 때 지역변수나 매개변수를 스택에 저장하는 걸로 아는데,
재귀호출이 반복되면 스택이 넘쳐서 스택오버플로우의 위험이 있는 걸로 알고 있어요.
개선 방향으로는 재귀 대신 반복문을 사용하거나 Memoization을 사용하면 좋은 걸로 알고 있는데 이번 미션에서 다루고자 하는 목표가 아니어서 개선 보류 중입니다...!
혹시 틀린 내용이 있다면 알려주세요!
public void earnDouble() { | ||
earnedMoney.increase(bettingMoney.getDouble()); | ||
} | ||
|
||
public void earnSingle() { | ||
earnedMoney.increase(bettingMoney.getMoney()); | ||
} | ||
|
||
public void earnOneAndHalf() { | ||
earnedMoney.increase(bettingMoney.getOneAndHalf()); | ||
} |
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.
bettingMoney.getDouble()
같은 표현이 Money의 상태를 직접 가져와서 처리하는 방식이네요. 캡슐화를 지키는 방식으로 개선해볼까요?
- increase 보다
add~
같은 네이밍이 더 적절할 것 같아요
package blackjack.bettingMachine; | ||
|
||
public class BettingMachine { |
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.
패키지 구조를 보니 DDD + MVC 구조가 합쳐진 것으로 보여지는데요
이렇게 구성한 이유가 궁금합니다 :)
도메인 응집을 높이기 위해 이 방법을 선택하였을까요?
public void win() { | ||
bettingMachine.earnDouble(); | ||
} | ||
|
||
public void draw() { | ||
bettingMachine.earnSingle(); | ||
} |
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 BettingMoneyInput implements InputView { | ||
|
||
private static final String INPUT_WANT_HIT = "%s의 배팅 금액은?"; | ||
private static final Scanner scanner = new Scanner(System.in); |
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.
Scanner
를 모두 직접 주입하고있네요
생성자주입은 어떨까요?
|
||
public class Constant { | ||
|
||
public static final String LINE = System.lineSeparator(); |
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.
LINE
까지는 상수화가 불필요한 것 같아요
class BlackjackTest { | ||
|
||
@DisplayName("딜러와 플레이어에게 모두 2장씩 나눠준다.") | ||
@Test | ||
void deal() { | ||
// given |
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.
매 테스트마다 Dealer, Players가 생성되는데 공통 생성로직을 만들어볼까요?
private static void printInitialCards(final Dealer dealer, final Players players) { | ||
final ResultView resultView = new ResultView(); | ||
resultView.printEmptyLine(); | ||
resultView.printCards(dealer.getNickName(), dealer.showInitialCards()); | ||
for (Player player : players.getPlayers()) { | ||
resultView.printCards(player.getNickName(), player.showInitialCards()); | ||
} | ||
} |
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.
Dealer와 Player의 상태(showInitialCards())를 직접 View에 전달하고 있네요~ Player와 Dealer를 직접 참조해야 되어서 View와 도메인이 강하게 결합될 것 같아요
개선할 수 있을까요?
체크 리스트
안녕하세요 제로, 엠제이입니다.
기존 코드 참고 없이 제로베이스부터 다시 설계를 해봤어요.
질문사항과 리뷰 고려 사항은 아래에서 자세히 설명해볼게요!
test
를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?
1~5점 중에서 선택해주세요.
선택한 점수의 이유를 적어주세요.
객체에 자율적 책임을 부여하려고 노력했는데, 만족스러운 부분도 있고 아닌 부분도 있어요.
어떤 부분에 집중하여 리뷰해야 할까요?
이번 단계 목표
책임이 자율적일수록 적절하게 '추상화'되며, '응집도'가 높아지고, '결합도'가 낮아지며, '캡슐화'가 증진된다고 생각해서
객체에게 자율적 책임을 부여하려고 했습니다.
제가 생각한 자율적 책임이란,
예를 들어
손님
이란 객체가요리사
라는 객체에게 피자를 만들어오라는 메시지를 던지면,요리사
라는 객체는 자기가 알아서 피자를 만들어오면 된다고 생각을 했어요.어떤 재료를 먼저 손질할지, 반죽을 어떻게 만들지
손님
이라는 객체가 정해줄 필요가 없는거죠.그래서?
객체들은 그 객체가 담당해야하는 책임만 갖고 있도록 설계를 했습니다.
그리고, 어디에선가는 블랙잭이라는 게임에 맞게, 게임순서대로 메서드들을 호출해줘야하는데,
저는 그게
main
의 역할이라고 생각했어요.main
은 다시 말해 게임이라는 프로그램의 프로세스를 담당하도록 했습니다.이번 단계를 하면서 여러번 구조를 바꿨는데, 어떤 때는
blackjack
이라는 객체가view
들의 내부 구현 로직을 알 때도 있어서 main으로의 프로세스 분리가 불가피해지기도 했어요.전체적인 구조와 흐름, 결합도 부분에 대해서 피드백 해주시면 감사하겠습니다!