Conversation
seondongpyo
left a comment
seondongpyo
left a comment
There was a problem hiding this comment.
2단계 미션 잘 작성해주셨네요 👍
다만 핵심 도메인 클래스들에 대한 단위 테스트가 누락되어 있는 것 같아요 😢
몇 가지 코멘트 드렸는데 확인 후 다시 리뷰 요청을 부탁 드리겠습니다 🔥
| import java.util.List; | ||
| import java.util.Scanner; | ||
|
|
||
| public class InputView { |
There was a problem hiding this comment.
InputView도 PrintvView와 같이 정적 메서드만을 갖는 유틸리티 클래스로 만들어보면 어떨까요?
| public List<String> names; | ||
| public int ladderHeight = 0; | ||
|
|
||
| public void saveInput() { |
There was a problem hiding this comment.
- 하나의 메서드에서 참가자 이름도 입력 받고 사다리 높이도 입력을 받는 두 가지 기능을 수행하고 있는데요,
각각의 기능별로 메서드를 분리해보면 어떨까요? - 사용자로부터 입력 받는 값들을 내부에 상태로 가지기 보다는 메서드에서 바로 반환해도 충분할 것 같습니다.
|
|
||
| import java.util.List; | ||
|
|
||
| public class PrintView { |
There was a problem hiding this comment.
정적 메서드만을 포함하는 유틸리티 클래스이니 인스턴스를 생성하지 않아도 사용이 가능합니다만,
누군가는 실수로 생성자를 호출하여 인스턴스를 생성하려고 할 수도 있으니,
private 생성자를 추가하여 불필요한 인스턴스화를 막아보면 어떨까요?
| int padding = 5 - name.length(); | ||
| String paddedName = name + " ".repeat(padding); | ||
| return paddedName; |
There was a problem hiding this comment.
해당 부분을 별도의 메서드로 추출해보는 건 어떨까요?
메서드 이름을 통해 어떤 로직을 수행하는지를 보다 쉽게 표현해볼 수 있지 않을까 생각합니다.
| } | ||
|
|
||
| private static void printRow(boolean point){ | ||
| if(point){ |
There was a problem hiding this comment.
이번 과정에서는 if-else를 사용하지 않기로 했으니, early return 방식으로 바꿔보시면 좋을 것 같아요.
https://jheloper.github.io/2019/06/write-early-return-code/
|
|
||
| public Line(int countOfPerson) { | ||
| IntStream.range(0, countOfPerson - 1) | ||
| .mapToObj(i -> isInValidPosition(i) ? false : RandomValueGenerator.generate()) |
There was a problem hiding this comment.
삼항 연산자 또한 if-else의 축약형이기 때문에, 다른 방식으로 작성해보시면 좋을 것 같습니다.
| public static void main(String[] args) { | ||
| InputView inputView = new InputView(); | ||
| inputView.saveInput(); | ||
| System.out.println(inputView.names); |
There was a problem hiding this comment.
중간에 사다리게임 참가자 이름을 출력하는 요구사항은 없기 때문에 제거하셔도 될 것 같습니다.
| import java.util.stream.Collectors; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| public class Ladder { |
There was a problem hiding this comment.
TDD를 중점적으로 학습하는 과정인데, 핵심 도메인 클래스들에 대한 단위 테스트가 작성되어 있지 않네요 😢
Ladder에 대한 단위 테스트를 작성해보면 어떨까요?
| public class Ladder { | ||
| private List<Line> lines; | ||
|
|
||
| public Ladder(int height, int nameSize) { |
There was a problem hiding this comment.
만약 사다리 높이 값이 음수로 전달된다면 문제가 생길 수 있을 것 같아요.
| import java.util.List; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| public class Line { |
넵 view에서 도메인 객체 내부의 값을 출력하기 위함이기 때문에 사용하셔도 괜찮습니다. |
2 participants
안녕하세요 리뷰어님~
최근에 일이 바빠서 pr을 늦게올리게 되었어요..ㅜㅜ
리뷰 받고싶은 부분