[1단계 - 블랙잭] 이프(송인봉) 미션 제출합니다.#265
Conversation
JunHoPark93
left a comment
JunHoPark93
left a comment
There was a problem hiding this comment.
안녕하세요 이프! 리뷰어 제이입니다.
블랙잭 1단계 구현 잘해주셨네요 👍 수정하실 부분은 거의 없습니다.
질문 주신부분 포함해서 몇 가지 코멘트 남겼어요.
확인해주시고 코멘트 다시 남겨주시면 머지하도록 할게요~
| public void drawCards(final Deck deck, final CardDrawCallback callback) { | ||
| while (isPossibleToDrawCard() && callback.isContinuable(getParticipantName())) { | ||
| drawCard(deck); | ||
| callback.onUpdate(name, cards.getCardNames()); |
There was a problem hiding this comment.
[해결 시도]
그래서 고민 끝에 도입한 방법이 콜백함수입니다.
제대로 알고 있는 것은 아니지만 어렴풋이 보았던 [블로킹 Vs. 논블로킹, 동기 Vs. 비동기] 개념이 떠올랐고
제어권을 넘긴다는 점으로부터 BlackjackApplication에서 Players로 입출력 로직를 넘기게끔 코드를 작성했습니다.[질문]
괜찮은 방식인지, 그렇지 않다면 어떠한 문제가 있는지 지적해주시면 감사하겠습니다!!
많은 고민을 해주셨네요 👍
구조만 봤을때는 크게 문제가 없어보이지만, 이프가 해결하려고하는 문제 "view 로직과 도메인 영역 분리" 에 있어서는 백프로 분리가 되었다라고 말할순 없을 것 같아요.
겉보기에는 콜백형태로 분리가 된것처럼 보이지만 실제로 뷰에서 보여줘야할 값들이 바뀌게되면(추가 혹은 제거) 인터페이스 스펙이 계속 해서 바뀌게 될거에요. 인터페이스 스펙이 바뀌면 도메인 코드도 같이 수정을 해주어야 합니다. 보이지 않는 의존성, 결합도가 생기는 것이죠.
결론은 잘못된 것은 전혀아니고(둘 다 만족시킬수 없기 때문에) 말씀주신 1번 BlackjackApplication for문을 통해서 턴을 진행하는 것 vs 2번 콜백으로 분리하는것 에서 2번을 선택하신 이유를 말씀주셨기 때문에 저는 지금 구조도 괜찮다고 생각해요.
There was a problem hiding this comment.
좋은 말씀 감사합니다!! 해당 부분은 계속 고민해보면서 보다 나은 방법을 찾아보겠습니다!!!
| protected Participant(final String name, final Deck deck) { | ||
| validateNameNotBlank(name); | ||
| this.name = name; | ||
| drawTwoCard(deck); |
There was a problem hiding this comment.
두장을 draw한다보다는 초기상태를 세팅하는 의미가 드러나는 메서드명을 사용하면 좋을 것 같아요.
이 부분은 제가 다른분들도 공통적으로 피드백 드리는 내용인데요, 블랙잭이라는 정책을 처음보는 개발자들도 이해하기 쉽도록 하기 위해서 블랙잭 룰에 의해서 전개되는 코드는 최대한 정책이 드러나도록 네이밍하는것을 말씀드리고 있어요.
개인적인 경험으로는 처음보는 프로젝트를 중간에 들어갔을때 메서드명이 잘 지어져있다면 정책을 금방 숙지하고 아니라면 파악에 정말 오래걸렸어요. 하여 하나의 메서드명을 짓더라도 고민을 통하게된다면 추후 팀의 전체적인 생산성이 높아질거에요.
There was a problem hiding this comment.
인지하고 있던 부분이었는데 미처 확인하지 못하고 넘어갔네요ㅜ
drawTwoCard라는 메서드명이 내부로직에 의존되어 있다고 느끼고 있었습니다.
만약 정책이 변경되어 drawCard(deck);을 한번만 실행하거나, 서너번을 실행하게되면
그에 맞춰서 drawTwoCard라는 명칭이 변경되어야 하니까요.
해당 부분은 initiallyDrawCards라는 메서드명으로 변경하여 의미를 추상화했습니다!
| private void drawTwoCard(final Deck deck) { | ||
| drawCard(deck); | ||
| drawCard(deck); | ||
| } |
There was a problem hiding this comment.
위 부분과 마찬가지로 처음보는 사람은 drawCard를 두번 부르기 때문에 이게 실수로 두번 들어간건지, 정말 두번 불러야되는지 모릅니다. (메서드명이 drawTwoCard에서 init... 로 바뀐다면)
이럴때 사용하는게 주석인데요, 보통 주석을 사용한다 하면 "주석을 달기보다 코드에 표현하라" 등의 교과서적인 말들을 많이 접하게 될거에요.
저는 이 말에 반은 동의하고 반은 동의하지 않습니다. 코드에 표현하더라도 정책자체를 모른다면 코드를 봐도 알 수 없는게 당연합니다. 코드가 어떻게 돌아가는지에 대한 설명은 필요하지 않지만 왜 이렇게 짰는지에 대한 설명이 필요하다면 주석에 남겨두는 편입니다.
// 카드를 두장 뽑는다 <- 최악
// 블랙잭 규칙상 최초 세팅시 ... + 규칙에 대한 official link <- ok
There was a problem hiding this comment.
전체적으로 코드를 정말 잘 짜주셔서 지엽적인 부분이지만 남깁니다. 의견 남겨주셔도 좋아요~
There was a problem hiding this comment.
맞습니다ㅜ 메서드명을 수정한다고 해서, 그 의미가 내부 로직을 온전히 뒷받쳐주지는 못합니다.
말씀하신대로 메서드 중복 호출 아닌가? 하는 의문이 생길테고, 정책을 들여다본 후에야 이유를 파악하겠죠.
하지만 읽기 좋은 코드라면! 그러한 의문을 갖지 않고 술술~ 코드를 읽어 가겠죠.
해당 부분의 내부로직을 수정했습니다! drawCard 메서드의 호출 횟수를 initiallyDrawCardCount 변수에 담고
for 반복문을 통해 drawCard 메서드를 반복 호출했습니다.
이로써 오해의 여지를 없애고, 보다 명확하게 의미를 표현할 수 있었습니다!
저 또한 주석을 달지 말라는 것에 100% 동의하지 않습니다! 어쩔 수 없는 상황에서는 불가피하게 주석을 달아야죠.ㅜ
하지만 아직 프로그래밍에 익숙하지 않은 현 시점에서부터 코드에 표현하기보다 주석 달기에 의존하는 것은
좋지 못한 선택이라고 생각해요. 나의 의도를 코드로 표현하는 것이 언제쯤 익숙해질지 모르겠지만,
개발자라면 앞으로 꾸준히 지녀야할 자세라고 생각하고 있고, 무엇보다 지금 우아한테크코스 기간 동안
주석 없이 코드만으로 나의 생각을 표현하는 스킬을 피드백받아 성장해나가고 싶습니다ㅎ
정말 좋은 말씀 감사합니다!!
| public boolean isBurst() { | ||
| return calculateScore() > BLACKJACK; | ||
| } |
There was a problem hiding this comment.
Cards에 점수계산 역할을 부여해주셨어요.
클래스이름을 일반적인 Cards라는 네이밍보다는 BlackjackCards가 더 낫지 않을까요? Cards라는 클래스안에 블랙잭정책인 숫자들이 들어있으니까요.
There was a problem hiding this comment.
Deck과 Cards의 이름에서 오는 느낌이 불명확해서 마음에 걸리던 부분이었는데요.
말씀하신대로 BlackjackCards도 괜찮다고 생각하지만, 이미 최상위 패키지명이 blackjack인 관계로
동일한 이름을 클래스명에 추가 작성하는 것은 의미 중복이 되지 않을까 싶어요.
잠시 고민해봤는데, Cards는 각 참가자의 손에 쥐어진 카드들의 의미를 지니고 있으니
CardHands라는 명칭은 어떤가요? 번역기를 돌려보니 Hand라는 용어를 사용하네요.
| import org.junit.jupiter.params.provider.Arguments; | ||
| import org.junit.jupiter.params.provider.MethodSource; | ||
|
|
||
| class CardsTest { |
| public MatchResult judgeWinners(Dealer dealer) { | ||
| final Map<String, MatchStatus> matchStatuses = new HashMap<>(); | ||
| for (final Player player : players) { | ||
| final String playerName = player.getParticipantName(); | ||
| final MatchStatus matchStatus = dealer.judgeWinner(player); | ||
| matchStatuses.put(playerName, matchStatus); | ||
| } | ||
| return new MatchResult(matchStatuses); | ||
| } |
There was a problem hiding this comment.
이해가 잘 안됩니다ㅜ 해당 부분에서 어떤 부분을 분리해야 하나요? 힌트를 주시면 감사하겠습니다!!
There was a problem hiding this comment.
for문에서 해주는 작업(map에 담는 행위)을 메서드로 분리하는것에 대한 것이었어요. 확인해보시고 다음단계때 반영해주셔도 좋아요~
JunHoPark93
left a comment
JunHoPark93
left a comment
There was a problem hiding this comment.
안녕하세요 이프!
피드백 반영하시느라 고생많으셨습니다.
남겨주신 코멘트들 잘 보았어요. 전체적으로 잘해주셔서 이만 머지하도록할게요.
다음 단계에서 뵙겠습니다~
3 participants
안녕하세요 이프입니다!!
리팩토링이 아직 마무리되지 않아, 저 스스로도 맘에 들지 않는 부분이 여럿 있습니다.
하지만 계속해서 PR 리뷰 요청을 미룰 수 없기 때문에, 고민에 대한 질문과 함께 리뷰를 요청 드립니다.
다음은 현재 고민하고 있는 부분입니다.
MVC 패턴을 깨지 않기 위해 콜백함수를 적용했으나, 바람직한 방법인지 의문입니다.
[문제 상황]
현재
BlackjackApplication를 보시면 (Player를List로 지니고 있는 )Players를 가지고 있습니다.플레이어에게서
한장의 카드를 더 받겠습니까?응답에 따라 각 플레이어의 턴이 진행되는데BlackjackApplication에서Players의 인스턴스(List<Player>)를 가져와서직접
for문을 통해 각 플레이어의 턴을 진행하는 것은 옳지 않다고 생각했습니다.(외부에서
Players의 상태를 꺼내와 직접 사용하는 모습이니까요)하지만 그렇다고 해서
BlackjackApplication에서 다뤄지는BlackjackView의 입출력 로직을Players내부에 넣을까 생각해보면,View 로직이 도메인 영역을 침범하니까 MVC 패턴이 깨집니다.
[해결 시도]
그래서 고민 끝에 도입한 방법이 콜백함수입니다.
제대로 알고 있는 것은 아니지만 어렴풋이 보았던 [블로킹 Vs. 논블로킹, 동기 Vs. 비동기] 개념이 떠올랐고
제어권을 넘긴다는 점으로부터BlackjackApplication에서Players로입출력 로직를 넘기게끔 코드를 작성했습니다.[질문]
괜찮은 방식인지, 그렇지 않다면 어떠한 문제가 있는지 지적해주시면 감사하겠습니다!!
현재 리팩토링 작업중인 부분
MatchResult 클래스 리팩토링
Cards 클래스 리팩토링(점수 계산 로직 수정)