🚀 2단계 - 사다리(생성) by jeounpar · Pull Request #1963 · next-step/java-ladder

jeounpar · 2023-11-29T06:17:42Z

안녕하세요! 사다리 미션 2단계 리뷰 요청드립니다!

testrace

안녕하세요 정규님
몇 가지 코멘트 남겨두었는데 확인해 주시고 다시 리뷰 요청 부탁드려요

testrace · 2023-11-29T09:04:29Z

src/main/java/nextstep/ladder/model/Ladder.java

+    public Ladder(LadderHeight heightOfLadder, PersonCount countOfPerson, Random random) {
+        this.lines = new ArrayList<>();
+        IntStream.range(0, heightOfLadder.getHeightOfLadder())
+            .mapToObj(line -> new Line(countOfPerson.getCountOfPerson(), random))
+            .forEach(this.lines::add);
+    }


생성자에 로직이 포함되었네요
링크를 참고해 보시면 좋을 것 같습니다

IntStream 은 for문으로 대체할 수 있을 것 같은데요
IntStream 으로 Line 객체들을 생성하신 이유가 궁금합니다

Random을 인자로 받으면 멱등성이 지켜지지 않는 것 같아요
랜덤을 사용하더라도 인터페이스를 만들어서 Ladder는 인터페이스에 의존하도록 변경하면 어떨까요?

testrace · 2023-11-29T09:04:35Z

src/main/java/nextstep/ladder/model/LadderHeight.java

+
+    private static final int MIN_HEIGHT = 1;
+
+    private final Integer heightOfLadder;


wrapper 타입은 NPE가 발생할 수 있습니다
꼭 써야하는 이유가 아니라면 원시 타입을 사용하면 어떨까요?

testrace · 2023-11-29T09:04:52Z

src/main/java/nextstep/ladder/model/LadderService.java

+import java.util.List;
+import java.util.Random;
+
+public class LadderService {


사용하지 않는 주석, 코드(import 포함)은 제거해 주세요

testrace · 2023-11-29T09:05:56Z

src/main/java/nextstep/ladder/model/Line.java

+    public Line(int countOfPerson, Random random) {
+        this.points = new ArrayList<>();
+        this.points.add(random.nextBoolean());
+        IntStream.range(0, countOfPerson - 2)
+            .forEach(index -> initPoint(index, random.nextBoolean()));
+        validateLine(countOfPerson);
+    }


여기도 생성자 내부에 로직이 있네요
인자 countOfPerson 은 PersonCount 타입을 활용하면 어떨까요?

가로라인은 겹칠 수 없다는 요구사항은 Line의 책임일 것 같은데요
생성자에 로직이 있어서 요구사항을 검증할 수 없을 것 같아요
요구사항을 검증할 수 있는 구조를 고민해 보시면 좋을 것 같아요

testrace · 2023-11-29T09:08:08Z

src/main/java/nextstep/ladder/model/Line.java

+        if (this.points.get(index) && nextBoolean || this.points.get(index) && !nextBoolean
+            || !this.points.get(index) && !nextBoolean) {
+            this.points.add(false);
+            return;
+        }
+        if (!this.points.get(index) && nextBoolean) {
+            this.points.add(true);
+        }


getter가 너무 많이 사용되고 있는 것 같아요
메시지를 보내서 해결할 수 없는지, 메시지를 보낼 객체를 도출할 수 없는지 고민해 보시면 좋을 것 같아요

testrace · 2023-11-29T09:10:04Z

src/main/java/nextstep/ladder/model/Name.java

+    @Override
+    public String toString() {
+        return IntStream.range(0, MAX_LENGTH - name.length())
+            .mapToObj(i -> SPACE)
+            .collect(Collectors.joining("", name, ""));
+    }


출력을 위해서 공백을 추가하는 로직을 출력의 책임 아닐까요?
참가자가 많은 경우 사다리의 시작 위치에 이름이 출력되고 있지 않아요

testrace · 2023-11-29T09:15:23Z

src/test/java/nextstep/ladder/model/LadderHeightTest.java

+    @Test
+    @DisplayName("사다리의 높의는 1 이상이다")
+    void ladder_height() {
+        Assertions.assertThatThrownBy(() -> {
+            new LadderHeight(0);
+        }).isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("높이는 1 이상 이어야 합니다.");
+
+        Assertions.assertThatThrownBy(() -> {
+                new LadderHeight(-1);
+            }).isInstanceOf(IllegalArgumentException.class)
+            .hasMessage("높이는 1 이상 이어야 합니다.");
+    }


경계값인 0으로 검증했는데 추가로 -1도 검증하신 이유가 있으신가요?

testrace · 2023-11-29T09:15:58Z

src/test/java/nextstep/ladder/model/LadderTest.java

+    @Test
+    void ladder() {
+        Ladder ladder = new Ladder(new LadderHeight(5), new PersonCount(4), new AlwaysReturnTrue());
+    }


단위 테스트 작성이 누락되었네요

testrace · 2023-11-29T09:18:21Z

src/test/java/nextstep/ladder/model/LineTest.java

+    @Test
+    @DisplayName("사다리의 라인 하나를 생성한다")
+    void line() {
+        int countOfPerson = 5;
+        Line line = new Line(countOfPerson, new AlwaysReturnTrue());
+
+        assertThat(line.getPoints()).hasSize(countOfPerson - 1)
+            .containsExactly(true, false, true, false);
+    }


DisplayName과 테스트 내용이 일치 하지 않네요
테스트의 의도와 테스트 내용을 일치 시켜주시면 좋을 것 같아요

추가로 가로 라인은 겹칠 수 없다는 요구사항이 있는데 이 검증은 어떻게 할 수 있을까요?

testrace · 2023-11-29T09:19:22Z

src/test/java/nextstep/ladder/view/OutputViewTest.java

+class OutputViewTest {
+
+    @Test
+    void print_ladder() {
+        Ladder ladder = new Ladder(new LadderHeight(10), new PersonCount(5),
+            new RandomTrueOrFalse());
+        OutputView outputView = new OutputView();
+//        outputView.printLadder(ladder);
+
+    }


테스트가 필요하다면 테스트를 마저 작성해 주시면 좋을 것 같아요

jeounpar added 6 commits November 28, 2023 22:23

refactor : step1 리뷰 반영

42a5899

feat : model, view, controller 초기 코드 생성

2942a15

feat : 사다리 타기가 정상적으로 동작하기 위해 라인이 겹치지 않도록 한다

05cd1ca

feat : 사다리를 출력 한다

a613e83

feat : LadderHeight, PersonCount 클래스 추가

0e440fd

feat : TODO 모든 기능 구현 완료

684953b

testrace requested changes Nov 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 2단계 - 사다리(생성)#1963

🚀 2단계 - 사다리(생성)#1963
jeounpar wants to merge 6 commits intonext-step:jeounparfrom
jeounpar:step2

jeounpar commented Nov 29, 2023

Uh oh!

testrace left a comment

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

testrace Nov 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		private static final int MIN_HEIGHT = 1;

		private final Integer heightOfLadder;

Conversation

jeounpar commented Nov 29, 2023

Uh oh!

testrace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants