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

Open

sang5c wants to merge 36 commits into

next-step:sang5c from

sang5c:step2

Open

🚀 2단계 - 사다리(생성) #2120
sang5c wants to merge 36 commits into
next-step:sang5c from
sang5c:step2

Conversation

@sang5c

@sang5c sang5c commented Apr 7, 2024 •

edited

Loading

Copy link

Copy Markdown

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

잘 부탁드립니다 🙇

sang5c added 30 commits

April 3, 2024 00:18

@sang5c


 build: java 11

f851cb7

@sang5c


 docs: step2 요구사항 작성

f887f8a

@sang5c


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

0c09d1e

@sang5c


 docs: 요구사항 분석 추가

f6311a6

@sang5c


 build: gradle 5.0

066ce9d

@sang5c


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

1adfd82

@sang5c


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

dff1072

@sang5c


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

85f0fcc

@sang5c


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

ad63923

@sang5c


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

2368cef

@sang5c


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

7f26af3

@sang5c


 feat: 래더게임 완성

7b8e81d

@sang5c


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

fab1fe5

@sang5c


 feat: 일급 컬렉션 적용

9427f15

@sang5c


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

6929a1f

@sang5c


 refactor: LadderGame 분리

53309ed

@sang5c


 refactor: domain 패키지 분리

e3bdceb

@sang5c


 test: 도메인 테스트 작성

5f53b78

@sang5c


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

73d2e59

@sang5c


 refactor: Line 메서드 분리

99c8db7

@sang5c


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

d10073d

@sang5c


 refactor: Line 생성 로직 LineBuilder로 분리

e4da849

@sang5c


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

a241c10

@sang5c


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

9d2ef6c

@sang5c


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

82cb335

@sang5c


 refactor: ConsoleUtil 분리

82abe22

@sang5c


 refactor: 멤버 변수 final

6aaabc1

@sang5c


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

7e1e4ec

@sang5c


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

1b2b225

@sang5c


 refactor: 불필요한 static 제거

30de861

sang5c

sang5c commented

Apr 15, 2024

View reviewed changes

build.gradle

version = '1.0.0'

sourceCompatibility = 1.8

sourceCompatibility = 11

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

import java.util.List;

import java.util.Random;

public class LineBuilder {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

seok-2-o

seok-2-o reviewed

Apr 17, 2024

View reviewed changes

@seok-2-o seok-2-o left a comment •

edited

Loading

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

Comment on lines +25 to +26

.collect(Collectors.toList());

return new Lines(lines);

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change

.collect(Collectors.toList());

return new Lines(lines);

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

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

Comment on lines +16 to +18

if (name.length() > 5) {

throw new IllegalArgumentException("이름은 5자 이하여야 합니다.");

}

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

throw new IllegalArgumentException("점이 없습니다.");

}

if (points.size() < 2) {

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

@sang5c sang5c Apr 23, 2024 •

edited

Loading

Copy link

Copy Markdown

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

import java.util.List;

import java.util.Random;

public class LineBuilder {

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Comment on lines +13 to +22

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);

}

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sang5c sang5c Apr 23, 2024 •

edited

Loading

Copy link

Copy Markdown

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

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

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

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

Comment on lines +24 to +35

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;

}

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

(제가 자바를 오랜만에 쓰고 + 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()
}
}

@sang5c sang5c Apr 23, 2024

Copy link

Copy Markdown

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

import java.util.List;

import java.util.Random;

public class LineBuilder {

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

public class Line {

private final List<Point> points;

Line(List<Point> points) {

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sang5c sang5c Apr 23, 2024

Copy link

Copy Markdown

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Comment on lines +40 to +42

printPlayers(ladderGame.getPlayers());

printLines(ladderGame.getLines());

}

@seok-2-o seok-2-o Apr 17, 2024

Copy link

Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@sang5c sang5c Apr 23, 2024 •

edited

Loading

Copy link

Copy Markdown

Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

sang5c added 6 commits

April 19, 2024 03:18

@sang5c


 refactor: 매직 넘버 상수화

439d1cc

@sang5c


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

6498f49

@sang5c


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

a8a84a8

@sang5c


 refactor: collectingAndThen 사용하여 바로 반환

9ad91e8

@sang5c


 refactor: LineBuilder를 Line 내부로 이동

9a91a2f

@sang5c


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

4a9ad30

@sang5c

sang5c commented Apr 23, 2024

Copy link

Copy Markdown

Author

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

Labels

None yet

2 participants

@sang5c @seok-2-o

Conversation

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

Uh oh!

Uh oh!

@sang5c sang5c Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

@sang5c sang5c Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@sang5c sang5c Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o 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!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@sang5c 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!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@sang5c 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!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@sang5c sang5c Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

@sang5c sang5c Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

@seok-2-o seok-2-o Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

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

edited

Loading

@seok-2-o seok-2-o left a comment •

edited

Loading

@sang5c sang5c Apr 23, 2024 •

edited

Loading

@sang5c sang5c Apr 23, 2024 •

edited

Loading

@sang5c sang5c Apr 23, 2024 •

edited

Loading