-
Notifications
You must be signed in to change notification settings - Fork 6
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
[박이슬] 숫자 야구 게임 구현 완료 #5
base: Yiseull
Are you sure you want to change the base?
Conversation
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.
안녕하세요 이슬님! 전체적으로 너무 잘 짜주신 것 같습니다! 궁금했던 부분에 대해서 코멘트를 따로 달아놓았고, 이슬님의 PR이 업데이트 되면 그 때 따로 궁금하신 점에 대해서 추가로 코멘트 달도록 하겠습니다!
객체지향적으로 구현하기 위해 많은 부분 고민을 하시고, 새롭게 배운 부분도 적용하신 모습이 너무 보기 좋았습니다! 바쁜데 이것까지 하시느라 고생많으셨어요! 그래도 고생 끝에 행복이 오니 같이 화이팅합시다! 👍
객체에 대한 역할과 책임이 제대로 분리되었는지 궁금합니다!🧐 SRP원칙을 위배한 객체가 있을까요~??
- 전체적으로 깔끔하다는 느낌을 많이 받았습니다! 잘 분리하신 것 같습니다!!
MVC 패턴으로 설계를 해봤습니다!! MVC 패턴을 잘 적용한 것일까요?? 또한 MVC 패턴에 대해 찾아봐도 명확하게 이해가지 않는데 참고할만한 좋은 자료 있으면 공유 부탁드립니당 :)
- 저희 이번 과제에 대해서 대부분 MVC 패턴을 적용하려고 하셨던 것 같더라구요!!
- 저는 Model 은 진짜 POJO 가 있어야 하는 부분, View는 사용자에게 보여주기 위한 관점, Controller가 클라이언트가 무엇인가를 요청하는 부분으로 Model 과 View를 연결해주는 부분으로 이해를 하고 있습니다. 이 부분은 스터디때 얘기를 나눠보아요! 😄
- 정리 자료
숫자 야구 결과를 계산하는 로직이 어디에 있어야 하나 고민을 많이 했습니다...🤨 컴퓨터가 계산해야하나? 아니면 BaseballNumbers 에서 처리를 다 해야하나? 고민하다 저는 따로 숫자 야구 결과만 계산하는 클래스를 따로 만들었습니다! 숫자 야구 게임 계산 책임이 어디에 있어야 하는지 다른 분들의 의견도 들어보고 싶습니다!!
- 저도 비슷한 관점으로 고민을 했었던 것 같아요. 저는 공 자체가 서로를 비교할 수 있도록 하고, 심판이 판결을 할 때 공끼리 비교를 하면서 그 결과를 반환하는 형태로 했어요!
BaseBallNumbers
를 저장하는 일급 컬렉션이 있다면 해당 클래스는 데이터를 담기 위한 용도라고 생각합니다! 거기서 계산하는 로직은 따로 빼는게 좋을 것 같다는 생각이에요!
일급 컬렉션을 사용하여서 숫자 야구 게임에 대한 결과를 계산할 때 어려움이 있었습니다... getter 메소드를 통해서 불러와서 다시 List로 변환하는 등등...(매우 복잡, 설명 어렵)🤦🏻♀️ 일급 컬렉션은 상태와 행위를 한 번에 관리하기 위한 것이라고 알고 있습니다! 그렇다면 숫자 야구 결과 계산도 일급 컬렉션인 BaseballNumbers에서 관리하는 게 맞는 것일까요?! + 제가 일급 컬렉션을 잘 몰라서 제대로 사용하지 못하는 것일까요???
- 일급 컬렉션을 반환한다면 Collections.unmodifable... 을 찾아보시면 좋을 것 같습니다! 그리고 일급 컬렉션에 대해 향로님이 잘 설명해주신 글이 있으니 공유해놓도록 하겠습니다!
- 일급 컬렉션
이번에 처음 TDD를 해보았습니다! 저는 도메인 위주로 TDD를 먼저 진행하였습니다. 입출력에 제한을 받지 않고 TDD를 통해 기능 하나씩 구현해나갔습니다! 후에 기능 명세서에 작성한 기능을 모두 작성하고 나서, 입출력에 대해서는 TDD 방식으로 진행하지 않고 코드를 작성하였습니다!! 입출력에 대해서도 TDD를 진행해야 하나요?? 모든 기능을 TDD로 해야하는지, 중요 기능에 대해서만 TDD를 하는 것인지 이 부분에 대해서 궁금합니다😲😲
- 저도 도메인에 대해서는 TDD로 진행을 했습니다! 하지만 입출력 관련해서는 TDD로 진행하지 않았어요! 우선 만들기 복잡하다는 관점도 있고... 사실 주성님이 해보라고 하셨는데, 다음 자동차 경주때에 한 번 해볼까 생각중입니다! 😊
- 우선 중요 기능에 대해선 확실히 TDD로 하는게 좋은 것 같다는 생각이 들어요! 개인적으로는 TDD로 전체적인 맥락을 잡아놓고 짜니 구현하기 더 편리하기도 하더라구요!
|
||
public void run() { | ||
BaseballNumbers answers = BaseballNumbersGanerator.generateRandomNumbers(); | ||
while (true) { |
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.
true 말고 반복적인 것을 명시 할 수 있는 다른 방법은 없을까요!?
PrintView.printGameEndMessage(); | ||
PrintView.printMenu(); | ||
int menu = InputView.inputMenu(); | ||
if (!restartGame(menu)) { |
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.
여기까지 오게 되면 depth가 4가 넘는 것 같아요! 메서드 분리를 하거나 줄여야 할 것 같아요!
return false; | ||
} | ||
|
||
public boolean restartGame(int menu) { |
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.
매직 넘버는 enum으로 처리하는 건 어떤가요!?
} | ||
} | ||
|
||
public boolean isGameEnd(BaseballStatus status) { |
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.
간단하게 한 줄로 표현 할 수 있을 것 같아요!
List<BaseballNumber> playerNumberList = playerNumbers.getBaseballNumbers(); | ||
List<BaseballNumber> answerList = answers.getBaseballNumbers(); | ||
|
||
int strikes = (int) IntStream.range(0, NUMBERS_SIZE) |
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.
헉 저는 IntStream 생각 못하고 foreach로 돌렸는데 이런 방법이!! 잘 배워갑니다 😄
@@ -0,0 +1,27 @@ | |||
package com.nextstep.baseball.domain; | |||
|
|||
public record BaseballNumber(int position, int number) { |
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.
우와 이번 시간에 배운 record를 사용하셨군요! 바로 적용 리스펙! 그런데 position은 사용된 흔적이 보이지가 않아요!
} | ||
} | ||
|
||
public List<BaseballNumber> getBaseballNumbers() { |
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 static final int MIN = 1; | ||
public static final int MAX = 9; | ||
|
||
public static BaseballNumbers generateRandomNumbers() { |
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.
ThreadLocalRandom 을 사용하셨네요! 멀티쓰레드 상황을 염두한건가요!? 아니면 다른 이유가 있을까요!?
return integerToList(numbers); | ||
} | ||
|
||
private static List<Integer> integerToList(int number) { |
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.
이 부분에서 예외가 발생할 것 같은데 예외가 발생할 수도 있음을 명시하는 건 어떻게 생각하시나요!?
} | ||
|
||
private static List<Integer> integerToList(int number) { | ||
return Stream.of(String.valueOf(number).split("")) |
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.
저의 경우 integerToList라는 메소드가 여기 말고 BaseballNumbers 클래스 안에 있어도 되지 않았을까 싶어요! Input의 경우 그냥 정말 Input을 받아서 넘기는 책임을 가지고 있고 이제 BaseballNumbers 클래스 안에서 해당 문자열을 나누고, 검증하는 작업들이 모여있는게 더 책임을 분리한 것이라고 생각이 드는데 이슬님은 어떻게 생각하시나요?
@Test | ||
@DisplayName("자리 수가 3인지 검증") | ||
void validateNumbersSize() { | ||
assertThatCode(() -> BaseballNumbers.validateNumbersSize(List.of(1, 2, 3))) | ||
.doesNotThrowAnyException(); | ||
|
||
assertThatThrownBy(() -> BaseballNumbers.validateNumbersSize(List.of(1, 2, 3, 4))) | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessage("3자리 숫자여야 합니다."); | ||
} | ||
|
||
@Test | ||
@DisplayName("1부터 9까지의 수인지 검증") | ||
void validateNumbersRange() { | ||
assertThatCode(() -> BaseballNumbers.validateNumbersRange(List.of(1, 2, 3))) | ||
.doesNotThrowAnyException(); | ||
|
||
assertThatThrownBy(() -> BaseballNumbers.validateNumbersRange(List.of(1, 2, 10))) | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessage("1부터 9까지의 수여야 합니다."); | ||
} | ||
|
||
@Test | ||
@DisplayName("중복 숫자 검증") | ||
void validateNumbersDuplication() { | ||
assertThatCode(() -> BaseballNumbers.validateNumbersDuplication(List.of(1, 2, 3))) | ||
.doesNotThrowAnyException(); | ||
|
||
assertThatThrownBy(() -> BaseballNumbers.validateNumbersDuplication(List.of(1, 2, 2))) | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessage("중복된 수가 있습니다."); | ||
} | ||
} |
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.
지금 BaseballNumbers에서 검증과 관련된 메소드들이 전부 public으로 열어놓은 것이 테스트를 위해서 그렇게 하신걸까요?!
현재 코드를 보면 BaseballNumbers 클래스안에서만 사용되는 메소드들 같은데 private으로 변경하고 BaseballNumber 생성자로 생성할 때 문제되는 input 값을 예외마다 넣어줘서 hasMessage로 잡아줄 수 있지 않을까요?!
public static boolean validate(int number) { | ||
if (MIN <= number && number <= MAX) { | ||
return true; | ||
} | ||
throw new IllegalArgumentException("1부터 9까지의 수여야 합니다."); | ||
} |
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.
BaseballNumbers 클래스에서 입력받은 숫자에 대한 검증을 하는데 입력받은 숫자들이 split되어있으니까 해당 클래스에서 바로 검증하면 책임이 더 분리될 수 있지 않을까 생각이 드는데 이슬님은 어떻게 생각하시나요?!
import java.util.concurrent.ThreadLocalRandom; | ||
import java.util.stream.Collectors; | ||
|
||
public class BaseballNumbersGanerator { |
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.
Ganerator 오타가 있네요!
안녕하세요 이슬님~! 황창현입니다! ✅ PR 포인트 & 궁금한 점
코드 짜시느라 늦게 주무신 것 같은데 너무 고생많으셨어요! 멋져멋져 |
📌 과제 설명
📃 기능 명세
기능 명세서는 docs. 에 더 자세히 정리되어 있습니다.
👩💻 구현 내용
Domain
BaseballNumber
BaseballNumbers
BaseballCounter
BaseballStatus
✅ PR 포인트 & 궁금한 점
BaseballNumbers
에서 처리를 다 해야하나? 고민하다 저는 따로 숫자 야구 결과만 계산하는 클래스를 따로 만들었습니다! 숫자 야구 게임 계산 책임이 어디에 있어야 하는지 다른 분들의 의견도 들어보고 싶습니다!!BaseballNumbers
에서 관리하는 게 맞는 것일까요?! + 제가 일급 컬렉션을 잘 몰라서 제대로 사용하지 못하는 것일까요???