[🚀 사이클2 - 미션 (블랙잭 베팅)] 재키 미션 제출합니다.#1134
Conversation
zzaekkii
changed the title
Chocochip101
left a comment
Chocochip101
left a comment
There was a problem hiding this comment.
안녕하세요, 재키 :) 리뷰어 초코칩입니다 🍪
블랙잭 배팅도 잘 구현해주셨네요.
커맨트로 리뷰 남겼으니, 확인해보시고 재요청주세요!
| private final int numerator; | ||
| private final int denominator; |
There was a problem hiding this comment.
소수점 하나의 변수로 관리할 수 있는데도, 분자 분모를 나눠서 관리하는 이유가 있나요?
There was a problem hiding this comment.
음, 알고리즘 문제를 풀면서 든 습관인데요.
소수 나눗셈은 아무래도 오차가 발생하기도 하고,
베팅 금액인 정수 타입이 다르다보니 같은 타입끼리 연산을 할 수 있도록 분자와 분모를 나눠서 두었습니다!
만약 소수로 연산을 하려고 했다면, 둘 다 BigDecimal 타입을 사용했을 것 같네요.
There was a problem hiding this comment.
답변 감사합니다 :)
처음 이 코드를 보는 입장에서 numerator, denominator를 통해 배당을 표현하는 방식은 직관적으로 이해하기 어렵다고 느껴졌어요.
예를 들어 블랙잭의 1.5배 배당을 3/2로 표현하고 있는데, 만약 1.5배에서 2배로 정책이 변경된다면 어떤 값을 수정해야 하는지 한눈에 들어오지 않아요. 단순히 숫자 하나를 바꾸는 문제가 아니라 분자와 분모를 함께 고려해야 하기 때문에, 변경 포인트를 빠르게 파악하기 어렵습니다.
또한 현재 구조는 하나의 의미(배당 비율)를 표현하기 위해 두 개의 필드(numerator, denominator)를 동시에 관리해야 하기도 하고요. 이는 곧 관리 포인트가 두 배로 늘어나는 것이고, 값이 서로 어긋나는 실수를 유발할 수 있겠네요.
백엔드 개발 관점에서도 이 구조는 불리하게 작용할 수 있습니다. 만약 GameResult를 DB에 저장하고, 대량의 트래픽이 발생하는 상황에서 특정 배당이 잘못 계산되는 문제의 문의가 들어왔다고 가정해보겠습니다. 예를 들어 "블랙잭 배당이 1.5배가 아니라 이상하게 계산된다"는 이슈가 들어왔을 때, 단순히 하나의 값만 확인하면 되는 것이 아니라 분자와 분모를 함께 추적해야 합니다.
이 경우 로그나 DB 데이터를 확인할 때도 3/2라는 표현을 해석해야 하고, 어느 한쪽 값이 잘못 들어갔는지까지 추가로 검증해야 하기 때문에 디버깅 난이도가 높아집니다. 특히 운영 환경에서는 이런 간접적인 표현이 문제 원인을 빠르게 파악하는 데 방해가 될 수 있겠네요.
결과적으로, 현재 방식은 “정확한 계산”이라는 장점은 있지만, 가독성, 변경 용이성, 디버깅 편의성 등의 측면에서는 오히려 비용이 더 큰 설계라고 생각합니다.
차라리 배당을 하나의 값으로 표현하고(double 또는 BigDecimal), 필요한 경우에만 정밀도를 고려하는 방식이 더 유지보수에 유리한 방향이라고 생각이 들어요.
말씀드린 부분이 납득되신다면, 해당 방향으로 한 번 수정해보시는 것도 좋을 것 같습니다 🙂
There was a problem hiding this comment.
네, 확실히 문제 풀이와는 다르게 개발에서는 추후 유지보수를 고려하지 않을 수 없는데요.
이 점에서 다른 사람이 코드를 봐도 해당 필드를 어떻게 활용하는지 쉽게 읽히게 하고,
요구사항이 변경되었을 때도 하나의 값만 확인하면 된다는 점이 더 유리할 것 같습니다.
BigDecimal을 이용해서 이러한 부분을 반영해보겠습니다!
Chocochip101
left a comment
Chocochip101
left a comment
There was a problem hiding this comment.
안녕하세요, 재키!
빠르게 변경해주셨네요 👍🏻
테스트와 numerator 관련해서 리뷰/커맨트 남겼습니다. 확인해주세요.
사이클 2의 요구사항을 만족해서 다음 리뷰 요청 때 머지하겠습니다.
| @@ -33,7 +43,27 @@ public List<Player> getPlayers() { | |||
|
|
|||
| private void validatePlayerCounts(final List<Player> players) { | |||
| if (players.size() < MINIMUM_BOUND || players.size() > MAXIMUM_BOUND) { | |||
There was a problem hiding this comment.
다시보니 players.isEmpty()로 대체 가능하겠군요
There was a problem hiding this comment.
아하, 맞아요.
MINIMUM_BOUND가 1인데도 .isEmpty() 대신 저렇게 두었던 이유는 추후 최소 인원에 대한 요구사항이 변경(ex. 최소 플레이어 2명 이상)될 수도 있지 않을까? 하는 이유 때문이었습니다.
지금 당장엔 MINIMUM_BOUND가 없어도 괜찮지만, 이런 경우엔 어떻게 두는 게 괜찮을지 모르겠더군요..
There was a problem hiding this comment.
그런 의도였군요 👍🏻
최소 플레이어 수 요구사항이 2명 이상으로 변경될 때, .isEmpty() 기반 구현보다 복잡해지거나 수정 범위가 커질 가능성이 있다면 MINIMUM_BOUND를 두는 방향을 고려해볼 수 있을 것 같아요.
현재 코드에서는 요구사항이 변경되더라도 수정 범위가 크지 않아 .isEmpty()로도 충분해 보입니다.
다만 MINIMUM_BOUND를 두는 방향도 확장성 측면에서 의미가 있어 해당 부분은 이 정도로 마무리해도 좋을 것 같아요 🙂
| private final int numerator; | ||
| private final int denominator; |
There was a problem hiding this comment.
답변 감사합니다 :)
처음 이 코드를 보는 입장에서 numerator, denominator를 통해 배당을 표현하는 방식은 직관적으로 이해하기 어렵다고 느껴졌어요.
예를 들어 블랙잭의 1.5배 배당을 3/2로 표현하고 있는데, 만약 1.5배에서 2배로 정책이 변경된다면 어떤 값을 수정해야 하는지 한눈에 들어오지 않아요. 단순히 숫자 하나를 바꾸는 문제가 아니라 분자와 분모를 함께 고려해야 하기 때문에, 변경 포인트를 빠르게 파악하기 어렵습니다.
또한 현재 구조는 하나의 의미(배당 비율)를 표현하기 위해 두 개의 필드(numerator, denominator)를 동시에 관리해야 하기도 하고요. 이는 곧 관리 포인트가 두 배로 늘어나는 것이고, 값이 서로 어긋나는 실수를 유발할 수 있겠네요.
백엔드 개발 관점에서도 이 구조는 불리하게 작용할 수 있습니다. 만약 GameResult를 DB에 저장하고, 대량의 트래픽이 발생하는 상황에서 특정 배당이 잘못 계산되는 문제의 문의가 들어왔다고 가정해보겠습니다. 예를 들어 "블랙잭 배당이 1.5배가 아니라 이상하게 계산된다"는 이슈가 들어왔을 때, 단순히 하나의 값만 확인하면 되는 것이 아니라 분자와 분모를 함께 추적해야 합니다.
이 경우 로그나 DB 데이터를 확인할 때도 3/2라는 표현을 해석해야 하고, 어느 한쪽 값이 잘못 들어갔는지까지 추가로 검증해야 하기 때문에 디버깅 난이도가 높아집니다. 특히 운영 환경에서는 이런 간접적인 표현이 문제 원인을 빠르게 파악하는 데 방해가 될 수 있겠네요.
결과적으로, 현재 방식은 “정확한 계산”이라는 장점은 있지만, 가독성, 변경 용이성, 디버깅 편의성 등의 측면에서는 오히려 비용이 더 큰 설계라고 생각합니다.
차라리 배당을 하나의 값으로 표현하고(double 또는 BigDecimal), 필요한 경우에만 정밀도를 고려하는 방식이 더 유지보수에 유리한 방향이라고 생각이 들어요.
말씀드린 부분이 납득되신다면, 해당 방향으로 한 번 수정해보시는 것도 좋을 것 같습니다 🙂
Chocochip101
left a comment
Chocochip101
left a comment
There was a problem hiding this comment.
안녕하세요, 재키 😄
빠르게 변경해주셔서 감사합니다 👍🏻
고생하셨고, 다음 미션도 응원하겠습니다! 🙌
| @@ -33,7 +43,27 @@ public List<Player> getPlayers() { | |||
|
|
|||
| private void validatePlayerCounts(final List<Player> players) { | |||
| if (players.size() < MINIMUM_BOUND || players.size() > MAXIMUM_BOUND) { | |||
There was a problem hiding this comment.
그런 의도였군요 👍🏻
최소 플레이어 수 요구사항이 2명 이상으로 변경될 때, .isEmpty() 기반 구현보다 복잡해지거나 수정 범위가 커질 가능성이 있다면 MINIMUM_BOUND를 두는 방향을 고려해볼 수 있을 것 같아요.
현재 코드에서는 요구사항이 변경되더라도 수정 범위가 크지 않아 .isEmpty()로도 충분해 보입니다.
다만 MINIMUM_BOUND를 두는 방향도 확장성 측면에서 의미가 있어 해당 부분은 이 정도로 마무리해도 좋을 것 같아요 🙂
2 participants
안녕하세요, 초코칩~
사이클1에서 구현했던 코드에 베팅 기능을 추가했습니다.
지난 PR merge 전에 초코칩이 마지막으로 남겨두었던 리뷰 역시 최대한 반영하려고 노력했습니다.
베팅 기능을 추가하는 것은 크게 어렵게 느껴지지 않았는데요.
그런데 여전히 뭔가,,
뭔가 아쉬운 부분들이 좀 있어서요 🤨
그런 부분들을 아래에 적어두었습니다
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
1. 에러 메시지의 관리
제가 이전 경험에 얽매여서 enum으로 에러 메시지를 통합 관리하고 있다는 걸 느꼈는데요.
그렇다면 에러가 발생할 수 있는 (throw가 직접 일어나는) 클래스에 메시지를 상수화해두는 것이 더 적절할까요?
확실히 지난 리뷰 내용처럼 리팩터링을 거치면서 더이상 사용되지 않는 메시지가 여전히 존재했었던 걸 보고, 각 클래스에 에러 메시지를 상수화해보았는데 접근 지정자 관련한 궁금증이 생겼어요.
예를 들어, Test에서 ThrownBy로
.hasMessage()내부에 메시지가 있는지 검증하는 경우엔, 해당 메시지 상수를 사용하기 위해 private이 아니라 public으로 둬야 하는지 고민이 들어서요. 아니면 애초에 에러 메시지가 쓰이는 기능은 모두 테스트 검증을 위해.hasMessage()를 쓸 수 있도록 public으로 두는 것이 맞는 방향일까요??테스트 목적을 위해 설계에 영향을 받는다는 게 혹시 이 경우인지가 걸려서,
PLAYER_COUNT_OUT_OF_RANGE는 아직 ErrorMesssage 내부에 두었습니다!2. 3개 이상의 인스턴스 변수를 가짐
베팅 금액은 플레이어가 가지는 게 자연스럽다고 생각해 Player 내부 인스턴스로 두었는데요.
이럴 경우엔 Participant 추상 클래스 내부
name과hand인스턴스와 합쳐져 총 3개가 되니 요구사항에 맞지 않아서요.보통 이럴 때는 3개 인스턴스 규칙을 철저히 지키기 위해 다른 방식을 사용하는지,
혹은 유연하게 허용하는지 궁금해 질문드립니다!