[사다리 미션] 황승준 미션 제출입니다. #23

davidolleh · 2024-10-31T17:18:45Z

안녕하세요 리뷰어님들! 늦은 PR작성 정말 죄송합니다... 잘 부탁드립니다!

�랜덤 사다리를 만드는 로직이 조금 복잡하여 코드가 복잡해진거 같습니다.
함수 이름이 이해가 안되는게 있다면 지적해주시면 감사하겠습니다!

davidolleh · 2024-11-20T14:29:44Z

README를 작성하고 보니 사다리 객체에서 중요하게 표현해줘야 하는 것은 RowLine이라는 생각을 하게 되어서 코드를 이리저리 뜯어 고치게 되었습니다! 감사합니다 :)

dooboocookie

안녕하세요, 승준님.
리뷰어 루카입니다!!🐳

🎯 리뷰 포인트

enum에게 책임 부여
단위 테스트 방향성

🔮 추가 리뷰

record같은 java에서 제공하는 기능을 활용해보는 것은 매우 좋은 방법인 것 같습니다. 다만, record로 선언하면 해당 object가 어떻게 되는지에 대해서 조금 더 신중히 다룰 필요는 있어보여요. 그래야 어떤걸 record로 선언할지 알 수 있기도하구요. equals를 재정의해버리는 실수같은 것은 하지 않겠죵?

충분히 많이 고민해보시고 리뷰 반영을 해주신 것 같아서 감사합니다.
해당 미션이 너무 늘어져서, 일단 PR은 여기서 merge하기로 하겠습니다.

테스트 관련해서나, 책임분리 관련해서 더 학습해보고 싶으시다면, 더 리팩터링해봐도 좋을 것 같습니다.

일단 저는 여기서 끝낼테니,
도움 필요하면 언제든 연락주세요.

dooboocookie · 2024-11-24T03:31:28Z

README.md

+# java-ladder
+
+자바 사다리 미션 저장소
+
+## 개요
+
+참여자와 상품을 입력 받아 랜덤 사다리를 만들어 참여자와 상품을 연결시켜주는 게임이다.
+
+## 요구사항
+
+### 사람(Person)
+
 - [x] 사람의 이름은 1자 이상 5자 이하여야 한다.
+
+### 참여자(Participant)
+
+- [x] 참여자들은 사다리에서 자신의 위치를 갖고 있다.
+- [X] 참여자들은 정해진 라인을 따라 이동한다.
+
+### 참여자들(Participants)
+
+- [x] 참여자들의 이름은 중복이 될 수 없다.
+- [x] 각 라인의 출발점에는 하나의 사람만 있을 수 있다.
+
+### 상품(Prize)
+
+- [x] 상품은 금액으로 이루어져 있다.
+- [x] 상품의 금액이 값을 표현한다.(vo를 표현할 수 있는 말이 무엇이 있을까?)
+
+### 라인(RowLine)
+
+- [x] 라인들의 각 지점은 오른쪽, 왼쪽, 아래쪽 방향성 중 하나만을 가질 수 있다.
+- [x] 라인이 연결성은 겹치지 않아야 한다.
+- [x] 라인의 연결 여부는 랜덤으로 결정한다.
+
+### 사다리(Ladder)
+
+- [x] 여러개의 Line이 모여 하나의 사다리를 만든다.
+
+### 사다리 게임(LadderGame)
+
+- [x] 참여자들을 라인 따라 이동해 상품과 최종적으로 연결을 맺는다.
+- [x] 게임이 완료되어야 최종 게임 결과를 확인할 수 있다.


[Approve]

이전보다 리드미를 비즈니스 로직에 가깝게 작성해주셨네요.

리뷰하는 입장에서도 승준님이 어떤 의도로 도메인을 설계했는지 이해하기 좋습니다👍👍👍👍👍👍

dooboocookie · 2024-11-24T03:39:59Z

src/main/java/LadderGameController.java

    public void ladderGame() {
-        List<Person> participants = readParticipants();
+        List<Person> people = readParticipants();
        List<Prize> prizes = readPrizes();

-        checkPrizeCountValidation(participants.size(), prizes.size());
+        validateInputsCount(people.size(), prizes.size());

        int height = readLadderHeight();
+        int width = people.size();

-        Ladder ladder = new Ladder(height, participants.size(), new RandomLadderGeneratorImpl());
+        LadderFactory ladderGenerator = new RandomLadderFactory(height, width);
+        Ladder ladder = ladderGenerator.newInstance();

-        outputView.printResult(participants, ladder, prizes);
+        Participants participants = Participants.fromPeople(people);
+        LadderGame ladderGame = new LadderGame(participants, ladder, prizes);

-        Statistic statistic = new Statistic(ladder, participants, prizes);
+        outputView.printResult(ladderGame.getParticipants(), ladderGame.getLadder(), ladderGame.getPrizes());

-        printSpecificParticipantResult(statistic, participants);
+        ladderGame.start();

-        outputView.printParticipantsPrizesResult(statistic.getParticipantPrize(), participants);
+        GameResult gameResult = ladderGame.getParticipantsPrizes();
+
+        printSpecificParticipantResult(ladderGame.getParticipants(), gameResult);
+
+        outputView.printParticipantsPrizesResult(ladderGame.getParticipants(), gameResult);
    }


[Commnet]

저희가 Controller를 사용한 것은

Model과 View를 따로 관리한다.

Controller가 호출하여 연결한다.

이런 역할을 부여하기 위해서 사용하였습니다.
이 ladderGame()메서드는 위 역할 외에도 게임의 흐름을 결정하는 역할도 있는 것 같아요.
그래서 메서드가 매우 길어진 것으로 보입니다.
이 메서드를 줄여보는 것도 좋은 연습이 될 것 같습니다.

리뷰는 이만 여기서 마칠 계획이라 추가적인 RC는 드리지 않을 것입니다.
혹시 해당 리뷰에 대해 더 알아보고 싶으시면 상희님 PR을 참고해주시고, 질문 있으면 DM 주세요.😄

dooboocookie · 2024-11-24T03:41:44Z

src/main/java/domain/Direction.java

-    LEFT, RIGHT, DOWN
+    LEFT(-1), RIGHT(1), DOWN(0);
+
+    final int offset;
+
+    Direction(int offset) {
+        this.offset = offset;
+    }


[Comment]

offset이라는 필드로 enum에 의한 움직임 동작을 정의하신 것이 인상적인네요!!
제 리뷰도 최대한 반영해주시려 한 것 같아서 감사합니다.

📝 Enum에 함수형 인터페이스 넣어보기 0

더 이 사다리 미션에 대한 리뷰 티키타카가 어려울 것 같습니다.
enum에 책임 부여를 더 했으면 좋겠다는 제 리뷰에 대해 제 예상 결과물을 일방적으로 전달을 드려보겠습니다.
물론 정답의 의미로 드린다기 보단 함수형 인터페이스 활용과 Enum에도 책임을 부여할 수 있다는 하나의 예시정도로 생각해주세요.

dooboocookie · 2024-11-24T03:46:34Z

src/main/java/domain/Direction.java

+    LEFT(-1), RIGHT(1), DOWN(0);
+
+    final int offset;
+
+    Direction(int offset) {
+        this.offset = offset;
+    }


📝 Enum에 함수형 인터페이스 넣어보기 1

enum에도 메서드로 동작을 정의해볼 수 있을 것 같아요!!

Suggested change

LEFT(-1), RIGHT(1), DOWN(0);

final int offset;

Direction(int offset) {

this.offset = offset;

}

public enum Direction {

LEFT(-1),

RIGHT(1),

DOWN(0);

private final int offset;

Direction(int offset) {

this.offset = offset;

}

public int move(int position) {

return position + offset;

}

dooboocookie · 2024-11-24T03:49:47Z

src/main/java/domain/Connection.java

+public enum Connection {
+    CONNECTED,
+    UNCONNECTED,
+}


📝 Enum에 함수형 인터페이스 넣어보기 2

함수형 인터페이스를 필드로 선언해서, 각각의 enum마다 다음 동작을 정의할 수 있겠네요!

Suggested change

public enum Connection {

CONNECTED,

UNCONNECTED,

}

public enum Connection {

CONNECTED(Connection::createUnconnected),

UNCONNECTED(Connection::createRandomConnection),

;

private static final ThreadLocalRandom random = ThreadLocalRandom.current();

private static final int RANDOM_BOUND = 10;

private static final int THRESHOLD = 5;

final Supplier<Connection> createNext;

Connection(Supplier<Connection> createNext) {

this.createNext = createNext;

}

static Connection createRandomConnection() {

int randomValue = random.nextInt(RANDOM_BOUND);

if (randomValue < THRESHOLD) {

return Connection.CONNECTED;

}

return Connection.UNCONNECTED;

}

private static Connection createUnconnected() {

return UNCONNECTED;

}

public Connection createNext() {

return createNext.get();

}

}

dooboocookie · 2024-11-24T03:51:33Z

src/main/java/domain/Participant.java

+    public void move(Direction direction) {
+        position = position + direction.offset;
+    }


📝 Enum에 함수형 인터페이스 넣어보기 3

그럼 위에서 정의한 enum의 메서드로 특정 동작을 수행해서 position을 얻어낼 수 있겠어요

Suggested change

public void move(Direction direction) {

position = position + direction.offset;

}

public void move(Direction direction) {

this.position = direction.move(position);

}

dooboocookie · 2024-11-24T04:01:14Z

src/main/java/domain/RandomLadderFactory.java

+    private Connection generateSpotToSpotConnection(int lineIndex, List<Connection> rowSpotsConnection) {
+        if (lineIndex != 0 && rowSpotsConnection.get(lineIndex - 1) == Connection.CONNECTED)
+            return Connection.UNCONNECTED;
+
+        int randomValue = random.nextInt(RANDOM_BOUND);
+        if (randomValue < 5)
+            return Connection.CONNECTED;
+
+        return Connection.UNCONNECTED;
+    }


📝 Enum에 함수형 인터페이스 넣어보기 4

이제 그럼 여기서 조금 더 분기를 간소화할 방법이 보이네요.

Suggested change

private Connection generateSpotToSpotConnection(int lineIndex, List<Connection> rowSpotsConnection) {

if (lineIndex != 0 && rowSpotsConnection.get(lineIndex - 1) == Connection.CONNECTED)

return Connection.UNCONNECTED;

int randomValue = random.nextInt(RANDOM_BOUND);

if (randomValue < 5)

return Connection.CONNECTED;

return Connection.UNCONNECTED;

}

private Connection generateSpotToSpotConnection(int lineIndex, List<Connection> rowSpotsConnection) {

if (lineIndex == 0) {

return Connection.createRandomConnection();

}

final Connection prevConnection = rowSpotsConnection.get(lineIndex - 1);

return prevConnection.createNext();

}

dooboocookie · 2024-11-24T04:02:10Z

src/main/java/domain/RandomLadderFactory.java

+    private Direction getFirstLineDirection(Connection connection) {
+        if (connection == Connection.CONNECTED) {
+            return Direction.RIGHT;
+        }
+        return Direction.DOWN;
+    }
+
+    private Direction getLastLineDirection(Connection connection) {
+        if (connection == Connection.CONNECTED) {
+            return Direction.LEFT;
+        }
+        return Direction.DOWN;
+    }
+
+    private Direction getMiddleLineDirection(Connection currentConnection, Connection nextConnection) {
+        if (currentConnection == Connection.CONNECTED) {
+            return Direction.LEFT;
+        }
+
+        if (nextConnection == Connection.CONNECTED) {
+            return Direction.RIGHT;
+        }
+
+        return Direction.DOWN;
+    }


📝 Enum에 함수형 인터페이스 넣어보기 5

이렇게 Connection의 enum 종류에 따라 다른 Direction을 만들어야되는 로직도 Connection 쪽으로 밀어넣을 수 있겠네요!!

정말 다양한 방법으로 책임 분리를 할 수 있다고 소개해보기 위한 enum에 함수형 인터페이스 넣어보기 시리즈 였습니다.
리뷰 보시고 승준님 만의 방법이 떠오르신다면 한번 적용해보시면 좋겠어요!!

dooboocookie · 2024-11-24T04:24:07Z

src/test/java/PersonTest.java

+@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
+@SuppressWarnings("NonAsciiCharacters")
+public class PersonTest {
+    @Test
+    public void 사람_이름은_1자_이상_5자_이하_여야_한다() {
+        String name1 = "Seung";
+        String name2 = "";
+        String name3 = "SeungJ";
+
+        Assertions.assertDoesNotThrow(() -> new Person(name1));
+        Assertions.assertThrows(IllegalArgumentException.class, () -> new Person(name2));
+        Assertions.assertThrows(IllegalArgumentException.class, () -> new Person(name3));
+    }
+}


[Approve]

README.md

### 사람(Person) - [x] 사람의 이름은 1자 이상 5자 이하여야 한다.

이 요구사항이 만족하고 있다고 확신을 할 수 있는 정말 좋은 테스트 입니다.
이처럼 요구사항을 거의 1:1로 반영하여 명세처럼 느껴지는 테스트가 좋은 테스트라고 생각합니다.

[Comment]

다만 테스트를 잘 짜는 방법에는 여러가지 방법론과 원칙이 존재하는데요.

테스트는 독립적이여야 한다.

신뢰할 수 있는 테스트를 만들기 위한 원칙이라고 생각합니다.

1자 미만나 5자 초과이 동일한 if문 분기에서 나온 상황은 맞지만
해당 테스트가 실패했을 때, name2때문인지 name3때문인지 알기 힘들지 않을까요?

이런 엣지 케이스의 경우에는 각각을 테스트 케이스로 만들어서 케이스를 분리시키는 것도 유지보수에 도움이 되겠습니다

dooboocookie · 2024-11-24T05:13:06Z

src/test/java/PrizeTest.java

+    public void vo_테스트() {
+        int cost1 = 1000;
+        int cost2 = 2000;
+        Prize prize1 = new Prize(cost1);
+        Prize prize2 = new Prize(cost1);
+        Prize prize3 = new Prize(cost2);
+
+        assertThat(prize1.equals(prize2)).isTrue();
+        assertThat(prize1.equals(prize3)).isFalse();
+    }


[Comment]

놓칠 수 있는 부분도 테스트로 만들어놓는 것은 좋은 습관이죠. 굿입니다.

하지만 테스트명은 더 분명히 작성해주세요.

테스트가 늘어나면 늘어날수록 특정 테스트가 실패하는 것을 인지하는 것은 매우 중요합니다.
@DisplayName 어노테이션도 없는 상테에서 vo_테스트라는 메서드명만 있으면, 해당 테스트가 실패했을때 어떤 로직이 잘 돌아가고 있지 않은지 파악하기가 쉽지 않겠습니다.

[Comment]

이 로직이 테스트가 필요할까요?

그렇게 테스트를 작성하라고 했으면서 이런 이야기가 어이없이 들릴 수 있지만요...

제가 생각하는 테스트를 작성하는 이유는 크게 다음과 같습니다.

새로 짠 로직이 요구사항을 잘 이행하는가

리팩토링하면서 이전 로직들이 깨지지 않는가

테스트를 작성한다는 것은 매우 피로한 일입니다.
당장의 개발 생산성을 떨어트리는 것 같고,
어쩔 떈 프로덕트의 코드를 변경하느라 테스트까지 같이 바꿔줘야할 때가 있죠.

테스트는 코드의 관리 포인트를 또 만들어버리는 것 같은 일이죠.
그래서 어떤 대상을 단위테스트로 만들까 하는 것은 매우 중요한 고민입니다.
이는 팀의 생산성과 직결되기 때문입니다.

equals같이 IDE가 짜주거나, 혹은 record가 만들어주는 매우 단순한 로직은 테스트를 만드는 것이 효율적인지 저는 공감이 되지 않습니다.
테스트를 짜는 리소스 vs 테스틀 짜서 좋은 점
이 두가지 가치 중에 좌측이 훨씬 큰것 같아요.

또한, 해당 prize라는 객체는 record로 구현하셔서 equals를 재정의를 할필요가 없습니다.
(실제로 프로덕트 코드의 equals 오버라이딩을 제거해도 이 테스트가 잘 돌아가죠.)

davidolleh added 30 commits October 31, 2024 00:19

<FEAT> 기능 명세서 파일 추가

b07a49c

<FEAT> input, ouput view class 추가

f493a05

<FEAT> 연결 관련 enum class들 추가

2254785

<FEAT> controller 객체 추가

e0ffcfb

<FEAT> line 객체 추가

02b6a8d

<FEAT> 참여자 객체 추가

7cf4555

<FEAT> main 함수 추가

dcf7dc4

<FEAT> 당첨 객체 추가

06cc958

<FEAT> 사다리 객체 추가

fc66931

<FEAT> 사다리 랜덤 생성 객체 추가

2e22fee

<FIX> 이름 수정

613461c

<FIX> 사다리 랜덤 생성 객체 추상화로 분리

ed047ab

<FIX> nextInt 입력 받는거 nextLine으로 변경

71d47c7

<FEAT> width, height ladder의 field로 추가

dd7174d

<FEAT> value object로 변경, 예외처리 추가

5781a34

<FEAT> 사다리 결과 출력 기능 추가

9b5c7b5

<FEAT> Prize vo로 변경

125810f

<FEAT> 실행 결과 기능들 추가

92408bb

<FEAT> 실행 결과 객체 추가

431bd39

<FEAT> controller 기능 추가

77f88a7

<FEAT> Ladder 자동생성 테스트 추가

9d693f0

<FEAT> 기능 명세서 추가

91e4ceb

<FIX> Indent 1로 마추려고 수정

b70c3a5

<FIX> tmp 변수로 변경

c76ef4a

<FIX> 함수로 분리

a09be2f

<FEAT> 개행문자 추가

9bacca2

<FEAT> Person, Statistic 관련 test 추가

d767643

<FIX> warning 제거

6aef072

<FIX> 함수로 분리

5402201

<FEAT> prize test 추가

726700e

davidolleh added 27 commits November 12, 2024 22:42

<REFACTOR> 1. Person 값 객체로 사용하지 않도록 수정, 2. 메서드 이름 동사형으로 수정

e278cc8

<REFACTOR> README 구체적으로 추가

bf26804

<REFACTOR> 변수 이름 수정

915c745

<REFACTOR> 금액의 꽝에 대한 처리를 outputview의 역할로 수정

d5eda95

<REFACTOR> Ladder 생성 로직과 entity 분리

6bf7754

<REFACTOR> Line 이름 수정

39d71d8

<REFACTOR> README 추가

0c759b5

<REFACTOR> line 특정 rowIndex의 방향 가져오는 함수 구현

d67a2e9

<REFACTOR> Ladder의 필드 RowLine으로 변경

936a471

<REFACTOR> main class 이름 수정

5aa5433

<REFACTOR> LadderGame 실행 구현

7057283

<REFACTOR> LadderGame 실행 구현

d766f09

<REFACTOR> Controller, OutputView LadderGame에 맞춰서 수정

f3dd305

<REFACTOR> 사용하지 않는 파일 삭제

1d25eb1

<REFACTOR> person record로 수정

8f5d014

<REFACTOR> README 추가 작성

3268173

<REFACTOR> 불필요한 함서 제거

19585d2

<REFACTOR> 필드 final로 변경

afe1ed8

<REFACTOR> class record로 변경

a866afb

<REFACTOR> class record로 변경

0c974c9

<REFACTOR> LadderFactory 추상화

8bda0a9

<REFACTOR> 오타 수정

4547ffe

<REFACTOR> 참여자 테스트 구현

7481d5d

<REFACTOR> 파일 삭제

a08a3c9

<REFACTOR> README 추가

27da6f5

<REFACTOR> 테스트 구현

7e3f2e8

<REFACTOR> 정적 변수러 설정

81e327f

dooboocookie approved these changes Nov 24, 2024

View reviewed changes

dooboocookie mentioned this pull request Nov 24, 2024

[사다리 미션] 정상희 미션 제출합니다. #19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[사다리 미션] 황승준 미션 제출입니다. #23

[사다리 미션] 황승준 미션 제출입니다. #23

davidolleh commented Oct 31, 2024

davidolleh commented Nov 20, 2024

dooboocookie left a comment

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

dooboocookie Nov 24, 2024

-public enum Connection {
-    CONNECTED,
-    UNCONNECTED,
-}
+public enum Connection {
+    CONNECTED(Connection::createUnconnected),
+    UNCONNECTED(Connection::createRandomConnection),
+    ;
+    private static final ThreadLocalRandom random = ThreadLocalRandom.current();
+    private static final int RANDOM_BOUND = 10;
+    private static final int THRESHOLD = 5;
+    final Supplier<Connection> createNext;
+    Connection(Supplier<Connection> createNext) {
+        this.createNext = createNext;
+    }
+   static Connection createRandomConnection() {
+        int randomValue = random.nextInt(RANDOM_BOUND);
+        if (randomValue < THRESHOLD) {
+            return Connection.CONNECTED;
+        }
+        return Connection.UNCONNECTED;
+    }
+    private static Connection createUnconnected() {
+        return UNCONNECTED;
+    }
+    public Connection createNext() {
+        return createNext.get();
+    }
+}

[사다리 미션] 황승준 미션 제출입니다. #23

Are you sure you want to change the base?

[사다리 미션] 황승준 미션 제출입니다. #23

Conversation

davidolleh commented Oct 31, 2024

davidolleh commented Nov 20, 2024

dooboocookie left a comment

Choose a reason for hiding this comment

🎯 리뷰 포인트

🔮 추가 리뷰

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

📝 Enum에 함수형 인터페이스 넣어보기 0

Choose a reason for hiding this comment

📝 Enum에 함수형 인터페이스 넣어보기 1

Choose a reason for hiding this comment

📝 Enum에 함수형 인터페이스 넣어보기 2

Choose a reason for hiding this comment

📝 Enum에 함수형 인터페이스 넣어보기 3

Choose a reason for hiding this comment

📝 Enum에 함수형 인터페이스 넣어보기 4

Choose a reason for hiding this comment

📝 Enum에 함수형 인터페이스 넣어보기 5

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment