[🚀 사이클2 - 미션 (블랙잭 베팅)] 은오 미션 제출합니다.#1120
[🚀 사이클2 - 미션 (블랙잭 베팅)] 은오 미션 제출합니다.#1120Huiyeongkim wants to merge 21 commits intowoowacourse:huiyeongkimfrom
Conversation
| public int readBettingAmount(String name) { | ||
| while (true) { | ||
| try { | ||
| System.out.printf("%s의 배팅 금액은?%n", name); | ||
| String money = userInput(); | ||
| return Integer.parseInt(money); | ||
| } catch (IllegalArgumentException e) { | ||
| OutputView.printErrorMessage("잘못된 입력입니다. 다시 입력해주세요."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
신규 입력 부분 잘 작성해주신것 확인했습니다.
다만 제가 확인해보니 아래와 같은 문제가 보입니다.
- Player 이름을 숫자나 한글로 입력시(영문 X일 경우) 프로그램 종료
- 공백 입력시에는 try-catch 를 통한 반복문 정상작동
- bettingMoney 음수로 입력시 프로그램 종료
- 영문 입력시에는 try-catch 를 통한 반복문 정상작동
이를 해결하려면, Player 및 Money 객체의 생성시점이 try-catch 내로 들어가야 합니다.
There was a problem hiding this comment.
추가로, 지금 InputView 내에 검증 로직이 validate 라는 이름이 붙은 메소드 외에서도 수행되는 것으로 보입니다.
- 예를 들어, splitPlayerNames 내에서 name이 공백인 경우 예외 발생시키고 있습니다.
그래서 우선, validation 메소드들을 모두 별개 메소드로 분리했으면 좋겠습니다
그리고 미션 진행중에 여유가 있다면, 추가로 아래 사항을 구현해주시면 좋겠습니다.
- InputView 내의 validation 메소드들을 모아 Validator 클래스 구현
- name / money validation을 다른 validator에서 할지 같은 validator에서 할지는 자유
- Validator는 테스트를 함께 구현할 것. 성공하는 케이스와 실패하는 케이스(예외 및 메시지도 검증) 테스트해야 함
- 현재 Player 및 Money 클래스에서 validation 하는 로직을 Validator로 옮길 수 있을지, 옮겨도 괜찮을지 고민
- 이부분은 필수가 아니고, 은오가 생각하기에 옮기는게 옳은지 아닌지에 따라 결정해주세요. (은오의 의견이 궁금해서 추가로 적어둔 내용입니다.)
| // then | ||
| Assertions.assertEquals(52, cards.size()); | ||
|
|
||
| Assertions.assertThrows(IllegalArgumentException.class, deck::drawCard); |
There was a problem hiding this comment.
52회까지 정상 수행 후, assertThrows 검증한 것 좋습니다.
다만 assertThrows 에서 message도 인자로 넣어서 검증할 수 있는데, 함께 검증해주면 좋을듯해요. 😄
- 혹은 assertThatThrownBy 를 사용할수도 있습니다. 👍
| private static final String STRING_REGEX = "^[a-zA-Z]*$"; | ||
|
|
||
| private final String name; | ||
| private Optional<Money> money = Optional.empty(); |
There was a problem hiding this comment.
이부분을 PR 본문 2번에서도 적어주셨는데, 제 생각엔 수정이 필요해 보입니다.
우선 적어주신 것처럼, money는 객체 생성 시점에 정의되는 것이 좋다고 생각합니다.
- 물론 해당 값이 없는 경우를 고려해서 Optional을 사용해주신 것은 좋습니다.
- 다만 제가 생각하기에, Optional은 특정한 메소드 등에서 값 반환시 해당값이 null일수 있음을 명시하는데에 효과적이지, 클래스 내부 필드로 사용하기에 적절하지는 않습니다. getMoney 시점에 money가 비어있는 것을 검증하고 있긴 하지만, money가 nullable한 필드인 것과 비교해서 (관리된 예외를 발행할 수 있다는 것 외에는) 큰 장점이 없어 보입니다.
위의 문제를 해결하는 방법을 고민해보자면...
- Player 이름을 받은 뒤, Player 생성 전에 money 값을 받기 -> 이 경우 player 생성시 검증하는 로직이 늦게 수행되므로, 검증 관련 코드의 수정이 필요해집니다.
- Players 생성 후, money를 받아서 player와 money의 Map을 필드로 가지는 일급컬렉션에 저장하기 -> 이 경우 결과를 구할때 해당 객체를 전달하면 되지만, 클래스가 하나 더 늘어나서 코드의 복잡도가 늘어날 수 있습니다.
이외에도 다른 방법들도 있을 듯한데, 관련해서 은오도 의견 적어주시고 생각하기에 좋은 방향으로 진행해주시면 좋겠습니다. (위에 적힌 방법 외로 수정하거나, 혹은 현재 방식 유지해도 괜찮습니다. 다만 수정하는 방향 결정한 이유를 적어주시면 참고가 될 듯 합니다.)
| public Map<String, BettingResult> calculateBettingResults() { | ||
| Map<String, MatchResult> matchResults = calculateResults(); | ||
| Map<String, BettingResult> bettingResults = new LinkedHashMap<>(); |
There was a problem hiding this comment.
이부분을 잘 수정해 주셨네요! PR 본문에도 관련된 내용 적어주셔서, 제 의견도 남기겠습니다.
우선 어떤 메소드의 동작 수정시, 의도된 기존 동작에 영향을 줄 수 있다는 점에 공감합니다.
이부분의 영향도를 체크하기 위해, 우선 아래 단위테스트 작업이 선행되어야 합니다.
- 해당 메소드의 성공 테스트 존재 (로직 변경되었을때 테스트가 실패하는지 확인해서, 정상동작 여부 확인)
- 해당 메소드의 예외 테스트 존재 (기존과 검증하는 로직이 같은지 다양한 예외 케이스 확인)
다만 이러한 테스트 및 팀 내 코드 리뷰가 선행된다면, 기존 메소드를 수정하는 것 자체에는 문제 없다고 생각합니다.
말씀주신대로 기능이 수정될때마다 새로운 메소드가 기존 메소드를 호출하게 할 수는 없으니까요.
다만 메소드 하나는 명확한 작은 단위의 책임을 가져야 하기 때문에,
예를 들어 기존 기능에 새로운 기능이 추가되면 새로 추가된 기능을 별도 메소드로 구현하고, 기존 메소드에서 새로 운 메소드를 호출하게 하는 등의 방법이 더 모범적이겠지요.
이번 작업에서는 베팅 기능이 추가되었고, 이를 기존 기능 + 베팅 기능을 수행하는 메소드를 만들어서 해결했기 때문에 매우 좋은 방향이라고 생각됩니다.
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class MoneyTest { |
There was a problem hiding this comment.
Money를 생성 후 getValue 정상 수행되는지 확인하는 테스트 추가해주세요.
추가로, 숫자가 0인 경우에 대해서도 추가하면 좋을 것으로 생각됩니다.
|
|
||
| public static BettingResult from(int betAmount, MatchResult matchResult, boolean isBlackJack) { | ||
| if (matchResult == MatchResult.WIN && isBlackJack) { | ||
| return new BettingResult((int) (betAmount * BLACKJACK_MULTIPLIER)); |
There was a problem hiding this comment.
(int) (betAmount * BLACKJACK_MULTIPLIER) 부분에서 연산 및 casting이 모두 수행되고 있는데요,
이부분을 별도 메소드로 분리해도 좋을듯 합니다.
include42
left a comment
include42
left a comment
There was a problem hiding this comment.
안녕하세요 은오, 리뷰어 카프카입니다.
작업 잘 수행해주셨네요! 기존에도 코드 좋았는데,
이번에 적은 수정으로도 기능 잘 구현해주셔서 매우 좋았습니다.
PR에 적어주신 질문 내용도 좋아서, 저도 함께 고민해볼 수 있었네요. 💯
수정할 사항이 몇 가지 있어서, 코멘트 남겨두었습니다.
궁금하신 내용 있으시면 편하게 코멘트 남겨주세요!
작업하시느라 고생 많으셨습니다 👍
3 participants
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
안녕하세요 카프카!
이번 step2에서는 먼저 최대한 본래의 기능을 그대로 가져가며 기능만 추가하는 방식으로 진행하려고 했습니다.
step2 기능을 구현하면서 기존 로직을 수정하는 것이 좋을지, 아니면 그대로 두고 재사용하는 것이 좋을지에 대해서도 고민이 있었습니다. 현재 구현에서는 step1에서 사용하던 calculateResults() 메서드는 수정하지 않고 그대로 유지하고, step2에서는 calculateBettingResults()라는 새로운 메서드를 만들어 내부에서 기존 로직을 재사용하는 방식으로 구현했습니다.
이렇게 한 이유는 기존 로직을 수정하게 되면 step1에서 의도했던 동작이 영향을 받을 수 있다는 점이 걱정되었기 때문입니다. 가능하면 기존 기능의 안정성을 유지하면서 새로운 기능만 확장하는 방향으로 구현해보고 싶었습니다. 다만 한편으로는 기능이 추가될 때마다 새로운 메서드가 계속 생기게 되는 구조가 되는 것은 아닐지, 또는 기존 메서드를 리팩토링하여 하나의 흐름으로 정리하는 것이 더 나은 방식인지에 대한 고민도 들었습니다.
보통 실무에서는 기능 확장 시 기존 메서드를 그대로 두고 재사용하는 방식이 일반적인지, 아니면 기능이 추가되는 시점에 기존 로직을 함께 리팩토링하여 구조를 다시 정리하는 편인지 궁금합니다.
Player가 베팅 금액을 어떻게 관리해야 하는지에 대해서도 고민이 있었습니다. 처음에는 Player 생성 시 Money를 함께 받아 객체를 불변으로 유지하는 방식을 생각했습니다. 하지만 이 방식을 적용하려고 하니 Player 생성자가 변경되면서 Players 객체까지 수정이 필요해졌고, 생각보다 수정 범위가 커지는 문제가 있었습니다. 또한 현재 구조에서는 Player 생성 시점에 반드시 베팅 금액을 알고 있는 것도 아니라는 점도 고민이었습니다.
그래서 우선은 Money 필드를 Optional로 두고, 처음에는 베팅 금액이 없는 상태로 두었다가 bet() 메서드를 통해 한 번만 설정할 수 있도록 검증 로직을 추가하는 방식으로 구현했습니다. null을 직접 사용하는 것보다는 Optional을 사용하는 것이 의도를 조금 더 명확하게 표현할 수 있을 것 같다고 판단했습니다.
다만 이 방식은 결국 Player의 상태가 변경 가능한 구조가 된다는 점에서 조금 신경이 쓰입니다. 그래서 구조를 조금 더 수정하더라도 생성자에서 Money를 받아 불변 객체로 유지하는 방향으로 리팩토링하는 것이 더 좋은 선택인지, 아니면 현재처럼 Optional과 검증 로직으로 상태 변경을 제어하는 방식도 충분히 괜찮은 접근인지 의견을 듣고 싶습니다.