Step2 by bb-worm · Pull Request #2324 · next-step/java-ladder

bb-worm · 2025-04-05T12:25:02Z

https://edu.nextstep.camp/s/Ie5Dwep0/ls/UGd9q4uG

사다리 생성 미션 리뷰 요청합니다.

고민했던 건 아래와 같습니다.

Optional을 어디에 써야할지 감이 잘 오지 않습니다. 적용하기 좋은 곳이 있을까요?
Point 클래스를 만들어서 이웃한 Point와의 연결 유무를 가지도록 했습니다. 원래는 좌우로 연결 유무를 가지도록 하려고 했는데 너무 복잡해지는 거 같아서.. 일단 오른쪽 연결 유무만 갖도록 했습니다. 그런데 사용자가 이를 알고 사용해야 하니 뭔가 찜찜하더라고요. 더 좋은 방법이 있을까요?
그리고 위처럼 Point가 오른쪽 연결 유무를 갖도록 하니, 마지막 Point는 오른쪽에 이웃한 Point가 없어도 연결되는 경우가 있습니다. 처음에는 생성될 때 막으려고 했는데요, 그러면 필드 값을 set 하는 것 같아서 좀 아닌 거 같더라고요. 아직 구현하지는 않았지만 나중에 실행 단계에서는 마지막 위치인지 확인하는 조건절을 넣을 생각인데요. 애초에 생성 단계에서 막아주는 게 나을까요?
추가 고민점은 코드에 코멘트로 달아놓겠습니다.

- computer version 가져올 때 optional 사용

bb-worm · 2025-04-05T12:26:21Z

src/main/java/ladder/Main.java

+    private static void makeLadder() {
+        People people = InputView.inputPeople();
+        int ladderHeight = InputView.inputLadderHeight();
+        Ladder ladder = new Ladder(ladderHeight, people, (idx, preConnected) -> !preConnected && new Random().nextBoolean());


일부러 람다 사용해보려고 ConnectStrategy를 람다로 표현했습니다. 그런데 오히려 코드 읽기가 어려워지는 거 같아서.. 이런 경우엔 따로 클래스로 빼는 게 나을까요?

클래스도 좋고 변수로 선언해도 좋을 것 같아요

Suggested change

Ladder ladder = new Ladder(ladderHeight, people, (idx, preConnected) -> !preConnected && new Random().nextBoolean());

final ConnectStrategy defaultStrategy = (idx, preConnected) -> !preConnected && RANDOM.nextBoolean();

final Ladder ladder = new Ladder(ladderHeight, people, defaultStrategy);

Optional을 어디에 써야할지 감이 잘 오지 않습니다. 적용하기 좋은 곳이 있을까요?

null을 활용한 설계가 아니라면 억지로 사용하지 않으셔도 됩니다 😃
기능 목록을 정의하실 때 마지막 포인트는 null 이다. 라는 내용이 있었는데요.
실제로 null을 활용했다면 Optional을 활용할 수 있는 지점이 아닐까 싶어요.

testrace

안녕하세요 태룡님 😃
2단계 구현 잘해주셨네요 👍
커밋 흐름이 인상적이었습니다!
몇 가지 코멘트 남겨두었는데 확인해 주시고 다시 리뷰 요청해 주세요.

README.md

testrace · 2025-04-06T04:30:57Z

src/main/java/ladder/Main.java

+    private static void makeLadder() {
+        People people = InputView.inputPeople();
+        int ladderHeight = InputView.inputLadderHeight();
+        Ladder ladder = new Ladder(ladderHeight, people, (idx, preConnected) -> !preConnected && new Random().nextBoolean());


클래스도 좋고 변수로 선언해도 좋을 것 같아요

Suggested change

Ladder ladder = new Ladder(ladderHeight, people, (idx, preConnected) -> !preConnected && new Random().nextBoolean());

final ConnectStrategy defaultStrategy = (idx, preConnected) -> !preConnected && RANDOM.nextBoolean();

final Ladder ladder = new Ladder(ladderHeight, people, defaultStrategy);

Optional을 어디에 써야할지 감이 잘 오지 않습니다. 적용하기 좋은 곳이 있을까요?

null을 활용한 설계가 아니라면 억지로 사용하지 않으셔도 됩니다 😃
기능 목록을 정의하실 때 마지막 포인트는 null 이다. 라는 내용이 있었는데요.
실제로 null을 활용했다면 Optional을 활용할 수 있는 지점이 아닐까 싶어요.

testrace · 2025-04-06T04:32:32Z

src/main/java/ladder/domain/Ladder.java

+    public Ladder(int height, People people, ConnectStrategy connectStrategy) {
+        this.lines = IntStream.range(0, height)
+                .mapToObj(i -> new Line(people.size(), connectStrategy))
+                .collect(Collectors.toList());
+    }


생성자는 유효성 검증과 변수 초기화에 집중하고,
생성 로직은 정적 팩토리 메서드나 별도의 클래스로 분리하면 어떨까요?

public Ladder(List<Line> lines) { // validate this.lines = lines; } public static Ladder create(int height, People people, ConnectStrategy connectStrategy) { final List<Line> lines = ...; return new Ladder(lines); }

매번 생성자가 복잡해질 때마다 고민이 있었는데 말씀하신 것처럼 분리해보겠습니다

testrace · 2025-04-06T04:32:37Z

src/main/java/ladder/domain/Line.java

+import java.util.List;
+import java.util.Objects;
+
+public class Line {


Line 객체는 특별한 기능이 없는 것은 같아요
Points와 어떤 차이가 있나요?

엇 그러게요.. 작업하면서도 뭔가 이상하다 싶었는데 의미없이 사용하고 있었네요.
합쳐놓겠습니다!

testrace · 2025-04-06T04:32:42Z

src/main/java/ladder/domain/Line.java

+    public Line(List<Point> points) {
+        this.points = new Points(points);
+    }
+
+    public Line(int size, ConnectStrategy connectStrategy) {
+        this.points = new Points(size, connectStrategy);
+    }


주 생성자를 두어 호출하면 어떨까요?

public Line(Points points) { this.points = points; } public Line(List<Points> points) { this(new Points(points)); }

좋습니다!

testrace · 2025-04-06T04:33:00Z

src/main/java/ladder/domain/People.java

+import java.util.stream.Collectors;
+
+public class People {
+    private final static String DELIMITER = ",";


파싱과 관련된 로직은 view 또는 controller로 이동하면 어떨까요?
People 이라는 도메인은 구분기호에 따라 역할이 달라지지 않을 것 같아요.

말씀하신 것처럼 People이 구분자를 가지고 있을 필요가 없네요. 변경해두겠습니다.

testrace · 2025-04-06T04:33:04Z

src/main/java/ladder/domain/Person.java

+    private final String name;
+
+    public Person(String name) {
+        if (name.length() > 5) {


Suggested change

if (name.length() > 5) {

if (name.length() > NAME_MAX_LENGTH) {

testrace · 2025-04-06T04:33:09Z

src/main/java/ladder/domain/Point.java

+public class Point {
+    private final boolean rightConnected;


Point 클래스를 만들어서 이웃한 Point와의 연결 유무를 가지도록 했습니다. 원래는 좌우로 연결 유무를 가지도록 하려고 했는데 너무 복잡해지는 거 같아서.. 일단 오른쪽 연결 유무만 갖도록 했습니다. 그런데 사용자가 이를 알고 사용해야 하니 뭔가 찜찜하더라고요. 더 좋은 방법이 있을까요?

그리고 위처럼 Point가 오른쪽 연결 유무를 갖도록 하니, 마지막 Point는 오른쪽에 이웃한 Point가 없어도 연결되는 경우가 있습니다. 처음에는 생성될 때 막으려고 했는데요, 그러면 필드 값을 set 하는 것 같아서 좀 아닌 거 같더라고요. 아직 구현하지는 않았지만 나중에 실행 단계에서는 마지막 위치인지 확인하는 조건절을 넣을 생각인데요. 애초에 생성 단계에서 막아주는 게 나을까요?

좌우연결을 가진 상태로 끝까지 구현해 보셨어도 좋았을 것 같아요.

마지막 질문에 대한 답변을 드리자면 생성 단계에서 막아주는게 좋지 않을까 생각이 드는데요 😃
질문 내용으로 보아 여러 시도와 많은 고민 하신 것 같아 약간의 참고용 힌트를 드리면 어떨까 싶어요.
(힌트일뿐 정답이 아닙니다)

Point first = Point.first(); Point second = first.next(); Point third = second.next(); Point forth = third.last();

Point가 처음, 중간, 마지막에 대한 값을 스스로 결정할 수 있다면 어떨까요?

설명 감사합니다!
좌우연결을 가진 상태로 한 번 더 구현해보겠습니다..!

testrace · 2025-04-06T04:33:48Z

src/main/java/ladder/domain/Points.java

+    public void validate() {
+        IntStream.range(0, points.size()-1)
+                .filter(this::continuousConnected)
+                .findFirst()
+                .ifPresent(i -> {
+                    throw new IllegalArgumentException("각 포인트는 연속적으로 연결될 수 없음");
+                });
+    }


Point가 좌우연결을 가진 구조라면 유효성 검증도 단순화할 수 있을 것 같아요

new Point(true, true); // 생성 불가

testrace · 2025-04-06T04:38:22Z

src/test/java/ladder/LineTest.java

+    @Test
+    void 연속적으로_연결되면_예외() {
+        assertThatThrownBy(() -> new Line(4, (idx, preConnected) -> true)).isInstanceOf(IllegalArgumentException.class);


예외 테스트 👍
예외 메시지까지 함께 검증하면 좋을 것 같아요.

Point, Points, Ladder, Person 에 대한 단위 테스트도 추가하면 어떨까요?

보완해보겠습니다!

kakao-finn-h added 8 commits April 5, 2025 15:43

feat: 전체 테스트 코드 통과

0421b95

- computer version 가져올 때 optional 사용

chore: 메시지 보내는 메서드 사용

b1014ad

feat: 사다리 라인 생성

1702140

feat: 사다리 라인이 이웃한 곳과 이어져있도록 생성

3e3dfdc

feat: 사다리 라인은 연속적으로 이어져있지 않아야 함

dd877b8

feat: 라인의 각 포인트는 왼쪽/오른쪽 연결 유무를 갖음

400b0a9

refactor: 라인의 각 포인트는 오른쪽 연결 유무만 갖도록 변경

76cac9c

feat: 사용자 입력을 전달받아 사다리 생성 후 출력

348199e

bb-worm commented Apr 5, 2025

View reviewed changes

feat: 사용자 이름 길이는 최대 5자

7c642d4

testrace requested changes Apr 6, 2025

View reviewed changes

	Ladder ladder = new Ladder(ladderHeight, people, (idx, preConnected) -> !preConnected && new Random().nextBoolean());
	final ConnectStrategy defaultStrategy = (idx, preConnected) -> !preConnected && RANDOM.nextBoolean();
	final Ladder ladder = new Ladder(ladderHeight, people, defaultStrategy);

	if (name.length() > 5) {
	if (name.length() > NAME_MAX_LENGTH) {

Conversation

bb-worm commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

testrace left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

3 participants

bb-worm commented Apr 5, 2025 •

edited

Loading