-
Notifications
You must be signed in to change notification settings - Fork 388
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
[1단계 - 블랙잭 게임 실행] 상돌(이상진) 미션 제출합니다. #632
Conversation
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
Co-authored-by: seokmyungham <[email protected]>
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 class Hand { | ||
|
||
private static final int BLACKJACK_MAX_SCORE = 21; |
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.
intelliJ 에 포매팅 파일을 직접 넣어서 사용하시나요? 띄어쓰기가 많이 되어있는것처럼 보여서요~
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.
intelliJ 에 포매팅 파일을 직접 넣어서 사용하시나요? 띄어쓰기가 많이 되어있는것처럼 보여서요~
포매팅 파일은 https://naver.github.io/hackday-conventions-java/ 을 사용하고 있었고, 클래스 선언부 밑의 한 줄 공백처리는 가독성에 좋다고 판단되어 페어와 정한 컨벤션이었습니다!
우테코 컨벤션 파일이 있다는 것을 늦게 알아서, step2에는 이 파일을 이용해서 다시 정렬해볼 생각입니다!!ㅎㅎ
/** | ||
* Ace는 기본적으로 11로 계산되나, 합계가 21을 초과할 경우 1로 계산한다. | ||
*/ |
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.
Hand 클래스에 주석을 달아놓긴 했는데, 이 주석으로 충분히 설명이 안 될수도 있다는 생각에 별도로 작성하게 되었습니다.(주석에 대해서도 의견 주시면 감사하겠습니다 ㅎㅎ)
- Ace가 없으면 그냥 전체 합계를 구한다.
- Ace가 있으면, 일단 Ace는 모두 11로 계산한다.
- Ace의 개수 만큼 루프를 돌리며, 21 이하가 될때까지 합계에서 10씩 뺀다.
상돌은 주석에 대해서 어떻게 생각하시나요?
주석으로도 부족해서 추가로 설명을 주신다고 하는게 관리차원에서 많이 불편해보입니다.
흔히들 주석대신 코드로 표현하라 뭐 이런말이 있는데 그건잘모르겠고, 저는 개인적으로 주석에 대해서 좋게 생각하고 때에 따라 꼭 필요하다고 생각합니다. 다만 코드의 동작방식을 단순히 설명하는 주석은 좋지않고 왜 이렇게 짰어야만 하는 이유가 적힌 주석이 제일 좋습니다. 예를 들어, 뜬금없이 분기문이 존재하면서 return이 되는 코드가 있다고 가정했을때 코드의 동작방식을 이해하는것은 누구나 하겠지만 도대체 왜 이렇게 만들었는지는 그 당시 컨텍스트를 아는 사람만 알 수 있습니다.
상돌이 남기신 주석은 어떤 형태의 주석일까요?
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이 되는 코드가 있다고 가정했을때 코드의 동작방식을 이해하는것은 누구나 하겠지만 도대체 왜 이렇게 만들었는지는 그 당시 컨텍스트를 아는 사람만 알 수 있습니다.
상돌이 남기신 주석은 어떤 형태의 주석일까요?
사실 페어와 주석을 남기면서, 이 정도 주석이면 충분할 것 같다고 생각했으나 리뷰어의 이해를 돕기 위해 한번 더 작성하게 되었습니다..ㅎㅎ 하지만 주석을 잘 작성했다고 생각하면 메시지를 남기지 않는 것이 당연히 맞다고 생각합니다.
말씀해주신 것을 반영해서 주석을 수정해봤는데, 한번 더 확인해주시면 감사드리겠습니다!
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.
조금 나아지긴했는데 어쩄든 코드의 내용을 설명하는 주석으로 보여요~ 개인적으로는 없어도 될거같아요
package blackjack.dto; | ||
|
||
public record CardDto(String cardNumber, String cardShape) { | ||
} |
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를 사용하는 것이 더 도움이 될 것이라고 생각했습니다. DTO를 컨트롤러나 별도의 매퍼를 정의한 뒤 생성하는 것도 고려했으나, 객체에서 해당되는 DTO를 생성하는 것이 응집도를 높일 수 있고 불필요한 getter 를 만들지 않아도 되어 더 좋다고 판단하였습니다.
DTO의 종류가 많아 미리 요약을 해놓으면 좋을 것 같습니다 ㅎㅎ DTO의 종류는 다음과 같습니다.
(중략..)
잘 해보셨어요! 다만 기능개요 목록 작성하신것에 도메인에 대한 설명이 있었으면 더 좋았을거같아요. dto는 사실 부가적인 내용이고 이번미션에서 과연 블랙잭이라는것을 어떻게 구현할 것인가에 대한 고민이 더 많이 보였으면 좋겠어요. PR내용을 수정해보시라는건 아니고 프로덕트 결과물중에서 어떤 것을 중요하게 표출해서 이야기할지에 대해서 추후에 도움이 될까 싶어 말씀드렸어요.
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는 사실 부가적인 내용이고 이번미션에서 과연 블랙잭이라는것을 어떻게 구현할 것인가에 대한 고민이 더 많이 보였으면 좋겠어요. PR내용을 수정해보시라는건 아니고 프로덕트 결과물중에서 어떤 것을 중요하게 표출해서 이야기할지에 대해서 추후에 도움이 될까 싶어 말씀드렸어요.
도메인은 Readme와 코드를 보고 파악할수 있어야 한다는 생각이었습니다. 만약 코드를 보고도 파악이 안 된다면 그건 제가 코드를 잘못 작성한 것이니깐요 ㅎㅎ PR 메시지에는 코드를 보실 때 혼란을 느끼실수도 있다고 느껴지는 부분들을 작성하긴 했는데, 제이 의견처럼 그래도 기능 개요
라고 작성했으면 전체 도메인의 흐름 정도는 작성하는게 더 좋았을 것이라는 생각이 드는 것 같습니다! 다음에는 꼭 반영 해보겠습니다. 감사합니다!
public DealerResultDto convertDealerToResultDto(List<GameResult> playerResults) { | ||
Name name = getName(); | ||
int winCount = (int)playerResults.stream() | ||
.filter(GameResult::isLose) | ||
.count(); | ||
int loseCount = playerResults.size() - winCount; | ||
|
||
return new DealerResultDto(name.value(), winCount, 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.
메서드의 길이를 줄여보면 어떨까요?
함수를 최대한 작게 유지하라는 요구사항을 프로젝트 전체적으로도 확인주시면 좋을거같아요!
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.
메서드의 길이를 줄여보면 어떨까요? 함수를 최대한 작게 유지하라는 요구사항을 프로젝트 전체적으로도 확인주시면 좋을거같아요!
일단 이 부분은 구현 방법을 바꾸면서 제거하였습니다! 전체적으로 메서드는 10줄 이내
라는 요구사항을 지키려고 노력하는데, 가끔 어려운 경우도 있는 것 같습니다 ㅠㅠ
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 class Player extends BlackjackGamer { | ||
|
||
private static final int BLACKJACK_MAX_SCORE = 21; |
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.
블랙잭 최대 점수라는 값이 여러군데에서 관리가 되고있네요. 관리포인트를 줄이려면 어떻게 하면 좋을까요?
domain 패키지 안에 별도의 Enum을 만든 뒤 블랙잭과 관련된 상수들을 한 곳에서 관리하는 방법으로 수정하였습니다! 확인 부탁드립니다 ㅎㅎ
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 GamerHandDto convertGamerToDto() { | ||
String playerName = name.value(); | ||
List<CardDto> gamerHand = hand.convertHandToDto(); | ||
|
||
return new GamerHandDto(playerName, gamerHand); |
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변환로직이 있네요. 상돌이 말씀하신
출력 형식이 워낙 다양하여 DTO를 사용하는 것이 더 도움이 될 것이라고 생각했습니다
라는 부분은, 다양한 출력로직에 의해 도메인클래스가 계속 수정이 될 수 있다는 의미와 동일할거같아요.
저도 "이정도는 괜찮다" 라는 생각이었지만, 그래도 찝찝하다는 생각이 들어 리뷰를 받고 이 부분부터 수정하기 시작했어요. 제가 생각한 방법은 다음과 같습니다.
- 컨트롤러에서 직접 매핑한다. -> 이 경우는 컨트롤러 클래스가 너무 길어진다고 느꼈어요.
- 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.
답이 없기 때문에 잘못된건 없습니다. 다만 상황에 따라 다르겠지만 현재 상황 기반으로 잘 판단해주셨습니다.
3번정도가 딱 적당하네요!
public Card draw() { | ||
return cards.poll(); | ||
} |
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.
계속 poll을 하면 어떤일이 벌어질까요?
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.
계속 poll을 하면 어떤일이 벌어질까요?
카드의 숫자 합이 21을 초과하면 더 이상 받을 수 없고, 최대 플레이어의 수를 8명으로 제한하였기에 이 상황에선 52장의 카드를 모두 뽑을 수는 없다고 생각했습니다..ㅎㅎ
하지만 이건 충분히 열릴 수 있는 부분이라고 생각했고, 모든 카드를 다 뽑으면 새로 52장의 카드를 세팅한뒤 뽑도록 수정하였습니다!
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.
개인적인 의견이지만 비어있을때는 예외를 발생시키고 추후에 새로운 Deck객체를 생성하는게 더 나을거같아요. 덱이 비어있을때 같은 객체에서 계속해서 카드가 무한히 나오도록 세팅되는게 어색하지 않을까요?
public enum CardNumber { | ||
|
||
ACE("A", 11), |
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.
카드의 숫자는 CardNumber, 카드의 모양은 CardShape 라는 Enum을 이용하여 정의하였습니다.
이때, CardNumber는 ACE(”A”, 11) 과 같이 출력에 사용될 이름과 숫자 값으로 구성되어 있고, CardShape는 HEART("하트") 와 같이 출력에 사용될 이름으로 구성되어 있습니다.
여기서 질문드리고 싶은 것은, 사실 “A”와 “하트”는 출력 형식이라고 생각됩니다. 따라서 이렇게 정의하게 되면 Domain 과 View 가 완전히 독립은 아니게 될 것이라고 생각합니다.
그래서 “A”와 “하트”와 같은 값은 View에서 처리하는 별도의 Enum을 만드는 것이 이를 해결해줄 것이라고 생각하지만, 이 클래스를 생성해야 하는 단점이 크다고 생각했고, 동시에 Domain에 있는 Enum에 정의하는 것 정도는 허용될 수 있다고 생각하여 지금의 형태로 구현하게 되었습니다.
이것에 대한 제이님의 의견을 말씀해주시면 감사하겠습니다 ㅎㅎ
화면에 보여지는것이 무조건 view의 내용이다 라고 생각을 하신거같아요. 도메인에서 변하지 않는값들이 무엇인지 생각해보면 좋을거같아요. 일반적으로 KING을 K로 사용하고 점수는 10점으로 계산이 됩니다. 화면에서 KING을 킹 혹은 왕 같은 단어로 표기를 해야한다면 그건 view의 역할이겠지만 본연의 도메인이 가지고있는 속성은 지금처럼 enum에 들고있는게 괜찮다고 생각이 들어요~
지금 구현해주신 정도까지는 괜찮습니다. 다른 의견 있으시면 말씀해주세요!
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.
화면에 보여지는것이 무조건 view의 내용이다 라고 생각을 하신거같아요. 도메인에서 변하지 않는값들이 무엇인지 생각해보면 좋을거같아요. 일반적으로 KING을 K로 사용하고 점수는 10점으로 계산이 됩니다. 화면에서 KING을 킹 혹은 왕 같은 단어로 표기를 해야한다면 그건 view의 역할이겠지만 본연의 도메인이 가지고있는 속성은 지금처럼 enum에 들고있는게 괜찮다고 생각이 들어요~ 지금 구현해주신 정도까지는 괜찮습니다. 다른 의견 있으시면 말씀해주세요!
저는 이번에 구현된 것 처럼 View에서 J,Q,K와 같이 매핑하는 것이 좋겠지만, 이정도는 허용할만한 범위
라고 생각했습니다. 질문을 드렸던 이유는 "A,J,Q,K"는 도메인 패키지 안에 정의하지 않는 것이 좋다
는 다른 크루들의 의견도 합리적이라고 생각했었기 때문이에요.
그런데 제이의 의견을 듣고 생각해보니, "일반적으로 사용되는 것"에 대해선 제가 생각을 못했던 것 같아요. 앞으로 기준을 세울 때 정말 많은 도움이 될 것 같습니다. 감사합니다 ㅎㅎ
import blackjack.dto.GamerHandDto; | ||
import blackjack.dto.PlayerResultsDto; | ||
|
||
public class 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.
OutputView에서의 공백 줄 출력 방법
sysout을 쓰지않아서 이렇게 하고있다 라고 말씀드리긴 어렵겠지만.. 저는 디폴트로 println을 사용하고 추가적인 개행이 있다면 개행만 출력하는 함수를 따로 만들어서 호출하는 식으로 구현할거같아요~
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에서의 공백 줄 출력 방법
sysout을 쓰지않아서 이렇게 하고있다 라고 말씀드리긴 어렵겠지만.. 저는 디폴트로 println을 사용하고 추가적인 개행이 있다면 개행만 출력하는 함수를 따로 만들어서 호출하는 식으로 구현할거같아요~
의견 감사드립니다! 여기에 더해서, 저는 출력의 공백 줄은 컨트롤러에서 관리하는게 맞다는 생각이 들었고 빈 공백 줄을 출력하는 것을 컨트롤러로 이동하게 되었는데, 바뀐 형태는 어떻게 생각하시는지 궁금합니다!
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.
넵 개행 메서드를 따로 만들어서 공통으로 사용하는방식이 더 나아보이네요!
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
|
||
public class DeckTest { |
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를 사용하지 않고 구현했을 때는, 테스트 커버리지가 90% 이상이 나왔었는데 DTO를 생성하는 메서드 및 equals와 같은 테스트가 필요하지 않다고 판단되는 메서드가 추가되니 테스트 커버리지가 낮아졌습니다. 이러다 보니 도메인에서 DTO를 얻어내도록 구현한 것이 잘못된 방법이 아닌가? 라는 생각이 들기도 하는 것 같습니다.
테스트를 늘리면 해결이 되긴 하겠지만, getter 및 Collections.shuffle()과 같은 자바 기본 코드를 사용하는 메서드들에 대한 테스트를 작성하는 것은 여전히 의미가 없다는 생각이 드는 것 같아요.
TDD로 구현하였기에 테스트가 필요한 메서드들은 다 테스트 했다고 생각하는데, 그럼에도 커버리지가 낮은게 계속 신경이 쓰이긴 하네요 ㅎㅎ.. 그래서 이것에 대한 의견도 들어보고 싶습니다!
일단 equals와 같은 테스트는 꼭 필요합니다. 객체비교가 안돼서 장애가 나는경우도 있습니다.
그리고 테스트 커버리지 90%면 상당히 높은 수치로 보여지긴합니다만, 무의미한 테스트 작성보다는 도메인의 핵심로직은 빠지지않고 테스트한다 는 생각으로 접근해보면 어떨까요?
약간 다른 답변일수도있겠지만.. 극단으로 치달으면 효율성이 낮아집니다. 단순히 커버리지를 높이기위해 리소스를 쓰게 되면 정작 중요한 다른 부분에 신경쓸 시간이 줄어들게 됩니다.
다만 수치자체가 너무 민감해서 도저히 참을 수 없어 라고 하면 해야겠지만, 투자대비해서 얻을 수 있는게 무엇일까를 생각해보세요.
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를 사용하지 않고 구현했을 때는, 테스트 커버리지가 90% 이상이 나왔었는데 DTO를 생성하는 메서드 및 equals와 같은 테스트가 필요하지 않다고 판단되는 메서드가 추가되니 테스트 커버리지가 낮아졌습니다. 이러다 보니 도메인에서 DTO를 얻어내도록 구현한 것이 잘못된 방법이 아닌가? 라는 생각이 들기도 하는 것 같습니다.
테스트를 늘리면 해결이 되긴 하겠지만, getter 및 Collections.shuffle()과 같은 자바 기본 코드를 사용하는 메서드들에 대한 테스트를 작성하는 것은 여전히 의미가 없다는 생각이 드는 것 같아요.
TDD로 구현하였기에 테스트가 필요한 메서드들은 다 테스트 했다고 생각하는데, 그럼에도 커버리지가 낮은게 계속 신경이 쓰이긴 하네요 ㅎㅎ.. 그래서 이것에 대한 의견도 들어보고 싶습니다!일단 equals와 같은 테스트는 꼭 필요합니다. 객체비교가 안돼서 장애가 나는경우도 있습니다.
그리고 테스트 커버리지 90%면 상당히 높은 수치로 보여지긴합니다만, 무의미한 테스트 작성보다는 도메인의 핵심로직은 빠지지않고 테스트한다 는 생각으로 접근해보면 어떨까요? 약간 다른 답변일수도있겠지만.. 극단으로 치달으면 효율성이 낮아집니다. 단순히 커버리지를 높이기위해 리소스를 쓰게 되면 정작 중요한 다른 부분에 신경쓸 시간이 줄어들게 됩니다.
다만 수치자체가 너무 민감해서 도저히 참을 수 없어 라고 하면 해야겠지만, 투자대비해서 얻을 수 있는게 무엇일까를 생각해보세요.
-
우선, equals가 사용되는 부분을 보니
DeckTest
에서Card
에 대한 테스트가 유일했습니다. 프로덕션에서는 equals가 사용되지 않긴 하지만, 그래도 사용되는 곳이 있으니 Card에 대해서는 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.
제이 안녕하세요! 우선 작성해주신 커멘트들이 생각을 정리하는데 정말 많은 도움이 되었습니다. 감사합니다(__)👍
남겨주신 커멘트와 관련하여 수정한 부분은 답글에 달아놓았고, 답글에 없는 수정한 부분은 아래에 간단하게 요약해서 작성하겠습니다!
이번에도 잘 부탁드리겠습니다 ㅎㅎ 다가오는 한 주도 즐겁게 보내시길 바라겠습니다!😄
커멘트에 있는 것 이외에 수정된 사항은 다음과 같습니다
1. OutputView 수정
이전 OutputView를 보니, 중복되는 코드가 생각보다 많다고 느꼈어요. 그래서 이번엔 가급적 중복을 제거하는 방향으로 재구성 해봤습니다!
추가적으로 이전 커멘트를 반영해서, 빈 공백 줄 출력을 위해 있던 개행문자를 제거하고 공백은 컨트롤러에서 빈 줄을 출력하여 관리하도록 했고, printf에서 사용되는 개행문자
는 System.lineSeparator()
로 수정했어요.
2. 블랙잭일 때의 승패 처리 추가
이전 코드를 보니, 블랙잭인 경우
에 대한 승,패 처리를 제외했다는 것을 깨달아서 블랙잭인 경우도 추가해서 결과를 얻어내도록 수정했습니다!
다만 이렇게 했을 때의 문제는, PlayerTest
클래스가 �조금 지저분해진 것이라고 생각합니다. 이것을 해결하려면 사실 테스트를 위한 생성자를 하나 추가하면 되는데, 아직은 생성자를 추가하지 않아도 충분히 쓸 만 하다고 생각했어요.
수정을 고민중인 것이 있습니다.
플레이어가 카드를 받는 조건 수정
지금 상태에선, 카드의 숫자 합계가 21을 초과하지 않으면 카드를 다시 받을지를 물어봤었어요. 하지만 상식적으로 생각했을 때, 카드의 합이 이미 21인 상태에서 추가로 카드를 더 받을 플레이어는 단 한명도 없을 것이라고 생각해요 ㅎㅎ
그래서 카드의 합이 21인 경우는 카드를 안 받도록 하고 싶은데, 문제는 요구사항에 위배될 수도 있다는 것입니다. 만약 a,b,c라는 세 플레이어가 있는데, a의 카드 숫자 합이 21이라면 a에게는 한장의 카드를 더 받겠습니까?(예는 y, 아니오는 n)
를 물어보지 않기 때문이에요.
또한 기존에 21 이하이면 플레이어에게 카드를 받을지를 물어본 이유는, 요구사항에 21을 넘지 않는 경우
라고 되었기 때문입니다.
미션을 진행하며 항상 요구사항을 충족시킨다 vs 당연하다고 생각하는 것은 요구사항과 상관없이 구현한다
라는 고민을 하는 것 같습니다. 저는 후자의 경향이 강하다는 생각이 드는데, 어디까지 생각해야 할지에 대해 의견을 주시면 너무 감사할 것 같습니다!
public enum CardNumber { | ||
|
||
ACE("A", 11), |
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.
화면에 보여지는것이 무조건 view의 내용이다 라고 생각을 하신거같아요. 도메인에서 변하지 않는값들이 무엇인지 생각해보면 좋을거같아요. 일반적으로 KING을 K로 사용하고 점수는 10점으로 계산이 됩니다. 화면에서 KING을 킹 혹은 왕 같은 단어로 표기를 해야한다면 그건 view의 역할이겠지만 본연의 도메인이 가지고있는 속성은 지금처럼 enum에 들고있는게 괜찮다고 생각이 들어요~ 지금 구현해주신 정도까지는 괜찮습니다. 다른 의견 있으시면 말씀해주세요!
저는 이번에 구현된 것 처럼 View에서 J,Q,K와 같이 매핑하는 것이 좋겠지만, 이정도는 허용할만한 범위
라고 생각했습니다. 질문을 드렸던 이유는 "A,J,Q,K"는 도메인 패키지 안에 정의하지 않는 것이 좋다
는 다른 크루들의 의견도 합리적이라고 생각했었기 때문이에요.
그런데 제이의 의견을 듣고 생각해보니, "일반적으로 사용되는 것"에 대해선 제가 생각을 못했던 것 같아요. 앞으로 기준을 세울 때 정말 많은 도움이 될 것 같습니다. 감사합니다 ㅎㅎ
public Card draw() { | ||
return cards.poll(); | ||
} |
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.
계속 poll을 하면 어떤일이 벌어질까요?
카드의 숫자 합이 21을 초과하면 더 이상 받을 수 없고, 최대 플레이어의 수를 8명으로 제한하였기에 이 상황에선 52장의 카드를 모두 뽑을 수는 없다고 생각했습니다..ㅎㅎ
하지만 이건 충분히 열릴 수 있는 부분이라고 생각했고, 모든 카드를 다 뽑으면 새로 52장의 카드를 세팅한뒤 뽑도록 수정하였습니다!
/** | ||
* Ace는 기본적으로 11로 계산되나, 합계가 21을 초과할 경우 1로 계산한다. | ||
*/ |
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이 되는 코드가 있다고 가정했을때 코드의 동작방식을 이해하는것은 누구나 하겠지만 도대체 왜 이렇게 만들었는지는 그 당시 컨텍스트를 아는 사람만 알 수 있습니다.
상돌이 남기신 주석은 어떤 형태의 주석일까요?
사실 페어와 주석을 남기면서, 이 정도 주석이면 충분할 것 같다고 생각했으나 리뷰어의 이해를 돕기 위해 한번 더 작성하게 되었습니다..ㅎㅎ 하지만 주석을 잘 작성했다고 생각하면 메시지를 남기지 않는 것이 당연히 맞다고 생각합니다.
말씀해주신 것을 반영해서 주석을 수정해봤는데, 한번 더 확인해주시면 감사드리겠습니다!
import blackjack.dto.GamerHandDto; | ||
import blackjack.dto.PlayerResultsDto; | ||
|
||
public class 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.
OutputView에서의 공백 줄 출력 방법
sysout을 쓰지않아서 이렇게 하고있다 라고 말씀드리긴 어렵겠지만.. 저는 디폴트로 println을 사용하고 추가적인 개행이 있다면 개행만 출력하는 함수를 따로 만들어서 호출하는 식으로 구현할거같아요~
의견 감사드립니다! 여기에 더해서, 저는 출력의 공백 줄은 컨트롤러에서 관리하는게 맞다는 생각이 들었고 빈 공백 줄을 출력하는 것을 컨트롤러로 이동하게 되었는데, 바뀐 형태는 어떻게 생각하시는지 궁금합니다!
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
|
||
public class DeckTest { |
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를 사용하지 않고 구현했을 때는, 테스트 커버리지가 90% 이상이 나왔었는데 DTO를 생성하는 메서드 및 equals와 같은 테스트가 필요하지 않다고 판단되는 메서드가 추가되니 테스트 커버리지가 낮아졌습니다. 이러다 보니 도메인에서 DTO를 얻어내도록 구현한 것이 잘못된 방법이 아닌가? 라는 생각이 들기도 하는 것 같습니다.
테스트를 늘리면 해결이 되긴 하겠지만, getter 및 Collections.shuffle()과 같은 자바 기본 코드를 사용하는 메서드들에 대한 테스트를 작성하는 것은 여전히 의미가 없다는 생각이 드는 것 같아요.
TDD로 구현하였기에 테스트가 필요한 메서드들은 다 테스트 했다고 생각하는데, 그럼에도 커버리지가 낮은게 계속 신경이 쓰이긴 하네요 ㅎㅎ.. 그래서 이것에 대한 의견도 들어보고 싶습니다!일단 equals와 같은 테스트는 꼭 필요합니다. 객체비교가 안돼서 장애가 나는경우도 있습니다.
그리고 테스트 커버리지 90%면 상당히 높은 수치로 보여지긴합니다만, 무의미한 테스트 작성보다는 도메인의 핵심로직은 빠지지않고 테스트한다 는 생각으로 접근해보면 어떨까요? 약간 다른 답변일수도있겠지만.. 극단으로 치달으면 효율성이 낮아집니다. 단순히 커버리지를 높이기위해 리소스를 쓰게 되면 정작 중요한 다른 부분에 신경쓸 시간이 줄어들게 됩니다.
다만 수치자체가 너무 민감해서 도저히 참을 수 없어 라고 하면 해야겠지만, 투자대비해서 얻을 수 있는게 무엇일까를 생각해보세요.
-
우선, equals가 사용되는 부분을 보니
DeckTest
에서Card
에 대한 테스트가 유일했습니다. 프로덕션에서는 equals가 사용되지 않긴 하지만, 그래도 사용되는 곳이 있으니 Card에 대해서는 equals() 테스트를 추가했습니다! -
필요한 부분들은 모두 테스트를 했다고 생각합니다 ㅎㅎ 이 상황에서 커버리지를 높이기 위해 불필요한 테스트를 만들어서 얻는 장점이 크게 느껴지지 않았습니다. 수치적인 것도 중요하다고 생각하지만, 그것을 위해 불필요한 일을 하지 않도록 항상 주의하겠습니다!!ㅎㅎ
public class Player extends BlackjackGamer { | ||
|
||
private static final int BLACKJACK_MAX_SCORE = 21; |
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.
블랙잭 최대 점수라는 값이 여러군데에서 관리가 되고있네요. 관리포인트를 줄이려면 어떻게 하면 좋을까요?
domain 패키지 안에 별도의 Enum을 만든 뒤 블랙잭과 관련된 상수들을 한 곳에서 관리하는 방법으로 수정하였습니다! 확인 부탁드립니다 ㅎㅎ
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.
안녕하세요 상돌~
리뷰 반영하느라 고생많으셨습니다.
고민을 정말 많이 하신게 느껴지네요.
질문주신 부분에 대한 답변과 코멘트 몇가지 남겼는데 다음 스텝진행하실때 참고부탁드립니다!
Card card1 = new Card(CardShape.DIAMOND, CardNumber.ACE); | ||
Card card2 = new Card(CardShape.DIAMOND, CardNumber.ACE); | ||
|
||
assertThat(card1.equals(card2)).isTrue(); |
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.
👍🏼
assertTrue도 있을거에요
public boolean canReceiveCard() { | ||
return getScore() <= BlackjackConstants.BLACKJACK_VALUE.getValue(); | ||
} |
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.
미션을 진행하며 항상 요구사항을 충족시킨다 vs 당연하다고 생각하는 것은 요구사항과 상관없이 구현한다라는 고민을 하는 것 같습니다. 저는 후자의 경향이 강하다는 생각이 드는데, 어디까지 생각해야 할지에 대해 의견을 주시면 너무 감사할 것 같습니다!
이 부분 말씀하신거같아요~
개인적인 의견입니다. 저도 후자가 맞다고 봅니다.
필수적인 요구사항을 만족시키는것은 매우중요합니다. 다만 해당 요구사항들�에서, 혹은 요구사항외적인것들에서 애매한것들은 능동적으로 처리해야 매끄럽게 진행되는거같아요.
추가적으로 필수요구사항도 이상하다고 생각하는게 있으면 적극적으로 의견을 내서 설득하거나 설득당하거나 하는 과정들을 거치는게 좋습니다. 여러 반대의견들이 나와야 프로덕트가 좋은 방향으로 나아간다고 생각해요.
public GameResult getGameResult(Dealer dealer) { | ||
if (isBust()) { | ||
return GameResult.LOSE; | ||
} | ||
if (dealer.isBust()) { | ||
return GameResult.WIN; | ||
} | ||
if (dealer.isBlackJack()) { | ||
return GameResult.LOSE; | ||
} | ||
if (isBlackJack()) { | ||
return GameResult.WIN; | ||
} | ||
return compareScore(dealer.getScore(), getScore()); | ||
} |
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.
결과를 구하는 기능도 (정답은 없지만) 누가 수행하는지 생각해보면 좋을거같아요.
게임 진행담당은 딜러가 하는걸로 알고있어요. 실제 게임할때 플레이어가 딜러의 패를보고 결과를 계산하진 않을겁니다. 물론 판이 끝나면 서롤의 패를 보긴하겠지만, 결과를 계산해서 누가졌고 이겼는지 따지는건 플레이어 역할이 아닌거같아요.
한번 고민해보셔도 좋겠네요!
제이님 안녕하세요! 이번 미션에 함께하게 된 6기 상돌(이상진) 이라고 합니다.😄
이번 미션은 지난 미션들보다 더 많은 시간을 썼음에도 불구하고 난이도가 올라간 탓인지 여전히 부족한 부분들이 느껴지는 것 같습니다..ㅎㅎ
제출 전에 코드를 다시 확인하는데, 몇몇 부분은 사전에 설명드리는 것이 코드를 이해하시는데 도움이 될 것이라고 생각하여 작성하였습니다. 혹여나 이해가 안가시거나 더 궁금한 점이 있으시면 편하게 말씀해주시면 감사드리겠습니다.👍
3월이 되었지만 날씨가 여전히 춥네요 ㅎㅎ 환절기 감기 조심하시고 이번 미션동안 잘 부탁드리겠습니다! 감사합니다.
기능 개요
딜러와 플레이어
딜러와 플레이어에서 발생하는 중복 코드를 제거해야 한다.
라는 요구사항을 맞추기 위해, 딜러와 플레이어가 공통적으로 사용하는 로직을BlackjackGamer
라는 추상 클래스에 정의하고, 다른 부분만Dealer
와Player
에서 구현하였습니다.공통된 부분은,
카드 숫자 합을 구하는 것과 카드를 추가하는 것
이고, 다른 부분은카드를 더 받을 수 있는 조건과 DTO를 생성하는 것
입니다.카드의 숫자 합계 계산 방법
카드의 숫자 합계는
Hand
클래스에서 계산합니다.Hand
클래스에 주석을 달아놓긴 했는데, 이 주석으로 충분히 설명이 안 될수도 있다는 생각에 별도로 작성하게 되었습니다.(주석에 대해서도 의견 주시면 감사하겠습니다 ㅎㅎ)계산을 할 때, 다른 카드는 그냥 계산하면 되지만
Ace
는 상황에 따라1 또는 11
로 계산해야 했고 이 부분을 다음과 같이 구현하였습니다.결국 플레이어나 딜러가 계산하는 숫자의 합계는,
21을 초과하지 않 가장 큰 숫자
여야 한다는 생각으로 위와 같이 구현하였습니다.DTO
미션을 진행하는 초반에는 DTO를 사용하지 않고 구현해보려고 했는데, 출력 형식이 워낙 다양하여 DTO를 사용하는 것이 더 도움이 될 것이라고 생각했습니다. DTO를
컨트롤러나 별도의 매퍼를 정의한 뒤 생성하는 것
도 고려했으나, 객체에서 해당되는 DTO를 생성하는 것이 응집도를 높일 수 있고 불필요한getter
를 만들지 않아도 되어 더 좋다고 판단하였습니다.DTO의 종류가 많아 미리 요약을 해놓으면 좋을 것 같습니다 ㅎㅎ DTO의 종류는 다음과 같습니다.
PR 메시지에 어디까지 담아야할지는 항상 고민이네요. 일단 이정도를 작성하긴 했는데, 불필요한 부분이 있거나 코드를 읽으시면서 더 설명이 필요할 것 같다고 느끼신 부분이 있다면 말씀 부탁드립니다! 😄
질문
Enum의 사용
카드의 숫자는
CardNumber
, 카드의 모양은CardShape
라는 Enum을 이용하여 정의하였습니다.이때, CardNumber는
ACE(”A”, 11)
과 같이 출력에 사용될 이름과 숫자 값으로 구성되어 있고, CardShape는HEART("하트")
와 같이 출력에 사용될 이름으로 구성되어 있습니다.여기서 질문드리고 싶은 것은, 사실
“A”
와“하트”
는 출력 형식이라고 생각됩니다. 따라서 이렇게 정의하게 되면Domain
과View
가 완전히 독립은 아니게 될 것이라고 생각합니다.그래서
“A”
와“하트”
와 같은 값은View
에서 처리하는 별도의 Enum을 만드는 것이 이를 해결해줄 것이라고 생각하지만, 이 클래스를 생성해야 하는 단점이 크다고 생각했고, 동시에 Domain에 있는 Enum에 정의하는 것 정도는 허용될 수 있다고 생각하여 지금의 형태로 구현하게 되었습니다.이것에 대한 제이님의 의견을 말씀해주시면 감사하겠습니다 ㅎㅎ
OutputView에서의 공백 줄 출력 방법
위와 같은 출력 형식을 맞출 때, 두 문장 사이의 공백 줄을 출력하는 방법은 항상 고민거리인것 같습니다. 제가 생각한 방법은 다음과 같습니다.
크게 이런 방법으로 나눌 수 있을 것 같은데, 제이님께서 하고 계시는 방법이 있다면 말씀해주시면 감사드리겠습니다!
테스트 커버리지
처음에 DTO를 사용하지 않고 구현했을 때는, 테스트 커버리지가 90% 이상이 나왔었는데 DTO를 생성하는 메서드 및
equals
와 같은 테스트가 필요하지 않다고 판단되는 메서드가 추가되니 테스트 커버리지가 낮아졌습니다. 이러다 보니 도메인에서 DTO를 얻어내도록 구현한 것이 잘못된 방법이 아닌가? 라는 생각이 들기도 하는 것 같습니다.테스트를 늘리면 해결이 되긴 하겠지만,
getter
및Collections.shuffle()
과 같은 자바 기본 코드를 사용하는 메서드들에 대한 테스트를 작성하는 것은 여전히 의미가 없다는 생각이 드는 것 같아요.TDD로 구현하였기에 테스트가 필요한 메서드들은 다 테스트 했다고 생각하는데, 그럼에도 커버리지가 낮은게 계속 신경이 쓰이긴 하네요 ㅎㅎ.. 그래서 이것에 대한 의견도 들어보고 싶습니다!
자세하게 적는다고 하긴 했는데, 다 쓰고 나서 보니 너무 길다는 생각이 드네요ㅠㅠ 긴 글이지만 도움이 되었으면 좋겠습니다.
읽어주셔서 감사합니다!