[2단계 - 블랙잭 구현] 배럴(김나은) 미션 제출합니다. #209

knae11 · 2021-03-13T00:36:40Z

안녕하세요 😃 닉, STEP2 코드리뷰 요청드립니다 😁 저번에 많이 많이 고쳐보는 것에 대해 걱정하지 않아도 된다는 말을 해주셔서 힘이 되었습니다❤ 그리고 가독성을 좀 더 좋게 하는 방식으로 주시는 팁 등이 도움이 많이 됩니다. 평소에 크게 생각하지 않아서 무의식중에 넘어갔던 것들을 짚어주셔서 이런 것들을 작게 나누면 리팩토링할 때 도움이 많이 되는 것 같아요!

더 많이 리팩토링하고 부딪혀보면서 많이 배울게요! 리팩토링 하면서 어렵고 고민되는 부분도 많지만 재미있네요😁 여기 저기 뜯어 고쳐서 코드 보기도 더 혼란스러울 것 같은데 피드백 꼼꼼하게 해주셔서 감사합니다. 👍

느낀점 및 질문

수업시간에 상태패턴을 배워서 적용해보려고 했는데, 아무리 생각해봐도 왜 적용해야하는지ㅠㅠ 모르겠더라구요ㅠㅠ 그래서 몇일 고민 끝에 적용을 포기하고 제 방식대로 작성해보았습니다! 승패로직과 카드를 받을 수 있는지 여부는 또 별개의 작동방식이라는 생각이 들어서 상태패턴이 적용되는 것이 어떤 의미를 가지는거지?? 라는 생각이 계속 들더라구요. (카드를 받을 수 있는지 여부가 상태로 관리되고 승패는 따로 관리되는 것 같아서요.) 제가 만든 로직에서는 카드를 받을 수 있는지 여부를 Score를 통해서 Player, Dealer 스스로 해주는 느낌이 들었어요.
DM으로 Outcome 로직에서 Score로 값을 2개 받지 않고 Player를 넘기는 것을 질문드렸는데.. 이것 역시 아무리 고민해도ㅠㅠ 떠오르지가 않았어요ㅠㅠ 어떤 의도인지 최대한 반영해보고 싶은데 변화할 수 있는 해결방안이 저에게는 안 보이나 봅니다.😂
해결방안을 찾기 위해 객체의 역할과 책임과 관련하여 실제 게임과 관련해서 생각해보려고 했습니다.
카드를 나눠주는 것도 딜러가 아닌 제3자(GameTable)가 하였으니 점수계산에 대한 승패 판단 역시 제 3자기 해주는 것이 옳다고 생각했어요. 그게 Outcome이 되는 것이고, 다만 이번에는 ScoreBoard라는 객체를 만들어서 Score값을 그대로 넘겨주는 것에 대한 오류 상황을 최대한 줄여주려고 해보았습니다! 여기서 피드백에 제가 상당히 어려워하는 것 같은데 혹시 좀 더 구체적인 로직방향에 대해 힌트를 주시면 찾아보겠습니다~! 😃
이렇게 객체를 나누다보니 궁금한 점이 생겼습니다. PlayerResult, ScoreBoard는 도메인에 관련된 결과를 가지는 객체인데, 이것들이 Dto라고 하기엔 실질적 정보를 가진 객체이니 애매하다는 생각이 들었습니다. 하지만, 도메인 로직을 가지는 객체라고 하기엔 Dto에 정보를 넘겨주는 성격이 강해서 getter로 Dto에게 정보를 넘겨줄 수 밖에 없는 것 같습니다. 이런 애매한 객체들은 무엇일까요..??
리팩토링하면서 TDD를 적용하기가 너무 어렵네요ㅠㅠ 여기저기 얽혀있는 순간 (특히 생성자를 고치는 순간) TDD는 포기하게 되는 것 같아요ㅠㅠ 결국 프로덕션 먼저 다 고치고 돌려보고 단위테스트를 작성하는 방식으로 하게 됩니다. 여기서 TDD를 좀 더 잘 적용할 수 있는 팁이 있을까요?

학습로그

[Java] Stream - 2

내용

2중 for문을 사용하기 위한 flatmap 사용.

List<Card> cards = Arrays.stream(Suits.values())
        .flatMap(suit -> Arrays.stream(Denominations.values())
                .map(denomination -> Card.from(suit, denomination))
        )
        .collect(Collectors.toList());

flatMap 내부에서 map을 한 번 더 연결해주어야 사용가능하다. flatMap 외부에서 suit를 사용하려면 사용이 불가능하다. 중간연산은 내부 값을 사용하여 다른 값으로 리턴해주기 때문이다.
원하는 대로 출력하려면 flatMap 내부 스트림에서 map을 한 번 더 연결해준다.

[OOP] 가독성 - 3

내용

부정연산자(!)는 가독성을 해치고, 실수도 유발할 수 있는 부분이라 반대 의미의 메서드를 만들어서 사용하는 것이 좋다.
compareTo 같은 경우 직접 연산을 해도 되긴 하지만 조금 위험해서요, 기본 타입의 경우는 Wrapper 클래스에서 비교 메서드를 제공하고 있다.

Integer.compare(score, o.score);

하나씩 선언과정을 끊는 것이 좋음

// BEFORE
return sumCards().isHigherThan(MINIMUM_SCORE_OF_NOT_TAKING_CARD);
// AFTER
Score score = sumCards();
return score.isHigherThan(MINIMUM_SCORE_OF_NOT_TAKING_CARD);

boolean 반환 메서드의 경우 바로 return 값으로 사용하는 것 고려
불필요한 정보를 다 알아야 할까? 라는 접근 보다는, 메서드 시그니처를 보았을 때 명확해야한다.
API를 설계할 때 다음 두 가지를 고려해보면 좋다.
1. 이 메서드가 선언되었을 때의 모양이 자연스러운가?
2. 클라이언트가 이 메서드를 사용할 때 사용법을 고민하지 않을 수 있는가?

[OOP] abstract class - 3

내용

abstract class () implements (interface) 는 interface의 모든 메소드를 구체화할 필요가 없다. 일부만 구체화하고 abstract class를 상속받을 자식 클래스에게 구체화 의무를 넘길 수 있다.

링크

stackoverflow 관련 답글

[Java] BigDecimal VS BigInteger - 1

내용

돈과 관련된 것은 BigDecimal을 사용하는 것이 좋다.
둘다 문자열로 표현된다. 하지만, BigDecimal은 소수점 자리도 표현되지만 BigInteger는 소수점 표현이 되지 않는다.

[Java] Integer.compare() - 1

내용

compareTo 같은 경우 직접 연산을 해도 되긴 하지만 조금 위험할 수 있다.
기본 타입의 경우는 Wrapper 클래스에서 비교 메서드를 제공한다. Integer.compare(score, o.score)

1. Dto 분리 2. 결과를 담는 객체 생성

wbluke

2단계 구현하시느라 고생 많으셨어요 👏
피드백 남겨보았는데요, 일급컬렉션의 역할을 중점적으로 고민해보면 좋겠어요!
수정 후 재요청 부탁드려요 🙂

src/main/java/blackjack/domain/gametable/ScoreBoard.java

src/main/java/blackjack/domain/gametable/PlayerResult.java

docs/step1_학습로그.md

src/main/java/blackjack/Application.java

src/main/java/blackjack/domain/gamer/Name.java

src/main/java/blackjack/dto/ProcessDto.java

src/main/java/blackjack/domain/gametable/ScoreBoard.java

src/main/java/blackjack/domain/Game.java

wbluke · 2021-03-13T15:55:53Z

src/main/java/blackjack/view/InputView.java

        System.out.println("게임에 참여할 사람의 이름을 입력하세요.(쉼표 기준으로 분리)");
-        return sc.nextLine();
+        final String namesValue = sc.nextLine();
+        return namesValue.split(",");


매직 스트링!

스트링과 관련하여 상수화하는 의견이 궁금합니다~! 모든 스트링은 상수화해주는게 좋은가요? 아니면 위와 같이 온전한 문장?! 으로 이루어졌을 경우 상수화해주는 것이 좋을까요? (위와 같다면 모든 예외 메세지도 상수화해주는 것은 좋은 편인가요?)

OutputView에서 출력을 위해서 잘게 쪼개져서 있는 단어?들도 매직 스트링으로 만들어 주는게 좋을까??라는 생각이 들어서 질문 남깁니당!ㅎㅎㅎ

저는 모든 String을 무조건 상수화해야한다고는 생각하지 않아요. 예외 메시지 등은 그냥 놔두는 편이에요.
상수로 관리하는 경우는 애플리케이션, 핵심 로직에서 명확한 역할을 담당하고 있는 문자열과 숫자들일 경우입니다.
예를 들어 예외 메시지가 조금 변경된다고 애플리케이션의 큰 흐름이 바뀌거나 하지는 않지만,
위와 같이 외부에서 들어온 input의 구분자 라는 역할은 요구사항이 바뀌어서 구분자가 ;, | 등으로 변경되거나 했을 경우에 해당 문자열을 변경해야 하는 관리 포인트를 가지기 때문에, 유지보수 측면에서 상수로 관리해야한다고 생각해요.

같은 이유로 OutputView에서 굳이 모든 문자열을 상수로 가지고 있지 않아도 된다고 생각하지만, , 같은 joining을 위한 구분자는 상수로 관리해야 한다고 생각해요.

추가적으로 OutputView에서 전체적으로 String.format을 이용해 print를 해보면 좋겠어요 :)

관리 포인트로 들어가는 것들을 상수화하는게 좋군요!! ㅎㅎㅎ 문자열을 +로 println으로 하는 것보다 format 방식이 선호되는 것은 가독성 때문일까요?

수정하였습니다~!

knae11 · 2021-03-14T23:20:28Z

학습로그

[Java] Cache - 3

내용

List와 같이 자료구조를 담는 클래스는 캐싱하여 static으로 관리하여 사용할 시, 문제가 될 수 있다.
public static final Cards EMPTY_CARDS = new Cards(Collections.emptyList())를 사용하려고 했더니 테스트케이스에서
통과되지 않는 경우가 발상하였다.
불변객체가 아니라면 캐싱하지 않는다.

knae11 · 2021-03-14T23:27:46Z

안녕하세요:) 닉! 즐거운 주말 보내셨나요?ㅎㅎㅎ 피드백 주신 내용에 맞춰서 리팩토링 해보았어요!
Players에 집중해보니 역할이 훨씬 명확해진 것 같아요! 😁

리팩토링 하면서 몇가지 질문사항이 있어서 해당 내용은 resolve conversation하지 않고 남겨두었습니다 :)

지난 피드백 내용을 반영한다고 했는데 여기저기 고치다 보니 놓친 부분들도 많았네요! 같은 부분 다시 피드백 보내게 해서 죄송해요 🙏 꼼꼼한 피드백과 더불어 여러 질문사항에 친절하게 답변해주셔서 감사해요❤

knae11 · 2021-03-15T06:55:59Z

[Java] interface, abstract class - 4

내용

interface는 field를 가질 수 없다.
abstract class는 field를 가질 수 있지만, 생성자로 초기화해줘야 예외상황이 발생하지 않는다.
이때, 생성자에서 초기화해주는 항목이 만족하지 않는다면 불온전한 계층관계일 가능성이 높다. 명확한 계층관계가 아닐 때는 상속을 사용하면 안 된다.

wbluke

상태패턴까지 잘 적용하셨네요 👏
주신 질문들에 대해서는 답변 남겼고요,
조금 더 수정해보면 좋을 분들이 있어서 코멘트 남겼으니 확인 부탁드려요 🙂

src/main/java/blackjack/domain/Game.java

wbluke · 2021-03-15T14:10:56Z

src/main/java/blackjack/domain/Game.java

+    public Player turnForPlayer(final Player player) {
+        if (player.isAbleToTake()) {
+            player.takeCard(cardDeck.pop());
+        }
        return player;
    }

    public void turnForDealer() {
-        gameTable.giveCard(dealer);
+        if (dealer.isAbleToTake()) {
+            dealer.takeCard(cardDeck.pop());
+        }
    }


하나의 로직으로 합칠 수 있을까요? :)

이 부분은 생각해봤는데ㅠㅠ View 로직이 달라서 온전히 합치기 어려울 것 같다는 생각이 들었어요. 메소드의 리턴해주는 값이 달라서 합치기 어렵더라구요. Player들에게는 한번씩 물어보고 대답을 듣고 받을 때마다 그에 따른 카드 내역을 보여주어야 합니다. 하지만 Dealer의 카드받기는 1회적이고, 내부적으로 16 이하인 경우에만 1장을 더 받고 단순히 받았습니다 메세지만을 던져주고 있어서요.

if (dealer.isAbleToTake()) { dealer.takeCard(cardDeck.pop()); }

다만 이 부분 로직이 겹쳐서 해당 부분은 메소드로 분리했어요! 혹시 이걸 의도하신 걸까요? :) 더 좋게 합칠 수 있는 방법이 있다면 힌트 부탁드려요🙏 View 부분이랑 겹치는 부분 뭔가 가능하면 더 좋게 만들고 싶은데ㅠㅠ 현재 저에게는 이 방법 말고는 방안이 보이지 않아요. 혹시 View와 묻고 응답하는 부분을 더 내부적으로 처리할 수 있는 방안이 있을까요??

src/main/java/blackjack/domain/Game.java

wbluke · 2021-03-15T14:12:27Z

src/main/java/blackjack/domain/card/Cards.java

+            .map(Card::getCardName)
+            .collect(Collectors.toList());
+
+        return Collections.unmodifiableList(cards);


이미 Card 리스트를 가공한 형태이기 때문에 불변 리스트일 필요는 없어보여요 :)

무의식중에 불변을 리턴한 것 같아요!ㅎㅎㅎ 그런데 가공한 데이터여도 해당 리스트에 새로운 이름을 추가하지 못하게 unmodifiableList로 리턴하는건 어떻게 생각하세요? 지나친 생각일까요..?ㅠ

아뇨 불변으로 리턴하지 않는 것이 좋습니다.

일급컬렉션(Cards)에서 필드(List<Card>)를 리턴할 때 불변 리스트로 반환하는 이유는 무엇일까요?
외부에서 Cards의 내부 필드인 List<Card>를 조작할 수 없게 하기 위함입니다. 방어 로직인 것이죠.

반대로 클라이언트 입장에서 생각해볼까요?
List<Card>는 Cards 일급컬렉션의 핵심 필드니 조작할 수 없도록 반환되어도 납득을 할 수 있지만,
이미 가공된 List<String> 등이 불변이라면, 클라이언트는 이를 사전에 인지하기도 힘들 뿐더러 가공된 데이터를 받아도 자유로운 사용을 위해서는 추가 가공을 거쳐야하는 불편함이 생기게 됩니다.
일급컬렉션은 그 자체로 확장성이 결여된, 제한된 기능만을 제공하는 객체가 되어버리죠.

이미 한번 가공을 거친 반환값은 원재료인 필드와 무관한 값이 되었기 때문에, 불변이라는 방어로직을 가질 필요가 없습니다 :)

src/main/java/blackjack/domain/card/Score.java

knae11 · 2021-03-16T00:38:45Z

안녕하세요 :) 닉! 해당 코멘트 준 부분들 다시 리팩토링 해보았어요! 세세한 부분까지 코멘트 주셔서 하면서 도움이 많이 되네요! 간단하고 분명한데 신경쓰지 못한 부분들도 많아서 반성도 하고 있습니다!😂 좀 더 세세하게 신경쓰는 습관을 들여야 되는데 아직은 놓치는 부분이 많은가봐요ㅋㅋㅋ 같은 내용 피드백하시느라 좀 피곤하실 텐데 덕분에 반성하게 되고 좋은 습관 들일 수 있는 것 같아요:) 감사합니다!ㅎㅎㅎ

이번에도 몇가지 질문들이 있네요ㅎㅎㅎ 해당 부분은 resolve conversation 하지 않고 남겼습니다 :)

오늘도 좋은하루 보내세요😀

wbluke

마지막까지 피드백 반영 감사합니다 ㅎㅎ
질문 주신 부분 중에 일급컬렉션에 관련한 내용 답변 남겼어요!
이번 미션도 고생 많으셨고 다음 미션도 화이팅입니다 🙂

knae11 added 20 commits March 12, 2021 12:13

docs: step1 피드백 및 학습로그 정리

b14d10c

docs: 학습로그 추가

b8b0712

docs: README 정리

caf428f

feat: 배팅 금액 객체 구현

d7c5e75

feat: 결과에 따라 최종수익 계산하여 보여주는 구현

9a8ec74

1. Dto 분리 2. 결과를 담는 객체 생성

refactor: ScoreBoard 메소드 이름 수정

c3d5f48

refactor: 테스트코드 Name 객체 비교 equals, hashcode 오버라이드

3460ade

refactor: Outcome 테스트 이동 및 계산로직 Outcome으로 다시 이동

7be06e8

test: PlayerResultTest 테스트코드 작성

02afe1b

docs: step2_README 중간점검

05410a0

style: reformat codes

5695d4a

refactor: 정규표현식 Util로 이전

f3f78e4

refactor: 딜러이름 상수화

aa7e9e9

refactor: Application에서 메소드 분리

13f3475

refactor: BettingMoney.ZERO, Cards.EMPTY_CARDS 상수화

5dc5b66

refactor: 가독성을 위해 체이닝 값 분리선언

360c8bc

refactor: compareTo에서 기본타입 Wrapper로 비교진행

2ab65ab

feat: 딜러의 수익결과 출력

04d2730

refactor: 승패로직 메소드 분리

43c83c8

docs: 학습로그

7dd475c

wbluke suggested changes Mar 13, 2021

View reviewed changes

knae11 added 9 commits March 15, 2021 06:25

refactor: 수정사항 반영

6299406

docs: 캐싱관련 학습로그 작성

addb9b4

refactor: player 생성자 수정 및 추가

9e33841

refactor: 매직스트링 및 예외 메세지 추가

b2a1ac5

docs: 캐싱관련 학습로그 추가

f6d77eb

refactor: 이름들 로직 players에게 이동

4e21af9

refactor: Players에게 역할과 책임 분배

b9a4326

refactor: BettingMoney 객체생성 Player 내부로 이동

44aa2da

refactor: ResultDto에서 dealerScore 인자 제거

f84a2fb

docs: 오타수정

bf52489

knae11 added 5 commits March 15, 2021 14:56

refactor: 상태패턴 적용

5dcc521

refactor: Stay 상태 삭제

ebc0001

refactor: GameTable 삭제, Game으로 편입

c5f895c

refactor: State 중복코드 Finished, Running으로 한 단계 추상화

b560deb

docs: 학습로그 추가

9de0163

wbluke suggested changes Mar 15, 2021

View reviewed changes

knae11 added 7 commits March 16, 2021 08:03

refactor: Game을 BlackjackGame으로 변경

45263e8

refactor: Score 기존메소드에 의존하게 변경

3c6051d

refactor: OutoutView printf 방식으로 변경

ff9776d

refactor: InputView 이름과 베팅금액 받는 로직 Map으로 변경

4ace4cc

refactor: Player이름과 Cards를 담는 DTO 사용

1121256

refactor: OutputView 매직스트링 사용

59596e3

refactor: 겹치는 로직 메소드 추출

8018d74

refactor: Application, BlackjackGame 중복 메소드 수정

d83b41f

wbluke approved these changes Mar 16, 2021

View reviewed changes

wbluke merged commit 39bdcf2 into woowacourse:knae11 Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 블랙잭 구현] 배럴(김나은) 미션 제출합니다. #209

[2단계 - 블랙잭 구현] 배럴(김나은) 미션 제출합니다. #209

knae11 commented Mar 13, 2021 •

edited

Loading

wbluke left a comment

wbluke Mar 13, 2021

knae11 Mar 14, 2021

knae11 Mar 14, 2021 •

edited

Loading

wbluke Mar 15, 2021

knae11 Mar 15, 2021

knae11 Mar 15, 2021

knae11 commented Mar 14, 2021

knae11 commented Mar 14, 2021

knae11 commented Mar 15, 2021 •

edited

Loading

wbluke left a comment

wbluke Mar 15, 2021

knae11 Mar 16, 2021 •

edited

Loading

wbluke Mar 15, 2021

knae11 Mar 15, 2021

wbluke Mar 16, 2021

knae11 commented Mar 16, 2021 •

edited

Loading

wbluke left a comment

[2단계 - 블랙잭 구현] 배럴(김나은) 미션 제출합니다. #209

[2단계 - 블랙잭 구현] 배럴(김나은) 미션 제출합니다. #209

Conversation

knae11 commented Mar 13, 2021 • edited Loading

느낀점 및 질문

학습로그

[Java] Stream - 2

내용

[OOP] 가독성 - 3

내용

[OOP] abstract class - 3

내용

링크

[Java] BigDecimal VS BigInteger - 1

내용

[Java] Integer.compare() - 1

내용

wbluke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knae11 Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knae11 commented Mar 14, 2021

학습로그

[Java] Cache - 3

내용

knae11 commented Mar 14, 2021

knae11 commented Mar 15, 2021 • edited Loading

[Java] interface, abstract class - 4

내용

wbluke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knae11 Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knae11 commented Mar 16, 2021 • edited Loading

wbluke left a comment

Choose a reason for hiding this comment

knae11 commented Mar 13, 2021 •

edited

Loading

knae11 Mar 14, 2021 •

edited

Loading

knae11 commented Mar 15, 2021 •

edited

Loading

knae11 Mar 16, 2021 •

edited

Loading

knae11 commented Mar 16, 2021 •

edited

Loading