[1단계 - 사다리 생성] 망고(고재철) 미션 제출합니다. #129

Go-Jaecheol · 2023-02-16T08:18:10Z

안녕하세요 토니!
사다리 생성 - 1단계 미션 PR 제출합니다. 🙇‍♂️
아직 부족하지만 잘 부탁드립니다! :)
감사합니다.

toneyparky

안녕하세요 망고🥭 사다리 미션의 리뷰어 토니에요 👨‍💻

구현해주신 내용 잘 확인했습니다~! 커밋을 확인해봤는데 TDD를 최대한 적용하려하신 점이 인상적이었어요 👍 우테코에서 학습한 내용을 충실히 수행하고 있음이 느껴지네요!

제가 보기에 개선하면 좋을 점과 망고의 생각을 들어보고 싶은 내용에 대해 코멘트 남겼으니 확인 후 답변도 부탁드려요~! 보시다가 궁금한 사항에 대해서는 편하게 DM 주셔요~~! 화이팅입니다!

toneyparky · 2023-02-16T12:11:27Z

docs/README.md

@@ -0,0 +1,38 @@
+# 🪜사다리 타기


기능 요구사항과 구현할 내용을 잘 적어주셨네요~!
어떤 내용을 구현할지, 구현했는지를 확인하며 개발하면 실수할 일이 줄어들겠죠 👍

toneyparky · 2023-02-16T12:13:54Z

src/main/java/controller/LadderGameController.java

+import domain.Height;
+import domain.Ladder;
+import domain.Line;
+import domain.User;
+import domain.Users;
+import java.util.ArrayList;
+import java.util.List;
+import view.InputView;
+import view.OutputView;


MVC 패턴에 맞게 Controller에서 Model과 View 객체를 잘 연결해주고 있군요! 좋습니다!
OutputView에 도메인 객체가 넘어가고 있는데 이부분까지 DTO를 만들어서 넘겨주면 도메인과 View의 의존관계를 완전하게 끊을 수 있겠어요. 지금의 상황이라면 OutputView에서 도메인 객체를 조작할 수 있으니까요.

확인해보니 아래에 Model 레이어에 출력에 관한 로직이 많이 들어있어보이네요. DTO에 대해 학습하고 적용하여 개선해봐도 좋고 자바의 원시 타입을 사용해봐도 좋겠어요!

DTO를 만들어서 OutputView에 넘겨주도록 리팩토링 해보았습니다!

DTO를 처음 써보고 아직 DTO에 대해서 잘 몰라서 제대로 한 건지 확신이 들지 않습니다.😅
수정할 부분이 있을 것 같은데, 수정이 필요한 부분이 있다면 피드백 부탁드립니다!

잘 해주셨어요. 아직 익숙하지 않은 개념이라 확신이 들지 않는건 당연해요. DTO를 왜 사용하는지와 같은 부분에 대해서 고민해보면 망고도 망고만의 기준이 생길거에요.
참고 글

지금 수정하면 좋을 부분은, LadderDto의 getLines 메서드에서 List<Line> 을 반환하며 OutputView에 도메인(모델) 객체인 Line이 드러났다는 부분이에요. Dto가 필드로 Dto를 가져도 괜찮으니 다음 단계에서 수정해보죠!

그리고 OutputView에서 사용한 User.MAX_NAME_LENGTH 와 같은 도메인 값이 정말 도메인의 규칙인지 View의 규칙인지 생각해봐도 좋겠네요

toneyparky · 2023-02-16T12:14:35Z

src/main/java/controller/LadderGameController.java

+    private Users initializeUsers() {
+        try {
+            List<String> userNames = inputView.inputUserName();
+            return new Users(createUsers(userNames));
+        } catch (IllegalArgumentException e) {
+            return initializeUsers();
+        }
+    }
+
+    private List<User> createUsers(List<String> userNames) {
+        List<User> users = new ArrayList<>();
+        for (String userName : userNames) {
+            users.add(new User(userName));
+        }
+        return users;
+    }


initialize => create 로 넘어가는 규칙이 일관되게 적용되어서 좋네요. 다른 코드를 봐도 해당 규칙을 따르겠거니 생각이 드니까요!

toneyparky · 2023-02-16T12:17:04Z

src/main/java/controller/LadderGameController.java

+        outputView.printLadder(ladder);
+    }
+
+    private Users initializeUsers() {


게임을 진행하다보면 다음과 같은 상황이 발생하네요. 사용자가 당황스러워할 것 같은데 어떻게 조치할 수 있을까요?

initializeUsers() 메서드에서는 exception 캐치 후에 에러 메세지 출력을 안했네요,, 주의하겠습니다!

넵~ 확인했습니다. 항상 사용자를 고려하며 개발하도록 해요.

toneyparky · 2023-02-16T12:18:17Z

src/main/java/controller/LadderGameController.java

+        }
+    }
+
+    public Ladder createLadder(int personCount, Height height) {


해당 로직은 Ladder를 만드는군요! 이 로직을 컨트롤러에 두신 이유가 궁금해요~!

처음에는 단순히 사람 수만큼 반복문을 돌려서 lines 리스트를 만들고 -> 이걸 Ladder 생성자에 넘겨주면 되겠다고 생각했습니다.
하지만 이 과정 또한 생성하는 과정이니까 Ladder 생성자 내부에서 처리해도 되겠다고 생각해서 Ladder 생성자 내부로 옮겨보았습니다!

제가 생각한 방식이 올바른 접근일까요??
그러면 createUsers() 메서드도 Users 생성자 내부로 옮기는 게 맞을까요...?

저는 좋다고 생각해요. Users는 List<String> names를 받아서 자신이 스스로를 생성하는거죠~!

toneyparky · 2023-02-16T12:34:09Z

src/main/java/validate/InputVerifier.java

@@ -0,0 +1,35 @@
+package validate;
+
+public class InputVerifier {


이 객체도 현재 테스트코드에서만 사용하고 있군요. 이렇게 입력 값에 대한 검증 내용을 객체로 추출하신 이유가 있을까요~?

처음에는 입력 값이 들어오면 값을 검증하고 -> 검증한 값을 생성자로 넘기는 방식으로 구현했습니다.
그래서 검증하는 부분은 InputVerifier라는 클래스를 따로 만들어서 해당 클래스에 모아서 처리하도록 하면 가독성이 좋을 것 같다고 생각했습니다!

하지만 입력 값이 들어오면 값을 검증하고 -> 검증한 값을 생성자로 넘기는 방식보다는 입력 값을 생성자로 넘기고 -> 생성자에서 검증한 후 생성하는 방식이 더 깔끔할 것 같다고 생각해서 로직을 변경했습니다.
그 과정에서 InputVerifier 클래스를 삭제하기로 했는데, 제대로 리팩토링 되었는지 검사하지 않고 커밋해서 남아있는 것 같습니다. 😢

저도 객체 자체가 검증하는게 좋아요. 리팩터링을 진행하며 빼먹는 부분에 대해서는 어떻게 하면 쉽게 탐지할 수 있을까요?

인텔리제이의 전역 검색 (cmd + shift + f) 기능으로 검색한다.

해당 코드의 사용처 탐색 (cmd + b) 기능으로 탐색한다.

요정도가 생각나네요!

toneyparky · 2023-02-16T12:35:34Z

src/test/java/HeightTest.java

+        void shouldFailHeightWithNotNumber(String heightInput) {
+            assertThatThrownBy(() -> new Height(heightInput)).isInstanceOf(
+                            IllegalArgumentException.class)
+                    .hasMessageContaining(InputVerifier.HEIGHT_FORMAT_ERROR_MESSAGE);


실패에 대한 검증을 해주셨군요! 좋아요 👍

어떤 타입의 예외가 던져지는지, 어떤 메시지를 포함하는지를 다 검증하셨어요. 예외만 검증할 때와 메시지도 함께 검증할 때는 어떤 차이점이 있다고 생각하세요~? 망고의 생각을 들어보고 싶습니다!

예외만 검증하게 되면 같은 IllegalArgumentException이 발생했을 때, 그 예외가 해당 테스트에 맞는 예외인지 구분을 못할 거라고 생각해서 메시지까지 함께 검증하도록 했습니다!

오오 생각을 말씀해주셔서 감사해요 😉

말씀해주신대로 예외의 메시지까지 상세하게 검증할 수 있다는 장점이 있죠! 다만 예외 메시지가 변경될 때에 테스트 코드도 바뀌어야한다는 단점도 있구요. 망고는 이를 방지하기 위해 도메인 객체의 메시지 상수를 사용하셨네요~!

toneyparky · 2023-02-16T12:37:17Z

src/test/java/HeightTest.java

@@ -0,0 +1,53 @@
+import static org.assertj.core.api.Assertions.assertThatThrownBy;


테스트 파일은 해당 테스트 파일이 테스트하는 객체와 동일한 이름의 패키지에 있어야해요. (지금은 test/java/domain 이겠죠?)

이렇게 위치시켜야하는 이유는 뭘까요? 망고의 생각을 적어주세요!

동일한 이름의 패키지에 있어야 한다는 얘기를 들으니 protected나 default 접근 제어자로 된 메서드를 테스트 하기 위해서가 이유일 것 같습니다.
또한 프로덕션 코드와 비교할 때 가독성이 좋아진다는 것도 이유가 될 수 있을 것 같구요!

토니가 생각한 이유도 제가 생각한 이유와 같은가요??

네 우선 1:1 매칭이 되어서 코드를 보는 사람이 찾기 쉽다라는 측면도 있구요 말씀해주신 접근 제어자라는 이유도 있어요.
망고가 생각을 잘 적어주셨네요 👍👍

toneyparky · 2023-02-16T12:37:40Z

src/test/java/LineTest.java

+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+import domain.Line;
+import java.util.List;


사용하지 않는 import문이 왕왕있네요~!

toneyparky · 2023-02-16T12:38:11Z

src/test/java/UserTest.java

+        @DisplayName("변환된 입력값을 통해 올바르게 유저가 생성되는지 확인한다.")
+        @ParameterizedTest
+        @ValueSource(strings = {"i", "am", "fun", "dino", "mango"})
+        void shouldSuccessMakeUser(String name) {
+            assertDoesNotThrow(() -> new User(name));
+        }
+
+        @DisplayName("생성된 유저 객체가 올바르게 저장되는지 확인한다.")
+        @ParameterizedTest
+        @ValueSource(strings = {"i", "am", "fun", "dino", "mango"})
+        void shouldSuccessStoreUsers(String name) {
+            assertDoesNotThrow(() -> new User(name));
+        }


두 테스트는 어떤 차이가 있어요?

프로덕션 코드를 변경하고 테스트 코드도 이에 맞게 변경했는데, 테스트 코드가 중복되게 바뀌었다는 걸 인지하지 못하고 넘어간 것 같아요. 😢

시간이 많이 부족하셨군요..! 다음 미션은 리팩터링시 테스트 코드도 잘 챙겨줍시다!

Go-Jaecheol · 2023-02-21T02:50:32Z

toneyparky · 2023-02-21T11:30:12Z

docs/README.md

+
+---
+
+## 1단계 리팩토링 요구사항


리드미에 제가 추가로 요청드린 내용을 잘 적어주셨네요 굳굳

toneyparky · 2023-02-21T11:31:00Z

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

+    private final List<Line> lines;
+    private final Height height;
+
+    public Ladder(int personCount, Height height) {


Ladder 객체 입장에서 personCount 즉 사람의 명수라는 개념을 알 필요가 있을까요?
요 논의는 다음 단계를 진행하며 요구사항에 따라 답변이 달라지겠네요!

Ladder를 생성할 때 사람 수만큼 너비를 가져야된다고 생각해서 personCount를 넘겨주었습니다.

이렇게 다시 생각하고 보니 personCount라는 이름보다는 width라는 이름으로 넘겨주는 게 더 맞는 것 같네요!

toneyparky · 2023-02-21T11:31:26Z

src/main/java/dto/ladder/LadderDto.java

+        this.lines = lines;
+    }
+
+    public static LadderDto from(Ladder ladder) {


정적 팩터리 메서드를 사용하셨군요! from이라는 이름은 어떻게 정하셨어요?

정적 팩터리 메서드에 네이밍 컨벤션이 있는지 찾아보고, 하나의 매개변수를 받아서 객체를 생성하는 경우에는 from이라고 네이밍을 하는 걸 보고 이름을 정하게 되었습니다!

toneyparky · 2023-02-21T11:31:34Z

src/main/java/controller/LadderGameController.java

+import domain.Height;
+import domain.Ladder;
+import domain.Line;
+import domain.User;
+import domain.Users;
+import java.util.ArrayList;
+import java.util.List;
+import view.InputView;
+import view.OutputView;


잘 해주셨어요. 아직 익숙하지 않은 개념이라 확신이 들지 않는건 당연해요. DTO를 왜 사용하는지와 같은 부분에 대해서 고민해보면 망고도 망고만의 기준이 생길거에요.
참고 글

지금 수정하면 좋을 부분은, LadderDto의 getLines 메서드에서 List<Line> 을 반환하며 OutputView에 도메인(모델) 객체인 Line이 드러났다는 부분이에요. Dto가 필드로 Dto를 가져도 괜찮으니 다음 단계에서 수정해보죠!

그리고 OutputView에서 사용한 User.MAX_NAME_LENGTH 와 같은 도메인 값이 정말 도메인의 규칙인지 View의 규칙인지 생각해봐도 좋겠네요

toneyparky

코멘트가 많았는데 잘 반영해주셨네요 고생 많으셨어요 💯

망고가 남겨주신 답변에 대한 재질문이나 제가 전해드리고 싶은 내용에 대해 답변을 달았으니 확인하여 다음 단계에서 반영해주세요!
2단계는 이번 단계에서 부족하다고 느낀 부분을 개선하여 달려봅시다!!

jjongwa added 30 commits February 14, 2023 16:12

docs: 기능 목록 작성

594c69c

feat: 사람 이름 입력 기능 추가

cd3eb5f

test: 이름 길이 제한 테스트

6a32b6a

feat: 이름 길이 검증 기능 추가

bf8ee65

refactor: 이름 길이 검증 기능 리팩터링

0d84176

test: 이름 형식 검증 테스트

db3fd01

feat: 이름 형식 검증 기능 추가

a083c73

refactor: 이름 검증 테스트 구조 변경

4b1b221

test: 입력 문자열 분리 테스트

fedfd5a

feat: 입력 문자열 분리 기능 추가

d181963

refactor: 예외 메시지를 포함하여 검증하도록 테스트 로직 변경

8fb20fb

docs: 사람 이름을 입력받을 수 있다.

6c66fd1

test: 문자열 변환 기능 테스트

4c5eff6

feat: 문자열 변환 기능 추가

db4236a

refactor: 문자열 변환 기능 리팩터링

f26fa4c

test: 유저 생성 확인 테스트

4cadd0e

test: 유저 객체 저장 테스트

919a112

feat: 유저 객체 저장 기능 추가

4ced02b

refactor: User에 있던 검증 기능 InputVerifier 생성해 분리

9343348

docs: 사람 이름을 출력 형식에 맞게 변환하여 저장할 수 있다.

1e56d5f

test: 사다리 높이 형식 검증 테스트

f3f1406

feat: 사다리 높이 형식 검증 기능 추가

0d79d4f

test: 사다리 높이 제한 조건 검증 테스트

f37cfd4

feat: 사다리 높이 제한 조건 검증 기능 추가

5f82aec

refactor: 이름 조건 검증 기능 위치 변경

3e07a77

refactor: 이름 조건 검증 테스트 리팩터링

60344a0

refactor: 사다리 높이 검증 기능 위치 변경

1623c8d

refactor: 사다리 높이 검증 테스트 리팩터링

778f1b2

feat: 사다리 높이 입력 기능 추가

686606a

docs: 사다리 높이를 입력받을 수 있다.

c6e3097

jjongwa added 7 commits February 16, 2023 13:56

test: 라인 생성 제한 조건 테스트

926d1d4

feat: 라인 객체 생성 기능 추가

3b47699

docs: 라인을 추가할 수 있다.

7e572cb

feat: 실행결과 출력 기능 추가

9f61e91

docs: 사다리 게임을 출력할 수 있다.

536b5bc

refactor: 전체 기능 리팩터링

9a366a1

docs: 사다리 폭을 조정할 수 있다.

8a2708f

toneyparky requested changes Feb 16, 2023

View reviewed changes

Go-Jaecheol added 18 commits February 20, 2023 17:13

refactor: 불필요한 InputVerifier 클래스 삭제

5e89964

docs: 1단계 리팩토링 요구사항 정리

9acf81c

refactor: 중복된 테스트 삭제

4f1319c

refactor: User 생성에 대한 예외 처리 문구 출력하도록 변경

82825de

refactor: domain 패키지 내부에 하위 패키지를 두도록 변경

ae6527f

docs: 완료한 1단계 리팩토링 요구사항 체크

a052996

refactor: 테스트 파일이 테스트하는 객체와 동일한 이름의 패키지에 있도록 패키지 변경

d3f5ba7

refactor: createLadder() 메서드를 Ladder 생성자 안으로 위치 이동

56c9448

refactor: points에서 line으로, point에서 status로 네이밍 변경

9c61281

refactor: FixedNumberGenerator 클래스를 테스트 패키지로 이동

559d90f

refactor: NumberGenerator에서 BooleanGenerator로 클래스 네이밍 변경

6f73c76

refactor: LineStatus enum을 domain 패키지에서 view 패키지로 이동

02ea1aa

refactor: randomNumberGenerator 객체를 생성하는 코드 위치 변경

848ba48

refactor: isContinuousTrue() 메서드에 index를 직접 넘겨주지 않도록 변경

492d48d

refactor: 이름에 빈칸 추가하는 로직을 출력 부분으로 이동

150d8d2

refactor: DTO를 사용하도록 리팩토링

df5b83c

refactor: printLadder 메서드를 private으로 변경

0102385

refactor: 사용하지 않는 import문 삭제

a44b2a6

toneyparky reviewed Feb 21, 2023

View reviewed changes

toneyparky approved these changes Feb 21, 2023

View reviewed changes

toneyparky merged commit 3ba4dd3 into woowacourse:go-jaecheol Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1단계 - 사다리 생성] 망고(고재철) 미션 제출합니다. #129

[1단계 - 사다리 생성] 망고(고재철) 미션 제출합니다. #129

Go-Jaecheol commented Feb 16, 2023

toneyparky left a comment

toneyparky Feb 16, 2023

toneyparky Feb 16, 2023 •

edited

Loading

Go-Jaecheol Feb 21, 2023

toneyparky Feb 21, 2023

toneyparky Feb 16, 2023

toneyparky Feb 16, 2023

Go-Jaecheol Feb 20, 2023

toneyparky Feb 21, 2023

toneyparky Feb 16, 2023

Go-Jaecheol Feb 20, 2023

toneyparky Feb 21, 2023

toneyparky Feb 16, 2023

Go-Jaecheol Feb 20, 2023

toneyparky Feb 21, 2023

toneyparky Feb 16, 2023

Go-Jaecheol Feb 20, 2023

toneyparky Feb 21, 2023

toneyparky Feb 16, 2023

Go-Jaecheol Feb 20, 2023 •

edited

Loading

toneyparky Feb 21, 2023

toneyparky Feb 16, 2023

toneyparky Feb 16, 2023

Go-Jaecheol Feb 20, 2023

toneyparky Feb 21, 2023

Go-Jaecheol commented Feb 21, 2023

toneyparky Feb 21, 2023

toneyparky Feb 21, 2023

Go-Jaecheol Feb 27, 2023

toneyparky Feb 21, 2023

Go-Jaecheol Feb 24, 2023

toneyparky Feb 21, 2023

toneyparky left a comment

		@@ -0,0 +1,35 @@
		package validate;

		public class InputVerifier {

		@@ -0,0 +1,53 @@
		import static org.assertj.core.api.Assertions.assertThatThrownBy;

[1단계 - 사다리 생성] 망고(고재철) 미션 제출합니다. #129

[1단계 - 사다리 생성] 망고(고재철) 미션 제출합니다. #129

Conversation

Go-Jaecheol commented Feb 16, 2023

toneyparky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toneyparky Feb 16, 2023 • 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

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

Go-Jaecheol Feb 20, 2023 • 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

Go-Jaecheol commented Feb 21, 2023

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

toneyparky left a comment

Choose a reason for hiding this comment

toneyparky Feb 16, 2023 •

edited

Loading

Go-Jaecheol Feb 20, 2023 •

edited

Loading