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

[엘리] 블랙잭 1단계 미션 제출합니다. #37

Merged
merged 19 commits into from
Mar 18, 2020

Conversation

YebinK
Copy link

@YebinK YebinK commented Mar 12, 2020

안녕하세요 후니 님! 엘리라고 합니다 :)
이번 리뷰 잘 부탁드립니다!

YebinK and others added 16 commits March 10, 2020 14:16
	- Card의 타입을 나타내는 CardType enum 추가
	- Card의 심볼을 나타내는 CardSymbol enum 추가
Dealer의 카드 합이 16 이상인지 체크하는 메소드 구현
	- Dealer와 Player가 Gamer 상속
	- 카드의 합을 구하는 기능을 CardCalculator로 이동
	- Gamer에서 bust 됐는지 확인
	- 딜러가 16이하인 경우 카드를 계속해서 더 받음
	- 플레이어는 bust되지 않은 경우 카드를 더 받을지 물어보고 카드를 받음
- 딜러와 플레이어들 간 승부 구하는 PlayerResultMatcher 클래스 구현
- 딜러/플레이어 각각 결과 출력
	- List<Card>를 Hand 일급 컬렉션으로 생성
	- List<Player>를 Players 일급 컬렉션 생성
	- PlayerResultMatcher에 대한 test case 추가
	- PlayerResultMatcher와 GamersResult의 역할 분리
Copy link

@jeonghoon1107 jeonghoon1107 left a comment

Choose a reason for hiding this comment

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

안녕하세요 엘리!
리뷰가 늦어서 죄송합니다 ㅠㅠ
간단한 피드백 남겼으니 확인부탁드려요 :)

Dealer dealer = new Dealer();
Players players = Players.ofComma(InputView.askPlayerNames());

HandInitializer.initialize(dealer, players, deck);

Choose a reason for hiding this comment

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

메인에서 너무 많은 역할을 하고 있는 것 같아요.
게임을 진행하는 클래스를 만들면 어떨까요?

Copy link
Author

@YebinK YebinK Mar 15, 2020

Choose a reason for hiding this comment

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

BlackJackController 클래스를 만들어서 로직을 이동했습니다!

}

@Override
public String getName() {

Choose a reason for hiding this comment

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

getName을 override하지 않고 Gamer의 속성으로 가지고 있으면 어떨까요?

Copy link
Author

@YebinK YebinK Mar 15, 2020

Choose a reason for hiding this comment

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

이 부분은 출력 로직을 위해 override했습니다!
OutputView에서 중복을 줄이려면

private static void printCardStatusAndResult(Gamer gamer) {
        System.out.println(String.format(CARD_STATUS_AND_RESULT_FORMAT, gamer.getName(), makeHandResult(gamer.getCardStatus()), gamer.calculateSum()));
    }

이렇게 파라미터로 gamer를 받아와서

printCardStatusAndResult(dealer);
printCardStatusAndResult(player);

이런 방식으로 호출하고 싶었습니다. 그리고 딜러 역할을 수행하는 자의 이름이 더 이상 '딜러'가 아니게 될 경우, 도메인을 변경해도 된다고 생각했습니다.

이 부분에 대해 후니 님은 어떻게 생각하시는지 궁금합니다! :)

Choose a reason for hiding this comment

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

파라미터로 gamer를 받아도 같은 결과가 나올 것 같아요!
피드백 남긴 부분을 참고해주세요!

}

public int calculateSum() {
return CardCalculator.calculate(hand);

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.

Score 객체를 추가했습니다!

YebinK added 3 commits March 15, 2020 23:33
    - 출력을 위한 메소드들 OutputView로 이동
    - GamersResultAssembler로 GamersResultDto 생성 로직 구현
    - 플레이어 이름 입력값 검증/타입 변환하는 NamesDto 생성
Copy link

@jeonghoon1107 jeonghoon1107 left a comment

Choose a reason for hiding this comment

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

안녕하세요 엘리!
깔끔하게 리팩토링 하셨네요 👍
피드백은 다음 단계에서 반영해주세요 :)


public abstract boolean canDrawCard();

public abstract String getName();

Choose a reason for hiding this comment

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

제가 이전에 피드백했던 부분은 Gamer가

private String name;
public String getName() {
   return name;
}

을 가지면 어떨까였습니다!
그렇게 되면 각 구현체에서는 public abstract String getName(); 해당 메서드를 구현할 필요 없어질 것 같아요!

@jeonghoon1107 jeonghoon1107 merged commit 9b84d19 into woowacourse:yebink Mar 18, 2020
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.

3 participants