[2단계 - 사다리 게임 실행] 구름(김민수) 미션 제출합니다. #332

alstn113 · 2024-02-24T13:35:36Z

안녕하세요. 웨지 감자! 구름입니다 😃

⭐️ 사다리 미션 step2에서 늘어난 요구 사항을 다음과 같이 이해했어요.

사다리의 결과(상품들)를 입력 받는다. -> 참여자의 수와 같아야함.
상품들은 참여자들과 같이 1~5자 이다. -> 정해진 사다리 간격을 위함.
결과를 보고 싶은 특정 플레이어 이름을 입력 -> 결과(상품)을 보여주고 재입력 받음.
- "all"을 입력하면 전체 결과를 보여주고 종료.

⭐️ step1에서의 코드 중 달라진 점은 다음과 같아요.

LadderHeight를 제거
- Ladder 필드에서 사용하지 않기에 원시 값 포장의 필요성이 낮은 것 같아서 제거 하고 Ladder에서 검증함.
Response Dto 테스트 제거
- 비즈니스 로직이나 검증 로직이 없고, 단순히 값만 파싱해줘서 테스트할 필요성을 느끼지 못함.
정적 팩토리 메서드 작성
- Ladder, Line, LadderGame의 생성자에 팩토리의 역할을 하는 코드가 있음.
- 정적 팩토리 메서드로 생성과 검증을 분리하고, 일반 생성자에 원하는 값만을 전달함.
- 네이밍: from은 컬렉션 하나일 경우, of는 여러 파라미터나 원시 타입을 받을 경우로 정함.
retryOnException 재입력 코드 작성
- Supplier가 복잡하다고 생각할 수 있으나, 메서드 명으로 충분히 이해할 것이라 생각함.
- 재입력을 받는 코드가 많아서 while, try catch가 반복될 것 같아서 분리함.

⭐️ 미션을 진행하면서 고민했던 점은 다음과 같아요.

🚀 TDD로 개발하면서 겪었던 문제입니다.

시작 인덱스를 받으면 연결된 마지막 인덱스를 가져오는 메서드를 작성할 때, Ladder이 먼저 떠올랐어요.
LadderTest에서 테스트를 작성하고, 그 문제를 해결하기 위해, Line과 Rung을 돌아다니면서 코드를 적다보니 Line에서의 테스트를 먼저하지 않고, 모든 코드를 작성했어요.
큰 부분을 개발할 때 작은 부분이 먼저 떠오르지 않아 작은 부분에 대한 테스트를 먼저 작성하지 않는 경우가 많은 것 같은데 웨지는 어떻게 하시나요? 단순히 경험 부족이라 그런걸까요?

🚀 Response Dto에 관한 고찰입니다.

다른 크루들의 코드를 보니 Domain을 View에 넘기는 코드가 많았습니다.
저는 mvc는 관심사 분리, 의존성 분리, 권한 분리를 위해 사용한다고 생각해요.
View에서 Domain의 getter만 쓸 것이라고 생각해도 다른 개발자들과 협업 시 View에서 Domain 내부 비즈니스 로직을 사용할 위험이 있기 때문에 저는 Response를 통해 분리해줬어요.
하지만 dto를 사용함으로써 복잡도가 많이 올라갔어요. 과한걸까요? 웨지는 어떻게 생각하세요??

🚀 패키지화에 대한 고찰입니다.

저는 공통 관심사 별로 패키지화를 해줬어요.
예를 들어, Ladder에서 Line, Rung, RungGenerator 등등을 사용하기 때문에 넣어줬어요.
다른 크루들은 domain에 모든 클래스를 동일한 depth로 놓더라고요.

저는 폴더로 분리하는 것이 처음보는 사람한테 더 읽기 쉽겠다고 생각했는데, 다른 크루들의 의견을 들어보니 폴더를 열어봐야하니 더 불편할 수도 있다고 하더라고요.
웨지는 어떻게 생각하세요??

마지막으로 제가 소프트 스킬이 많이 부족해서 소프트 스킬을 기를려고 노력하고 있어요.. ㅎㅎ
제 PR이나 답글의 말투가 이상하다거나, 공격적이다거나, 이해하기 힘들다면 피드백 부탁드립니다..

미리 리뷰 감사합니다. 웨지~ 😃

sihyung92

안녕하세요 구름? 리뷰어 웨지입니다
역시 손이 엄청 빠르시군요
몇가지 리뷰 남겼으니 확인 후 반영해주셔요!

Q&A 타임
Q. 단위가 큰 개발을 진행할 때 TDD의 어려움?
A. 저도 잘 안 되서 에라모르겠다 쭉쭉 코드 짜놓고 나중에 테스트 궁리합니다. 미리 스포하자면 사다리 피드백 강의에 이런 고민에 대한 내용도 있으니 그때 토의해보시면 될거같습니다

Q. DTO를 통한 레이어 분리?
A. Dto를 둬가며 '레이어 살려야 한다..'식 코딩은 서로 다른 레이어끼리는 상호 간섭을 안 받게 하려는 거잖아요? 상호 간섭을 안 받는다는 말은 한 쪽이 수정되도 다른 쪽은 안 고쳐도 된다는 거고요? 근데 실제로 도메인 / UI 로직 수정하시면서 domain 패키지와 view 패키지 수정한 코드보다 Dto 고치는 시간이 더 길지 않았나요? (아니었을 수도 있고요?) 이러면 손해(이득) 아닐까요?

Q. 패키지화 어떻게?
A. 음 패키지란건 어쩌면 '공부하는 책상 위' 같은 거 아닐까요?
'책상 위에서 공부를 해야하는' 것과 '패키지를 보며 기능을 만들어야하는' 것을 비슷하다고 생각해보면요,
어머니(아버지)가 책상 치우라고 했을 때 '아 난 지금 뭐가 어딨는지 다 안다고 지금이 정리된거라고'고 주장하지만 실은 우리도 진실을 알고 있잖아요? 유투브 나오는 깔끔하게 학용품끼리, 참고서끼리, 공부노트끼리 정리된 책상이랑은 조금 차이가 있다는 걸요
분류에 맞게 정리된(패키지화 된) 쪽이 조금 더 능률이 좋겠죠 다 같이 쓰는 책상이니까 규칙은 다같이 얘기 해봐야겠지만요

이 비유는 실제 패키지에서 고려되는 접근제어자나 모듈화 가능성 등이 다 뭉개져서 아주 좋은 비유는 아닌데요,
비단 코딩뿐만 아니라 사람 하는 일이 '보기 좋게, 이해하기 쉽게, 쓰기 편하게' 만들면 더 능률이 오른다는 걸 돌이켜보면요 어렵게 생각할거 없이 스스로 개선되었다고 느낀다면 지금처럼 시도해보시면 좋을거같아요~

sihyung92 · 2024-02-24T21:57:05Z

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

+    }
+
+    private boolean canMoveLeft(int index) {
+        return index > 0 && rungs.get(index - 1).isExist();


요구사항이 어려워지니 슬슬 List.get(index) 메서드에 의존하는 코드가 많아졌네요
Rungs를 만들어보시면 어때요?
저희 사다리는 index(-1)가 들어오면 예외가 아니라 index(0)을 뱉으면 충분하잖아요? (최대값 쪽도 마찬가지)
우리 도메인에 특화된 컬렉션이 들어와주기 좋은 때로 보이네요 (index 체크를 까먹어서 오류가 발생하는 실수를 방지해줌)

고민이 되네요...
사실 Line이 웨지가 말한 Rungs의 역할을 하게 하고 싶었어요.

저희 사다리는 index(-1)가 들어오면 예외가 아니라 index(0)을 뱉으면 충분하잖아요? (최대값 쪽도 마찬가지)

이 부분이 Ladder의 findEndIndex쪽 검증을 말하시는 것 같은데 -1이 0이 되게 하는게 맞는지 잘 모르겠어요...
Line의 findConnectedIndex가 어느 정도 그 역할을 하는 것 같기도 하구요.

음 저도 실무였다면 굳이 Rungs로 분화는 안 했을거에요.
그러나 미션 요구사항에 일급컬렉션을 적용해보라는 내용도 있는데,
일급컬렉션 (향로님의 레퍼런스로 ㅎ)는 도메인 컬렉션 외 다른 상태를 같지 않는 컬렉션으로 정의하고 있어요.

도메인 컬렉션으로서의 Rungs는 이번 미션에선 비즈니스 컬렉션으로서 IndexOutofException이 발생하는 상황에서 끝 객체를 반환하는 비즈니스 특화적인 역할을 수행해줄 수 있습니다.

Floor은 Rungs의 생성 역할과 일급컬렉션의 제어 역할을 맡을 수 있고요.

굳이 쪼개는 것의 당위성을 부여해보자면 그렇단건데 요건 생각해보고 반영해주세요~

sihyung92 · 2024-02-24T21:59:16Z

src/main/java/ladder/domain/ladder/Ladder.java

+    }
+
+    private static void validateHeightRange(int height) {
+        if (MAXIMUM_HEIGHT < height || height < MINIMUM_HEIGHT) {


(초미세 가독성 훈수 2탄)
이건 저만 그런걸수도 있는데 최소 표현부터 먼저하고 최대 표현을 나중에 해야 잘 읽히는거 같아요 -.-ㅎ

Suggested change

if (MAXIMUM_HEIGHT < height || height < MINIMUM_HEIGHT) {

if (height < MINIMUM_HEIGHT || MAXIMUM_HEIGHT < height) {

비교해보시고 그냥 구름이 잘 읽힌다 싶은걸로 해주세용

음 height를 안으로 모아주고 싶었어요!

아래와 같이 최소를 먼저 표현하는걸로 변경했어요.

if (MINIMUM_HEIGHT > height || height > MAXIMUM_HEIGHT)

혹시 부등호 방향도 정해진 기준이 있나요? "<" 가 읽기 쉽다던지 ">"가 읽기 쉽다던지

아뇨 이런 부분은 좀 느낌에 가까운거라서요 ㅋ ㅋ 개인적으로 잘 읽힌다고 생각하시는 걸 쓰시면 됩니다 (사실 우린 정확히 남 입장이 될 수 없으니까요, 그냥 "내가 이 코드를 처음 보는 사람이라면 어떨까?" 하고 상상하면서 짜는거죠. 어쨌든 스스로 타자화 해보는 것도 한계는 있죠)

매직넘버 상수화 같은건 전 인류 공통의 가독성 증진으로 봐도 좋은데
이런 부분 가독성은 문화권 마다 다를거에요.

저는 가독성과 관련된 개인적인 입장 정리는 프로그래머의 뇌 읽어보면서 좀 됐어요~
가독성의 이론적 배경을 알려주니까요, 심심할 때 한번 가벼운 마음으로 읽어보세요

sihyung92 · 2024-02-24T22:01:56Z

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

@@ -26,14 +26,38 @@ public List<Rung> generateRungs(int playerCount) {
    }


+ Line 객체 명에 관해..

다른 크루 리뷰하다가 동일한 구성을 봤어요
Line = 사다리 한 줄을 뜻함
Rung = 가로장

그 쪽에 남겼던 리뷰를 그대로 옮겨올께요

Line보단 Floor는 어때요?
뭔가 Line하면 가로줄 느낌인데 Rung (가로장)이 함께 있으니까 조금 헷갈리는 느낌이 있어요.
그래서 가로세로가 모두 느껴지는 네이밍인 floor가 어떤가 생각해봤습니다

도메인 용어는 이 시스템(Line은 Ladder의 총 가로줄을 나타낸다)에 익숙해지면 개발자들 멘탈모델에 스며드는 거라서 크게 이슈인건 아니지만..
개인적으로는 Line과 Rung이 서로 헷갈렸어요 ㅎ

step1 기능 구현 사항에 힌트로 아래와 같이 Line을 사용해서 대부분의 크루가 그렇게 사용한 것 같아요. 저도 마찬가지구요.
헷갈릴 수도 있겠네요.

사실 랜덤으로 생성되는 가로줄도 이름을 어떻게 할 지 모르겠어서 아래의 그림을 참고했어요.
지금 생각해도 이상한 것 같네요...

ㅋㅋ 객체지향이 현실을 베끼라는 말은 아니긴 한데요, Rung이라는 표현에 대해 저는 수용해요~ 왜냐면 Rung이 낯선건 어휘력 부족이라는 생각이 들어서요
Rung = '가로장' 임을 한번 학습하고 나면 사다리게임의 가로줄을 뜻하는 도메인 용어로 손색이 없지 않나 싶긴해요

저도 어제쯤? 나라면 명명을 뭐라고했을까 생각해봤는데 가로장은 crossLine 이라고 했을거 같아요 (만날때마다 건너야하니까.. 건넌다에 좀 착안했었어요)

Floor로 수정해봤습니다. 인텔리제이라 그런가 rename이 쉽고 편하네요 ㅎㅎ

sihyung92 · 2024-02-24T22:05:31Z

src/main/java/ladder/domain/prize/Prize.java

@@ -0,0 +1,19 @@
+package ladder.domain.prize;
+
+public record Prize(String name) {


2단계 요구사항 읽고 pr checkout 받고 디렉토리 슥 훑어보면서 "아 이게 결과구나" 느낌이 왔습니다 ㅎ
객체 명이 적절하다는 생각이 들었네요

감사합니다!

sihyung92 · 2024-02-24T22:15:01Z

src/main/java/ladder/domain/LadderGame.java

+    public Prize getResultByPlayerName(String playerName) {
+        for (Entry<Player, Prize> entry : results.entrySet()) {
+            if (entry.getKey().name().equals(playerName)) {
+                return entry.getValue();
+            }
+        }
+
+        throw new IllegalArgumentException("존재하지 않는 참가자입니다.");
+    }


혹시 시간 복잡도에 대해 알고 계신가요?
빅O 표기법으로 표현하자면 해당 코드는 O(1) 을 O(n)으로 풀어낸 코드예요 (Map 자료구조의 이점을 못 누리고 있음)

Player 키 값을 가지고 있으니 playerName을 Player 타입으로 바꾸어서 조회해주세용.
혹은 key를 String으로 교체하는 방법도 있겠죠. 컴퓨팅 성능이 좋아져서 작은 성능 이득 때문에 가독성을 포기할 필요는 없는 시대지만 있는 성능을 악화시키는 건 피해야합니다

Suggested change

public Prize getResultByPlayerName(String playerName) {

for (Entry<Player, Prize> entry : results.entrySet()) {

if (entry.getKey().name().equals(playerName)) {

return entry.getValue();

}

}

throw new IllegalArgumentException("존재하지 않는 참가자입니다.");

}

public Prize getResultByPlayerName(String playerName) {

Player player = new Player(playerName);

if (isEmpty(player)) {

throw new IllegalArgumentException("존재하지 않는 참가자입니다.");

}

return results.get(player);

}

private boolean isEmpty(Player player) {

return !results.containsKey(player);

}

앗... 실수네요.

하나 궁금한 점이 있어요.
!results.containsKey(player)이 충분히 이해가 간다고 생각해서 isEmpty로 분리를 하지 않았어요.
private이라도 메서드가 늘어나면 전체 코드가 읽기가 힘들기도 하고, 메서드로 분리할 경우 찾으러 가야하잖아요.
웨지는 어떻게 생각하시나요?

요건 부정연산자를 지양하자는 의미로 썼어요 ㅎ 부정연산자 단독으로 있으면 읽을만한데 저는 부정연산자랑 AND / OR 조건 하나만 섞이면 너무 어렵더라구요 ㅋㅋ
!isEmpty || valid(abc) 이런거요.
혼자서 입으로 '빈게 아니면서.. 유효한건데.. 둘이 따로..' 하는데 부정연산자 섞여 있으면 30초씩 스턴 당합니다
요런 경험 몇번 하고나니 최대한 좀 !는 빼버리는 습관이 생겼어요

말씀하신 것처럼 !results.containsKey(player)가 메서드 분리할 필요없이 충분히 읽을만 하다는 데에는 동의합니다
분리해두면 굳이 cmd + b 찍어서 찾아서 체크해야하기도 하고요

근데 코드를 많이 봐야되는 상황이면 제 관심사가 아닌 부분들이 있거든요?
예를들어 LadderGame 인원 중 두명이 중복 prize를 받는 버그가 났어요.
그럼 버그를 찾기 위해 코드 흐름을 죽 눈으로 훑어야 되는데 if( isEmpty(player) ) 이 부분의 조건까지는 안 궁금하니까 cmd + b 눌러보지도 않아요. 그냥 메서드명만 읽고 안에서 그런 검사를 할거라고 믿고 지나가는거죠 (조건이 충족됐으면 IllegalArgument이 발생했을텐데 안 발생했으니까 배제하고 넘어간다는 의미입니다. 버그랑 연관 있어 보였으면 메서드명 못믿고 들어가서 보고 지나갔겠죵)
이럴 땐 cmd + b도 안 찍어볼테니까, 오히려 잘 읽히는 표현이 메서드화 되어있는게 낫기도 하고요

그 작성해주신 코드 중에 조건문 좀 복잡한거 있었는데 index-1 뺀거랑 isExist()랑 || 비교한거.. 그런건 메서드로 빼는게 맞다는 확신이 있고요, 요런 상황에선 '!containsKey 하거나 메서드로 빼거나 둘다 아무래도 좋다' 라고 생각합니다 ㅎ

sihyung92 · 2024-02-24T22:22:15Z

src/main/java/ladder/view/OutputView.java

+
+        System.out.println();
+        System.out.println("실행결과");
+        allResults.forEach((playerName, prizeName) -> System.out.printf(resultFormat, playerName, prizeName));


읽다가 람다 부분이 좀 헷갈려서 개행해줘도 좋겠네용

Suggested change

allResults.forEach((playerName, prizeName) -> System.out.printf(resultFormat, playerName, prizeName));

allResults.forEach(

(playerName, prizeName) -> System.out.printf(resultFormat, playerName, prizeName)

);

흠... 저는 이상하게 forEach에서 개행하면 어색하더라고요.

개행했습니다!

혹시 프로그래밍 언어 어떤걸로 시작하셨어요?
요런 감각은 출신 언어(??)에 따라 달라지기도 해서요, 감이 다를 땐 한번씩 여쭤봅니다 ㅎ ㅎ

타입스크립트를 했었습니다! ㅎㅎ

sihyung92 · 2024-02-24T22:27:04Z

src/main/java/ladder/domain/prize/Prizes.java

+    }
+
+    public static Prizes of(List<String> prizeNames, int playersCount) {
+        validateSize(prizeNames.size(), playersCount);


이 검사를 여기서 하는 게 맞을까요?
결국 검사가 필요한건 Prizes와 Players 사이즈가 맞냐는 건데, 이렇게 player count를 받아서 검사하는게 시스템에서 Prizes와 Players 사이즈가 일치하다는 보장을 해주진 않습니다.

실제로 Prize와 Player를 매칭하는 곳에서 해줘야 될 검사라고 생각해요

음... 처음에는 controller 부분에 작성했다가 코드가 보기 불편해서 넣었습니다.

LadderGame에서 참여자들의 수와 상품들의 수가 일치하는가를 검증하는 것이 맞는 것 같네요.
아래와 같은 상황에서 LadderGame까지 내려 간 것을 어떻게 재입력 받을지 고민을 좀 해보겠습니다.

public void run() { Players players = retryOnException(() -> Players.from(inputView.readPlayerNames())); Prizes prizes = retryOnException(() -> Prizes.of(inputView.readPrizes(), players.size())); Ladder ladder = retryOnException(() -> Ladder.of(inputView.readLadderHeight(), players.size(), rungGenerator)); LadderGame ladderGame = LadderGame.of(players, ladder, prizes); printLadderResult(players, ladder, prizes); printResultForSelectedPlayer(ladderGame); }

재입력때문에 고민하다가 다음과 같이 변경했습니다.

우선 Prizes를 입력 받을 때 한 번 검증을 합니다.

private Prizes initPrizes(int playerCount) { List<String> prizeNames = inputView.readPrizeNames(); validatePrizeCount(playerCount, prizeNames.size()); return Prizes.from(inputView.readPrizeNames()); }

이후에 LadderGame 생성자 안에서 검증을 합니다.
참여자 수, 상품 수, 사다리의 출발 지점 수. 이 3가지가 동일해야 해서 검증을 나눴습니다.

private static void validateSize(Players players, Ladder ladder, Prizes prizes) { if (players.size() != ladder.getColumnSize() + 1) { throw new IllegalArgumentException("참가자 수와 사다리의 출발 지점 수가 일치하지 않습니다."); } if (prizes.size() != ladder.getColumnSize() + 1) { throw new IllegalArgumentException("상품 수와 사다리의 도착 지점 수가 일치하지 않습니다."); } }

sihyung92 · 2024-02-24T22:30:44Z

src/main/java/ladder/domain/LadderGame.java

+        for (int i = 0; i < players.size(); i++) {
+            Player player = players.getPlayers().get(i);
+            int endIndex = ladder.findEndIndex(i);
+            Prize prize = prizes.getPrizes().get(endIndex);


직전 리뷰가 부른 곳이 바로 여기입니다.
여기서 prizes.size() == players.size()가 보장이 되나요? (지금 코드에선 보장이 되지만 이 코드를 다른 개발자들이 재활용한다는 관점에서 봐야합니다)

sihyung92 · 2024-02-24T22:40:05Z

src/main/java/ladder/domain/ladder/Ladder.java

+        }
+    }
+
+    public int findEndIndex(int index) {


코드는 좀 늘어나겠지만 인자로 받은 index에 새로운 값을 할당하지 말고 별도의 로컬 변수를 선언해보시면 어떨까요?
final을 붙이는 습관을 유지하셨다고 생각해보면 지금의 로직전개는 불가능할텐데요, 그 관점에서 리팩토링 해주세요

좋은 생각 같아요!
validateIndexRange 검증 이후 부터 로컬 변수를 만들어서 사용하도록 했어요.

public int findEndIndex(int index) { validateIndexRange(index); int currentIndex = index; for (Line line : lines) { currentIndex = line.findConnectedIndex(currentIndex); } return currentIndex; }

sihyung92

안녕하세요 구름?
리뷰 머지가 임박했는데 진도가 �좀 빠른거 같아서 히든 미션 하나 드렸습니다..
원하시는대로 반영해주세용~

sihyung92 · 2024-02-26T13:16:21Z

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

+    private static void validatePrizeCount(int playerCount, int prizeCount) {
+        if (prizeCount != playerCount) {
+            throw new IllegalArgumentException("참여자 수와 상품 수가 일치하지 않습니다.");
+        }
+    }


view와 도메인 양쪽 검증에 대해선 괜찮다고 생각합니다! (웹 개발할 땐 프론트랑 백 다 막자 로 귀결됩니다)
그러나 컨트롤러의 역할은 아닌거 같아요.
view로 이동하는 거에 대해 어떻게 생각하시나요?

오 좋은 생각 같아요!!
view에서 playerCount를 상태관리하고 prizeCount를 검증할까 하다가 너무 복잡해지는 것 같아서
아래와 같이 view로 playerCount를 넘겨서 처리했습니다.

private Prizes initPrizes(int playerCount) { List<String> prizeNames = inputView.readPrizeNames(playerCount); return Prizes.from(prizeNames); }

public List<String> readPrizeNames(int playerCount) { System.out.println(); System.out.println("실행 결과를 입력하세요. (결과는 쉼표(,)로 구분하세요)"); String input = readLine(); List<String> prizeNames = parseStringToList(input); validatePrizeSize(playerCount, prizeNames.size()); return prizeNames; }

sihyung92 · 2024-02-26T13:41:56Z

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

+    }
+
+    private boolean canMoveLeft(int index) {
+        return index > 0 && rungs.get(index - 1).isExist();


음 저도 실무였다면 굳이 Rungs로 분화는 안 했을거에요.
그러나 미션 요구사항에 일급컬렉션을 적용해보라는 내용도 있는데,
일급컬렉션 (향로님의 레퍼런스로 ㅎ)는 도메인 컬렉션 외 다른 상태를 같지 않는 컬렉션으로 정의하고 있어요.

도메인 컬렉션으로서의 Rungs는 이번 미션에선 비즈니스 컬렉션으로서 IndexOutofException이 발생하는 상황에서 끝 객체를 반환하는 비즈니스 특화적인 역할을 수행해줄 수 있습니다.

Floor은 Rungs의 생성 역할과 일급컬렉션의 제어 역할을 맡을 수 있고요.

굳이 쪼개는 것의 당위성을 부여해보자면 그렇단건데 요건 생각해보고 반영해주세요~

sihyung92 · 2024-02-26T13:48:18Z

src/main/java/ladder/domain/ladder/Floor.java

+        if (shouldGenerateExistRung(rungs, rungGenerator.generate())) {
+            return Rung.EXIST;
+        }
+
+        return Rung.EMPTY;


진도가 너무 빠르셔서.. 히든 미션 하나 드릴게요

실은 이 사다리게임은 약점이 하나 있는데요
알고리즘상 1열은 random boolean으로 판단하고, 2열에선 앞쪽이 true가 아니면서 && random boolean으로 판단하다보니까요
확률적으로 균일한 가로대가 생기는게 아니라 홀수열에만 가로대가 몰리는 현상을 보실 수 있을 거에요
(몇번 실행하면서 확인해보시면 상관관계가 명확히 보일거에요)

이걸 수정해달라는 요구사항이 주어졌다고 생각해보시고 리팩토링 해보시겠어요?

아마 다음 리뷰쯤에는 머지될거라서요, 심심하면 해보세요. 다른 학습할 게 많다면 다른 걸 공부하시는 것도 좋고요~ 알고리즘 하는 시간은 아니니까요

음 저도 실무였다면 굳이 Rungs로 분화는 안 했을거에요.
그러나 미션 요구사항에 일급컬렉션을 적용해보라는 내용도 있는데,
일급컬렉션 (향로님의 레퍼런스로 ㅎ)는 도메인 컬렉션 외 다른 상태를 같지 않는 컬렉션으로 정의하고 있어요.

도메인 컬렉션으로서의 Rungs는 이번 미션에선 비즈니스 컬렉션으로서 IndexOutofException이 발생하는 상황에서 끝 객체를 반환하는 비즈니스 특화적인 역할을 수행해줄 수 있습니다.
Floor은 Rungs의 생성 역할과 일급컬렉션의 제어 역할을 맡을 수 있고요.

굳이 쪼개는 것의 당위성을 부여해보자면 그렇단건데 요건 생각해보고 반영해주세요~

사다리에 있던 index 검증 로직을 지우고, Rungs라는 일급 컬렉션 안에 "index가 0보다 작으면 0으로, rungs.size보다 크면 rungs.size로 설정"을 원하시는게 맞나요?? 이 방식으로 하면 사다리에서 인덱스 검증을 안해도 되긴 하겠네요!

public int findEndIndex(int index) { int currentIndex = index; for (Floor floor : floors) { currentIndex = floor.findConnectedIndex(currentIndex); } return currentIndex; }

public int findConnectedIndex(int index) { // index가 0보다 작으면 0으로, rungs.size보다 크면 rungs.size로 설정한다. int currentIndex = Math.max(0, Math.min(index, rungs.size() - 1)); if (canMoveLeft(currentIndex)) { return currentIndex - 1; } if (canMoveRight(currentIndex)) { return currentIndex + 1; } return currentIndex; }

실은 이 사다리게임은 약점이 하나 있는데요
알고리즘상 1열은 random boolean으로 판단하고, 2열에선 앞쪽이 true가 아니면서 && random boolean으로 판단하다보니까요
확률적으로 균일한 가로대가 생기는게 아니라 홀수열에만 가로대가 몰리는 현상을 보실 수 있을 거에요
(몇번 실행하면서 확인해보시면 상관관계가 명확히 보일거에요)

어우... 아무리 생각해도 안떠오르네요 ㅠㅠ

sihyung92

안녕하세요 구름~
머지할 때가 된 것 같아 이만 머지할게요 🚀
간단한 코멘트 몇가지 남겼으니 확인해주셔요 ㅎ
수고하셨습니다~~

sihyung92 · 2024-02-27T14:00:46Z

src/main/java/ladder/domain/ladder/Floor.java

+    }
+
+    private static boolean shouldGenerateExistRung(List<Rung> rungs, Rung currentRung) {
+        boolean previousRungIsEmpty = rungs.isEmpty() || rungs.get(rungs.size() - 1).isEmpty();


요런 부분도 일급컬렉션에서 감쌀수 있으면 좋을텐데 생성 전이라서 어렵네요 ㅎ

sihyung92 · 2024-02-27T14:09:45Z

src/main/java/ladder/domain/ladder/Rungs.java

+    }
+
+    public int findConnectedIndex(int index) {
+        // index가 0보다 작으면 0으로, rungs.size보다 크면 rungs.size로 설정한다.


요즘 우테코에서 좋은코드, 나쁜코드를 권장하는 걸로 알고 있는데요, (책 몇권 사다놨다고 알고 있어요)
p127부터 보시면 주석문의 좋은 활용에 대해 나옵니다. 공감가는 내용이 담겨있으니 확인해보셔요 ㅎ

중복된 주석문(코드가 무얼하는지 설명하는 주석)은 유해할 수 있다.
주석으로 설명해야하는 만큼 복잡하다면 리팩토링의 신호이다

Math로 저렇게하면 코드로는 편한데 처음보는 사람이면 이해하기 힘들까봐 주석으로 했어요...

지금 읽는 책 마저 읽고 좋코나코 빨리 읽어보도록 하겠습니다!

sihyung92 · 2024-02-27T14:11:15Z

src/main/java/ladder/view/InputView.java

+
+    private void validatePrizeSize(int playerCount, int prizeCount) {
+        if (playerCount != prizeCount) {
+            throw new IllegalArgumentException("참가자 수와 결과 수가 일치해야 합니다.");


가능하면 예외문은 풍부하게 맥락을 담아주면 좋습니다.

Suggested change

throw new IllegalArgumentException("참가자 수와 결과 수가 일치해야 합니다.");

throw new IllegalArgumentException("참가자 수와 결과 수가 일치해야 합니다. 참가자 수 : " + playerCount + ", 결과 수 : " + prizeCount);

오.. 이걸 생각 못했네요

sihyung92 · 2024-02-27T14:18:48Z

src/main/java/ladder/domain/LadderGame.java

+    }
+
+    private static void validateSize(Players players, Ladder ladder, Prizes prizes) {
+        if (players.size() != ladder.getColumnSize() + 1) {


ladder Column하면 세로대가 생각나는데, API 내부에선 rungs의 개수, 즉 가로대의 개수를 제공하고 있어서 c내부적으로 rungs.size() + 1 을 해서 제공해도 되겠어요.

alstn113 · 2024-02-27T14:35:02Z

우테코 생활도 슬슬 적응되고 미션도 2주라서 멍한 시간이 많네요. 생각도 고이는 것 같구요. 다른 크루들 코드 읽으면서 다양한 생각을 해보고 다시 만들어봐야겠어요.

웨지! 코드 리뷰해주셔서 정말 감사합니다.

alstn113 added 19 commits February 23, 2024 22:23

refactor: players에 생성 책임 부여

2e45cd2

refactor: 사다리 왼쪽 마진으로 변경

ad8937c

feat: 재입력 메서드 작성

8a093f4

refactor: 발판 생성 메서드 가독성 높임

00d663c

docs: 기능 구현 목록 추가 작성(step2)

0442a5c

feat: 상품들 생성

66728fa

feat: 발판으로 연결된 인덱스를 찾음

b238a04

feat: 사다리 시작 인덱스로 마지막 인덱스를 찾음

a3eca83

feat: 사다리 게임의 매칭 결과 구현

64f9324

feat: 상품들을 입력 받음

fa37a18

docs: 상품 입력 구현 완료

7500c67

refactor: 정적 팩토리 메서드로 생성 분리

591d9c4

feat: Prize 테스트

d5d2d11

refactor: dto 테스트를 제거

713a83e

refactor: 사다리 출력 메서드를 개선

ae551e1

refactor: 가독성 높임

74c0158

test: 사다리의 높이와 시작 인덱스를 테스트

d88b8b1

feat: 결과를 보고 싶은 참여자 입력 후 출력

da03a2d

refactor: 코드 가독성 개선

fa3a9b8

sihyung92 requested changes Feb 24, 2024

View reviewed changes

alstn113 added 8 commits February 25, 2024 10:12

refactor: 높이 범위 가독성 높임

e5470b4

refactor: 플레이어 이름에 따라 상품을 가져오는 로직 성능 개선

4d2c855

refactor: 실행 결과 출력 코드 개행

efa7888

refactor: findEndIndex 내부 로컬 변수로 수정

e30376d

refactor: readPrizeNames 메서드 명 변경

5e13b7d

refactor: 사다리 출발 지점 수, 참여자 수, 상품 수 검증 위치 변경

8fc2117

refactor: Line을 Floor로 변경

b57191a

docs: 점 수정

e88e03c

sihyung92 requested changes Feb 26, 2024

View reviewed changes

refactor: 상품 수 검증 로직 view로 이동

048df20

alstn113 added 2 commits February 27, 2024 00:08

refactor: 인덱스를 넘으면 최소, 최대 범위로 변경

eb34aa3

test: findConnectedIndex 범위 테스트 추가

f17d17e

sihyung92 approved these changes Feb 27, 2024

View reviewed changes

sihyung92 merged commit 498ae46 into woowacourse:alstn113 Feb 27, 2024

	if (MAXIMUM_HEIGHT < height \|\| height < MINIMUM_HEIGHT) {
	if (height < MINIMUM_HEIGHT \|\| MAXIMUM_HEIGHT < height) {

		@@ -26,14 +26,38 @@ public List<Rung> generateRungs(int playerCount) {
		}

		@@ -0,0 +1,19 @@
		package ladder.domain.prize;

		public record Prize(String name) {

	throw new IllegalArgumentException("참가자 수와 결과 수가 일치해야 합니다.");
	throw new IllegalArgumentException("참가자 수와 결과 수가 일치해야 합니다. 참가자 수 : " + playerCount + ", 결과 수 : " + prizeCount);

[2단계 - 사다리 게임 실행] 구름(김민수) 미션 제출합니다. #332

[2단계 - 사다리 게임 실행] 구름(김민수) 미션 제출합니다. #332

Conversation

alstn113 commented Feb 24, 2024 • edited Loading

⭐️ 사다리 미션 step2에서 늘어난 요구 사항을 다음과 같이 이해했어요.

⭐️ step1에서의 코드 중 달라진 점은 다음과 같아요.

⭐️ 미션을 진행하면서 고민했던 점은 다음과 같아요.

🚀 TDD로 개발하면서 겪었던 문제입니다.

🚀 Response Dto에 관한 고찰입니다.

🚀 패키지화에 대한 고찰입니다.

sihyung92 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

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

sihyung92 Feb 25, 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

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

sihyung92 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihyung92 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alstn113 commented Feb 27, 2024

alstn113 commented Feb 24, 2024 •

edited

Loading

sihyung92 Feb 25, 2024 •

edited

Loading