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단계 미션 코드리뷰 제출합니다 #26

Merged
merged 31 commits into from Mar 15, 2020
Merged

Conversation

lxxjn0
Copy link

@lxxjn0 lxxjn0 commented Mar 12, 2020

블랙잭 1단계 미션 코드리뷰 제출하겠습니다.

궁금한 부분들이 많은데 얄짤없이 리뷰해주시면 감사하겠습니다 😄

1. Symbol 열거형 인스턴스 구현.
2. Type 열거형 인스턴스 구현.
1. Hand 인스턴스의 add 메서드 오버로딩.
1.Hand 인스턴스 패키지 이동.
1. Hand 인스턴스의 카드 점수를 계산하여 반환하는 기능 구현.
2. Hand 인스턴스에서 카드를 뽑을 수 있는지 여부를 판단하는 기능 구현.
3. Score 인스턴스에 점수의 합을 구하는 기능 구현.
1. User 추상 클래스에 equals 오버라이딩.
1. Report 인스턴스 구현.
2. ResultType 열거형 인스턴스의 from 메서드 수정.
3. 테스트를 위한 Dealer, Player 인스턴스의 정적 팩토리 메서드 구현.
Comment on lines 8 to 13
public enum ResultType {
WIN(value -> value > 0, "승"),
DRAW(value -> value == 0, "무"),
LOSE(value -> value < 0, "패");

private Predicate<Integer> compareResult;
Copy link
Author

Choose a reason for hiding this comment

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

ResultType에서 Predicate<Integer>를 어떤 ResultType인지 확인하기 위해 변수로 두었습니다.

Java Enum 활용기라는 글에서 Enum의 장점 중 하나가 상태와 행위를 한 곳에서 관리한다는 것이라고 하였습니다.
그런데 제가 작성한 코드에서 Predicate가 상태라고 생각해야 할지 행위라고 생각해야 할 지, 그리고 이렇게 Predicate를 단지 판단하기 위한 용도로 변수로 선언하는 방식이 Enum에서 일반적으로 사용되는 방식인지 궁금합니다.

Choose a reason for hiding this comment

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

predicate이라는 행위를 상태로 갖는 것으로 생각하면 됩니다.
슬랙 dm으로 답변 드린 내용이라 간략히 답변 남길게요

Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

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

안녕하세요 스티치, 리뷰어 구구입니다
게임규칙과 뷰를 역할에 맞게 잘 분리했네요 👍
몇 가지 피드백 남겼으니 수정해보시면서 궁금한 점 생기면 코멘트 남기거나 dm 주세요

}

public static List<Card> cards() {
return CARDS;

Choose a reason for hiding this comment

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

반환할 때 new ArrayList를 해주면 이 메서드를 사용하는 클라이언트에서 new ArrayList를 하지 않아도 될거에요

Copy link
Author

Choose a reason for hiding this comment

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

넵! 해당 부분 반영하겠습니다.

카드에서 poll을 해주기 위해서 LinkedList<Card>로 반환하도록 구현해두겠습니다.

.collect(collectingAndThen(toList(), Collections::unmodifiableList));
}

private static Stream<Card> createByType(Symbol symbol) {

Choose a reason for hiding this comment

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

createByType인데 ByType에 해당 되는 부분은 굳이 안써도 되겠네요
Symbol이라는 인자 값으로 표현하고 있으니깐요

Copy link
Author

Choose a reason for hiding this comment

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

해당 메소드는 연속적으로 읽어질 때 자연스럽게 하기 위해서 createBy(Symbol symbol)과 같은 식으로 수정해보겠습니다 👍

import java.util.List;
import java.util.stream.Stream;

public class CardRepository {

Choose a reason for hiding this comment

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

Repository를 보통 영속성 장치처럼 데이터를 저장해두는 곳과 관련된 객체에 쓰다보니 이 클래스에 어울리는 이름으로 보이진 않네요

Copy link
Author

@lxxjn0 lxxjn0 Mar 14, 2020

Choose a reason for hiding this comment

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

처음엔 캐싱해두는 카드를 가진다는 느낌이 강해서 CardRepository로 이름을 명명했는데 결국 카드를 만들어서 주는 역할이니 CardFactory로 변경하겠습니다.

}

public Card draw() {
if (cards.isEmpty()) {

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.

저도 이 부분에 대해 고민을 했는데, 실제 블랙잭 규칙은 사용자가 8명이란 제한이 있어서 한 카드 팩으로 게임이 가능한 것 같았습니다.

그래도 혹시나 사용자의 제한이 없다면 추가적인 카드팩이 필요하다고 생각이 들어서 해당 경우를 만들어줬는데 한 카드팩이 소진되면 예외를 발생시켜 게임이 종료되도록 수정하겠습니다!!

import java.util.List;

public class PlayerFactory {
private static final String DELIMITER = ",";

Choose a reason for hiding this comment

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

DELIMITER가 도메인에 해당되는 값이 맞을까요? view 아닐까요?
콘솔에서 콤마로 구분해서 이름 입력 받다가 웹으로 블랙잭이 확장되면 사용 가능한 도메인 코드일까요?
이름 리스트를 받아서 플레이어를 만들면 콘솔이든 웹이든 모바일이든 좀더 넓게 사용할 수 있지 않을까요?

Copy link
Author

@lxxjn0 lxxjn0 Mar 14, 2020

Choose a reason for hiding this comment

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

제가 StringUtil 클래스를 생성해서 문자열에 대한 처리를 모두 해당 클래스에서 해주었습니다.
말씀하신 부분도 parsingPlayerNames()라는 메서드로 만들어서 처리하려고 하는데 StringUtil과 같은 유틸성 클래스를 지양하라는 말을 들은 적이 있습니다.

해당 클래스를 지양해야 하는게 맞는지와 만약 사용해도 괜찮다면 어디까지 해당 클래스에서 처리해도 괜찮은지 궁금합니다!!

Copy link

Choose a reason for hiding this comment

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

StringUtil는 정말 String만 처리할 때 써야합니다.
지금 StringUtil 클래스는 도메인 객체를 받아와서 문자열 처리를 하고 있는데 이렇게 되면 StringUtil 클래스는 전역 변수처럼 모든 도메인에 대한 처리를 하는 클래스가 됩니다.
그래서 유틸성 클래스는 쓰지 말라고 하죠.
도메인 객체에 List<String> getPlayerNames() 메서드를 추가하고, StringUtil에 List<String> 파라미터를 받아서 String으로 반환하는 기능이면 StringUtil에 추가해도 괜찮겠네요


public abstract class User {
private static final int DRAW_LOWER_BOUND = 1;
private static final int BLACKJACK_SCORE = 21;

Choose a reason for hiding this comment

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

블랙잭 조건이 카드 합계가 21일 때 일까요?
처음 받은 카드 2장의 합이 21일 때로 알고 있는데 이렇게 상수명에 블랙잭으로 쓰면 게임 규칙을 아직 다 파악 못한 협업하는 다른 개발자는 헷갈리지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

말씀하신 부분은 정말 생각하지도 못했습니다.

생각해보니 정말 제가 코드를 작성하는데 있어서 편하려고 그냥 21을 블랙잭 스코어라고 정해뒀는데, 사실 실제의 규칙을 생각해보면 충분히 혼동을 줄 수 있다는 생각이 듭니다.

정말 사소하게 생각할 수도 있는 부분인데 이런 부분까지도 협업을 고려하면서 작성해야 한다는 것을 알게 되었습니다.
정말 해당 피드백을 해주셔서 감사합니다. 저도 저만 보는 코드를 작성하는 것이 아니라 상대방도 볼 수 있다는 생각을 가지고 작성하도록 노력하겠습니다!!!

Copy link
Author

Choose a reason for hiding this comment

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

버스트는 블랙잭 여부와 상관없이 22라는 점수로 판별 가능하기 때문에 버스트 스코어를 활용해서 수정하겠습니다!!!!

}

public static Score valueOf(Card card) {
return CACHE.get(card.getSymbolValue());

Choose a reason for hiding this comment

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

card.getSymbolValue()
이 메서드 명이 헷갈리네요;;
그냥 스코어 반환하는 메서드 아닐까요?

Copy link
Author

Choose a reason for hiding this comment

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

처음에 Symbol의 필드 이름을 value로 해서 이처럼 지었는데 getScore()로 수정하였습니다!!

return score;
}

public static Score valueOf(Card card) {

Choose a reason for hiding this comment

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

인자가 null일 때 방어로직도 필요하겠네요

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분도 처리해줬습니다!!
그런데 null에 대한 처리를 어디까지 해주어야 하는지 궁금합니다.
객체가 인자로 들어오는 부분은 모두 처리를 해주어야 할까요?

Choose a reason for hiding this comment

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

public으로 열린 메서드가 파라미터로 객체를 받으면 해줘야죠.
파라미터가 null로 입력되는게 의도한 값은 아니었을테니깐요
다른 개발자나 다른 객체가 null을 전달되지 않을거라고 확신할 수는 없으니 잘 막아두고 컴파일이나 테스트에서 잡을 수 있도록 만드는게 버그 발생하는 것보다 낫겠죠

return CACHE.get(card.getSymbolValue());
}

public Score add(Card card) {

Choose a reason for hiding this comment

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

이 메서드도 인자가 Null일 때 처리를 해주는게 좋겠어요

Copy link
Author

Choose a reason for hiding this comment

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

위와 같이 Objects.requireNonNull() 메서드를 활용하여 처리하였습니다.

public Score add(Card card) {
int currentScore = score + card.getSymbolValue();

if (card.isAce() && currentScore + ADDITIONAL_ACE_SCORE < BUST_SCORE) {

Choose a reason for hiding this comment

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

currentScore + ADDITIONAL_ACE_SCORE
이 부분이 중복되고 있네요
메서드로 추출할 수 있지 않을까요?

@lxxjn0
Copy link
Author

lxxjn0 commented Mar 15, 2020

최대한 피드백을 반영하고자 노력해봤습니다.

기존의 BlackjackController에서의 게임 규칙을 새로운 BlackjackTable로 추출하였습니다.
그래도 플레이어가 카드를 뽑는 기능이 입력이 관여가 되어서 어쩔수 없이 일정 부분은 컨트롤러에 위치하였습니다. 이 부분에 대해서 가능한 다른 방법이 있는지 궁금합니다.

그리고 테스트를 작성하는데 많이 노력을 기울였는데 테스트에서

@Test
void drawInitialCards_DealerAndPlayer_DrawInitialCardsForEachUsers() {
	BlackjackTable blackjackTable = new BlackjackTable(deck);
	List<User> users = blackjackTable.collectToUsersFrom(dealer, players);
	blackjackTable.drawInitialCards(users);

	// NOTE: 2020-03-15 테스트에서 forEach를 사용해도 괜찮은가
	for (User user : users) {
		assertThat(user).extracting("hand").asList().hasSize(INITIAL_DRAW_NUMBER);
	}
}

이 부분에서 forEach로 테스트를 진행하는게 잘못된 방법인지 궁금합니다.

정말 세세하게 놓칠 수 있는 부분까지도 피드백을 해주셔서 감사합니다.
최대한 많은 것을 배우고 고쳐나가고 싶습니다. 부족한 부분 가감없이 피드백 해주시면 정말 감사하겠습니다!!!

this.deck = deck;
}

public List<User> collectToUsersFrom(Dealer dealer, List<Player> players) {

Choose a reason for hiding this comment

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

BlackjackTable 클래스가 딜러, 플레이어를 인스턴스 변수로 갖는 것도 괜찮겠네요

Copy link
Author

Choose a reason for hiding this comment

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

아직 완벽하게 컨트롤러와 분리를 못해서 이렇게 뒀는데, 좀 더 확실히 도메인으로 분리하면서 인스턴스 변수로 두어보겠습니다 :)


private static void validateUser(Dealer dealer, List<Player> players) {
if (Objects.isNull(dealer) || Objects.isNull(players) || players.isEmpty()) {
throw new InvalidReportException(InvalidReportException.NULL);

Choose a reason for hiding this comment

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

NULL 대신 EMPTY가 적절해 보이네요
아예 존재 하지 않는 것(null)도 체크하지만 빈값(isEmpty)도 체크하고 있으니 두가지를 포함하는 empty가 적절해보여요

Copy link
Author

Choose a reason for hiding this comment

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

넵!!! 저도 empty가 좀 더 넓은 범위를 포괄할 수 있으니 empty가 나을 것 같아요!!

.collect(collectingAndThen(groupingBy(Function.identity(), counting()), EnumMap::new));
}

public Map<ResultType, Long> getDealerResult() {

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.

필요한 값만 리스트로 반환하라는 말씀은 혹시 로그와 같이 String으로 찍어서 반환하는 형식을 말씀하시는 건가요??

}
}

private boolean canDraw(User user) {

Choose a reason for hiding this comment

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

이 메서드는 없어도 되지 않을까요?
dealer.canDraw()나 player.canDraw()로 써도 충분하지 않을까요?
메서드로 한 번 더 감싸면 추가적인 어떤 기능이 있나? 라는 생각이 들어서 이 메서드가 어떻게 구현됐는지 파악해야되요

Copy link
Author

Choose a reason for hiding this comment

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

조건문의 형식을 맞추기 위해서 한번 더 분리를 진행했는데 특별한 기능이 없는 메서드인 만큼 다시 원래대로 돌려두겠습니다!!

}

private boolean wantDraw(Player player) {
return DrawOpinion.of(inputDrawOpinion(player)).isYes();

Choose a reason for hiding this comment

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

Suggested change
return DrawOpinion.of(inputDrawOpinion(player)).isYes();
return DrawOpinion.of(inputDrawOpinion(player))
.isYes();

보통 한 줄에 점 하나만 찍혀야 읽기 좋아요

import blackjack.domain.card.Symbol;

public class Score {
public static final Score ZERO = new Score(0);

Choose a reason for hiding this comment

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

접근제어자를 default로 둬도 문제 없겠네요
될 수 있으면 접근제어자를 활용해서 캡슐화 시키는게 좋습니다.
아래 BUST_SCORE도 마찬가지네요

Copy link
Author

Choose a reason for hiding this comment

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

원래는 항상 이 부분을 체크하는데 해당 상수를 자주 이동시켰다보니 체크가 안됐던 것 같습니다. 해당 부분 수정하겠습니다!!!

return score;
}

public static Score valueOf(Card card) {

Choose a reason for hiding this comment

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

public으로 열린 메서드가 파라미터로 객체를 받으면 해줘야죠.
파라미터가 null로 입력되는게 의도한 값은 아니었을테니깐요
다른 개발자나 다른 객체가 null을 전달되지 않을거라고 확신할 수는 없으니 잘 막아두고 컴파일이나 테스트에서 잡을 수 있도록 만드는게 버그 발생하는 것보다 낫겠죠

@kang-hyungu
Copy link

  1. 입력에 대한 의존성을 낮추면 되겠죠. 저는 function 인터페이스를 활용해서 입력, 출력에 대한 의존도를 줄였어요.
  2. 네 필요하면 for문을 쓸 수 있겠죠. 다만 테스트 코드도 계속 수정하고 관리해야되는 코드라는 점만 유의하시면서 복잡하지 않게 작성하시면 될 것 같네요

Copy link

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

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

피드백 반영 잘하셨어요 스티치 👍
간단한 피드백 남겼는데 다음 미션 진행하면서 반영해도 충분해보이네요
pr은 머지 처리할게요 수고하셨어요!

@kang-hyungu kang-hyungu merged commit d6c6bac into woowacourse:lxxjn0 Mar 15, 2020
kingbbode pushed a commit that referenced this pull request Mar 29, 2020
* [스티치] 블랙잭 1단계 미션 코드리뷰 제출합니다 (#26)

* feat : 상속, interface 학습테스트 요구사항 1단계 구현

* feat : 상속, interface 학습테스트 요구사항 2단계 구현

* docs : 구현 기능 목록 작성

* feat : Card 인스턴스 구현

1. Symbol 열거형 인스턴스 구현.
2. Type 열거형 인스턴스 구현.

* feat : CardRepository 클래스 구현

* feat : Deck 인스턴스 구현

* feat : Hand 인스턴스 구현

* feat : User 인스턴스 구현

1. Hand 인스턴스의 add 메서드 오버로딩.

* test : Deck 인스턴스와 User 인스턴스의 테스트 코드 수정

* refactor : User 인스턴스 draw 메서드 오버로딩 구현

* feat : Score 인스턴스 구현

1.Hand 인스턴스 패키지 이동.

* feat : Hand 인스턴스의 카드 점수를 계산하는 기능 구현

1. Hand 인스턴스의 카드 점수를 계산하여 반환하는 기능 구현.
2. Hand 인스턴스에서 카드를 뽑을 수 있는지 여부를 판단하는 기능 구현.
3. Score 인스턴스에 점수의 합을 구하는 기능 구현.

* feat : User 인스턴스를 추상 클래스로 수정 및 이를 상속받는 Dealer, Player 인스턴스 구현

* feat : DrawOpinion 열거형 인스턴스 구현

* feat : User의 버스트 여부를 판단하는 기능 구현

* feat : PlayerFactory 클래스 구현

1. User 추상 클래스에 equals 오버라이딩.

* feat : Player들의 이름을 입력받아 생성하는 기능 구현

* feat : User들이 처음 2장의 카드를 받았다는 메시지 출력 기능 구현

* test : User 테스트 코드 수정

* refactor : User 추상 클래스의 추상 메서드 접근 지정자 수정

* feat : 각각의 플레이어들이 카드를 더 받을지 여부에 따라 draw를 하는 기능 구현

* feat : 딜러가 카드를 더 뽑는 기능 구현

* feat : 유저들의 최종 카드와 점수를 출력하는 기능 구현

* feat : ResultType 열거형 인스턴스 구현

* feat : 유저들의 최종 결과를 구하는 기능 구현

1. Report 인스턴스 구현.
2. ResultType 열거형 인스턴스의 from 메서드 수정.
3. 테스트를 위한 Dealer, Player 인스턴스의 정적 팩토리 메서드 구현.

* feat : 유저들의 최종 결과를 출력하는 기능 구현

* test : StringUtil 클래스 테스트 수정

* refactor : OutputView 메서드 일부 수정

* refactor : 버스트 처리 추가 및 기타 코드 수정

* refactor : Score 인스턴스 유효성 검사 추가

* refactor : 피드백 반영

* refactor : hand 패키지 내에서 사용하는 상수의 접근 지정자 수정

* chore : package 수정

* refactor : 1단계 피드백 반영 완료

* refactor : symbol 타입 변경

* refactor : todo 리스트 추가 및 일부 리팩토링

* refactor : ScoreCalculator 분리 및 todo 리스트 진행

* refactor : User 클래스에 BlackjackParticipant 인터페이스 구현

* feat : ScoreType 열거형 객체와 ResultScore 인스턴스 구현

* refactor : 딜러가 블랙잭일 경우 결과 출력으로 넘어가는 기능 구현

* refactor : ResultType 열거형 인스턴스 수정

1. 먼저 ResultScore의 ScoreType을 비교하여 ResultType을 계산하는 기능 구현.
2. ResultScore의 Score를 비교하여 ResultType을 계산하는 기능 구현.
3. Score 인스턴스와 ResultScore 인스턴스에 Comparable 인터페이스 구현.

* feat : BettingMoney 인스턴스 구현

* refactor : Report 인스턴스 수정

1. Report 인스턴스의 결과 계산 메서드 수정.

* chore: 불필요한 코드 삭제

* refactor: BettingMoney를 Player 필드로 이동

* refactor: 플레이어의 bettingMoney를 setter가 아닌 생성자에서 초기화

Co-authored-by: Junyoung Lee <48052622+lxxjn0@users.noreply.github.com>
Co-authored-by: junyoung lee <ljy3510@gmail.com>
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.

None yet

2 participants