[2단계 - 블랙잭(베팅)] 기론(김규철) 미션 제출합니다.#328
Conversation
jnsorn
left a comment
jnsorn
left a comment
There was a problem hiding this comment.
안녕하세요. 기론! 2단계 잘 구현해주셨어요. 👍🏼
리뷰 반영해주신 부분과 새 요구사항에 의해 변경된 부분 모두 확인했습니다.
우선 몇가지 코멘트 남겼는데, 확인해보시고 다시 요청주세요! 😃
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class Betting { |
There was a problem hiding this comment.
betting이라는 단어가 조금 어색하게 느껴지는데, 기론은 어떻게 생각하시나요?
There was a problem hiding this comment.
배팅 금액이어서 Betting으로 했었는데 금액을 명확히 표시하도록 Bet 또는 Money와 같이 하는게 좋을까요?
일단 Bet으로 진행해보겠습니다!
VO객체 특징은 불변이라고 생각합니다! 한 번 생성되면 상태가 변하지 않도록이요!
There was a problem hiding this comment.
Betting, Bet이라는 단어는 배팅금액보다는 배팅 그 자체에 대한 이름 같아요. 배팅 룰이나, 참여자 등이 담길 수 있는 느낌이에요. 조금 더 명확하게 money, amount 등의 의미를 넣어보면 좋을 것 같아요. (혹시 betting, bet에 금액과 관련된 의미가 있다면 이대로 네이밍해도 좋구요!)
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class BlackjackController { |
There was a problem hiding this comment.
아 저는 run()매서드 내에서 순서만 생각했었네요..😂😂
전체적인 컨트롤러를 보니 순서가 뒤죽박죽이었네요!
바로 수정했습니다!!
| public boolean isSameScoreWithNotBlackJack(Cards cards) { | ||
| if (this.isBlackJack() && cards.isBlackJack()) { | ||
| return false; | ||
| } | ||
| return !isBust() && this.getScore() == cards.getScore(); | ||
| } |
There was a problem hiding this comment.
Cards의 역할이 맞을까요? 기론이 생각하는 Cards의 역할은 무엇인지 궁금해요!
There was a problem hiding this comment.
Cards는 유저들이 가지고 있는 카드들을 관리하는 역할로 만들었었습니다. 카드들의 합이 버스트인지 아닌지와 같이요.
isSameScoreWithNotBlackJack()메서드도 원래 카드들 점수를 비교한다고 생각되어서 Cards 클래스 내에서 처리했었는데
지금은 User끼리 카드들을 비교하여 승패를 결정하므로 우선 User에서 처리하도록 변경했습니다!
| private Map<String, Bet> startBettings(List<String> inputPlayerNames) { | ||
| return inputPlayerNames.stream() | ||
| .collect(Collectors.toMap(inputPlayerName -> inputPlayerName, | ||
| inputPlayerName -> Bet.from(InputView.askBetAmount(inputPlayerName)) | ||
| , (a, b) -> b, | ||
| LinkedHashMap::new)); | ||
| } |
| public Card getOneCard() { | ||
| return cards.getCards().iterator().next(); | ||
| } |
- Bet -> Money
jnsorn
left a comment
jnsorn
left a comment
There was a problem hiding this comment.
기론! 리뷰 반영한 부분 확인했습니다. 😃
이번에는 batchService에 대해 고민해보면 좋을 것 같아요. 관련 코멘트 남겨두었으니 확인해주세요.
궁금한 점은 언제든 DM 주시구요!
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class BlackjackController { |
| finishGame(dealer, players, userScoreDtos); | ||
| } | ||
|
|
||
| private List<PlayerDto> converToPlayerDtos(Players players) { |
There was a problem hiding this comment.
| private List<PlayerDto> converToPlayerDtos(Players players) { | |
| private List<PlayerDto> convertToPlayerDtos(Players players) { |
오타 발견 🔍
| } | ||
| } | ||
|
|
||
| public Card(Shape shape, Number number) { |
| public boolean canDealerDraw(){ | ||
| return getScore() <= DEALER_ADD_CARD_LIMIT; | ||
| } |
| import java.util.LinkedHashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class batchService { |
There was a problem hiding this comment.
네 개인적으로 이 클래스를 조금 수정해보면 좋을 것 같아요. 우선 이름을 통해 이 클래스가 어떤 역할을 하는지 파악하기 어렵다는 문제가 있고, calculate 메서드에서 getter가 상당히 많이 사용되고 있어요. getter가 많이 사용되었다는 건 어떤 의미일지 고민해보시면서 getter를 최대한 제거하여 수익률을 구하도록 수정해봅시다! 그 과정에서 클래스가 제거 되어도 좋아요.
converToPlayerDtos -> convertToPlayerDtos로 수정
jnsorn
left a comment
jnsorn
left a comment
There was a problem hiding this comment.
안녕하세요, 기론! 피드백 반영해주신 부분들 확인했습니다.😃
질문주신 부분에 대해 코멘트 남겨두었어요. 이 부분에 대해 고민해보시고 다시 요청주세요!
애매하거나 궁금한 내용은 언제든 DM 주시구요!
| return new ArrayList<>(players); | ||
| } | ||
|
|
||
| public UserProfitDto getStatistics(Dealer dealer) { |
There was a problem hiding this comment.
많은 고민을 해보셨네요! 질문에 대해 답을 드리기에 앞서, 기론에게 궁금한 것이 있어요. 🤔
- 기론이 생각하는 Players, PlayerResult 객체의 역할은 무엇인가요?
- DTO의 역할은 무엇인가요? DTO와 관련된 로직이 도메인 안에 있어도 괜찮을까요?
There was a problem hiding this comment.
-
Players는
List<player>들을 관리해주고List<Player>처리에 대한 로직들을 Players 내부에서 일급컬션으로 처리하는 역할이라고 생각합니다.
그리고 PlayerResult는 dealer와 player를 받아서 결과를 산출해주는 역할을 합니다.
처음에 Players 내부에서 dealer와 비교하여 결과를 처리하는 로직이 있어서 players에 있는게 맞나 싶었는데 컨트롤러에서 처리하기엔 중요한 비즈니스 로직이라고 생각되어 players내부로 넣었습니다! -
DTO는 도메인 로직에서 반환한 결과들을 담아서 전달만 하는 역할로 해야 하는데 도메인안에 들어있으면 순수한 비즈니스 로직이 아니여서 파악하기 어려울것 같네요.
그렇다면 dto로 변환은 어디서 하는게 좋을까요? dto내부에도 도메인이 들어가면 도메인이 수정될때 Dto도 영향받아서 안좋다고 알고있는데 그러면 controller에서 new Dto()에 인자값으로 도메인 객체의 get()함수를 통해서 상태를 넣어주면 될까요?
There was a problem hiding this comment.
-
Players는 말씀하신대로 Player에 대한 로직을 관리하는 일급컬렉션입니다. 하지만 지금은 플레이어들의 수익률 계산 뿐만 아니라 딜러에 대한 수익률도 같이 계산하고 있는데, 과연 Players의 책임이 맞을까요? 아니라면 어디로 이동되어야 할까요? controller밖에 없을까요? 수익률에 대한 객체를 만들어도 좋고, 현재 결과에 따른 수익률을 가지고 있는 PlayerResult에서 처리해도 좋을 것 같아요.
-
https://tecoble.techcourse.co.kr/post/2021-04-25-dto-layer-scope/ 이 글이 도움될 것 같네요! 현재 구조에서는 말씀하신대로 Controller가 가장 적합해보입니다. (view와 domain을 연결해주는 역할을 하니까요)
jnsorn
left a comment
jnsorn
left a comment
There was a problem hiding this comment.
안녕하세요, 기론! 2단계 구현하느라 수고 많으셨어요.
다음 미션을 위해 이만 머지하겠습니다.
체스 미션도 화이팅입니다.
2 participants
안녕하세요 또링!
오래 걸려서, 이제야 pr을 보내네요😢
블랙잭이 생각보다 너무 복잡하네요 ㅎㅎ.. 많은 조건들때문에 코드 길이 줄이려고 노력을 했습니다.
1단계에 해주셨던 리뷰를 반영했고 2단계도 잘 부탁드립니다!!
항상 감사합니다!