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

SANGHEEJEONG · 2024-10-30T14:25:01Z

최대한 주어진 조건을 만족시키면서 미션을 해결하려고 했는데 enum은 어디에 써야할 지 감이 안 잡혀서 사용 못했습니다 ㅠ

학습 주제가 함수형 프로그래밍이라서 스트림 사용을 좀 해보려고 했는데 코드에 딱히 많이 사용하진 못한 것 같아서 리뷰하시면서 스트림으로 바꾸면 더 나을 것 같은 부분이 있다면 알려주세요!

사다리 구현은
0 1 2 3
|-----| |-----|
[true, false, true]

이런 식으로 인덱스와 boolean 값으로 구현했습니다.
player가 이름과 현재 위치를 가지고 있고 결과 위치에 따라 사용자가 입력한 사다리 결과들을 출력하도록 만들었습니다.

0094-Gengar · 2024-11-01T04:08:57Z

안녕하세요. 상희님! 코드리뷰를 해보았는데,,아직 자바를 배우는 중이고 코드리뷰도 처음이라 좋은 리뷰를 하진 못한 것 같아서 죄송합니다..ㅜ
상희님 코드 리뷰를 하면서 배워가는게 정말 많았는데요! 하나 요청드리고 싶은 점은 README 작성도 해주시면 좋을 것 같아요! 출력 전 사다리 관련 로직들이 모두 논리식으로 되어 있어서 바로 이해하기 어려웠던 것 같습니다. 사실 제가 코드 해석하는 속도가 많이 느리기 때문인데,, 저처럼 아직 실력이 부족한 사람들도 얼마든지 상희님의 코드를 볼 수 있다고 생각해요! 로직에 관한 내용을 조금 더 자세하게 작성해주시면 코드를 읽기에도, 상희님 코드를 읽고 공부하는 사람에게도 더 도움이 되지 않을까 싶습니다...!
다음에 또 상호리뷰를 할 수 있는 기회가 있다면 더 유의미한 리뷰를 드릴 수 있도록 노력할게요! 코드 읽어보면서 많이 배웠습니다 :)
양질의 코드리뷰는 리뷰어님께서 해주실 것이라 믿고 저는 이만 가보겠습니다! 화이팅

SANGHEEJEONG · 2024-11-02T10:37:28Z

헉 저도 자바를 배우고 있어서 공감이 가네요
리뷰하시느라 수고하셨습니다.
README 작성해서 올리도록 하겠습니다!

dooboocookie

안녕하세요, 상희님!
리뷰어 루카입니다!!🐳

🌻 리뷰 종류

정확한 리뷰 전달을 위해 리뷰를 3가지로 나눴습니다.

[Request Change]

컨벤션에 어긋남
버그를 유발
꼭 학습이 필요한 부분
...
위와 같이 꼭 변경이 필요할 때 한 리뷰입니다.

[Comment]

더 나은 방법을 고민해보면 좋은 점
의도를 파악하기 힘든 부분
...
이와 같이 심각한 부분은 아니지만 생각 공유가 더 필요한 부분에 달았습니다.

[Approve]

정말 좋다고 생각하는 부분
너무 좋으나, 추가적인 생각을 달고 싶은 부분
...

🙆‍♀️ 리뷰 방향성

정답은 없다는 것 이제는 알고 계시겠죠?
아무래도 미션의 주제가 있고 학습 포인트가 있어서, 학습을 위해 알아보면 좋을 부분에 리뷰를 달긴 했으나,
상희님의 코드와 생각도 충분하다고 생각하기 때문에 "어떻게 바꿔주세요!!" 같은 리뷰는 최대한 지양했습니다.
"왜 그렇게 생각하시나요?" 같은 질문류의 리뷰를 많이 달았는데, 뭔가 의도가 있는 질문이라고 생각하시기 보단 정말 상희님이 왜 그렇게 했는지 다시한번 고려해보시고 생각을 공유해주세요.

💡 리뷰 포인트

객체 책임 분리 (Ladder, Line)
Controller run() 메서드 10줄 넘는 것 분리해보기
Boolean과 String 의미있는 객체로 래핑해보기

이번 미션에서 꼭 가져가셨으면 하는 부분들 중점으로 리뷰 남겨봤습니다.
더 이야기 나눠봐야할 것 같아 RC 드렸으니, 적용하시고 맨션 부탁드릴게요.
리뷰에 대해 궁금한점 있으시면 DM이나 질문 채널에 맨션주셔도됩니다!!

dooboocookie · 2024-11-04T12:48:15Z

README.md

+# 기능 구현
+
+사다리의 연결은 Boolean을 통해 표현
+ true면 "-----" 연결되었다는 뜻
+ false면 연결되지 않았다는 뜻
+
+## **controller**
+ LadderController
+
+## **domain**
+ Ladder
+  * List&lt;Line&gt; 사다리 전체를 표현하는 객체
+ LadderGame
+  * 사다리 게임 실행과 관련된 메서드를 담은 클래스
+ LadderGenerator
+  * 사다리 생성과 관련된 메서드를 담은 클래스
+ Line
+  * List&lt;Boolean&gt; 사다리 한 줄을 표현하는 객체
+ PlayerName
+  * 플레이어 이름을 원시값 포장한 객체
+ Player
+  * 플레이어 객체
+ Players
+  * 플레이어 리스트 일급 컬렉션
+ Positon
+  * 플레이어 위치를 원시값 포장한 객체
+ ResultTypes
+  * 결과 리스트 일급 컬렉션
+
+## **View**
+ InputException
+ InputView
+ OutputView


[Request Change]

README를 작성하신 이유가 있을까요?

저는 상희님이 작성하신 패키지 목록만 봐도 위의 내용은 예상 가능합니다.

만약, Position 클래스의 이름을 Location으로 변경한다하면, README도 변경해줘야겠죠?
오히려 관리 포인트가 늘어나는 일인데요.

고민포인트

이렇게 번거러움에도 불구하고 사람들이 README를 작성하는 이유가 무엇일까요?

개선점

README가 읽는 대상이 누구일지 생각하고 더 이해하기 쉽게 작성하시면 좋을 것 같습니다

앗 리뷰어님이 말씀하신대로 제가 '읽는 대상'과 '쓰는 목적'에 대한 고민을 덜 한 채로 README를 작성한 것 같네요

많은 사람들의 README를 참고해서 고민한 결과로

구현할 기능을 스스로 작성함으로써 전체적인 틀을 잡는다

자신의 코드를 볼 사람들의 이해를 돕는다

이 두 가지가 가장 큰 목적인 것 같습니다.
물론 저는 README를 기능 구현 후에 작성했지만 다음 프로젝트에서는 구현 전에 작성해보는 게 좋겠네요! 수정해서 올렸는데 더 수정 및 보완할 부분이 있다면 알려주시면 감사하겠습니다.

dooboocookie · 2024-11-04T12:50:57Z

src/main/java/Application.java

+import domain.Ladder;
+import domain.LadderGame;
+import domain.LadderGenerator;
+import domain.Players;
+import domain.ResultTypes;
+import view.OutputView;
+import view.InputView;


[Request Change]

사용하지 않는 코드에 대한 import는 꼭 제거해주세요.

import는 클래스간 의존성을 한눈에 볼 수 있는 부분입니다.

IDE를 통해 commit을 할 때, import를 최적화 하는 방법도 있으니 검색해보시면 좋겠네요!!

dooboocookie · 2024-11-04T13:04:08Z

src/main/java/controller/LadderController.java

+    public void run() {
+        // 게임 로직 시작
+        LadderGame ladderGame = new LadderGame();
+
+        // 플레이어 생성
+        List<String> playerNames = InputView.splitString(InputView.inputNames());
+        Players players = new Players(playerNames);
+
+        // 결과 생성
+        List<String> kindOfResults = InputView.splitString(InputView.inputLadderResults());
+        ResultTypes resultTypes = new ResultTypes(kindOfResults, players.getPlayersSize());
+
+        // 사다리 생성
+        int width = players.getPlayersSize();
+        int height = InputView.inputHeight();
+
+        LadderGenerator ladderGenerator = new LadderGenerator();
+        Ladder ladder = ladderGenerator.createLadder(width, height);
+
+        // 사다리 출력
+        OutputView.printPlayers(playerNames);
+        OutputView.drawLadder(ladder);
+        OutputView.printResultTypes(resultTypes.getResultTypes());
+
+        // 게임 시작 및 결과 출력
+        ladderGame.runGame(ladder, players);
+        OutputView.printResult(players, resultTypes.getResultTypes());
+    }


[�Comment]

로또 미션 요구사항에서 위와같이 10줄이 넘어가지 말라는 요구사항이 있었는데요.

이 run 함수는 매우 기네요.
길어진 이유는, 주석을 달아주신 것 처럼 많은일을 하고 있어서인 것 같네요.

다른 분들도 Controller의 함수가 길어지는 현상을 간혹 봤는데요.

상희님만의 새로운 요구사항이 생길 수 있다 가정해볼까요?

네이버 사다리타기는 이렇게, 한명 한명 결과를 확인하거나, 전체 결과를 한번에 해결하거나 할 수 있죠.
지금 이 Controller 함수는 유저가 할 수 있는 시나리오 1개밖에 실행을 못하네요.
더 유연한 방법은 뭐가있을까요?

헉 규칙을 까먹어 버렸네요ㅜ
메서드를 분리함으로써 다양한 시나리오에 유동적으로 대처가 가능할 것 같습니다!

이거는 함수 분리랑 다른 내용이긴 한데 전부터 궁금했었어서 질문 드립니다
Application 클래스가 있는데 많은 사람들이 Application에 직접 메서드를 작성하지 않고 굳이 Controller를 따로 만드는 이유가 무엇인지 잘 모르겠습니다,,

글쎄요... 저도 잘모르겠어요!!

controller에 동작을 1개(run)만 정의하는 이유를 모르겠어요!!

controller는 게임의 흐름(run)을 결정하는 주체라고 생각하지 않아요.
입력(요청)을 모델에게 전달하거나 혹은 모델의 상태를 뷰가 출력하도록 전달(응답)하는 역할이라고 생각해요.

이에 대해서는 밑에서 더 다뤄보죠!! 아주 좋은 고민이네요

dooboocookie · 2024-11-04T13:05:34Z

src/main/java/domain/Ladder.java

+public class Ladder {
+
+    private final List<Line> lines;
+
+    public Ladder(List<Line> lines) {
+        this.lines = lines;
+    }
+
+    public List<Line> getLadder() {
+        return Collections.unmodifiableList(lines);
+    }
+}


[Comment]

getter 만 존재하는 일급컬렉션을 선언하신 이유가 있을까요?

List<Line>을 그냥 사용하는 것보다 일급 컬렉션으로 선언했을 때

관련된 로직을 직접 처리한다

각 컬렉션에 의미부여가 명확해진다

이 두 가지의 큰 장점이 생기는 것 같습니다.
근데 리뷰어님이 말씀하신대로 getter만 존재한다는 것은 장점1이 적용되지 않은 클래스라고 생각되네요! 다시 한 번 코드를 보면서 해당 클래스에서 처리할 메서드가 있다면 옮기도록 하겠습니다

[Comment]

여전히 허전하네요. 일급컬랙션으로 래핑할 이유가 없어보여요.

근데, 너무 중요한 도메인이죠? 이 미션이름이 **'사다리'**미션인데 어떻게 객체로 안만들어요.😭😭😭😭😭😭😭😭😭

그래도 아쉬운 마음에 조금 더 영업을 해보겠습니다.
저는 사다리 게임의 어떤 모양?을 상상하면

(출처: 네이버 사다리 게임)

(출처: 위키피디아)

사다리 꼭대기에 놓여진 players들이 너무 input 같지 않나요?🧐

저는 moveAllPlayers 메서드가 '플레이어들이 사다리 위를 움직인다'는 관점으로 봐서 1차 리뷰 수정 때 Players 클래스에 넣었는데 생각해보니까 '사다리' 게임인 만큼 리뷰어 님이 말해주신 저 관점으로 보면 Ladder 클래스에 넣는 게 더 적합할 수도 있겠네요

dooboocookie · 2024-11-04T13:09:35Z

src/main/java/domain/LadderGame.java

+    public void movePlayer(Line line, Player player) {
+        if (line.canMoveRight(player.getPosition())) {
+            player.moveRight();
+            return;
+        }
+
+        if (line.canMoveLeft(player.getPosition())) {
+            player.moveLeft();
+        }
+    }
+
+    public void moveEachPlayer(Ladder ladder, Player player) {
+        ladder.getLadder().forEach(line -> movePlayer(line, player));
+    }


[Request Change]

이 두 메서드는 테스트를 제외하곤 LadderGame 내부에서만 사용되네요.
private으로 변경해야할 것 같아요.

만약 이 로직이 test 대상처럼 느껴진다면, 고민해볼 필요가 있겠네요.

단위 테스트 대상은 어디까지?

이것만으로 테스트할 가치가 있는 내용이라면, 이것들을 결국 호출하는 runGame()이란 메서드가 너무 많은 책임을 갖고 있는 것 아닐까?

자료를 찾아보니 private 함수는 단위 테스트를 진행하지 않는 방향이 맞을 것 같아요

이 질문에 대한 답은 “어떻게든 private 메서드를 테스트하고 싶다면, 그 메서드는 private이면 안된다"다. 메서드를 public으로 바꿔도 될지 마음에 걸린다면, 이 메서드는 별도의 책임의 일부로서 원래는 다른 클래스에 들어있어야 했던 것이다.

dooboocookie · 2024-11-04T13:47:32Z

src/main/java/domain/Position.java

+    public void movePositionLeft() {
+        position--;
+    }


[Request Change]

position이 0인 객체의 이 메서드가 호출되지 않을 것이란 보장이 있을까요? 🧐🧐🧐🧐
그렇게된다면 심각한 버그가 발생할텐데요.

일단 저는 저를 포함한 동료개발자들을 그리 믿진 않아요.

dooboocookie · 2024-11-04T13:48:37Z

src/main/java/domain/Position.java

+    public int getPosition() {
+        return position;
+    }


[Comment]

position을 getter로 꺼내는 방식 보다 조금 더 이 값에 대해서 판단을 이 Position이란 객체에게 맡겨보는 것은 어떨까요?

위의 리뷰들을 참고해서 한번 변경해보면 좋을 것 같아요

dooboocookie · 2024-11-04T13:56:22Z

src/main/java/domain/ResultTypes.java

+public class ResultTypes {
+
+    private final List<String> resultTypes;
+
+    public ResultTypes(List<String> resultTypes, int width) {
+        validate(resultTypes, width);
+        this.resultTypes = resultTypes;
+    }
+
+    private void validate(List<String> resultTypes, int width) {
+        resultTypes.forEach(this::validateNotBlank);
+        validateSize(resultTypes, width);
+    }
+
+    private void validateNotBlank(String resultType) {
+        if (resultType.isBlank()) {
+            throw new IllegalArgumentException("실행 결과는 공백일 수 없습니다.");
+        }
+    }
+
+    private void validateSize(List<String> resultTypes, int width) {
+        if (resultTypes.size() != width) {
+            throw new IllegalArgumentException("실행 결과는 사다리의 개수와 일치해야 합니다.");
+        }
+    }
+
+    public List<String> getResultTypes() {
+        return resultTypes;
+    }
+}


[�Comment]

이 List은 List 같이 의도가 분명한 Wrapper클래스로 감싸지 않은 이유가 있나요?

Result에 대한 속성이 단순히 문자형 결과이고 이에 따른 책임이 유효성 검증밖에 없다고 생각해서 굳이 나누지 않았습니다

[Comment]

사실 위에서 한 유효성 검증은 ResultType 하나의 요소에 대한 검증이지, List 자체에 대한 검증은 아닌 것 같기도 하네요.

그치만, 저도 상희님이 말씀하신 것 처럼 꼭 래핑할 이유는 없다고 생각해요 해당 클래스만으로도 충분히 의미전달이 된다고 생각합니다

dooboocookie · 2024-11-04T13:58:45Z

src/main/java/view/InputException.java

@@ -0,0 +1,16 @@
+package view;
+
+public class InputException {


[Comment]

Input에 대한 Validate를 따로 분리시킨 이유가 있을까요?

원래는 Input에 대한 예외가 많을 줄 알고 나눴었는데 2개 밖에 찾아내지 못해서 그냥 합치는 게 나을 것 같네요 수정했습니다!

dooboocookie · 2024-11-04T13:59:45Z

src/main/java/view/InputView.java

+    public static String inputViewerName() {
+        System.out.println("\n결과를 보고 싶은 사람은?");
+        String viewerName = input.nextLine();
+        InputException.validateViewerNameNotBlank(viewerName);
+        return viewerName;
+    }


[Request Change]

유저는 실수로 잘못 입력한건데 프로그램이 에러로 꺼지는 것은 너무 가혹한 것 같아요.

재입력을 받아보게 바꿀 수 있을까요?

dooboocookie

상희님, 안녕하세요.
너무 오랜만에 2차리뷰를 드리는데요. 정말 많이 성장하셨다는 생각이 드네요.

컨트롤러 분리
생성 로직 분리
예외 상황?
enum 활용
equals hashcode 재정의

이정도 내용을 중점으로 다뤄봤습니다.
RC한번더 드릴테니 리뷰 반영 부탁드려요

dooboocookie · 2024-11-13T15:00:11Z

README.md

+# 기능 목록
+
+### 1. 플레이어 및 사다리 정보 입력 기능
+ 플레이어 이름과 결과 종류를 입력한다.
+ 사다리 넓이 : (플레이어 수 - 1)
+ 사다리 높이 : 사용자 입력
+### 2. 사다리 실행 기능
+ |-----|-----| 가로 라인이 겹치지 않게 사다리를 생성한다.
+=> 즉, 앞 라인이 true면 다음 라인은 무조건 false이다.
+### 3. 사다리 출력 기능
+ 플레이어 이름 입력 순으로 출력
+ 생성된 사다리 출력
+ 결과 종류 입력 순으로 출력
+ 보고 싶은 실행 결과 출력 ("all"일 때는 모두 출력)
+
+### <사다리 구현>
+ Boolean (다리)
+  + ture : 연결
+  + false : 연결 X  
+ List&lt;Boolean&gt (한 층의 다리 모음)
+  + 사다리의 넒이만큼
+  + 다리가 연결되어 있으면(=true) 수평 방향으로 이동이 가능하다.
+ List&lt;Line&gt (모든 층의 다리 모음)
+  + 사다리의 높이만큼


[Comment]

제가 코드를 잘 이해할 수 있도록 리드미에 의도를 잘 담아주셔서 감사합니다.😄

이 리드미는 훌륭한 가치와 내용을 담은 리드미라고 생각해요.

기능을 명세화할 떄 여러가지 방법 있겠죠?

유저 시나리오 대로 동작을 정의한다 (1-3번)

각각의 요소(모델)들이 갖는 규칙과 동작을 정의한다 (<사다리 구현>)

이런 측면에서 잘 구조화 해주신 것 같습니다.
개선할 점이라고 한다면, 조금 더 코드보단 비즈니스 규칙에 가치를 두는 것이 읽는 사람의 이해를 도울 수 있을 것입니다.
코드는 개발자의 의도에 따라 설계 등이 바뀔 수 있지만, 더 명확하게 전달해야되는 것은 비즈니스 요구사항이니까요.

dooboocookie · 2024-11-13T15:05:27Z

src/main/java/controller/LadderController.java

+    public void run() {
+        List<String> playerNames = InputView.splitString(InputView.inputNames());
+        List<String> kindOfResults = InputView.splitString(InputView.inputLadderResults());
+
+        // 플레이어 생성
+        Players players = new Players(playerNames);
+        // 결과 종류 생성
+        ResultTypes resultTypes = new ResultTypes(kindOfResults, players.getPlayersSize());
+        // 사다리 초기화
+        Ladder ladder = initializeLadder(players);
+
+        // 사다리 및 결과 출력
+        displayLadder(resultTypes, playerNames, ladder);
+        playGameAndDisplayResults(resultTypes, players, ladder);
+    }


[Comment]

🎮 컨트롤러 나누기 1

이전 리뷰 질문에도 답변을 달았습니다.

Controller는 저희가 게임의 흐름을 관리하도록 만들어진 애는 아니라고 생각해요.
만약 run이라는 것이 1개의 동작이라고 생각한다면 생각을 존중하겠습니다만,
저희는 Controller를 어떤 동작을 할 때 View와 Model의 협업을 위해 연결다리로 둔 대상이죠.

제가 이 밑에 하나의 동작이라고 생각하는 영역들을 묶어볼거에요.

dooboocookie · 2024-11-13T15:16:50Z

src/main/java/controller/LadderController.java

+        List<String> playerNames = InputView.splitString(InputView.inputNames());
+        List<String> kindOfResults = InputView.splitString(InputView.inputLadderResults());
+
+        // 플레이어 생성
+        Players players = new Players(playerNames);
+        // 결과 종류 생성
+        ResultTypes resultTypes = new ResultTypes(kindOfResults, players.getPlayersSize());
+        // 사다리 초기화
+        Ladder ladder = initializeLadder(players);


[Comment]

🎮 컨트롤러 나누기 2

이만큼이 한 동작(혹은 한 요청)에 해당 될 수 있을 것 같아요.

이제 어느정도 http api 도 익숙해지셨을테니,
Spring의 컨트롤러로 예시를 들겟습니다.

Suggested change

List<String> playerNames = InputView.splitString(InputView.inputNames());

List<String> kindOfResults = InputView.splitString(InputView.inputLadderResults());

// 플레이어 생성

Players players = new Players(playerNames);

// 결과 종류 생성

ResultTypes resultTypes = new ResultTypes(kindOfResults, players.getPlayersSize());

// 사다리 초기화

Ladder ladder = initializeLadder(players);

@PostMapping("/api/ladders")

public ResponseEntity<Void> createLadder(@RequestBody LadderSaveRequest request) {

// 요청

List<String> playerNames = request.playerNames;

List<String> kindOfResults = request.kindOfResults;

// 비즈니스 로직 호출

Players players = new Players(playerNames);

ResultTypes resultTypes = new ResultTypes(kindOfResults, players.getPlayersSize());

Ladder ladder = initializeLadder(players); // 메모리에 저장하거나, 영속화

// (추가) 응답 생성

return ResponseEntity.create(이 레더를 찾아갈수있는 경로)

}

당연히 이게 정답이라고 공감하실 필요도 없고, 하셔도 안됩니다.

이렇게 사다리 생성(or게임 시작 or 게임방 생성 or 사다리 저장)이라고 정의할 수 있는 독립적인 동작이 완성됐네요.

넵 특히 이 예시를 통해 아직 분리가 덜 됐다는 것이 확연히 보이네요!

dooboocookie · 2024-11-13T15:24:35Z

src/main/java/controller/LadderController.java

+        Ladder ladder = initializeLadder(players);
+
+        // 사다리 및 결과 출력
+        displayLadder(resultTypes, playerNames, ladder);


[Comment]

🎮 컨트롤러 나누기 3

Suggested change

displayLadder(resultTypes, playerNames, ladder);

@GetMapping("/api/ladders/{id}")

public ResponseEntity<Ladder> createLadder(@PathVariable Long id) {

// 비즈니스 로직, 찾는 작업... (아이디가 존재하지 않는 콘솔 어플리케이션이라해도)

Ladder ladder = 메모린지_디빈지_모를_저장소.findByIdOrNull(id);

// 응답 생성 -> 콘솔로 따지면 outputview가 출력할 수 있게 outputView로 전달

return ResponseEntity.�body(ladders)

}

dooboocookie · 2024-11-13T15:25:25Z

src/main/java/controller/LadderController.java

+
+        // 사다리 및 결과 출력
+        displayLadder(resultTypes, playerNames, ladder);
+        playGameAndDisplayResults(resultTypes, players, ladder);


[Comment]

🎮 컨트롤러 나누기 4

이 영역도 2-3과 같이 해볼 수 있겠죠?

dooboocookie · 2024-11-13T16:02:57Z

src/main/java/domain/Point.java

+public enum Point {
+    ENABLED(true),
+    DISABLED(false);
+
+    private final boolean enabled;
+
+    // 생성자를 통해 상태를 설정
+    Point(boolean enabled) {
+        this.enabled = enabled;
+    }
+
+    public boolean isEnabled() {
+        return enabled;
+    }
+
+    public static final Function<Boolean, Point> FROM_BOOLEAN = value -> {
+        if (value) {
+            return ENABLED;
+        }
+        return DISABLED;
+    };
+}


[Comment]

만약 제 훈수를 따라오셨다면, 해당 enum에 생성로직을 만드셨을텐데요.

현재 이 enum은 true false 값을 대신한다는 느낌만 갖고있어요. 역하과 책임이 빈약하다는 생각이 듭니다.

생성로직을 가져보기 (DISABLED가 될수밖에 없는 건 누구 책임이야...?)

사다리 이동에 대한 책임을 가질 수 있지 않을까?

이 리뷰에서 말씀드린 학습테스트 해보셨나요? => Point 이넘의 객체 별 동작을 정의하거나 할때 많은 도움이 될 것입니다.

이름도 조금 더 의미있는 값으로 바꿔주세요

아 이거 잘 모르겠어서 고민을 꽤 했는데 인터페이스를 구현해서 익명 클래스로 만드는 방식을 말씀하신 건가요? (아니라면,, 말씀해주세요 ㅎ)
일단 이 방식으로 수정했습니다. 사실 익명 클래스랑 인터페이스 사용이 익숙하지 않았었는데 이번 기회를 통해 어떻게 사용하면 좋은지 알게 된 것 같아요 감사합니다 !

dooboocookie · 2024-11-13T16:05:23Z

src/main/java/domain/Position.java

+    public void moveLeft() {
+        if (position != 0) {
+            position--;
+        }
+    }
+
+    public void moveRight(int maxPosition) {
+        if (position != maxPosition) {
+            position++;
+        }
+    }


[Request Change]

이정도 대처로 충분하신가요?
만약 0번 인덱스에서 moveLeft를 호출해버렸다면, 0인채로 한칸 내려가겠네요.
그렇다면 그건 그 나름대로 심각한 버그 아닐까요?

그렇게 프로그램 내부에서 런타임에 예상도 확인도 불가능한 에러가 나게 두면 0인 인덱스가 동일한 층에서 여러개나올 수도 있겠네요...
어떻게 개발자가 이런 위험들을 감지하면서 서비스 운영을 할 수 있을까요?

dooboocookie · 2024-11-13T16:08:37Z

src/main/java/domain/ResultTypes.java

+public class ResultTypes {
+
+    private final List<String> resultTypes;
+
+    public ResultTypes(List<String> resultTypes, int width) {
+        validate(resultTypes, width);
+        this.resultTypes = resultTypes;
+    }
+
+    private void validate(List<String> resultTypes, int width) {
+        resultTypes.forEach(this::validateNotBlank);
+        validateSize(resultTypes, width);
+    }
+
+    private void validateNotBlank(String resultType) {
+        if (resultType.isBlank()) {
+            throw new IllegalArgumentException("실행 결과는 공백일 수 없습니다.");
+        }
+    }
+
+    private void validateSize(List<String> resultTypes, int width) {
+        if (resultTypes.size() != width) {
+            throw new IllegalArgumentException("실행 결과는 사다리의 개수와 일치해야 합니다.");
+        }
+    }
+
+    public List<String> getResultTypes() {
+        return resultTypes;
+    }
+}


[Comment]

사실 위에서 한 유효성 검증은 ResultType 하나의 요소에 대한 검증이지, List 자체에 대한 검증은 아닌 것 같기도 하네요.

그치만, 저도 상희님이 말씀하신 것 처럼 꼭 래핑할 이유는 없다고 생각해요 해당 클래스만으로도 충분히 의미전달이 된다고 생각합니다

dooboocookie · 2024-11-13T16:10:00Z

src/test/java/domain/PlayerNameTest.java

+    @Test
+    public void 이름이_공백이면_예외를_던진다() {
+        assertThatExceptionOfType(IllegalArgumentException.class)
+                .isThrownBy(() -> new PlayerName(" "))
+                .withMessage("이름은 공백일 수 없습니다.");
+    }


[Request Change]

Suggested change

@Test

public void 이름이_공백이면_예외를_던진다() {

assertThatExceptionOfType(IllegalArgumentException.class)

.isThrownBy(() -> new PlayerName(" "))

.withMessage("이름은 공백일 수 없습니다.");

}

@Test

public void 이름이_공백이면_예외를_던진다() {

assertThatExceptionOfType(IllegalArgumentException.class)

.isThrownBy(() -> new PlayerName(" "))

.withMessage("이름은 공백일 수 없습니다.");

}

이렇게 하면 테스트 터져지네요🧐

dooboocookie · 2024-11-13T16:13:17Z

src/test/java/domain/PlayersTest.java

+        assertThat(players.getPlayersSize()).isEqualTo(2);
+        assertThat(players.getPlayers().get(0).getPlayerName()).isEqualTo("Test1");
+        assertThat(players.getPlayers().get(1).getPlayerName()).isEqualTo("Test2");


[Comment]

테스트는 독립적일 수록 좋다고 하는데요.

독립적인 테스트를 위해,
다른 테스트들과 격리하는 것도 중요합니다.
또한, 테스트 내부에서도 테스트의 성공/실패 여부가 여러개 있는것을 지양해야합니다.
어떤것이 문제인지 / 어떤것이 잘되는지 바로 파악이 어렵습니다.

해당 18-20번은 그런 부분이 아주 두드러지는 내용은 아니지만, 객체 자체를 비교하는 방식이나 하나의 단위라고 생각되는 만믐 AssertAll 같은 것들로 묶어주세요

dooboocookie

안녕하세요, 상희님.

정말 고생많으셨습니다.
난해한 리뷰임에도 불구하고, 잘 적용해주셔서 감사합니다.

이미 충분히 좋은 코드인 것 같아서 사다리미션은 이만 Approve하겠습니다.
더 나눠보고 싶은 대화가 있다면 언제든지 연락주세요.

🎯 리뷰포인트

테스트 코드

🔮 추가

enum에서 함수형 인터페이스 활용 방법에 대해서 더 궁금하신 내용이 있다면, 승준님 PR을 참고해주세요.

dooboocookie · 2024-11-24T07:57:14Z

src/main/java/domain/Players.java

+    private void validateDuplicateName(List<String> playerNames) {
+        Set<String> playerNamesSet = new HashSet<>(playerNames);
+
+        if (playerNames.size() != playerNamesSet.size()) {
+            throw new IllegalArgumentException("동명이인인 플레이어가 존재합니다.");
+        }
+    }


[Approve]

놓칠 수 있지만, 현재 서비스에선 중요한 로직이죠!
훌륭합니다

dooboocookie · 2024-11-24T07:59:56Z

src/main/java/domain/Position.java

+    public void moveRight(int maxPosition) {
+        if (position == maxPosition) {
+            throw new IllegalStateException("가장 오른쪽 사다리이므로 오른쪽으로 이동이 불가합니다.");
+        }
+        position++;
+    }


[Approve]

꼼꼼한 예외처리를 이용해 예상치못한 버그를 방지하는 것 매우 좋습니다!

추가적으로 해당 예외 상황에 대한 테스트를 추가해두면 누군가 해당 로직을 수정했을 떄, 잘못됨을 알아차릴수 있겠네요!!

dooboocookie · 2024-11-24T08:34:05Z

src/test/java/domain/LineTest.java

+    @BeforeEach
+    void 설정() throws Exception {
+        line = new Line(3);
+
+        Field ladderStepsField = Line.class.getDeclaredField("ladderSteps");
+        ladderStepsField.setAccessible(true);
+
+        List<LadderStep> fixedSteps = Arrays.asList(
+                LadderStep.CONNECTED,
+                LadderStep.NOT_CONNECTED,
+                LadderStep.CONNECTED
+        );
+        ladderStepsField.set(line, fixedSteps);
+    }


[Approve]

랜덤을 포함하는 테스틀 하기 쉽지 않으셨을텐데, 리플렉션을 활용하셔서 잘 해결하셨군요!!
정말 훌륭합니다!

Line이 갖고 있는 큰 로직은 2가지가 있는 것 같은데요.

생성

움직임

리플렉션으로 상태를 고정해서 생성하여, 2번은 원활이 할 수 있었네요!!
1번은 테스트는 어떻게 해도 테스트할 방법이 영 떠오르지 않네요.

어떻게 하면 좋을까요?
사실 Random이라는 것을 어떤 관점에서 보냐에 따라 다를 수 있을 것 같아요.

Random에 의한 Line 내 로직들이 아무 문제 없이 잘 일어날 수 있다고 판단되면, 굳이 필요하지 않은 고민일 수 있곘죠.

반대로 Line이나 Step 입장에선 Random이 그리 믿음직스럽지 않을 수도 있겠네요.
그럴땐, 도메인과 Random이 너무 강한 결합을 하고 있는지 의심해 볼 필요도 있어요.

특정 랜덤한 값이 생성되는 것은 Line이나 Step보다 밖에 있는 비즈니스 로직으로 판단하고
그 랜덤한 값에 의해서 Connected인지 Unconnected인지 판단하는 것을 도메인 내에 넣을 수도 있겠어요.

그래서 그냥 Line과 Random을 분리하는것이죠!

랜덤에 대한 테스트는 어떻게 ... 블로그글

이미 충분히 잘 대처하고 잘 작성한 테스트 코드라고 생각해서 별 RC는 안드리겠지만,

테스트할 대상이 잘 보이지 않는다면, 객체간의 결합도를 한번은 둘러볼 필요도 있겠네요.
(테스트 작성의 어려움 자체가 빨간불로 작용하는)

보내주신 글을 참고하여 생각해봤습니다!

사실 Random이라는 것을 어떤 관점에서 보냐에 따라 다를 수 있을 것 같아요.

랜덤값을 생성하는 메서드

랜덤값에 대한 객체

이렇게 두 가지로 랜덤에 대한 관점을 나눌 수 있겠네요

아래와 같이 랜덤한 수를 생성하는 객체(RandomNumber)를 RandomUtil 인터페이스의 구현체로 만들고, 이를 Car에게 주입시키도록 구조를 바꾸었다.

그 중 2번의 관점에서 본다면 구현체를 활용한 구조를 통해 Line과 Random의 결합도를 낮추는 방법이 있었네요!!
사실 첫 번째 자동차 미션 할 때 인터페이스를 활용해서 Random을 주입시키는 전략이 있다는 것을 들었었는데 잊고 있었어요 ㅎㅎ 덕분에 다시 한 번 랜덤 생성 단위 테스트에 대해 생각하고 정리해보는 시간을 가져서 이제는 안 까먹을 것 같아요 감사합니다!

SANGHEEJEONG added 30 commits October 25, 2024 00:59

[FEAT] Ladder & Line 클래스 생성

1196afa

[FEAT] Line 생성 메서드 작성 (1단계)

8b96693

[FEAT] LineView Enum 클래스 생성 (1단계)

f20be9f

[REFACTOR] domain과 view 분리 (1단계)

19995ef

[FEAT] Ladder 생성 메서드 작성 (1단계)

49d105c

[FEAT] OutputView 메서드 작성 및 Main 구현 (1단계)

090cb0f

[FEAT] InputView 메서드 작성 및 Main 구현 (2단계)

06a00a7

[FEAT] 사다리 방향 확인 메서드 작성 (3단계)

3e47a2a

[FEAT] 사다리 게임 실행 메서드 작성 (3단계)

07fe975

[FEAT] 사다리 게임 실행 메서드 수정 (3단계)

2265787

[FIX] 사다리 방향 결정 인덱스 오류 수정(3단계)

2fef17c

[FEAT] 4단계 입력 메서드 작성 (4단계)

f7ab094

[FEAT] Players 클래스 생성 (4단계)

dbbb328

[FEAT] GameResult 클래스 생성 및 OutputView 메서드 작성 (4단계)

65d58d2

[FEAT] main 작성 (4단계)

56b093f

[REFACTOR] decideWhereToGo 메서드 분리 (5단계)

5c6b21e

[REFACTOR] Player 객체 분리 (5단계)

67fcc5a

[REFACTOR] LadderGenerator 메서드 수정 (5단계)

74f098d

[REFACTOR] LadderGame forEach 람다 형식으로 바꿔보기(5단계)

9763754

[REFACTOR] OutputView 메서드 순서 수정(5단계)

a16726a

[REFACTOR] findPlayerByName 메서드를 Players로 책임 분리(5단계)

69871ca

[REFACTOR] OutputView 주석 및 수정 (5단계)

169ec00

[REFACTOR] Name, Position 원시값 포장 (5단계)

ea543eb

[REFACTOR] Name 예외처리 (5단계)

e81fa34

[REFACTOR] ResultTypes 예외처리 (5단계)

047d483

[REFACTOR] Players 예외처리 (5단계)

2dfe464

[REFACTOR] 입력 예외처리 (5단계)

42cdbd6

[REFACTOR] 불변객체 적용 (5단계)

62f451d

[REFACTOR] 단위테스트 작성 (5단계)

422af42

[REFACTOR] 코드 정렬 (5단계)

494e2d9

SANGHEEJEONG added 3 commits November 3, 2024 00:38

Create README.md

b2b1220

[REFACTOR] 리뷰 반영

7dbb8cd

[REFACTOR] README 작성

67b09e4

dooboocookie suggested changes Nov 4, 2024

View reviewed changes

SANGHEEJEONG added 9 commits November 7, 2024 03:29

[REFACTOR] README 수정

5c280c4

[REFACTOR] Controller 메서드 분리

8eac61b

[REFACTOR] 미사용 import 정리

beeb626

[REFACTOR] LadderGame 메서드 책임에 따른 클래스 분리

570c4d7

[REFACTOR] 접근지정자 수정 public -> private

6f6b09a

[REFACTOR] Boolaen -> Point ENUM 사용

95f8ec1

[REFACTOR] position 메서드 수정

f31cacc

[REFACTOR] 입력 메서드 수정

e43ee2d

[REFACTOR] LadderController 수정

7e4cd52

dooboocookie suggested changes Nov 13, 2024

View reviewed changes

SANGHEEJEONG added 12 commits November 17, 2024 00:25

[REFACTOR] LadderGenerator 삭제 및 Controller 메서드 분리

f8dce70

[REFACTOR] moveLeft & moveRight 예외 처리

ac02d9c

[REFACTOR] Controller 와 Application 책임 분리

d48bce2

[REFACTOR] Ladder 클래스 책임 부여

ea12de3

[REFACTOR] decideWhereToGo 메서드 line 클래스 이동

01b8efe

[REFACTOR] Point -> LadderStep 이름 변경 및 학습테스트(익명 클래스 & 인터페이스) 적용

7cf78a6

[REFACTOR] nextStep 미사용 인자 삭제

174ff32

[REFACTOR] 동명이인 예외 처리

6b26030

[REFACTOR] PlayerName VO로 만들기

1d96fb1

[REFACTOR] test 수정

0bd1928

[REFACTOR] test 수정

76e1d31

[REFACTOR] 코드 정렬

6473cca

dooboocookie mentioned this pull request Nov 24, 2024

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

Open

dooboocookie approved these changes Nov 24, 2024

View reviewed changes

-        displayLadder(resultTypes, playerNames, ladder);
+@GetMapping("/api/ladders/{id}")
+public ResponseEntity<Ladder> createLadder(@PathVariable Long id) {
+    // 비즈니스 로직, 찾는 작업... (아이디가 존재하지 않는 콘솔 어플리케이션이라해도)
+    Ladder ladder = 메모린지_디빈지_모를_저장소.findByIdOrNull(id);
+    // 응답 생성 -> 콘솔로 따지면 outputview가 출력할 수 있게 outputView로 전달
+    return ResponseEntity.�body(ladders)
+}

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

Are you sure you want to change the base?

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

Conversation

SANGHEEJEONG commented Oct 30, 2024 • edited Loading

0094-Gengar commented Nov 1, 2024

SANGHEEJEONG commented Nov 2, 2024

dooboocookie left a comment

Choose a reason for hiding this comment

🌻 리뷰 종류

[Request Change]

[Comment]

[Approve]

🙆‍♀️ 리뷰 방향성

💡 리뷰 포인트

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

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

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

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

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

🎮 컨트롤러 나누기 1

dooboocookie Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

🎮 컨트롤러 나누기 2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🎮 컨트롤러 나누기 3

Choose a reason for hiding this comment

🎮 컨트롤러 나누기 4

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

Choose a reason for hiding this comment

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

SANGHEEJEONG commented Oct 30, 2024 •

edited

Loading

dooboocookie Nov 13, 2024 •

edited

Loading