[2단계 - 블랙잭 베팅] 페드로(류형욱) 미션 제출합니다. #677
Conversation
- `createEmptyCards()` -> `empty()`
- `drawEachPlayer()` -> `drawEach()`
- PlayerName -> PlayerMeta 이름 변경 후 필드 추가
- 점수 비교 로직이 Score로 이동함에 따라, 테스트 코드도 ScoreTest에서 관리함
| public class TestDeck extends Deck { | ||
| public TestDeck(List<Card> cards) { | ||
| super(cards); | ||
| } | ||
| } |
There was a problem hiding this comment.
1단계 리뷰에서 말씀하신
사실 정답이 없는 문제긴 한데요, 일부러 프레임워크 단의 제약사항때문에 열어두는 경우도 있긴 하지만 개인적으로 테스트 코드에서만 쓰이는 코드는 소스코드 작성 시에 자동완성에 뜨는 것 자체가 위험하다고 생각해서 지양하자는 주의예요. package-private으로 두는 방법도 있긴 한데... 그냥 안 쓰는 것 같습니다 ㅋㅋ 차라리 protected로 해두고 테스트 클래스에서 상속 받아서 쓴 적은 있는 것 같네요.
protected 를 활용하는 방법이 이런 느낌이 맞을까요?
There was a problem hiding this comment.
넵, 상속받은 객체들만 사용할 수 있게 되어서 개인적으로는 이렇게도 구현하는 것 같아요. 다만 class를 final class로 하는 컨벤션이 적용되면 사용할 수 없겠죠?
| private boolean isGreaterThan(Score relativeScore) { | ||
| return value > relativeScore.value; | ||
| } | ||
|
|
||
| private boolean isLessThan(Score relativeScore) { | ||
| return value < relativeScore.value; | ||
| } |
There was a problem hiding this comment.
[의견] 다른 메소드들처럼 스스로를 다른 것과 비교하니 relativeScore보다는 other가 직관적일 것 같아요.
| public static Score blackJackScore() { | ||
| return new Score(BLACKJACK_MAX_SCORE, true); | ||
| } |
There was a problem hiding this comment.
[의견] 이것도 Score.blackJack()으로 해도 될 것 같네요 :)
| public Map<Player, GameResult> compareEach(Score dealerScore) { | ||
| return playerGroup.stream() | ||
| .collect(Collectors.toMap( | ||
| Function.identity(), | ||
| player -> player.compareScoreWith(dealerScore)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
[질문] Player에는 hashCode가 없는데 올바른 키 값이 될 수 있을까요?
There was a problem hiding this comment.
오.. 어찌 보면 당연한데 고민해 보지 못했던 문제네요.
Player에서 hashCode() 를 오버라이드하지 않았으므로, Object.hashCode()가 호출될 것이고, 해당 메서드는 객체의 메모리 주소에 기반하여 해시 값을 생성하는 것으로 알고 있습니다.
현재 구현에서 Player는 final 필드로 PlayerMeta와 PlayerCards를 가지고 있는데, PlayerMeta는 레코드이므로 불변객체이고 PlayerCards에는 .append()를 통해 내부의 List에 카드를 추가할 수는 있으나, Player는 해당 일급 컬렉션에 대한 참조값만을 가지고 있고, Player는 참조값 자체를 변경할 수 있는 로직을 가지지 않으므로 카드를 뽑는 행위와 상관없이 해시는 일정하게 유지될 것으로 예상됩니다.
또한
PlayerMeta playerMeta = new PlayerMeta("name", 10);
Player p1 = new Player(playerMeta);
Player p2 = new Player(playerMeta);와 같이 동일한 PlayerMeta 객체를 통해 Player를 생성하는 경우에도 두 플레이어는 다른 플레이어로 인식되어야 합니다(동명이인인데, 베팅 금액도 똑같은 경우).
Player의 생성자에서 PlayerCards를 항상 새로 만들고 있으므로 두 인스턴스의 해시는 달라질 것이고, 이러한 점들로 미루어 볼 때 키 값으로 사용하기에 큰 무리가 없을 것 같다고 생각합니다.
혹시 제가 잘못 이해하고 있는 점이 있다면 말씀해주세요!
There was a problem hiding this comment.
일반적으로 객체의 동등성을 정의할 때는 equals와 hashCode를 함께 오버라이드하는 것이 좋아요 :)
PlayerMeta를 공유하는 두 Player 객체가 다르게 인식되어야 한다면, 이 로직을 equals와 hashCode에 반영해 주세요 :)
| @@ -0,0 +1,14 @@ | |||
| package blackjack.domain.participant; | |||
|
|
|||
| public record PlayerMeta(String name, int betAmount) { | |||
There was a problem hiding this comment.
[의견] PlayerMeta에 대한 테스트가 있으면 좋겠어요.
| public class TestDeck extends Deck { | ||
| public TestDeck(List<Card> cards) { | ||
| super(cards); | ||
| } | ||
| } |
There was a problem hiding this comment.
넵, 상속받은 객체들만 사용할 수 있게 되어서 개인적으로는 이렇게도 구현하는 것 같아요. 다만 class를 final class로 하는 컨벤션이 적용되면 사용할 수 없겠죠?
There was a problem hiding this comment.
안녕하세요 페드로 👋
2단계 잘 구현해주셨네요 💯
(고칠 부분이 많이 없긴하네요 ㅎㅎ)
코멘트 확인 부탁드려요 🙇
개인적으로 지역변수의 선언부와 사용부, 메서드의 반환문은 공백으로 분리되어 있는 게 읽기 편하다고 생각합니다! 지금은 각 줄이 그리 길지 않아 과한 개행으로 보일 수도 있을 것 같긴 한데, 저기서 지역변수 선언이 두 줄만 늘어나도 개행되어 있는 것이 편하지 않을까요? 그렇다고 선언부의 길이에 따라 개행 여부가 바뀌는 것도 조금 어색할 것 같구요.
공백 줄을 나누는 것에 대해 의미를 담으신거라면 전혀 이슈 없습니다 👍 (취향의 영역이라 생각합니다 ㅋㅋ 물론 팀 컨벤션이 최우선!)
프로덕션 코드에서는 이러한 형태로 사용해야 할까요? 정적 팩토리 메서드의 장점 중 하나가 객체를 생성하는 목적에 따라 이름을 부여하고, 해당 목적에 불필요한 값은 외부에서 전달받지 않아도 쉽게 객체를 생성할 수 있는 점이라고 생각하는데 저렇게 되면 팩토리 메서드를 제대로 활용하지 못하는 것 같아서 고민이 됩니다.
naive하게 해결책을 생각해봤을 때는 테스트에서 쓰이는 CardShuffler는 이런식으로 구현했을 것 같긴한데요, 이것도 사실 리스코프 치환원칙 위배라서 ㅋㅋ
class TestCardShuffler implements CardShuffler {
private final List<Cards> cards;
public TestCardShuffler(List<Cards> cards) {
this.cards = cards;
}
@Override
public void shuffle(List<Card> cards) {
return this.cards;
}
}만일 52장의 full card를 외부에서 주입받는 것이 어색하다 느껴 캡슐화를 해야겠다고 느껴진다면 CardShuffler의 이름을 변경해서 상속받은 객체에서 full cards를 만드는 로직을 가지거나, 카드 리스트를 넣어주는 별도 객체를 만들어서 책임을 위임할 수 있을 것 같네요 :) 다만, 지금 코드에서도 큰 이슈는 없으니 변경사항은 꼭 반영하지 않으셔도 됩니다 :)
아, 그리고 1단계에서 코멘트를 드리지 못한 것이 있는데요..!
Record는 기본적으로 주생성자를 public으로 열어서, 지금 DTO들은 정적 팩토리 생성자를 사용하고 있어서 주 생성자는 사용되지 않고 있는 것 같아요. record를 사용할 때는 팩토리 생성자 말고 주 생성자를 사용하려고 하는게 좀 더 직관적일 수 있겠다는 생각이 드네요 ㅎㅎ
woonjangahn
left a comment
woonjangahn
left a comment
There was a problem hiding this comment.
안녕하세요 페드로 👋
이번 미션은 여기서 마무리할게요 💯
고생하셨어요! 앞으로도 화이팅 🚀
2 participants
안녕하세요 호돌!
1단계에서 언급해 주신 내용들과 2단계에서 추가된 요구사항들을 반영하여 리뷰 요청 드려요.
이번 리뷰도 잘 부탁드립니다😀
1단계에서 말씀하셨던 전략패턴의 적용과 관련하여 의문점이 남아 1단계 댓글로 남겨 두었습니다. (#619 (comment))
확인하시고 답변 부탁드려요!