[Currenjin] Baseball #4
Conversation
currenjin
requested review from
taiyoung12,
Heonbyeong,
leeMK09,
gyeongjae-ham and
hongxeob
| import org.gonza.javaplayground.baseball.PlayResult; | ||
| import org.gonza.javaplayground.util.NumberConverter; | ||
| import org.gonza.javaplayground.util.RandomNumberGenerator; | ||
| import org.gonza.javaplayground.util.Validator; | ||
| import org.springframework.boot.autoconfigure.SpringBootApplication; | ||
|
|
||
| @SpringBootApplication | ||
| public class JavaPlaygroundApplication { |
There was a problem hiding this comment.
메인 클래스에서 너무 많은 책임을 하고 있는거 같습니다!~
java-playground/src/main/java/org/gonza/javaplayground/JavaPlaygroundApplication.java
Outdated
Show resolved
Hide resolved
| List<Integer> numbers = new ArrayList<>(); | ||
| String[] strings = string.split(""); | ||
|
|
||
| for (String s : strings) { | ||
| numbers.add(Integer.parseInt(s)); | ||
| } | ||
|
|
||
| return numbers; |
There was a problem hiding this comment.
어떤 측면에서 그렇게 생각하시나요?
문자열을 숫자로 변경해주는 역할로는 한 가지 역할만 하고있는 듯 해서요!
There was a problem hiding this comment.
개념적으로는 그렇지만 그 행위를 하기 위해서 스트링을 쪼개고, 반복하고 그 안에서 리스트에 넣고..리스트에 넣기위해 숫자로 변환하고 등등의 작업을 하고 있는거 같아요~ 그게 변환기가 모두 다 가질 책임과 역할 일까요?
java-playground/src/main/java/org/gonza/javaplayground/util/NumberConverter.java
Outdated
Show resolved
Hide resolved
| System.out.print("숫자를 입력해 주세요 : "); | ||
| Scanner scanner = new Scanner(System.in); | ||
| List<Integer> numbers = NumberConverter.convertBy(scanner.nextLine()); | ||
| validate(numbers); |
There was a problem hiding this comment.
어떤걸,어떻게 validate 한다는건지 들어가면 좋을거 같아요.
| public BallStatus play(Ball ball) { | ||
| if (ball.position == this.position && ball.number == this.number) { | ||
| return BallStatus.STRIKE; | ||
| } | ||
|
|
||
| if (ball.number == this.number) { | ||
| return BallStatus.BALL; | ||
| } | ||
|
|
||
| return BallStatus.NOTHING; |
| public enum BallStatus { | ||
| BALL, STRIKE, NOTHING; | ||
|
|
||
| public boolean isNotNothing() { |
There was a problem hiding this comment.
메서드명이랑 상태가 헷갈리는거 같은데 어떤 용도일까요? (부정+부정의 의미라서 그런거 같아요)
| public static List<Integer> generate(int size, int start, int end) { | ||
| Set<Integer> set = new HashSet<>(); |
There was a problem hiding this comment.
static 메서드인데 Set은 메서드내에서 생성해주는 이유가 있을까요?
There was a problem hiding this comment.
A collection that contains no duplicate elements. More formally, sets contain no pair of elements e1 and e2 such that e1.equals(e2), and at most one null element. As implied by its name, this interface models the mathematical set abstraction.
중복 요소가 없는 컬렉션입니다. 더 공식적으로, 집합에는 e1.e2와 e2가 같은 요소 쌍이 포함되지 않으며, 최대 하나의 널 요소만 포함됩니다. 이름에서 알 수 있듯이, 이 인터페이스는 수학적 집합 추상화를 모델링합니다.
중복되지 않은 수로 number를 generate 하기 위함입니다.
| } | ||
|
|
||
| public static Boolean validNotDuplicate(List<Integer> numberList) { | ||
| return numberList.size() == numberList.stream().distinct().count(); |
There was a problem hiding this comment.
객체지향 생활체조에 한 라인엔 한 . (점) 만 있도록 하는 것을 지향합니다!
There was a problem hiding this comment.
@hongxeob 여기는 stream이라서 위배되어 보이진 않는데 어떠신가요? 해당 규칙은 객체안의 객체의 기능을 끌어다가 쓰지 말라는 규칙으로 이해하고 있습니다!
There was a problem hiding this comment.
스트림이어도 한 줄로 늘어뜨리는 것 보다, 적절하게 개행해서 체이닝 하는게 좋다고 생각합니다!
ktlint 쓸 때 파라미터에 2개 이상 들어가면 한 줄씩 분리 해주는거처럼요!
| import org.junit.jupiter.api.DisplayName; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class BallTest { |
There was a problem hiding this comment.
모든 테스트 전반적으로 실패(예외) 케이스 테스트도 있으면 좋을거 같아요!
| } | ||
|
|
||
| public static Boolean validNotDuplicate(List<Integer> numberList) { | ||
| return numberList.size() == numberList.stream().distinct().count(); |
There was a problem hiding this comment.
@hongxeob 여기는 stream이라서 위배되어 보이진 않는데 어떠신가요? 해당 규칙은 객체안의 객체의 기능을 끌어다가 쓰지 말라는 규칙으로 이해하고 있습니다!
| public PlayResult play(List<Integer> targetBallList) { | ||
| Balls targetBalls = new Balls(targetBallList); | ||
| PlayResult playResult = new PlayResult(); | ||
|
|
||
| this.ballList.forEach(ball -> { | ||
| BallStatus ballStatus = targetBalls.play(ball); | ||
| playResult.report(ballStatus); | ||
| }); | ||
|
|
||
| return playResult; | ||
| } |
There was a problem hiding this comment.
Balls가 게임을 진행시키는 부분이 좀 어색하게 느껴지네요 Balls가 Ball들의 일급컬렉션이면 Balls를 가지고 게임을 진행하는 친구는 다른 친구가 아닐까요?
| private final int position; | ||
| private final int number; |
There was a problem hiding this comment.
오우 저는 BallNumber를 하나의 객체로 봤는데 이렇게 세분화하고 position을 가질 수도 있겠네요 👍👍👍👍👍
| @Test | ||
| @DisplayName("1에서 9 사이의 숫자가 생성된다") | ||
| void range() { | ||
| List<Integer> randomNumbers = RandomNumberGenerator.generate(3, 1, 9); | ||
| randomNumbers.forEach(number -> { | ||
| assertThat(number).isGreaterThan(0); | ||
| assertThat(number).isLessThan(10); | ||
| }); | ||
| } |
There was a problem hiding this comment.
랜덤성의 데이터도 제어해서 테스트할 수 있어야 한다고 생각하는데 어떠신가요?
| } | ||
|
|
||
| private static Balls getBalls(Balls balls, Boolean start, PlayResult result) { | ||
| if (start && result.isGameEnd(END_NUMBER)) { |
There was a problem hiding this comment.
이 부분은 반대에 경우에 먼저 리턴을 하고 이후에는 이미 모두 검증된 값들이 인자로 들어왔다고 가정하고 작성하는건 어떤가유?
There was a problem hiding this comment.
이왕 Ball 이라는 객체를 만들었으니 해당 객체에게 가져와달라고 하는건 어떨까유?
|
|
||
| private static boolean isRestart() { | ||
| System.out.println("다시 시작하시겠습니까?"); | ||
| System.out.println("재시작 : 1"); |
There was a problem hiding this comment.
JavaPlaygroundApplication 이 너무 많은 역할을 하는 것 같슴다
- 값 출력
- 값 입력
- 값 검증
- 시작, 재시작, 종료 판단
| } | ||
|
|
||
| public BallStatus play(Ball ball) { | ||
| if (ball.position == this.position && ball.number == this.number) { |
There was a problem hiding this comment.
해당 조건문이 눈에 잘 안들어오는데 뭔가 명시적으로 읽을 수 있을만한 단계로 추상화하는건 어떤가유?
| private int ball = 0; | ||
| private int strike = 0; | ||
|
|
||
| public void report(BallStatus ballStatus) { |
There was a problem hiding this comment.
report 메서드는 이름만 때고보면 보고서에 관련된 역할을 하는건가? 뭔가를 출력하는건가? 라는 생각을 했는데
실제 코드는 자신의 상태를 변경하는 코드가 들어있어서 이름이 명시적으로 맞지 않는 듯 합니다
|
|
||
| class BallsTest { | ||
| @Test | ||
| void _1_Ball_0_Strike() { |
There was a problem hiding this comment.
BallTest 는 "@DisplayName" 으로 테스트를 설명해줬는데 여기서는 왜 없는걸까유?
넣을거면 넣고 안넣을거면 안넣는게 좋을 것 같슴다
@gonza-st/gonza 리뷰해주십쇼 여러분들