[2단계 - 사다리 게임 실행] 페드로(류형욱) 미션 제출합니다. #404

hw0603 · 2024-03-01T12:58:10Z

안녕하세요 피케이, 1단계에서 리뷰 주셨던 부분들을 수정하고, 2단계 요구사항 충족하도록 구현 완료하여 리뷰 요청 드려요.

1단계 머지된 이후에 해 주셨던 말씀들에 대해서 궁금한 사항들이 있어서 1단계 PR에 질문을 남겼었는데, 혹시 몰라 이곳에도 링크로 첨부할게요!

이번 2단계를 진행하면서 추가로 궁금해진 점은 다음과 같습니다!

RequestDto 사용 시 도메인과 DTO의 의존 관계

현재 적용되지는 않았지만 제일 처음 구상했던 설계는 Controller -> RequestDto -> LadderGame -> ResponseDto -> View 와 같은 구조였는데요.
이와 같이 '요청 받는 객체'를 별도로 두고 사용하는 경우가 분명 있을 듯 한데, 이는 모델이 DTO에 의존하게 되는 경우가 아닌가요? 이처럼 사용하는 것이 권장되지 않은 설계인 것인지, 아니면 이 역시도 결국 트레이트오프를 잘 따져보고 부분적으로 사용하는 것인지 궁금해요.

Swap 로직의 메서드 파라미터 타입 관련

내부적으로 Collections.swap() 을 사용하는 메서드를 작성했는데, 원본 리스트에 대한 swap이 일어나야 하므로 mutable list만 받아야 합니다.
해당 메서드 내에서 전달된 리스트가 mutable 한지 여부를 확인하고 immuatable이라면 예외를 던지는 방법도 있겠지만, 그것보다는 메서드 시그니처에서 "여기는 mutable한 리스트만 넘겨야 해" 라고 명시해 주거나, 적어도 immutable 리스트를 전달했을 때 컴파일 타임에 감지할 수 있도록 제한할 수 있다면 훨씬 좋을 것 같아요.
이것저것 찾아 봐도 간단한 해결책이 잘 안 보이는데, 피케이라면 혹시 어떻게 구현하실 건가요?
코드를 보시는 게 좋을 것 같아 해당 부분에 코멘트 달아 놓겠습니다!

결과를 볼 사람을 입력할 때 `all` 이라는 문자열 상수는 어디서 관리해야 할까요?

해당 상수가 사용되는 곳은 다음 두 곳입니다.

Player에서 all 이라는 이름의 사용자의 생성을 금지할 떄
컨트롤러에서 뷰 호출 시 입력된 값에 따라 호출할 메서드를 결정할 때

현재는 두 곳 모두 독립적으로 유지하고 있는데, 나중에 all이 everyone 등으로 변경될 수 있다는 것을 생각해 보면 수정해야 할 곳이 여러 곳이라는 건 별로 좋지 못한 것 같아요.
그렇다고 한 쪽으로 넣자니 public 상수를 만들어야 한다는 문제가 생기구요.

프리코스 때는 이렇게 프로그램 전반적으로 사용되는 상수들은 /constants 패키지에 클래스를 하나 만들고 그곳에서 public으로 관리했었는데, 이번에는 상수 하나를 위해서 새 패키지와 클래스를 만들어야 하나? 라는 생각이 들었습니다. 물론 그렇게 해도 public 상수가 여전히 사용된다는 점은 바뀌지 않구요😅
이 상수를 어떻게 관리해야 할까요?

- Ladder -> LadderController

- 기존) leftPath, rightPath 모두 connected 면 경로가 있는 것으로 판단 - 변경) leftPath는 RIGHT, rightPath는 LEFT 여야 경로가 있는 것으로 판단 - 예외 케이스 ) R L R L 에서 "L R" 이 경로가 있는 것으로 판단됐었음

- 원래 LadderGame에서 계산했었으나, Line이 담당하도록 변경

hw0603 · 2024-03-02T06:22:29Z

src/main/java/ladder/model/Line.java

+    // TODO: 메서드 시그니처로 mutable 리스트만을 받는다는 사실을 전달할 수 없나?
+    // 또는 적어도 immutable List가 전달되었을 때 컴파일타임에 감지할 수 있게는?
+    public <T> List<T> climbDown(List<T> initialPosition) {
+        List<Boolean> isConnected = this.getConnected();
+        List<Integer> connectdIndices = IntStream.range(0, isConnected.size())
+                .filter(isConnected::get)
+                .boxed()
+                .toList();
+
+        for (int connectedIndex : connectdIndices) {
+            Collections.swap(initialPosition, connectedIndex, connectedIndex + 1);
+        }
+
+        return initialPosition;
+    }
+


Swap 로직의 메서드 파라미터 타입 관련

이 부분입니다!

Q. immutable 리스트를 전달했을 때 컴파일 타임에 감지할 수 있도록 제한할 수 있다면 훨씬 좋을 것 같아요.
그 목적을 달성할 수 있는 방법이 있기는하죠. 파라미터로 List 를 받지 않고 ArrayList 와 같이 원하는 특징을 가진 List 의 구현체를 특정지으면 됩니다. 이게 '컴파일 타임에 감지' 라는 목적을 이루게 해주는 방법이니까요 😂

Q. 이것저것 찾아 봐도 간단한 해결책이 잘 안 보이는데, 피케이라면 혹시 어떻게 구현하실 건가요?
일단 저라면 Swap 을 사용하지 않을 것 같습니다. LadderGame 쪽에도 리뷰 몇 개 달아놨어요. 전체적으로 사다리 게임의 결과를 도출하는 과정을 다시 고민해보는게 어떨지 제안드려요. LadderPath 에 direction 값도 만들었는데 사용되지 않고 있어요...!

앗 제가 이걸 언급하는걸 깜빡했네요😅 파라미터 타입으로 ArrayList를 명시적으로 지정했더니 바로 경고가 뜨더라구요..ㅎㅎ

특정 구현체를 명시하는 것이 권장되지 않는 기법인 것 같아서 혹시 다른 방법이 있나 싶어서 질문드렸었습니다..!

이미 시도해보셨군요! 말해주신 것처럼 권장되지 않아요. 근데... 목적이 있었으니까...

저거 외에는 따로 생각나는 방법은 없어요. 🤔
javadoc 을 써서 메서드를 호출하는 개발자에게 “파라미터로 넘겨주는 리스트는 mutable 해야합니다” 라고 명시해주는 것도 방법이에요.

일단 저라면 Swap 을 사용하지 않을 것 같습니다. LadderGame 쪽에도 리뷰 몇 개 달아놨어요. 전체적으로 사다리 게임의 결과를 도출하는 과정을 다시 고민해보는게 어떨지 제안드려요. LadderPath 에 direction 값도 만들었는데 사용되지 않고 있어요...!

public List<Integer> climbDown(List<Integer> initialPosition) { List<Integer> positionAfterClimb = new ArrayList<>(IntStream.range(0, size()).boxed().toList()); for (int i = 0; i < initialPosition.size(); i++) { int nextIndex = i+row.get(i).direction; int value = initialPosition.get(i); positionAfterClimb.set(nextIndex, value); } return positionAfterClimb; }

direction 값을 최대한 활용하려고 해 봤는데, 특정 인덱스로 값을 설정해 줘야 해서 어째 코드가 더 복잡해 진 것 같아요😅 개선할 수 있는 방법이 있을까요?

책임과 로직을 다른 클래스로 위임해볼 수 있을 것 같아요. 객체지향적으로 코드를 짜고 싶으니까요. 그런데 이건 시간이 많이 없으니 꼭 진행해주시지 않아도 괜찮아요.

hw0603 · 2024-03-02T07:21:06Z

src/main/java/ladder/model/Line.java

+    // TODO: 매 번 새로 계산해서 반환해야 하는가?
    public List<Boolean> getConnected() {
        return IntStream.range(0, row.size() - 1)
-                .mapToObj(idx -> row.get(idx).equals(RIGHT))
+                .mapToObj(idx -> LadderPath.isPathExist(row.get(idx), row.get(idx + 1)))
                .toList();
    }


Line.row는 final 필드이기 때문에, 한 번 생성된 이후로는 변경될 일이 없어요. 때문에 row 에 대한 다른 관점에서의 표현 구조인 isConnected 리스트 역시 생성 시점에 그 결과를 확정할 수 있는데요, 이런 구조일 경우에 getConnected() 내에서 항상 새로 계산하여 반환하는 게 맞는가에 대한 의문이 들었습니다.

메서드 호출을 통해 매 번 계산할 경우 -> 의미 없는 중복 연산이 발생

필드에 저장해 둘 경우 -> 필드를 하나 늘려야 함 -> 결국 같은 의미의 데이터를 중복 저장하고 있는 것이 아닌지?

2번을 선택하여 필드를 하나 사용한다고 해도 두 가지 접근이 있을 것 같아요.

2-1. 생성자에서 계산하여 값을 할당
2-2. 싱글톤과 유사하게 접근하여 초기 상태에서는 null로 초기화 해 두고, 최초 getConnected() 호출 시 필드값이 null이면 계산 후 캐싱

피케이라면 어떤 선택을 하실 지, 그리고 그 선택의 이유가 궁금합니다!

오 캐싱이라는 주제까지...! 정말 좋은 질문이네요.
사실 저라면 그냥 두고 나중에 성능적으로 과부하가 온다면 그 때 개선할 것 같긴합니다.

그런데 실제로 그 상황이 왔다면, 우선 생성자 파라미터로 받지는 않을거에요. 굳이 객체 생성할 때 connected 관련 요소까지 고려하고 싶지 않고, 외부적인 요소에 문제가 있어서 List<LadderPath> 와 connected 정보간의 불일치가 발생하는걸 막고 싶어요. 그러기 위해서는 꼭 connected 정보는 List<LadderPath> 로부터 도출해야합니다.

문제가 발생한다는건 getConnected 연산이 지나치게 무겁거나 너무 많이 호출되기 때문일텐데, getConnected 때문에 생기는 과부화를 최소화하기 위해서는 아예 Line 객체를 생성할 때 미리 연산해두고 캐싱해놓는게 좋을 것 같기도 해요. 그러니 생성자 파라미터로 받지는 않더라도 생성자에 connected 관련 할당 로직은 있겠네요. 정적 팩토리 메서드를 사용해볼 수도 있을 것 같구요.

climbDown 메서드가 for 문을 돌면서 여러번 호출하기 때문에 이 고민을 하신 것 같네요 :)
덕분에 저도 재밌는 생각해봤습니다 ㅋㅋㅋ

현재는 생성자 파라미터로 List<LadderPath> 만 받고, 생성자에서 그에 따라 connected 리스트를 구축해 주는 것이 최선일 것 같네요:) 그렇게 수정했습니다!

hw0603 · 2024-03-02T07:23:37Z

src/main/java/ladder/service/LadderGame.java

+
+    private static boolean playerAndLadderDoesNotMatch(Players players, Ladder ladder) {
+        // TODO: 게터 깊이 줄이기
+        return players.getSize() != ladder.getLadder().get(0).size();


ladder.getLadder().get(0).size() 와 같이 연속적인 메서드 호출이 일어나고 있는데요, 이 부분을 줄이기 위해서 + Ladder.getLadderWidth()와 같은 메서드를 추가하는 것이 좋을까요?

return players.getSize() != ladder.getWidth();

처럼 Ladder 안에 메서드를 하나 만들어주면 좋겠네요! Ladder.getLadderWidth() 라고 하신게 정적 메서드를 의미하신게 아니라 해당 클래스 내에 인스턴스 메서드를 말씀하신거죠?

아아 넵 인스턴스 메서드를 말했던 게 맞습니다!

pkeugine

안녕하세요 페드로!
step 1 에서 저희 정말 많은 대화를 나눴네요.
그 때 마무리하지 못한 것 + 이번 step 에서 추가로 생긴 질문들을 포함하니 완전 양이 방대하더라구요!
그래도 덕분에 저도 오랜만에 우테코 페어프로그래밍 하는 느낌이 들었어요.
혹시라도 제가 남긴 리뷰에 대해 궁금한 점이 있다면 언제든지 DM 주시구요,
만약 제 리뷰에 코멘트를 추가로 달더라도 DM 주세요. 제가 깃허브에 종종 들어오긴 하는데 댓글 추가하시고 기다리는 것보단 제가 바로 답변하는게 좋은 것 같아서요 :)

step 2 파이팅이고 남은 연휴 잘 보내시길 바랍니다!

pkeugine · 2024-03-02T14:04:51Z

src/main/java/ladder/controller/LadderController.java

-    private Players ladderPlayers;
    private Ladder ladder;
+    private LadderGame ladderGame;


LadderGame 이 생기면서 Players 를 LadderGame 쪽으로 옮기셨는데요,
Ladder 는 그렇게 하지 않은 이유가 있을까요? LadderGame 에도 controller 에서 쓰이는 Ladder 객체를 필드로 두고 있고, LadderController 에서는 LadderGame 을 만드는 과정 외엔 dto 변환할 때만 ladder 가 쓰여요.

사실 저도 둘 다 빼거나 둘 다 남겨두거나 하나로 통일하고 싶었는데요..
Ladder는 말씀하셨던 그 DTO 변환할 때 사용되어서 남겨뒀었어요.

DTO 변환 로직에서 ladder.getladder()처럼 이미 게터로 데이터를 가져오고 있어요. 이를 ladderGame에서 가져오려면 ladderGame.getladder().getladder() 와 같은 구조가 되거나 ladderGame.getLines() 가 있어야 하는데, 전자는 게터가 반복 사용되고 있고 후자는 '엄연히 game -> ladder -> line -> path 라는 계층구조를 가지고 있는데 중간 계층을 무시하고 game에서 바로 line을 외부로 알려주는게 맞나?' 라는 생각이 들구요

보다 근본적으로 생각해 보면 애초에 Ladder를 컨트롤러에서 생성하여 LadderGame을 만든 것인데, 출력할 때 Ladder가 필요하다고 해서 굳이 LadderGame안에 있는걸 가져와야 하나 싶었어요.
Players 같은 경우는 객체 자체를 가져오는 게터라기보다는 출력에 필요한 List<String> playerNames을 조회하는 .getPlayerNames() 를 사용했기 때문에 필드에서 제거하고 LadderGame 안으로 넣는 데 큰 거부감이 없었던 면도 있었네요😅

단순히 한 방법으로 통일하면 되는 걸까요? 아니면 컨트롤러에서 넣어준 값들을 컨트롤러에서 쓰는데 게터로 가져와야 하더라도 필드 수를 줄일 수 있다면 LadderGame 안으로 아예 이동시키는게 맞을까요?

(우선 Ladder도 LadderGame 안으로 이동시키는 것으로 수정해두겠습니다!)

pkeugine · 2024-03-02T14:04:58Z

src/main/java/ladder/controller/LadderController.java

        int height = readLadderHeight();
        int width = ladderPlayers.getSize();
+
+        List<String> rewards = readRewards();
        ladder = Ladder.of(height, width);
+        ladderGame = LadderGame.from(ladderPlayers, rewards, ladder);


Suggested change

int height = readLadderHeight();

int width = ladderPlayers.getSize();

List<String> rewards = readRewards();

ladder = Ladder.of(height, width);

ladderGame = LadderGame.from(ladderPlayers, rewards, ladder);

int height = readLadderHeight();

int width = ladderPlayers.getSize();

ladder = Ladder.of(height, width);

List<String> rewards = readRewards();

ladderGame = LadderGame.from(ladderPlayers, rewards, ladder);

step 1 에서 남겼던 리뷰처럼 이번에도 초기화와 사용처를 가깝게 배치할 수 있을 것 같네요 :)

이제는 Ladder 필드가 사라지면서 ladder 역시 초기화 구문이 돼 버렸네요ㅎㅎ
앞으로 꼼꼼히 체크하겠습니다!

pkeugine · 2024-03-02T14:05:03Z

src/main/java/ladder/controller/LadderController.java

+    private void showLadder() {
        OutputView.printResultDescription();
-        OutputView.printPlayerNames(ladderPlayers.getPlayerNames());
-        OutputView.printLadder(ladder.toLineDtoList());
+        OutputView.printPlayerNames(ladderGame.getPlayerNames());
+
+        List<LineDto> lineDtos = ladder.getLadder().stream()
+                .map(LineDto::from)
+                .toList();
+        OutputView.printLadder(lineDtos);
+        OutputView.printRewards(ladderGame.getRewards());
+    }


OutputView 의 메서드를 controller 가 단계별로 호출하게 만들면 view 쪽에 변화가 생겼을 때 controller 와 view 를 모두 수정해야 하는 상황이 생길 수 있을 것 같아요. 아예 OutputView 쪽에 playerNames, lineDtos, 그리고 rewards 를 다 주고 뭔가 변화가 생겼을 때 view 만 수정할 수 있게 만들어보는건 어떨까요? 출력 화면을 만들 재료를 다 주고 view 가 알아서 만들도록 하는거죠.

생각해보지 못했던 내용인데 좋은 것 같아요. 반영했습니다!

만약 playerNames, lineDtos, 그리고 rewards 뿐만 아니라, View에서 필요로 하는 데이터들이 아주 많아지면 어떻게 되나요? 메서드의 파라미터 개수가 아주 많아질 것 같은데 이런 경우에 DTO로 한번 더..감싸서 전달하면 될까요?

네, 많은 정보를 전달하게 될 때 새로운 dto 에 기존 것들을 감싸서 보내주기도 해요.

pkeugine · 2024-03-02T14:11:27Z

src/main/java/ladder/controller/LadderController.java

+    private void showReward(String target) {
+        Map<String, String> result = ladderGame.play();
+
+        if (target.equals("all")) {


Q. 결과를 볼 사람을 입력할 때 all 이라는 문자열 상수는 어디서 관리해야 할까요?

지금 만들어주신 구조로 봤을 때 저는 별도로 관리할 것 같아요.

모든 사용자의 결과를 보여주고 애플리케이션을 종료하는 all

사용자 이름으로 사용되면 안되는 all

둘은 서로 다른 방식으로 사용되고 있다고 생각하거든요. 물론 all 이라는 표현이 everyone으로 바뀐다면 두 곳을 수정해야해요. 하지만 사용되면 안되는 사용자 이름 역시 all 말고도 더 많은 이름이 추가 될 수도 있고, 사용자 편의성을 위해 all 과 everyone 둘 다 애플리케이션 종료 단어로 사용될 수도 있어요. 그렇게 되면 사용자 이름에 all 이 포함된다면 종료 단어는 everyone 만 받을 수도 있구요. 그래서 저는 그냥 따로 두고, 이름이나 종료 단어가 추가된다면 list 형태로 각각 바꿔서 관리할 것 같습니다 😄

그런데 isProhibitedName 이라는 메서드를 Player 클래스에서 잘 분리해주셔서 위와 같은 작업을 할 때 편하겠다는 생각도 했어요 👍

pkeugine · 2024-03-02T14:12:53Z

src/main/java/ladder/model/Ladder.java

-        if (randomPath.size() < width) {
-            randomPath.add(LadderPath.STAY);
-        }


사다리 생성 로직에서의 경곗값 테스트?

오 로직을 작성하신 의도를 이제 완벽히 이해했어요! 일타 강사 느낌이네요 😂

Q. 메서드의 반환값을 잘 활용해 보라는 리뷰를 받은 적이 있어요. 위 사례의 fillPathifNotEnough() 와 같이 이미 존재하는 리스트에 조건에 따라 값을 추가하거나, 추가하지 않아야 하는 함수의 경우 반환값을 어떻게 활용할 수 있을까요?

페드로가 제안해주신 첫번째 메서드는 페드로가 말해주신대로 의도는 좋았으나 의미없는 값을 반환하게 되고, 파라미터를 메서드의 return 객체로 만드는 것도 지양해야겠죠.

제안해주신 두번째 메서드는 리뷰와 상황 사이에서 딜레마가 느껴지네요 ㅋㅋㅋ
조건에 따라 무언가를 반환해야 하는데, 그 조건이 반환하는 메서드에 함께 있으면 무조건 조건에 해당하지 않는 상황에서도 무언가를 반환해야해요. 예외를 던지는 방법도 있긴한데 list 가 다 만들어진 상황이 예외는 아니죠. 그래서 조건에 해당하지 않을 때는 아무것도 반환하고 싶지 않다면 if 문은 메서드 밖으로 빠져야합니다. 그래서 이런 상황에서는 페드로가 만들어주신 방식대로 해도 괜찮다고 저는 생각해요. 그 방식을 지양하는게 중요하다면 메서드 분리를 하지 않고 원래대로 다시 돌려놓는 방법도... 있구요 ㅋㅋㅋ

별개로 페드로가 설명해주신 Row 가 추가되는 과정을 보니까

연결되어 있다면 2개의 row 가 한번에 생성되고

연결되어 있지 않다면 1개의 row 가 생성되니

width 가 짝수인지 홀수인지 가릴거 없이 이전에 만들어지는 row pair 에 따라 마지막 row 가 pair 인 상태로 만들어질 수 있고

pair 상태로 만들어지지 않았다면 width 보다 크기가 하나 작은 상태로 generatePairableRandomPath 가 List 를 반환한다는 거죠.

그런데 페드로의 해설을 읽다보니 '연결되어 있을 때 굳이 두 개를 한 번에 넣어줄 필요가 있을까?' 라는 생각이 들었어요. 하나씩 넣어주면 generateRandomPath() 에 if 문이 하나 추가되더라도 width - 1 이나 fillPathifNotEnough 가 사라지면서 로직이 훨씬 직관적일 것 같거든요. (직접 페드로의 코드를 수정해봤어요) 사실 제가 구현했더라면 페드로의 방식을 떠올리지 못했을만큼 굉장히 신기한 방식이라는 생각이 드는데, 유지보수 면에서는 신기한 방식보다는 직관적인 방식이 더 유리하지 않을까라는 생각도 들어요. 이건 그냥 제 생각이니 반영하지 않으셔도 괜찮아요!

유지보수 면에서는 신기한 방식보다는 직관적인 방식이 더 유리하지 않을까라는 생각도 들어요.

이 말에는 동의합니다! 그런데 저는 처음 미션을 진행했을 때 부터 두 개씩 넣는 걸로 생각을 먼저 해서 그런지 하나씩 넣는 게 훨씬 생각하기가 어렵더라구요..? ~~(구현하다가 코드가 너무 복잡해져서 롤백시켰습니다ㅠㅠ)~~

추가) 제 생각대로 구현해 봤는데 요런 느낌을 말씀하신게 맞는지 모르겠네요ㅎㅎ

private static List<LadderPath> makeRandomRow(int width) { List<LadderPath> randomPaths = new ArrayList<>(); while (randomPaths.size() < width) { randomPaths.add(generateRandomPath(randomPaths, width)); } return randomPaths; } private static LadderPath generateRandomPath(List<LadderPath> currentPaths, int width) { if (isLastPathConnected(currentPaths)) { return LadderPath.LEFT; } boolean isLastElement = currentPaths.size() == width - 1; if (isLastElement) { return LadderPath.STAY; } boolean isConnectedPath = random.nextBoolean(); if (isConnectedPath) { return LadderPath.RIGHT; } return LadderPath.STAY; } private static boolean isLastPathConnected(List<LadderPath> currentPaths) { if (currentPaths.isEmpty()) { return false; } LadderPath lastPath = currentPaths.get(currentPaths.size() - 1); return lastPath.equals(LadderPath.RIGHT); }

오 네 맞아요! 그런데 페드로의 얘기를 들어보니 너무 제 기준으로 로직을 맞추는거 아닌가 하는 생각도 드네요. 오히려 이 방식이 더 복잡하다 싶으면 적용하지 않으셔도 괜찮습니다!

메서드 하나를 줄일 수 있어서 저도 이 편이 더 맘에 들어요ㅋㅋㅋ👍

다만 swap이 아닌 direction을 활용하여 사다리를 타면서, 언급하셨던 안티패턴을 사용하지 않는 방법은 잘 모르겠어서 아래에 질문 남겨두었습니다..!

pkeugine · 2024-03-02T14:31:59Z

src/main/java/ladder/service/LadderGame.java

+
+    private static boolean playerAndLadderDoesNotMatch(Players players, Ladder ladder) {
+        // TODO: 게터 깊이 줄이기
+        return players.getSize() != ladder.getLadder().get(0).size();


return players.getSize() != ladder.getWidth();

처럼 Ladder 안에 메서드를 하나 만들어주면 좋겠네요! Ladder.getLadderWidth() 라고 하신게 정적 메서드를 의미하신게 아니라 해당 클래스 내에 인스턴스 메서드를 말씀하신거죠?

pkeugine · 2024-03-02T14:32:04Z

src/main/java/ladder/service/LadderGame.java

+        for (Line line : ladder.getLadder()) {
+            position = line.climbDown(position);
+        }


ladder.getLadder 를 한 뒤에 LadderGame 에서 for 문을 도는 것보다 그냥 ladder 에게 Map 형태, 또는 List 형태로 결과를 반환해달라고 하는건 어떨까요? 잘 생각해보면 ladder 는 '사다리 타기 결과' 를 도출하기 위해 List 형태의 playerNames 조차도 필요 없어요. 이미 height, width, 사다리의 구조가 다 만들어져 있기 때문이에요.

굳이 게터를 통해 꺼내와서 직접 일을 할 필요가 없겠네요. LadderGame에서 ladder.climb()을 호출하여 결과를 받아오는 구조로 변경하였습니다!

pkeugine · 2024-03-02T14:32:07Z

src/main/java/ladder/service/LadderGame.java

+        List<String> position = new ArrayList<>(players.getPlayerNames());
+
+        for (Line line : ladder.getLadder()) {
+            position = line.climbDown(position);
+        }
+        return createGameResult(position);


각 라인을 지날 때마다 position을 조정하는 방식이네요
그런데 특정 변수를 초기화한 뒤에 그걸 이용해서 변수를 재할당하는 것은 지양해야할 안티패턴이에요.
for 문의 각 단계마다 positions 리스트가 어떤 상태인지 예측하기가 어려워서 유지보수 난이도가 올라가기 때문인데요, 다른 방식으로 결과를 도출해볼까요?

이렇게 사용하는게 안티패턴이였군요..! 새로운 사실 알아 갑니다 🙂

그런데 구조가 잘못된 것인지 어떻게 변경해야 할지 잘 와닿지 않아요. 조금만 더 설명해 주실 수 있으실까요?

위치를 조정하긴 하는데 재할당을 하지 않으려면, 이전의 위치정보를 List<List<Integer>> history 같은 곳에 계속 add() 해 주고, 호출 시점에는 항상 history의 마지막 요소만을 참조하는 아래와 같은 방법밖에 떠오르지 않네요😅

public List<Integer> climb() { List<Integer> initialPositions = new ArrayList<>(IntStream.range(0, getWidth()) .boxed() .toList()); List<List<Integer>> history = new ArrayList<>(List.of(initialPositions)); for (Line line : ladder) { List<Integer> positionAfterClimb = line.climbDown(history.get(history.size() - 1)); history.add(positionAfterClimb); } return history.get(history.size() - 1); }

pkeugine · 2024-03-02T14:32:15Z

src/test/java/ladder/service/LadderGameTest.java

+    private Players generatePlayersOf(int count) {
+        return Players.from(
+                IntStream.range(0, count)
+                        .mapToObj(i -> "P" + i)
+                        .toList()
+        );
+    }


stream 을 파라미터 값으로 사용할거라면 지역변수를 분리해서 적어보는게 어떨까요?

수정했습니다👍

pkeugine · 2024-03-02T14:32:19Z

src/test/java/ladder/service/LadderGameTest.java

+        assertThatCode(() -> LadderGame.from(players, rewards, ladder))
+                .doesNotThrowAnyException();
+    }
+}


상수화된 네이밍 컨벤션

Random 이 불변일줄 알았는데 남겨주신 코멘트를 보고 구현을 보니 아니더라구요 🤔
저도 리뷰하면서 배우네요! 👍

pkeugine · 2024-03-02T16:26:42Z

Q. 상수 하나를 위해서 새 패키지와 클래스를 만들어야 하나? 라는 생각이 들었습니다. 물론 그렇게 해도 public 상수가 여전히 사용된다는 점은 바뀌지 않구요

이 질문에 대한 대답이 누락되었네요!
"all" 을 쓰이는 곳마다 두지 않고 상수로 만들어서 관리하고 싶으시다면, 사실 그렇게 해도 큰 상관은 없을 것 같아요. 실제로 꽤나 다양한 방법이 존재하고 팀마다 방식이 다 다를 수도 있어요. 이것 하나만을 위해서 패키지와 클래스를 만드는게 저도 조금 과하다고 생각이 들긴 하지만, 구조 관련 선택에는 늘 얻는 것과 잃는 것이 생기잖아요. 그냥 public 으로 열고 controller 에서 사용해도 문제 없어요!

- LadderGame에서 Ladder를 게터로 가져오지 않고 Ladder.climb()을 통해 사다리를 탄 이후의 결과를 반환하도록 메시지 전달

pkeugine

안녕하세요 페드로!
고생하셨어요. 남겨주신 몇 가지 질문에 대한 답변을 남겼는데요,
혹시나 변경하고 싶으신 부분이 있을까 싶어 마지막으로 request change 남길게요. 언제든지 다시 리뷰 요청 남겨주세요!

- 기존: 각 위치의 List 를 전달받아 Line 에서 일괄 변경 - 리팩토링 후: Line 에서는 특정 위치에 대한 다음 위치만 계산하고, Ladder 에서 일괄적으로 위치를 변경

hw0603 · 2024-03-05T16:18:53Z

Line 이 리스트 형태의 데이터라 그런지 그동안 계속 'Line은 리스트를 받아서 위치를 바꾸고, 리스트를 반환할 수 있어야 한다' 라는 생각에 갇혀 있었어서 오히려 다른 객체들에서 사용하기가 힘들어졌던 것 같아요😂

한 위치를 전달받고, 다음 위치를 반환하는 형태로 책임을 살짝 수정하니 테스트 코드 짜기도 수월하고 해당 메서드를 사용하는 부분의 구현도 훨 깔끔해졌네요ㅎㅎ

변경하고 싶었던 부분은 이것을 마지막으로 다 해결하여 다시 리뷰 요청 드리도록 하겠습니다.
꼼꼼하고 자세한 리뷰 감사했어요 피케이!

pkeugine

고생하셨어요 페드로 🙂
저도 재밌는 대화 많이 해볼 수 있어서 좋았습니다.
이 대화가 다음 블랙잭 미션에서 도움이 되었으면 좋겠네요. 파이팅입니다 👍

hw0603 added 16 commits February 28, 2024 19:27

style(lineTest): 테스트 메서드 설명을 명확하게 변경

1f48eb7

refactor(ladder): view로 전달하기 위한 DTO의 생성 위치 변경

621c559

- Ladder -> LadderController

refactor(ladder): 랜덤 경로 생성 후 부족한 경로를 채우는 로직의 메서드 분리

10cfc58

refactor(LadderPath): LadderPath enum에서 방향과 연결 여부를 모두 관리하도록 변경

d18fcdd

docs(README): 2단계(사다리 게임 실행) 기능 요구사항 명세 추가

006a734

feat(player): 사용자 이름이 'all' 일 경우 예외 처리

0ce5689

feat(ladderGame): 사다리 한 줄을 내려오는 기능 구현

39e37ec

feat(LadderGame): 참여자들의 최종 도착지를 계산하는 기능 추가

f99df60

feat(inputView): 결과를 보고 싶은 사람을 입력받는 기능 구현

f98adf7

feat: Line 하나를 내려왔을 때 위치를 구하는 기능 구현

72af0bf

- 원래 LadderGame에서 계산했었으나, Line이 담당하도록 변경

feat(ladderGame): 사다리 전체를 내려오는 기능 구현

a587612

refactor(ladderController): 불 필요 필드 제거

bd777cc

feat(OutputView): 실행 결과 출력 기능 구현

64be7f9

feat(ladderGame): LadderGame 생성 시 참여자 수, 실행 결과 수, 사다리 폭에 대한 검증 추가

a7e4b26

refactor(ladderGameTest): Players, Rewards 생성 로직 메서드 추출

0932791

hw0603 marked this pull request as draft March 1, 2024 12:58

hw0603 commented Mar 2, 2024

View reviewed changes

hw0603 marked this pull request as ready for review March 2, 2024 07:23

pkeugine requested changes Mar 2, 2024

View reviewed changes

hw0603 added 7 commits March 3, 2024 01:31

comment: 미 사용 TODO 삭제

87b4691

refactor(line): connected 리스트를 인스턴스 생성 당시 계산하도록 변경

6caa9fc

refactor(ladder): Ladder에서 자신의 폭을 반환할 수 있도록 변경

27e6322

refactor(ladderController): Ladder 필드 삭제

f4ce0d4

refactor(outputView): 생성된 사다리를 출력하는 형태를 View가 직접 결정하도록 변경

a403b73

refactor(ladderController): 상수 포장

b559835

fix: fix typo

0352e17

hw0603 added 7 commits March 3, 2024 02:33

refactor(lineDto): Ladder를 List<LineDto>로 변환하는 팩토리 메서드 추가

8f9d7cd

style(ladderGameTest): 파라미터로 사용되는 스트림의 지역 변수 추출

9d12d61

refactor(Ladder): 랜덤 경로 생성 시 루프 순회 한번 당 하나의 경로를 추가하도록 변경

43cdd0a

refactor(ladderGame): 사다리를 타도록 하는 주체를 LadderGame 에서 Ladder로 변경

4fc0172

- LadderGame에서 Ladder를 게터로 가져오지 않고 Ladder.climb()을 통해 사다리를 탄 이후의 결과를 반환하도록 메시지 전달

refactor(line): climeDown() 에서 swap 방식 대신 Path 내의 direction 값을 활용하도록 변경

ad64fa5

style(lineTest): 테스트 메서드 설명 변경

63a7802

style(line): 코드 스타일 수정

2432fd4

pkeugine requested changes Mar 5, 2024

View reviewed changes

refactor(line, ladder): 사다리를 내려오는 방식 변경

1a255a0

- 기존: 각 위치의 List 를 전달받아 Line 에서 일괄 변경 - 리팩토링 후: Line 에서는 특정 위치에 대한 다음 위치만 계산하고, Ladder 에서 일괄적으로 위치를 변경

pkeugine approved these changes Mar 5, 2024

View reviewed changes

pkeugine merged commit e387b1e into woowacourse:hw0603 Mar 5, 2024

hw0603 deleted the step2 branch March 17, 2024 09:03

hw0603 restored the step2 branch March 17, 2024 09:03

[2단계 - 사다리 게임 실행] 페드로(류형욱) 미션 제출합니다. #404

[2단계 - 사다리 게임 실행] 페드로(류형욱) 미션 제출합니다. #404

Conversation

hw0603 commented Mar 1, 2024 • edited Loading

RequestDto 사용 시 도메인과 DTO의 의존 관계

Swap 로직의 메서드 파라미터 타입 관련

결과를 볼 사람을 입력할 때 all 이라는 문자열 상수는 어디서 관리해야 할까요?

Choose a reason for hiding this comment

Swap 로직의 메서드 파라미터 타입 관련

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

hw0603 Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

pkeugine Mar 2, 2024 • edited Loading

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

pkeugine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkeugine Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hw0603 Mar 3, 2024 • edited Loading

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

hw0603 Mar 3, 2024 • edited Loading

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

pkeugine commented Mar 2, 2024

pkeugine left a comment

Choose a reason for hiding this comment

hw0603 commented Mar 5, 2024

pkeugine left a comment

Choose a reason for hiding this comment

hw0603 commented Mar 1, 2024 •

edited

Loading

결과를 볼 사람을 입력할 때 `all` 이라는 문자열 상수는 어디서 관리해야 할까요?

hw0603 Mar 2, 2024 •

edited

Loading

pkeugine Mar 2, 2024 •

edited

Loading

hw0603 Mar 2, 2024 •

edited

Loading

hw0603 Mar 2, 2024 •

edited

Loading

hw0603 Mar 2, 2024 •

edited

Loading

pkeugine Mar 2, 2024 •

edited

Loading

hw0603 Mar 3, 2024 •

edited

Loading

hw0603 Mar 3, 2024 •

edited

Loading