-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Lev1][미션3] 블랙잭 1단계 - 오잉 #1
base: hanueleee
Are you sure you want to change the base?
[Lev1][미션3] 블랙잭 1단계 - 오잉 #1
Conversation
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.
리뷰어님이 너무 꼼꼼하게 해주셔서 어떻게 해야할지 모르겠네 😢
암튼 고생했어 👍
|
||
--------- | ||
## 고민 | ||
- dto에도 정적 팩토리 메소드를 써야할까? |
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.
내부에 로직이 있거나, 편하게 사용하고 싶다면 사용할 수 있을 것 같아! 👍
나는 보통 dto를 사용할 때 값을 그대로 넣어서 사용하는 편이야
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.
저는 그냥 편하게 쓰려고 DTO.from(domain) 식으로 사용합니다
--------- | ||
## 고민 | ||
- dto에도 정적 팩토리 메소드를 써야할까? | ||
- playerStatusDto(필드2개) & playerResultDto(필드3개) -> 필드2개 겹치는데 두 dto를 따로 만들어야하나? 아니면 필드를 안쓰더라도 하나로 통합?(중복방현) |
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.
중복이라고 생각된다면 생성자를 두 개 만들 수 도 있을 것 같은데?
2개를 받는 생성자를 점수에 대한 부분을 0으로 만드는 방법으로
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 void handOutStartCards() { | ||
cardDeck.shuffle(); |
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.
시작할 때 덱을 셔플 해주네! 좋은데요? 👍
// TODO : string, number 어떤걸 get 해야할까? | ||
public Number getNumber() { | ||
return number; | ||
} |
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.
리뷰어 분이 남겨주는거 확인하면서 리뷰하고 있는데 나도 이 부분은 Number를 반환하는게 맞다고 생각해!
|
||
public Card pick() { | ||
validateCardExist(); | ||
return cards.remove(0); |
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.
0을 상수로 빼줄 수 도 있을 것 같아~ 아니면 다른 구현체를 사용해보는 건 어떨까?
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.
이 부분은 앞에서 빼는 것보다, 뒤에서 빼는 게 성능상 좋을 것 같아요
remove(0) 호출시, 뒷부분을 전부 다 복사해서 한칸씩 땡겨오는데, 뒤에서 빼면 그 작업이 없어서요
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.
혹은 Queue, Deque들을 사용해서 맨 앞에서 빼는 방법도 좋을 것 같아요
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.
deque으로 수정했습니다. 감사합니다~~
|
||
public class NoMoreCardException extends CustomException { | ||
|
||
private static final String MESSAGE = "더 이상 뽑을 카드가 없습니다. 결과 창으로 넘어갑니다."; |
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.
더 많은 의미를 담기위한 custom exception 👍
} | ||
|
||
private void validateName(String name) { | ||
if (name.equals(INVALID_NAME)) { |
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.
equals의 방향을 바꾼다면 NPE를 방지할 수 있을 것 같아~ 지금은 NPE가 발생하지 않겠지만 👍
ex)INVALID_NAME.equals(name)
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.
equals()를 부르는 인스턴스가 null인 경우 NPE가 발생한다.
방지 방법
1. null 여부를 체크한 후 equals 호출
2. 비교할 문자열이 equals 호출
<- 허브가 말한거
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.
감삼당!!
return players.stream() | ||
.filter(player -> player instanceof Dealer) | ||
.findFirst() | ||
.orElse(null); |
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.
딜러가 없다면 null을 반환하고 있네~ null보단 예외를 던져보는건 어떨까?
클린 코드 7장 p.138 null을 반환하지 마라
를 참고할 수 있을 것 같아!
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.
- null을 반환하지 마라
null을 반환할 경우 -> 누구 하나라도 null확인을 빼먹으면 애플리케이션이 통제 불능에 빠질 수도 있다
=> 메소드에서 null을 반환하려는 유혹이 든다면예외
를 던지거나특수 사례 객체
를 반환하라 (또는Collections.emptyList()
) - null을 전달하지 마라
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 static void terminate() { | ||
scanner.close(); | ||
} |
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.
system.in 을 종료해버리면 문제가 있을 수 있어서 안 닫는 방향도 좋을 것 같아요
https://coderanch.com/wiki/678613/Don-close-Scanner-tied-System
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 s = new Scanner(System.in);
...
s.close(); # <- DO NOT DO THIS
system.in을 close 하지마라
이유 : System.in 객체는 JVM에 의해 관리되기 때문에, JVM이 알아서 하도록 내비둬야 한다(사용자가 마음대로 close하면, 후에 System.in을 다시 사용하려 할 때 문제가 생긴다)
@@ -0,0 +1,10 @@ | |||
package blackjack.common.exception; | |||
|
|||
public class CustomException extends RuntimeException { |
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.
CustomException 이라는 네이밍이, 조금만 더 문맥을 담고 있어도 괜찮을 것 같아요
BlackjackException 같은 거라든가...?
왜 이 클래스가 존재하는지에 대한 이유가 네이밍에 드러나면 좋을 것 같아요
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.
이번 미션은 규모가 크지 않아서 굳이 최상단 익셉션명이 문맥을 담을 필요 없이 CustomException로 둬도 된다고 생각했습니다.
규모가 커지면 BlackjackException, 어쩌구Exception, 저쩌구Exception 일케 크게크게 상단 커스텀 익셉션 나누고 그 밑으로 관련된 익셉션들 만들면 되지 않을까용 ><
try { | ||
List<String> playerNames = InputView.inputPlayerNames(); | ||
blackJackGame = BlackJackGame.from(playerNames); | ||
} catch (CustomException e) { | ||
OutputView.printErrorMessage(e); | ||
init(); |
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.
다른 리뷰어 분들도 재귀함수는 지양하라는 내용들을 많이 봤었어서요 재귀 대신 while 로 바꿔보면 어떨까요?
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.
키워드 : call stack overflow
다른 언어는 꼬리재귀라는 키워드를 통해 이러한 문제를 해결할 수 있다고 합니다~
스포하자면 자바는 지원 안한대요!
시간 남으면 찾아보셔도 좋을것같네요ㅋㅋ (자바에서 의미는 없지만)
private void showStartStatus() { | ||
PlayerStatusDto dealerStatus = makeDealerStatus(); | ||
List<PlayerStatusDto> challengersStatus = makeChallengersStatus(); | ||
OutputView.printStartStatus(dealerStatus, challengersStatus); |
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.
그냥 궁금해서 여쭤보는건데요
OutputView.printStartStatus(dealerStatus,challengersStatus) 쪽과
OutputView.printStartDealerStatus(dealerStatus) 쪽중 어떤 부분이 더 나은 것 같으신가요
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.
저는 아래가 더 낫다고 생각합니다.
근데 이건 오잉이가 생각하는게 중요하니까 이유는 말하지 않을게요.
inputChoice(player); | ||
} catch (CustomException e) { | ||
OutputView.printErrorMessage(e); | ||
checkChoice(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.
이 부분도 while 로 바꿔보면 어떠신가요?
|
||
public Card pick() { | ||
validateCardExist(); | ||
return cards.remove(0); |
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.
이 부분은 앞에서 빼는 것보다, 뒤에서 빼는 게 성능상 좋을 것 같아요
remove(0) 호출시, 뒷부분을 전부 다 복사해서 한칸씩 땡겨오는데, 뒤에서 빼면 그 작업이 없어서요
|
||
public abstract class Player { | ||
|
||
protected final HoldingCards holdingCards; |
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.
부모의 변수에 protected 형태로 접근하는 것은 안 좋을 수도 있을 것 같아요
부모 클래스에 변수(내부 구현)가 조금만 바뀌어도 모든 것에 영향을 받을 테니까요
부모 클래스에서는 public 만을 이용해서 소통하는 방향으로 바꿔보면 어떨까요?
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.
저는 protected를 사용하는 것이 더 적합하다고 생각해요.
누누 말씀대로 부모 클래스에 강한 의존성을 가지게 되지만, 참여자와 딜러는 애당초 Player에 강하게 결합되는 것이 오히려 자연스럽다고 생각하거든요.
return holdingCards.getSum(); | ||
} | ||
|
||
public abstract Boolean canPick(); |
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.
boolean 타입이 아닌 이유가 혹시 있으신가요?
} | ||
|
||
@Override | ||
public Boolean canPick() { |
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.
여기도 boolean 쪽이 좋을 것 같아요
return List.copyOf(cards); | ||
} | ||
|
||
public List<Integer> getSums() { |
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.
아하 가능한 모든 경우를 다 List 형태로 만들어 주는 방식이네요 신기한 방식이네요 👍
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.
안녕하세요 오잉~ 리뷰어 박스터 입니다~
코드가 아주 깔끔하네요!
도메인에 패키지를 나눴던데 어떤 기준으로 왜 나눴는지 알 수 있을까요?
|
||
--------- | ||
## 고민 | ||
- dto에도 정적 팩토리 메소드를 써야할까? |
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 final String ERROR_PREFIX = "[ERROR] "; | ||
|
||
public CustomException(String message) { | ||
super(ERROR_PREFIX + message); |
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.
이 부분을 OutputView
에서 처리하는 방법도 있을텐데, 여기서 처리한 이유가 뭐죠?
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.
사실 custom exception을 한번도 써본적이 없어서 연습삼아 이번 미션에서 처음으로 적용해본건데,
뭐라도 엮어줄게 없을까 하다가 에러메시지로 묶어봤습니다 ><
public void run() { | ||
init(); | ||
start(); | ||
takeTurn(); | ||
showResult(); | ||
InputView.terminate(); | ||
} |
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.
깔끔해서 보기 좋아요~
List<Player> players = blackJackGame.getChallengers(); | ||
List<PlayerStatusDto> gameStatus = new ArrayList<>(); | ||
for (Player player : players) { | ||
PlayerStatusDto playerStatusDto = new PlayerStatusDto(player); | ||
gameStatus.add(playerStatusDto); | ||
} |
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.
Stream을 사용하면 깔끔하게 처리될 수 있을 것 같아요
boolean dealerCanPick = dealer.canPick(); | ||
if (dealerCanPick) { |
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.
굳이 변수로 할당한 이유가 있을까요?
this.holdingCards = new HoldingCards(); | ||
} | ||
|
||
public void pickStartCards(Card card1, Card card2) { |
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.
변수명이 card1, card2 안좋은데요? 아니면 파라미터를 List<Card>
는 어떠신가요?
public List<Player> getChallengers() { | ||
return players.stream() | ||
.filter(player -> player instanceof Challenger) | ||
.collect(Collectors.toUnmodifiableList()); | ||
} | ||
|
||
public Player getDealer() { |
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.
메소드 이름처럼 instanceOf로 찾은 Challenger와 Dealer를 형변환해서 반환해도 괜찮지 않을까요?
if (lowerCase.equals(YES)) { | ||
return true; | ||
} | ||
if (lowerCase.equals(NO)) { | ||
return false; | ||
} | ||
throw new InvalidChoiceException(); |
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.
Enum을 활용해보는 것은 어떤가요?
System.out.print(challenger.getName() + CARD); | ||
System.out.print(PLAYER_NAME_AND_CARDS_PARTITION); |
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.
System.out.printf()
를 사용하는 것은 어떨까요?
assertThat(spadeAce.getShape()).isEqualTo(Shape.SPADE); | ||
assertThat(spadeAce.getNumber()).isEqualTo(Number.ACE); | ||
|
||
assertThat(spadeTwo.getShape()).isEqualTo(Shape.SPADE); | ||
assertThat(spadeTwo.getNumber()).isEqualTo(Number.TWO); |
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.
assertAll()
을 사용해보시는 건 어떨까요? https://www.baeldung.com/junit5-assertall-vs-multiple-assertions
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.
좋은 리뷰가 많이 달려있어서 많이 못남겼네😂
.filter(sum -> sum < MAXIMUM_SUM) | ||
.mapToInt(sum -> sum) | ||
.max() | ||
.orElse(getMinimumSum()); |
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.
orElse의 경우 null이 아닌 상황에서도 내부 로직이 수행된다고해요~
orElseGet을 사용해보는건 어떨까요?
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.
orElse
vs orElseGet
공통점 : Optional 클래스 객체가 가지고 있는 실제 값이 null일 경우 무슨 값으로 대체해서 return 해줘야 하는지 정의
차이점 : 파라미터
orElse
메소드 : 해당 값이 null이거나 null이 아니어도 실행된다
orElseGet
메소드 : 해당 값이 null일때만 실행된다
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.
새로운 정보 배워가네요 감사합니다!!
p.s. 근데 블로그 글이 매우 어렵네여 조만간 공부해서 질문하러 갈게요 ㅎㅎㅎ
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("provideCards") |
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.
MethodSource 좋은데요?
private static Stream<Arguments> provideCards() { | ||
return Stream.of( | ||
Arguments.of( | ||
List.of( |
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.
List.of 부분은 변수로 한번 추출해줘도 좋을것 같은데 오잉은 어떻게 생각해?
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.
안그래도 List.of 부분 너무 지저분해서 고민하고 있었는데 굿굿
주노 의견은 저 provideCards
의 변수로 뺴라는??
근데 변수로 빼고 보니깐 중복되는 카드들 많은 것 같아서 상수로 빼봤어
근데 그랬더니 상수가 너무 많아지는 것 같네.. 어떻게 생각해?
class DealerTest {
private static final Card SPADE_SIX = new Card(Shape.SPADE, Symbol.SIX);
private static final Card HEART_SEVEN = new Card(Shape.HEART, Symbol.SEVEN);
private static final Card CLOVER_KING = new Card(Shape.CLOVER, Symbol.KING);
private static final Card DIAMOND_JACK = new Card(Shape.DIAMOND, Symbol.JACK);
private static final Card CLOVER_ACE = new Card(Shape.CLOVER, Symbol.ACE);
private static Stream<Arguments> challengerCards() {
return Stream.of(
Arguments.of(List.of(CLOVER_KING, SPADE_SIX), Result.LOSE),
Arguments.of(List.of(CLOVER_ACE, CLOVER_KING), Result.BLACKJACK),
Arguments.of(List.of(DIAMOND_JACK, CLOVER_KING), Result.DRAW));
}
private static Stream<Arguments> provideCards() {
return Stream.of(
Arguments.of(List.of(DIAMOND_JACK, SPADE_SIX), true),
Arguments.of(List.of(DIAMOND_JACK, HEART_SEVEN), false));
}
이런식
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.
good
|
||
--------- | ||
## 고민 | ||
- dto에도 정적 팩토리 메소드를 써야할까? |
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.
저는 그냥 편하게 쓰려고 DTO.from(domain) 식으로 사용합니다
--------- | ||
## 고민 | ||
- dto에도 정적 팩토리 메소드를 써야할까? | ||
- playerStatusDto(필드2개) & playerResultDto(필드3개) -> 필드2개 겹치는데 두 dto를 따로 만들어야하나? 아니면 필드를 안쓰더라도 하나로 통합?(중복방현) |
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.
나같은 경우에는 그냥 따로 만들어주는 편, 각자의 변경주기가 다르다고 생각해서?
@@ -0,0 +1,10 @@ | |||
package blackjack.common.exception; | |||
|
|||
public class CustomException extends RuntimeException { |
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 void showStartStatus() { | ||
PlayerStatusDto dealerStatus = makeDealerStatus(); | ||
List<PlayerStatusDto> challengersStatus = makeChallengersStatus(); | ||
OutputView.printStartStatus(dealerStatus, challengersStatus); |
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 PlayerStatusDto makePlayerStatusDto(Player player) { | ||
String playerName = player.getName(); | ||
List<Card> inputCards = player.getHoldingCards().getCards(); | ||
int playerPoint = player.getTotalPoint(); | ||
return new PlayerStatusDto(playerName, makeCardInfo(inputCards), playerPoint); | ||
} | ||
|
||
private static List<String> makeCardInfo(List<Card> inputCards) { | ||
List<String> cardInfo = new ArrayList<>(); | ||
for (Card card : inputCards) { | ||
cardInfo.add(card.getSymbol().getName() + card.getShape().getName()); | ||
} | ||
return cardInfo; | ||
} | ||
|
||
private ChallengerResultDto makeChallengerResultDto(ResultMap resultMap, List<Player> challengers) { | ||
Map<String, String> nameAndResult = new LinkedHashMap<>(); | ||
for (Player challenger : challengers) { | ||
ResultType challengerResultType = resultMap.getChallengerResult(challenger); | ||
nameAndResult.put(challenger.getName(), challengerResultType.getLabel()); | ||
} | ||
return new ChallengerResultDto(nameAndResult); | ||
} | ||
|
||
private DealerResultDto makeDealerResultDto(ResultMap resultMap, Player dealer) { | ||
String dealerName = dealer.getName(); | ||
Map<ResultType, Integer> dealerResult = resultMap.getDealerResult(); | ||
int winCount = dealerResult.getOrDefault(ResultType.WIN, 0); | ||
int drawCount = dealerResult.getOrDefault(ResultType.DRAW, 0); | ||
int loseCount = dealerResult.getOrDefault(ResultType.LOSE, 0); | ||
return new DealerResultDto(dealerName, winCount, drawCount, loseCount); |
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.
DTO 를 만드는 로직이 컨트롤러에 너무 과하게 들어가있는 것 같아요.
저라면 이런 경우에 DTO에 정적 팩터리를 만들 것 같아요
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.
저도 원래 dto 생성자에 domain을 넘겨주고 dto내부에서 알아서 만들어주는 방식으로 구현했었는데,
리뷰어님께서 (dto 생성 로직은 상당수 controller가 가져가고), dto는 가능한 필드, 심플한 생성자, getter만 가진다
는 것이 이상적이라고 말씀해주셔서, 저도 controller로 생성 로직을 이동했습니다.
하지만 말랑 말처럼 DTO 를 만드는 로직이 컨트롤러에 너무 과하게 들어가있다
+controller가 너무 비대해진다
는 느낌이 저도 들어서 다시 질문을 남겼었는데, woowacourse#415 (comment) 이런 대답을 주셨습니다.
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.
해당 부분은 저도 여전히 고민중 ,,,
dto 생성자에 domain 을 넘겨주고 dto 내부에서 알아서 만든다
dto는 가능한 필드, 심플한 생성자, getter만 가진다
} | ||
|
||
public boolean isBust() { | ||
if (holdingCards.getSum() > BLACKJACK_POINT) { |
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.
마찬가지로 holdingCards에 역할을 위임해보면 어떨까요
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.
안그래도 해당 책임을 player가 가져야 할지 holdingCards가 가져야 할지 고민 했었는데,
카드 뭉치가 bust를 판단한다
보다는 참가자가 bust를 판단한다
가 더 적절하다고 생각했습니다ㅎㅎ
public static Players from(List<String> names) { | ||
validateDuplicatedNames(names); | ||
List<Player> players = new ArrayList<>(); | ||
players.add(new Dealer()); |
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.
Player.from으로부터 Dealer가 추가된다는 것을 유추할 수 있을까요?
Player.createWithDealer 와 같이 네이밍을 변경해보는 것은 어떨까요?
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.
player를 상속받은 dealer, challenger가 존재하기 때문에
Players.createWithDealer
로 변경하게 되면 dealer가 player에 속하지 않는 별개의 무언가로 인식되지 않을까요..??
근데 말씀하신 것처럼 Players.from
만으로는 입력받은 names와 상관없이 dealer가 무조건 추가된다는 것을 유추할 수 없을 것 같네요.. 고민해보겠습니다ㅠ
|
||
import java.util.Map; | ||
|
||
public class ChallengerResultDto { |
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.
필요하지 않다고 생각하시는 이유가 있나요?
@@ -0,0 +1,32 @@ | |||
package blackjack.dto; | |||
|
|||
public class DealerResultDto { |
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.
2단계에서는 ChallengerResultDto
만으로 해결이 가능해서 DealerResultDto
는 삭제했습니다!
|
||
import java.util.List; | ||
|
||
public class PlayerStatusDto { |
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.
필요하지 않다고 생각하시는 이유가 있나요?
No description provided.