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

sang5c · 2024-04-07T07:16:25Z

정말 오랜만에 사다리 게임을 작성해봤습니다.
마음의 짐을 약간이나마 덜어낸 것 같네요 ㅎㅎ

잘 부탁드립니다 🙇

sang5c · 2024-04-15T10:05:50Z

build.gradle


 version = '1.0.0'
-sourceCompatibility = 1.8
+sourceCompatibility = 11


미션 다시 시작할 땐 리뷰요청을 염두에 두지 않고 진행해서 자바 버전을 바꿨었는데요!
혹시 버전다운이 필요하면 내리고 다시 요청드리겠습니다!

sang5c · 2024-04-15T10:07:26Z

src/main/java/nextstep/ladder/domain/LineBuilder.java

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


LineBuilder 클래스의 위치에 대해 고민이 많았습니다.
별도의 클래스로 분리하는게 좋을지 Line 클래스 내부에 있어야 할지 고민하다가 Factory의 역할도 일부 수행한다고 느껴져서 별도로 분리했습니다.

팩토리를 써서 얻고자 하는 이득이 무엇인가요? 🤔 💭

제가 생각하는 팩토리의 장점은 아래 정도인데 해당사항이 없는것 같아서요 🤔

객체 생성에 조건이 있을 때: 객체를 생성할 때 조건에 따라 다른 종류의 객체를 생성해야 하는 경우, 팩토리 패턴을 사용하여 조건에 따라 적절한 객체를 반환할 수 있습니다.

구현을 변경할 필요가 있을 때: 인터페이스를 통해 여러 구현 클래스 중 하나를 선택해서 사용할 수 있도록 하여, 추후 구현을 변경할 때 기존 코드를 수정하지 않고도 새로운 구현을 쉽게 추가할 수 있습니다.

해당 클래스를 통해 두 가지를 한 번에 달설하려고 했었습니다 😅
"랜덤 포인트를 갖는", "Line을 생성"한다

이런 상태에서 나중에 이동이 지정 가능한 Line을 만들 요구사항이 생기지 않을까? 라는 생각으로 애매모호한 클래스가 탄생했던 것 같습니다. 이 부분은 책임을 분리(RandomGenerator, Builder)하여 새로 구성했으니 확인 부탁드립니다!

seok-2-o

안녕하세요 상오님.
7기 리뷰어로 활동했던 이재석입니다.

오랜만에 슬랙에 들어온것 같은데 또 우연치 않게 리뷰 요청을 발견하여
오지랖일 수 있지만 간단히 의견을 나눠보았습니다!

제가 마지막까지 함께하지 못하더라
상오님의 열정에 조금이나마 도움이 되었기를 바랍니다!
감사합니다!

seok-2-o · 2024-04-17T14:19:33Z

src/main/java/nextstep/ladder/domain/Lines.java

+                .collect(Collectors.toList());
+        return new Lines(lines);


제가 자바를 안쓴지 오래되서 가물 가물하지만.... 아래와 같이 한번에 사용할 수 있어요.

Suggested change

.collect(Collectors.toList());

return new Lines(lines);

.collect(collectingAndThen(toList(), Lines::new)

seok-2-o · 2024-04-17T14:29:31Z

src/main/java/nextstep/ladder/domain/Player.java

+        if (name.length() > 5) {
+            throw new IllegalArgumentException("이름은 5자 이하여야 합니다.");
+        }


매직 넘버가 사용되었네요.
특히나 이름 같은 경우에는 상대적으로 변경이 쉽게 발생할 수 있는 요소이니까
상수로 관리한다면 장점이 있을 것 같아요.

"이름은 " + MAX_NAME_LENGTH +"자 이하여야 합니다."

위 처럼 예외에서도 사용한다면 사이드 이펙트도 줄일 수 있겠네요!

seok-2-o · 2024-04-17T14:31:09Z

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

+            throw new IllegalArgumentException("점이 없습니다.");
+        }
+
+        if (points.size() < 2) {


여기도 매직넘버가 사용되었어요.

다만 해당 값은 사다리 게임에서는 불변의 진리에 가까운 값이라 구지 상수로 선언해야할까 고민이 있네요.

상수로 선언했을때 오히려 네이밍을 잘 하지 않았을 경우 햇갈릴 수 있지 있을까 싶어서요!

상오님 생각이 궁금하네요!

코드 일관성과 에러 메시지 (직전의 name.legnth()와 같은 이유로)에서 동일한 값이 쓰일 수 있도록 상수로 분리했습니다.
포인트의 수는 참여자의 수를 의미하기에, 게임의 설정으로 빠질 수 있는 부분이라는 생각도 드네요!

예를 들어 "참여자가 한명일 수 있다" 또는 "세 명 이상이어야 게임이 가능하다"등으로 추가 요구사항이 생긴다면 주입받는 형태로 구성해야 한다고 느꼈습니다.
적용하지 않은 이유는 묵시적으로 사다리의 세로 라인은 2개 이상이라는 약속이 존재한다고 생각했고. 1개 가능 또는 3개 이상은 일반적이지 않은 케이스라 현재 상태(2개 이상)를 유지하는 방향으로 결정했습니다!

seok-2-o · 2024-04-17T14:33:10Z

src/main/java/nextstep/ladder/domain/LineBuilder.java

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


팩토리를 써서 얻고자 하는 이득이 무엇인가요? 🤔 💭

seok-2-o · 2024-04-17T14:35:57Z

src/main/java/nextstep/ladder/domain/LineBuilder.java

+    public static Line buildWithRandomPoints(int count) {
+        return new LineBuilder().initRandomMoveablePoint()
+                .addRandomMoveablePoints(count - 2)
+                .build();
+    }
+
+    private Line build() {
+        points.add(lastPoint().createRightmost());
+        return new Line(points);
+    }


클래스의 이름이 빌더인데 빌드함수가 private 하게 선언되면 사용할때 햇갈리지 않을까요?
🤔

이전 리뷰인 #2120 (comment) 에 댓글이 안달려 이곳에 한번에 답변드립니다!

팩토리 써서 얻고자 하는 이득

최초 작성 의도는 팩토리를 통해 원하는 형태로(manual) 생성하거나, 랜덤(auto)하게 생성한다. 를 목표로 했었습니다.

클래스의 이름이 빌더인데 build()가 private이 된 이유

현재는 원하는 형태로 생성(manual)에 대한 요구사항이 없으니 막아야 하지 않을까? 라는 생각과 나중에 직접 라인을 구성할 수 있다는 요구사항이 생겼을 때를 위해 build()를 두고 이를 public으로 열면 되지 않을까? 라는 여러가지 생각이 결합되어 애매한 코드가 나왔습니다 ㅎㅎ

결국 빌더와 제너레이터의 책임을 분리하고 build() 메서드는 public으로 적용했습니다 :)

seok-2-o · 2024-04-17T14:40:46Z

src/main/java/nextstep/ladder/domain/LineBuilder.java

+    private LineBuilder addRandomMoveablePoints(int numberOfMiddlePoints) {
+        for (int i = 0; i < numberOfMiddlePoints; i++) {
+            Point point = lastPoint().createNext(RANDOM.nextBoolean());
+            points.add(point);
+        }
+        return this;
+    }
+
+    private LineBuilder initRandomMoveablePoint() {
+        points.add(Point.createLeftmost(RANDOM.nextBoolean()));
+        return this;
+    }


두 단계를 나눈 이유가 궁금해요!

아래와같이 한단계로 사용하는것이 더 편리하지 않을까요? 🤔

(제가 자바를 오랜만에 쓰고 + IDE이 환경이 아니라서 문법오류 양해부탁드릴게요)

private LineBuilder create(int count) { Point current = Point.createLeftmost(RANDOM.nextBoolean()) potins.add(current) for (int i = 0; i < count - 2; i++) { current = current.next() potins.add(current() } }

첫 번째 포인트의 추가 방식이 다르다는 것을 명시적으로 나타내고 싶었습니다. 이전 댓글과 마찬가지로 builder 분리를 진행했고 로직이 하나로 통합되었습니다!

seok-2-o · 2024-04-17T14:45:44Z

src/main/java/nextstep/ladder/domain/LineBuilder.java

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


제가 생각하는 팩토리의 장점은 아래 정도인데 해당사항이 없는것 같아서요 🤔

객체 생성에 조건이 있을 때: 객체를 생성할 때 조건에 따라 다른 종류의 객체를 생성해야 하는 경우, 팩토리 패턴을 사용하여 조건에 따라 적절한 객체를 반환할 수 있습니다.

구현을 변경할 필요가 있을 때: 인터페이스를 통해 여러 구현 클래스 중 하나를 선택해서 사용할 수 있도록 하여, 추후 구현을 변경할 때 기존 코드를 수정하지 않고도 새로운 구현을 쉽게 추가할 수 있습니다.

seok-2-o · 2024-04-17T14:46:37Z

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

+public class Line {
+    private final List<Point> points;
+
+    Line(List<Point> points) {


팩토리로 사용하기 위해 default 로 선언하신걸까요? 👍
그렇다면 패키지 조정을 조금 더 잘 해볼수 있지 않을까요?

감사합니다
리뷰 주신 내용을 고민해봤는데요!
Line을 생성 방식(맨 좌측은 좌로 이동 불가, 맨 우측은 우측 이동 불가)에도 강제가 필요하여 직접 생성이 불가능(private 생성자)하도록 만들고 Builder를 Line 내부로 이동하는 형태로 해결해 봤습니다.
(LineBuilder를 통해야만 라인 생성이 가능하게 구성)

seok-2-o · 2024-04-17T14:50:24Z

src/main/java/nextstep/ladder/util/ConsoleUtil.java

+        printPlayers(ladderGame.getPlayers());
+        printLines(ladderGame.getLines());
+    }


요건 제가 수업들었을때...그리고 리뷰어로활동할떄부터 고민이긴한데
두 리스트에 순서에 의존하는 방식은 두 리스트의 데이터가 변경되었을 때 데이터의 일관성을 확인하는게 어렵지 않을까 생각이 드는데

상오님 생각이 궁금합니다!

왜 이렇게 짰을까 돌이켜봤는데 기존 코드는 API와 클라이언트 모두를 저 혼자 개발하다보니 암시적으로 반환 순서에 의미가 있다고 생각했네요!

명확한게 더 낫다고 느껴서 포인트와 플레이어가 인덱스를 갖도록 구성을 바꿔봤습니다.

댓글을 달면서 든 생각은 현재는 암시적으로(생성 순서에 의해) 플레이어의 인덱스와 포인트(세로 라인)의 인덱스가 결합된 형태이다. 라는 생각도 드네요.

sang5c · 2024-04-23T12:45:30Z

리뷰 너무 감사드립니다 🙇
말씀 주신 내용들 고민해서 다시 적용해 봤습니다!

sang5c added 30 commits April 3, 2024 00:18

build: java 11

f851cb7

docs: step2 요구사항 작성

f887f8a

feat: 사다리 게임 기본 출력 형태 작성

0c09d1e

docs: 요구사항 분석 추가

f6311a6

build: gradle 5.0

066ce9d

feat: 좌, 우 모두 이동 가능한 Point는 생성할 수 없다

1adfd82

test: Point는 좌, 우 이동 가능 여부를 가진다.

dff1072

feat: 사람 수를 받아 가로 라인을 생성한다

85f0fcc

refactor: 포인트 생성 메서드 포인트로 이동

ad63923

refactor: 랜덤 포인트 생성 메서드명 명확하게 변경

2368cef

docs: 완료된 요구사항 완료 처리

7f26af3

feat: 래더게임 완성

7b8e81d

refactor: 매직 리터럴 상수로 사용

fab1fe5

feat: 일급 컬렉션 적용

9427f15

feat: 참가자는 이름을 갖는다

6929a1f

refactor: LadderGame 분리

53309ed

refactor: domain 패키지 분리

e3bdceb

test: 도메인 테스트 작성

5f53b78

refactor: 중간 포인트 생성 메서드 분리

73d2e59

refactor: Line 메서드 분리

99c8db7

feat: 참가자는 1명 이상이다

d10073d

refactor: Line 생성 로직 LineBuilder로 분리

e4da849

feat: 이동 가능한 포인트간에 서로 이동 가능하지 않으면 예외가 발생한다

a241c10

feat: 마지막 포인트를 이용하여 다음 포인트를 생성하도록 변경

9d2ef6c

refactor: 변수명, 메서드명 일관성있고 명확하게 변경

82cb335

refactor: ConsoleUtil 분리

82abe22

refactor: 멤버 변수 final

6aaabc1

refactor: 메서드명 명확하게 변경

7e1e4ec

feat: 사용자 input 받아 게임 실행

1b2b225

refactor: 불필요한 static 제거

30de861

sang5c commented Apr 15, 2024

View reviewed changes

seok-2-o reviewed Apr 17, 2024

View reviewed changes

sang5c added 6 commits April 19, 2024 03:18

refactor: 매직 넘버 상수화

439d1cc

feat: player는 몇 번째 자리인지 확인할 수 있는 index를 갖는다

6498f49

feat: Point는 몇 번째 자리인지 확인할 수 있는 index를 갖는다

a8a84a8

refactor: collectingAndThen 사용하여 바로 반환

9ad91e8

refactor: LineBuilder를 Line 내부로 이동

9a91a2f

refactor: Lines 생성 책임을 LinesGenerator로 분리

4a9ad30

	.collect(Collectors.toList());
	return new Lines(lines);
	.collect(collectingAndThen(toList(), Lines::new)

Conversation

sang5c commented Apr 7, 2024 • 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!

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!

seok-2-o left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sang5c Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sang5c Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sang5c Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sang5c commented Apr 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sang5c commented Apr 7, 2024 •

edited

Loading

seok-2-o left a comment •

edited

Loading

sang5c Apr 23, 2024 •

edited

Loading

sang5c Apr 23, 2024 •

edited

Loading

sang5c Apr 23, 2024 •

edited

Loading