[2단계 - 사다리 게임] 에버(손채영) 미션 제출합니다. #408

helenason · 2024-03-01T15:42:43Z

안녕하세요 알렉스!
1단계 미션에서 좋은 피드백 정말 감사했습니다. 2단계 미션도 리뷰 요청드립니다!

먼저, 1단계 미션에서 마지막으로 알렉스가 주신 코멘트 중 궁금했던 부분이 있었습니다.

해당 코멘트에 대한 궁금증입니다.
저는 makeMembers() 메서드가 다른 메서드를 호출하는 역할만 가지고 있다고 생각했습니다. 하지만 알렉스의 역할 분리 필요성에 대한 코멘트를 듣고 여기서 어떻게 역할을 더 분리할 수 있을지 생각해보았지만, 마땅한 해결책이 떠오르지 않았습니다..🥲 좋은 방법이 있다면 언급해주시면 감사하겠습니다!

다음으로는 2단계 미션을 진행하는 과정에서 갖게 된 궁금증입니다.

검증 로직의 호출 위치?
Results 클래스에서는 정적 팩토리 메서드를 통해 객체를 생성하고 있습니다. 생성자에 오직 인스턴스 변수와 관련된 파라미터만 전달하고 싶어 검증 로직을 of() 메서드 내에서 호출하도록 구현하였습니다. 하지만 이럴 경우 검증 로직이 static으로 선언되며, 생성자에게 생성 및 검증의 역할을 부여하고 싶다는 바람도 무산됩니다. 이럴 경우 알렉스는 어떤 방식을 선호하시는지 궁금합니다!
테스트 작성의 용이함 vs 비즈니스 로직 숨기기

테스트 작성의 용이함을 위해 컨트롤러에서 도메인을 생성한 후 전달하는 방법이 적절한지, 비즈니스 로직을 숨기기 위해 도메인이 도메인을 생성하도록 구현하는 것이 적절한지 고민이 됩니다. 저는 테스트를 쉽게 작성하는 것보다, 비즈니스 로직을 숨기는 것과 컨트롤러보다 도메인에 더 많은 역할을 부여하는 것이 중요하다고 판단하여 후자를 선택했습니다. 이 경우 알렉스가 선호하는 방식이 궁금합니다!
저의 설명이 부족할 수도 있을 것 같아 간략한 예시 코드를 작성해두겠습니다!
- 전자 코드 예시
```
Member member = new Member("name");
Members members = new Members(member);
```
- 후자 코드 예시
```
Members members = new Members("name");
```

반복문의 코드 중복을 줄이기 위한 더 나은 방안?

ResultTarget resultTarget = showResult(members, gameResult);
int count = MAX_GAME_COUNT;
while (count-- > 0 && !resultTarget.isAllMembers()) {
    resultTarget = showResult(members, gameResult);
}

GameController의 manageResult() 메서드를 위와 같이 리팩토링하게 된 생각의 흐름은 아래와 같습니다.
들여쓰기를 1로 맞추어야 한다 -> while 내의 break문을 제거해야 한다 -> while의 조건문에서 break의 조건을 같이 체크하자 -> resultTarget이 null일 수 없으니 while문 실행 이전에 한 번을 먼저 호출하자입니다. 들여쓰기를 줄이려고 노력하다보니 중복되는 코드가 생겼는데 혹시 이를 개선할 수 있는 방법이 있을지 궁금합니다!

이번 미션도 잘 부탁드립니다.
감사합니다! 😁

…입으로 전달

yxxnghwan

안녕하세요 에버! 2단계도 잘 구현해주셨네요! 👍🏿
생각해볼만한 코멘트 몇개 남겼으니 확인 부탁드려요!

질문 답변

검증 로직의 호출 위치?
Results 클래스에서는 정적 팩토리 메서드를 통해 객체를 생성하고 있습니다. 생성자에 오직 인스턴스 변수와 관련된 파라미터만 전달하고 싶어 검증 로직을 of() 메서드 내에서 호출하도록 구현하였습니다. 하지만 이럴 경우 검증 로직이 static으로 선언되며, 생성자에게 생성 및 검증의 역할을 부여하고 싶다는 바람도 무산됩니다. 이럴 경우 알렉스는 어떤 방식을 선호하시는지 궁금합니다!

어떤걸 검증하는 지에 따라 위치가 다를 것 같아요. 해당 클래스가 절대적으로 지켜야하는 불변하는 규칙에 대한 검증은 생성자에 구현하면 불완전한 객체의 생성을 막을 수 있을 것 같아요!
정적 팩토리 메소드에 검증을 구현할 때는 그 팩토리 메소드가 가지는 역할에 집중해서 필요한 검증을 넣을 것 같네요! 도메인 로직이 될 수도 있고, 전달받은 파라미터 자체를 검증할 수도 있고 다양한 케이스가 존재할 것 같네요.

테스트 작성의 용이함 vs 비즈니스 로직 숨기기
테스트 작성의 용이함을 위해 컨트롤러에서 도메인을 생성한 후 전달하는 방법이 적절한지, 비즈니스 로직을 숨기기 위해 도메인이 도메인을 생성하도록 구현하는 것이 적절한지 고민이 됩니다. 저는 테스트를 쉽게 작성하는 것보다, 비즈니스 로직을 숨기는 것과 컨트롤러보다 도메인에 더 많은 역할을 부여하는 것이 중요하다고 판단하여 후자를 선택했습니다. 이 경우 알렉스가 선호하는 방식이 궁금합니다!

상황에 따라 다를 것 같아요. 특정 케이스에선 도메인이 파라미터를 통해 raw타입이 아닌 도메인으로 정의된 vo같은 것들을 받고싶을 수 있으니까요.
에버는 이번 미션을 진행할 때 도메인, 컨트롤러, 뷰의 구현 순서가 어떻게 되셨나요? 컨트롤러에서 어떤 값을 넘겨줘야하는지 도메인이 알고 시그니처를 맞출 필요가 있을까요?

반복문의 코드 중복을 줄이기 위한 더 나은 방안?
GameController의 manageResult() 메서드를 위와 같이 리팩토링하게 된 생각의 흐름은 아래와 같습니다.
들여쓰기를 1로 맞추어야 한다 -> while 내의 break문을 제거해야 한다 -> while의 조건문에서 break의 조건을 같이 체크하자 -> resultTarget이 null일 수 없으니 while문 실행 이전에 한 번을 먼저 호출하자입니다. 들여쓰기를 줄이려고 노력하다보니 중복되는 코드가 생겼는데 혹시 이를 개선할 수 있는 방법이 있을지 궁금합니다!

개인적인 생각으론 미션의 프로그래밍 요구사항은 요구사항을 지키는 것에 초점을 맞추기보단 해당 요구사항의 의도를 파악하는게 좀 더 도움되는 학습방법일 거라고 생각해요! 인덴트 뎁스를 줄이자는건 절차지향적인 코드를 지양하고 라인, 메소드, 클래스 등의 역할 분리를 고려하자는 의도로 저는 이해했어요! 그리고 이런 코드를 지향하는 이유는 가독성과 유지보수성이구요!
여기서 질문을 드리자면 break문이 들어가면 반드시 인덴트 뎁스가 2가 되는데, 그럼 그 코드는 과연 반드시 안좋은 코드일까요? 고민해보시면 좋을 것 같아요! 😁

src/main/java/controller/GameController.java

src/main/java/domain/ResultTarget.java

src/main/java/domain/Game.java

src/main/java/controller/GameController.java

yxxnghwan · 2024-03-02T14:20:04Z

src/main/java/domain/GameResult.java

+                .getValue();
+    }
+
+    private Map<String, Result> getResultOfAllMember() {


메소드의 시그니처만 보았을 때 String으로 되어있는 key가 어떤걸 의미하는지 알 수 없을 것 같아요.
Map으로 응답해도 괜찮을까요? 어떻게 응답을 주면 의미가 명확해질까요?

사실 구현하면서도 Member로 반환할지 String으로 반환할지와,
Map으로 반환할지 새로운 객체로 감싸 반환할지 고민을 하였습니다.

고민의 결과, 객체로 포장할 경우 출력 과정에서 불필요하게 추가되는 로직이 많을 것이라고 판단하였습니다.
또한 포장하지 않을 경우의 테스트 작성이 더욱 원활하여 위와 같이 반환하도록 결정하였습니다.

하지만 지금 다시 생각해보니, 불명확한 의미와 유지보수의 어려움이 문제가 될 것 같습니다.

따라서 오직 도메인 간 전달만을 위한 DTO 클래스를 새로 생성하고,
어려워진 테스트를 해결하기 위해서 Member와 MemberName의 equals를 재정의하였습니다!

src/main/java/domain/Members.java

src/main/java/view/OutputView.java

src/main/java/domain/Ladder.java

helenason · 2024-03-03T12:47:39Z

질문 답변에 대한 답변

구현을 하나의 방법으로 통일하고자하는 강박이 저에게 있다는 걸 깨달았어요. 검증 목적에 따라 얼마든 검증 위치가 달라질 수 있는 건데요..
다른 클래스들과의 교류보다는 해당 도메인의 목적과 역할에 먼저 초점을 맞추어 구현하겠습니다!
저도 인덴트 깊이를 줄이면서 이전의 코드가 더 잘 읽힌다고 생각했어요.. 저에겐 요구사항을 거부할 용기가 없던 것 같아요 ㅎㅎ
요구사항을 무조건 따르기보다는 왜 따라야 하는지 생각하며 앞으로의 미션 진행하겠습니다! 일단 이번 미션은 do-while 사용하여 리팩토링하였습니다.

좋은 피드백 감사합니다! ☺️

yxxnghwan

안녕하세요 에버! 코드가 많이 보기 편해진 것 같네요! vo, dto, 일급컬렉션 모두 적절히 잘 사용해주신 것 같아요! 생각해볼만한 코멘트 몇개 더 남기긴 했는데 이번 미션에서는 충분히 많은 고민을 하신 것 같아 다음 미션때 고민해서 반영해주시면 좋을 듯 합니다! 이만 머지하도록 할게요! 다음 미션도 화이팅입니다~💪

yxxnghwan · 2024-03-05T12:54:04Z

src/main/java/domain/GameResult.java

+    private GameResultDto getResultOfMember(String memberName) {
+        HashMap<Member, Reward> resolvedResult = new LinkedHashMap<>();
+        Entry<Member, Reward> memberResult = gameResult.entrySet().stream()
+                .filter(entry -> entry.getKey().hasSameNameWith(memberName))
+                .findFirst()
+                .orElseThrow(() -> new IllegalArgumentException("해당 이름을 가진 참여자가 없습니다."));
+        resolvedResult.put(memberResult.getKey(), memberResult.getValue());
+        return new GameResultDto(resolvedResult);
+    }


Suggested change

private GameResultDto getResultOfMember(String memberName) {

HashMap<Member, Reward> resolvedResult = new LinkedHashMap<>();

Entry<Member, Reward> memberResult = gameResult.entrySet().stream()

.filter(entry -> entry.getKey().hasSameNameWith(memberName))

.findFirst()

.orElseThrow(() -> new IllegalArgumentException("해당 이름을 가진 참여자가 없습니다."));

resolvedResult.put(memberResult.getKey(), memberResult.getValue());

return new GameResultDto(resolvedResult);

}

private GameResultDto getResultOfMember(String memberName) {

HashMap<Member, Reward> resolvedResult = new LinkedHashMap<>();

Member member = new Member(memberName);

if (!gameResult.containsKey(member)) {

throw new IllegalArgumentException("해당 이름을 가진 참여자가 없습니다.");

}

Reward reward = gameResult.get(member);

resolvedResult.put(member, reward);

return new GameResultDto(resolvedResult);

}

이렇게 쓰면 어떨까요? HashMap이라는 자료형을 사용하는데, 굳이 루프를 돌면서 찾을 필요가 없을 것 같아요!
HashMap에서 요소를 찾는 시간복잡도는 O(1)이지만 루프를 시도하는 경우 O(N)이 되어버리네요!
가독성 측면에서도 더 나아보이는데 에버는 어떻게 생각하시나요?!🤔

그렇네요..
매번 리스트를 사용하다가 맵을 사용해서
당연하게 루프를 돌린 것 같아요 😂
좋은 지적 감사합니다!

yxxnghwan · 2024-03-05T13:01:05Z

src/test/java/domain/ConnectionTest.java

+    @DisplayName("생성 성공: 비연결 다음은 전략에 따른다. - 무조건 연결을 반환하도록 하는 전략")
+    void test_ok_makeNextConnectionOfDisconnectedByConnectedStrategy() {
+        Connection now = Connection.DISCONNECTED;
+        Connection next = now.makeNextConnection(new RandomConnectionStrategy() {


구체 클래스를 오버라이드해서 익명클래스를 인스턴스화한 이유가 있을까요? 인터페이스를 사용해서 인스턴스화해도 될 것 같아서요!

사실 인터페이스를 잘 활용하는 방법을 몰라 구현하게 된 코드입니다.
코드 수정해보겠습니다!

yxxnghwan · 2024-03-05T13:03:46Z

src/test/java/domain/GameTest.java

+
+    @Test
+    @DisplayName("이동 성공: 사다리를 타고 참여자와 결과를 매칭한다. - 모두 연결되지 않은 경우")
+    void test_ok_matchResult_allDisconnected() {


모든 라인마다 앞뒤로 공백라인이 있어 가독성이 떨어지는 테스트코드인 것 같아요!
테스트코드에서 보편적으로 given-when-then 에 맞춰 작성하고 그런 맥락이 변경되는 경우에만 라인 공백을 두는건 어떠실까요?

나름 가독성을 높인다고 추가한 공백이었는데 오히려 가독성을 떨어뜨렸을 수 있겠네요..
앞으로는 공백에도 의미를 부여해야겠어요!

yxxnghwan · 2024-03-05T13:05:09Z

src/test/java/domain/LineTest.java

    @RepeatedTest(50)
    @DisplayName("가로줄 생성 성공: 랜덤으로 생성된 값이 Connection 객체이다.")
    void test_ok_generateRandomPoint() {
-        Line line = Line.from(4, new RandomConnectionStrategy());
+        Line line = Line.of(4, new RandomConnectionStrategy());
        line.getConnections()
                .forEach(connection -> assertThat(connection).isInstanceOf(Connection.class));
    }


어떤 의미를 가지는 테스트일까요? Connection 객체의 타입을 검사해야하는 이유가 있는걸까요?

저희가 생성한 Connection 객체를 잘 반환하는지 확인하기 위한 테스트였으나
다시 읽어보니 꼭 필요하지 않은 테스트인 것 같네요..

helenason added 30 commits March 2, 2024 00:27

docs: 결과 관련 요구사항 목록 추가

2df96df

test: 결과 도메인 생성 테스트 코드 작성

2fd81f2

feat: 결과 도메인 생성 및 검증

e93f2fc

test: 에러메시지 수정 및 매직넘버 상수값 재사용

13613fc

test: 결과들 도메인 관련 테스트 코드 작성

c38f3fe

feat: 결과들 도메인 생성

1e0448c

test: 게임 도메인 테스트 코드 작성

fb9b37c

feat: 사다리 게임 결과 매칭 로직 추가

99daf5b

feat: 결과 및 타겟 입력받기

4911b06

refactor: 재사용 변수를 인스턴스 변수로 묶어 사용

5a5628d

refactor: Results 객체 가공 로직 추가를 위해 정적 팩토리 메서드 구조 채택

c9f0918

feat: 매핑할 결과 입력받는 로직 컨트롤러에 추가

e0ff233

refactor: Results 객체 Game 클래스 내 인스턴스 변수로 관리

2102a7b

feat: 사다리 결과에 매칭될 결과 출력 로직 추가

2c3c901

feat: 사다리 결과 출력 시 불필요한 개행 제거

facd52d

feat: 참여자의 이름을 통해 Member 객체 찾기

beeef31

refactor: GameResult 클래스 도입하여 게임 결과 관리

f910203

feat: 게임 실행 결과 출력 로직 추가

1dd91dd

feat: 해당 이름을 가진 참여자가 존재하는지 체크하는 메서드 추가

ce49a3b

feat: 결과 도출 및 출력 로직 컨트롤러에 합치기

803e167

fix: 비정상적인 게임 결과 도출하는 메서드 수정

ed1f1a1

test: 사다리 게임 테스트 케이스 추가

9748681

refactor: 게임 결과 출력 시 입력받은 사용자 순서로 출력하기 위해 LinkedHashMap 사용

8d4519c

feat: 게임 결과 최대 50번까지만 출력 가능하도록 변경

3a47aee

refactor: Name 클래스 MemberName으로 이름 변경

c9cd9f5

refactor: Lines 클래스 Ladder 클래스로 이름 변경

d47f231

refactor: Height 클래스 제거 후 Ladder 클래스에서 높이 검증

1111cf5

refactor: 문자열 파싱 역할 도메인에서 분리

fe27c4e

refactor: name으로 Member 객체 만드는 함수 stream 사용하도록 수정

3c16742

refactor: Results 클래스 validate 메서드 호출 위치 변경

d2e8627

helenason added 8 commits March 2, 2024 00:27

test: 상수 재사용 및 에러 메시지 출력 검증 로직 구체화

aabdef1

test: 리스트 길이 검증 로직 hasSize() 사용하도록 수정

1bd1c79

test: Results 객체 생성 성공 로직 검증 구체화

57cd2a7

test: Result 클래스 테스트 객체 생성 성공 및 에러메시지 검증 로직 구체화

855c812

refactor: 결과 출력 메서드 private으로 변경 및 gameResult 전달 시 Member 대신 String 타…

b0da8e1

…입으로 전달

feat: 참여자 이름이 될 수 없는 형태 검증 로직 추가

b8f8ba6

refactor: 매직넘버 상수 처리 및 ResultTarget의 변수 이름 변경

742a584

docs: 구현 완료된 기능 요구사항 체크 표시

0c1b40c

yxxnghwan requested changes Mar 2, 2024

View reviewed changes

helenason added 17 commits March 3, 2024 14:21

refactor: 불필요한 인스턴스 변수 제거

af3d268

refactor: 이름 동일 여부 비교하는 역할 Member 객체로 전달

cbae669

refactor: 가독성을 위해 부정문 줄이기

010a7d7

refactor: 명확한 의미 전달을 위해 두 개의 변수로 분리

3258ff9

refactor: 메서드 위치 기준 통일

ff4ef9f

feat: 게임 결과 전달을 위한 클래스 생성

6f957d0

refactor: DTO를 사용하여 게임 결과 반환 및 원활한 테스트를 위해 equals 재정의

8abe223

refactor: 가독성을 위해 메서드 추출

9d1d287

refactor: 문자열 분리 및 타입 변환 메서드 호출 역할 컨트롤러에서 뷰로 이전

2c189ae

refactor: do-while문을 사용하여 코드의 중복 제거

8b03eb1

refactor: 테스트를 위해 열어두었던 상수의 접근제어자 닫기 및 관련 테스트 코드 수정

199bc66

refactor: 모든 멤버의 결과 반환하는 메서드 불필요한 코드 정리

9089a1e

refactor: getName 메서드 재사용하도록 수정

29840d2

refactor: lines -> ladder 메서드명 수정 보완

006cf63

refactor: 일급컬렉션 수정 불가능하도록 반환

ad0a4ef

refactor: 구분자 상수화 및 스캐너 생성자 선언

b57d70d

refactor: 메서드 순서 변경 및 stringBuilder 사용하도록 수정

0ee1469

refactor: 매칭될 결과 클래스 이름 변경

209d116

yxxnghwan approved these changes Mar 5, 2024

View reviewed changes

yxxnghwan merged commit d0cac60 into woowacourse:helenason Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2단계 - 사다리 게임] 에버(손채영) 미션 제출합니다. #408

[2단계 - 사다리 게임] 에버(손채영) 미션 제출합니다. #408

helenason commented Mar 1, 2024 •

edited

Loading

yxxnghwan left a comment

yxxnghwan Mar 2, 2024

helenason Mar 3, 2024 •

edited

Loading

helenason commented Mar 3, 2024

yxxnghwan left a comment

yxxnghwan Mar 5, 2024

helenason Mar 5, 2024

yxxnghwan Mar 5, 2024

helenason Mar 5, 2024

yxxnghwan Mar 5, 2024

helenason Mar 5, 2024

yxxnghwan Mar 5, 2024

helenason Mar 5, 2024

[2단계 - 사다리 게임] 에버(손채영) 미션 제출합니다. #408

[2단계 - 사다리 게임] 에버(손채영) 미션 제출합니다. #408

Conversation

helenason commented Mar 1, 2024 • edited Loading

yxxnghwan left a comment

Choose a reason for hiding this comment

질문 답변

Choose a reason for hiding this comment

helenason Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

helenason commented Mar 3, 2024

질문 답변에 대한 답변

yxxnghwan 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

helenason commented Mar 1, 2024 •

edited

Loading

helenason Mar 3, 2024 •

edited

Loading