Conversation
seong-wooo
left a comment
seong-wooo
left a comment
There was a problem hiding this comment.
구현에 대한 것보다는 객체 지향적인 설계에 초점을 두어서 개발해보면 좋을 것 같아요!
| public static List<Integer> generate() { | ||
| List<Integer> computer = new ArrayList<>(); | ||
| while (computer.size() < THE_NUMBER_OF_RANDOM_NUMBER) { | ||
| int randomNumber = Randoms.pickNumberInRange(1, 9); |
There was a problem hiding this comment.
랜덤 숫자의 범위가 1부터 9까지라는 것은 GameNumber의 로직인 것 같은데
이걸 어떻게 해결할 수 있을까요?
예를 들어 1부터 5까지로 범위가 변경되면 RandomNumberGenerator와 GameNumber 둘 다 수정해야하나요,,
There was a problem hiding this comment.
RandomNumberGenerator와 GameNumbers에 있는 3이라는 숫자도 마찬가지입니다.
3이라는 숫자는 GameNumbers에서 알고있는 핵심 필드인데, RandomNumberGenerator에서도 필드로 선언해서 사용하고 있네요.
캡슐화가 지켜지지 않고 있는 느낌..
There was a problem hiding this comment.
필드만 private로 선언한다고 캡슐화하는게 아니었네요. 수정 완료했습니다!
ray-yhc
left a comment
ray-yhc
left a comment
There was a problem hiding this comment.
리뷰를 너무 늦게 올려드린 것 같아 죄송해요 ㅜㅜ
이미 주말동안 충분히 수정을 거치신 것 같네요. 코드가 너무 깔끔해서 읽기 편했습니다
다만 테스트 코드를 더 활용해 보셔도 좋을 것 같아요.
특히 GameNumbers의 getComparingResult가 게임의 점수를 판정하는 가장 중요한 코드라고 생각하는데, 이 부분에 대한 테스트코드가 없어서 아쉽습니다!
수고 많으셨습니다!
| private void play(List<Integer> computer) { | ||
| String input = Input.readGameNumber(); | ||
| GameNumbers gameNumbers = GameNumberGenerator.generate(input); | ||
| GameResult gameResult = GameResult.from(gameNumbers.getComparingResult(computer)); | ||
| Output.printResult(gameResult.getValue()); | ||
| if (gameResult.isEnd()) { | ||
| return; | ||
| } | ||
| play(computer); | ||
| } |
There was a problem hiding this comment.
재귀를 이용해 play를 진행하면 메모리 낭비가 있을 것 같아요.
만약 사용자가 1억번 틀려서 게임이 계속된다면, 사용자의 input과 gameNumbers, gamdResult가 1억개씩 남아있지 않을까요?
There was a problem hiding this comment.
메모리에 대해서는 신경쓰지 않고 구현했는데, 앞으로는 신경써야겠습니다!
수정 완료
| private void askPlayAgain() { | ||
| GameStatus gameStatus = new GameStatus(Convertor.toInteger(Input.readGameStatus())); | ||
| if (gameStatus.isRestart()) { | ||
| start(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
여기도 위와 비슷한 맥락으로,
사용자가 1억번 게임을 재시작한다면, 불필요한 gameStatus가 많이 남아있을 것 같아요!
There was a problem hiding this comment.
해당 부분은 '게임 실행'과 '게임 재실행 여부 질문'의 역할을 분리했기 때문에 재귀 사용 방식을 유지했습니다!
| private static final int BALL = 0; | ||
| private static final int STRIKE = 1; | ||
| private static final int NOTHING = 2; |
There was a problem hiding this comment.
GameNumbers와 GameResult에서 comparingResult 배열을 이용하셨는데, 인덱스를 공유하는 것 보다 이렇게 클래스를 만들어서 공유하는게 더 좋지 않을까 생각합니다!
@Getter @Setter
public class ComparingResult {
private int strike;
private int ball;
private int nothing; // or boolean
}There was a problem hiding this comment.
값 객체를 만들면 GameNumber가 GameNumbers 외부로 노출된다고 생각해서 strike, ball 변수 선언으로 수정했습니다.
그런데 코멘트를 달기 위해 윤호님 리뷰를 다시 읽어보니 제가 잘못 이해했었네요ㅜㅜ
값 객체를 생성하는 것도 좋은 방법 같습니다! 이런 경우는 변수와 값 객체 사용 중 어느 방식이 더 좋을까 고민해보면 좋을 것 같네요:)
| } | ||
|
|
||
| public boolean isEnd() { | ||
| return value.contains(GAME_END_CONDITION); |
There was a problem hiding this comment.
개인적인 의견으로는, 문자열을 비교하는 것이 비효율적이라고 생각해요.
private static final boolean END;위와 같이 static 변수를 둔 뒤에
29열에서 END = true; 를 하는게 어떨까요?
There was a problem hiding this comment.
전체적인 로직이 수정되면서 살짝 방식은 달라졌지만, GameResult에서 게임 종료 여부(isEnd)를 갖도록 수정했습니다!
There was a problem hiding this comment.
GameResult는 baseball/domain에 있는데, 테스트는 baseball/util에 있네요
패키지 경로가 통일되면 좀더 보기 편할 것 같아요!
3 participants
이번 미션은 평소처럼 바로 설계하지 않고, Application에서 모든 기능 구현 후 리팩토링하는 방식으로 진행해봤습니다.
이 과정에서 테스트 코드를 작성하지 않아 중간에 에러가 발생하기도 했습니다🥲
(테스트 코드 작성법을 까먹어서 테스트 작성을 뒤로 밀어서 발생한 문제)
리팩토링 후 제 코드를 보니 유틸 클래스가 많았습니다!
게임을 실행할 때마다 객체를 생성할 필요 없다고 생각되는 클래스들은 유틸 클래스로 만들었는데, 개선해야 된다고 생각되는 부분은 코멘트 남겨주세요ㅎㅎ