[1단계 - 사다리 생성] 레디(최동근) 미션 제출합니다 by reddevilmidzy · Pull Request #281 · woowacourse/java-ladder

reddevilmidzy · 2024-02-22T07:21:16Z

안녕하세요 아서!! 👋

레디라고 합니다:) 이번 미션은 최대한 요구 사항만을 지키며 코딩하자는 목표로 구현을 하였습니다.

몇가지 질문이 생겨 남겨봅니다!

UI 로직과 TDD

초반에 TDD 방식으로 코드를 작성해보았는데, 확실히 구현하고자 하는 기능만 우선적으로 만들게 되어 다른 길로 빠지지 않는 다는 장점을 느꼈습니다.

미션 요구 사항에서 ui에 관련된 로직은 테스트 하지 않는다고 하여 ui 관련 로직은 TDD로 진행하지 않았습니다. 하지만 사다리 생성 1단계에서 가장 중요한 기능은 사다리를 생성하고 사용자에게 보여주는 기능이라고 생각합니다. 그리고 플레이어 이름을 입력받고 그 사이에 공백을 추가해주는 기능도 마찬가지로 ui와 관련된 로직이라고 생각하여 TDD로 하지 않았습니다.

왜 UI 로직은 TDD로 하지 않아도 된다는 요구 사항이 있는지 궁금합니다! 아니면 사다리를 String 으로 변환하는 로직들(Formatter)은 UI와 관련된 로직이 아니기에 TDD로 진행하는 것이 맞았을까요?

Formatter vs DTO

위의 질문과 연관되는 질문이긴 합니다만, 사다리를 생성하고 문자열 형태로 변경하는 로직을 처음에는 DTO로 만들었다가 Formatter 라는 util 클래스를 만들어서 하는 방식으로 변경하였습니다.
static 메서드로만 이루어진 util 클래스는 객체지향과 멀어진다고 생각하여 지양하고 있었는데, 그렇다고 DTO로 변환하여 넘겨주자니 DTO에 너무 많은 로직이 포함되는 문제가 발생하였습니다.

현 상황에서 어떤 방식이 더 적절할까요?

감사합니다!!

Hyunta

안녕하세요 레디!
사다리 미션으로 만나게 되어 반갑습니다, 아서입니다. 👋🏻

TDD 방식으로 페어와 미션을 잘 진행해주셨네요.
적절한 객체를 활용해서 메서드와 클래스도 잘 나눠주셨어요ㅎㅎ

테스트 코드 쪽에 메서드 이름들만 조금 수정해주시고,
남긴 커멘트만 확인해주세요!

왜 UI 로직은 TDD로 하지 않아도 된다는 요구 사항이 있는지 궁금합니다! 아니면 사다리를 String 으로 변환하는 로직들(Formatter)은 UI와 관련된 로직이 아니기에 TDD로 진행하는 것이 맞았을까요?

UI 로직을 TDD로 하지 않아도 점은 아마 System.Out , System.In 과 관련된 테스트를 짜기보다는 도메인 로직을 TDD로 경험해보라는 차원에서 있었던 요구사항 인듯합니다.
만들어주신 Formatter는 테스트 대상이라고 생각합니다. TDD로 진행하는 것이 좋았을 것 같아요.
리뷰요청을 보내기 전에 Formatter와 관련된 테스트코드는 꼭 추가해주세요

위의 질문과 연관되는 질문이긴 합니다만, 사다리를 생성하고 문자열 형태로 변경하는 로직을 처음에는 DTO로 만들었다가 Formatter 라는 util 클래스를 만들어서 하는 방식으로 변경하였습니다.
static 메서드로만 이루어진 util 클래스는 객체지향과 멀어진다고 생각하여 지양하고 있었는데, 그렇다고 DTO로 변환하여 넘겨주자니 DTO에 너무 많은 로직이 포함되는 문제가 발생하였습니다.

Formatter르 util 클래스로 보기에는 Ladder에 너무 종속적이라고 생각합니다.
util 클래스로 분류하는 본인의 기준이 있을까요?
Line 에 대한 처리는 범용적인 기능이라고 보이지는 않습니다. 어쩌면 Line이 해야할 일인데 위치를 찾다보니 애매해서 util에 넣지는 않았을까 고민해보세요.
지금은 객체 지향적인 설계에 대한 학습을 하는 과정이니 의도적으로 util 클래스는 최후의 보루로 생각하고, 어떤 객체가 책임을 맡아야 하는가 고민을 해보세요.
레디는 DTO가 어떤 역할을 해야한다고 생각하나요? 너무 많은 로직이 포함되었다면 DTO 라고 부르는게 맞았을지 고민해보시길 바랍니다. 때로는 저런 단어들이 혼란을 줄 때도 있습니다 지금은 학습 단계이니 DTO와 유틸클래스를 배제하고 누가 Formatter의 기능을 책임져야할까를 고민해보시면 도움이 될 것 같습니다.

Hyunta · 2024-02-22T13:38:35Z

src/test/java/model/LineTest.java

+class LineTest {
+
+    @Test
+    @DisplayName("사다리 생성 테스트")
+    void createLine() {
+        int personCount = 4;
+
+        Assertions.assertThatCode(() -> new Line(personCount))
+                .doesNotThrowAnyException();
+    }
+
+
+    @Test
+    @DisplayName("동일한 true 값을 가지지 않는지 확인")
+    void createSame() { //TODO: 이름 변경하기
+        //given
+
+        Line line = new Line(4);
+        List<Boolean> points = line.getPoints();
+
+        //when
+        for (int i = 0; i < points.size() - 1; i++) {
+            if (points.get(i) == points.get(i + 1)) {
+
+            }
+        }
+        //then
+    }
+}


TDD를 Line 이라는 객체를 만들면서 시작하셨네요,
페어와 어떻게 시작점을 결정하셨나요?

우선 사다리 생성 미션에서 가장 핵심적인 기능이 무엇일까 고민하다가 사다리 생성! 이라고 판단하였고 이 사다리를 가장 빨리 생성할 수 있는 테스트를 작성하고 싶었습니다
그래서 다른 것보단 사다리 생성을 위해 Line이라는 객체를 만들었습니다.

사실 이전에는 프로그램 흐름 순으로 구현 순서를 정했었는데 TDD를 적용해보기 위해 처음으로 이런 순서로 구현해보았습니다.

Hyunta · 2024-02-22T13:41:30Z

docs/README.md

 ## 참여 인원의 이름을 입력하는 기능
- [ ] 이름 입력 요청 메시지를 출력한다.
- [ ] 참여할 사람 이름을 입력 받는다.
+- [x] 이름 입력 요청 메시지를 출력한다.
+- [x] 참여할 사람 이름을 입력 받는다.


사다리를 만들고나서 다음 순서로 이름 입력 기능을 구현하였네요.
이름 혹은 Player 가 존재해야 입력값이 존재할 수 있는 기능이 아닐까요?

헉 맞습니다!!

Hyunta · 2024-02-22T13:42:45Z

src/main/java/view/InputView.java

-    public String readHeight() {
+    public int readHeight() {
        System.out.println("최대 사다리 높이는 몇 개인가요?");
        String rawHeight = scanner.nextLine();
-        return rawHeight;


점진적인 리팩터링👍🏻

Hyunta · 2024-02-22T13:43:41Z

src/test/java/model/LadderTest.java

+    @Test
+    @DisplayName("사다리 높이가 사다리 객체의 크기가 된다.")
+    void createLadder() {
+        // given
+        Height height = new Height(5);
+        int personCount = 7;
+
+        // when
+        Ladder ladder = Ladder.of(height, personCount);
+
+        // when
+        Assertions.assertThat(ladder.size()).isEqualTo(height.getValue());
+    }


테스트 메서드 이름을 조금 더 상세하게 적어주세요

Hyunta · 2024-02-22T13:44:52Z

src/test/java/model/LineTest.java

+class LineTest {
+
+    @Test
+    @DisplayName("사다리 생성 테스트")
+    void createLine() {
+        int personCount = 4;
+
+        Assertions.assertThatCode(() -> new Line(personCount))
+                .doesNotThrowAnyException();
+    }


사다리 생성 테스트인데 Line생성에 대한 테스트를 담고 있네요

Hyunta · 2024-02-22T14:00:12Z

src/main/java/view/InputView.java

+    private int convert(String rawHeight) {
+        try {
+            return Integer.parseInt(rawHeight);
+        } catch (NumberFormatException exception) {
+            throw new IllegalArgumentException(Message.INVALID_HEIGHT_ERROR.getMessage());
+        }
+    }


명확한 이름을 부여해주세요

Hyunta · 2024-02-22T14:01:11Z

src/main/java/model/Height.java

+    boolean isOutOfRange(int value) {
+        return value <= 0 || value > UPPER_BOUND;
+    }


접근제어자를 default로 두신 이유가 있을까요?

헉!😮 놓친 부분이네요 수정하겠습니다

Hyunta · 2024-02-22T14:10:27Z

src/main/java/view/OutputView.java

+    public void printResult(List<String> names, List<Line> lines) {
+        System.out.printf(FINAL_RESULT_FORMAT, FINAL_RESULT_MESSAGE);
+        printPlayers(names);
+        printLadder(names.get(0).length(), lines);
+    }


view 단에서 Line 도메인을 모르도록 하는게 좋을 것 같아요.
레이어를 잘 분리해봅시다.

Hyunta · 2024-02-22T14:20:26Z

src/main/java/view/Formatter.java

+        StringBuilder stringBuilder = new StringBuilder();
+        stringBuilder.append(names.get(0)).append(" ");
+        for (String name : names.subList(1, names.size() - 1)) {
+            stringBuilder.append(getNameWithSpace(6 - name.length(), name));
+        }
+        String lastPlayer = names.get(names.size() - 1);
+        stringBuilder.append(getNameWithSpace(5 - lastPlayer.length(), lastPlayer));
+        return stringBuilder.toString();


이 부분 로직이 너무 복잡한 것 같습니다.
각각의 기능들을 용도에 맞게 메서드로 분리해주세요.
첫번째 사람, 중간 사람들, 마지막 사람 이런식으로 나눌 수 있을 듯해요.

직접 구현해보는 것도 좋지만,
Java String 클래스를 활용해서 String.format() 를 학습해보는 것도 좋을 것 같아요.

Hyunta · 2024-02-22T14:21:11Z

src/main/java/view/Formatter.java

+        for (int index = 0; index < line.size(); index++) {
+            lineBuilder.append(getElement(line.isConnected(index)));
+            lineBuilder.append(LadderElement.COLUMN.getSymbol());
+        }


비즈니스 로직이 view 레이어에 녹아있네요

reddevilmidzy

리뷰 감사합니다!
코드 고칠 생각에 두근두근하네요:)

reddevilmidzy · 2024-02-22T14:47:13Z

src/main/java/Application.java

+    public static void main(String[] args) {
+        Scanner scanner = new Scanner(System.in);
+        LadderController controller = new LadderController(
+                new InputView(scanner),
+                new OutputView()
+        );
+        controller.run();
+    }


핑계이긴 거 같지만, 재입력을 해야한다는 요구 사항이 딱히 없어 구현하지 않았습니다. 재입력에 대한 고민을 하기보단 TDD나 리팩터링에 대해 신경을 썼던거 같습니다

2단계에서 구현해보도록 하겠습니다! 마침 새로운 방식으로 처리하는 방법을 배워 근질근질 하네요:)

reddevilmidzy · 2024-02-22T14:52:33Z

src/main/java/exception/Message.java

+public enum Message {
+    BLANK_INPUT_ERROR("빈 문자열 입력입니다. 다시 입력해주세요."),
+    INVALID_HEIGHT_ERROR("잘못된 사다리 높이 입력입니다. 다시 입력해주세요."),
+    INVALID_PLAYER_ERROR("잘못된 참여 인원 이름 입력입니다. 다시 입력해주세요."),
+    INVALID_SEPARATOR_ERROR("잘못된 구분자 입력입니다. 다시 입력해주세요.");
+


현재 예외 메시지는 꽤 높은 레벨로 추상화된 형태여서 여러 곳에서 사용할 수 있다고 생각하여 개별 class 마다 상수로 넣지 않고 한곳에 모아두었습니다!
(INVALID_PLAYER_ERROR 같은 경우 Player와 Players 두 군데에서 사용)

그렇다면 한 곳에 모을 때 class나 interface가 아닌 enum으로 관리한 이유는 이런 상수만 가지고 있는 경우 enum을 사용하는게 enum의 목적과 걸맞는다고 생각하여 메세지를 enum 으로 관리하였습니다

reddevilmidzy · 2024-02-22T14:53:44Z

src/main/java/model/Height.java

+    boolean isOutOfRange(int value) {
+        return value <= 0 || value > UPPER_BOUND;
+    }


헉!😮 놓친 부분이네요 수정하겠습니다

reddevilmidzy · 2024-02-22T14:55:39Z

src/test/java/model/PlayerTest.java

+    @Test
+    @DisplayName("플레이어 생성 테스트")
+    void createPlayer() {
+        String name = "pobi";
+        Assertions.assertThatCode(() -> new Player(name))
+                .doesNotThrowAnyException();
+    }
+
+    @Test
+    @DisplayName("플레이어의 이름이 5자 초과여서 오류가 발생한다.")


좋습니다!

Hyunta

피드백 빠르게 잘 반영해주셨네요 레디!
1단계 요구사항을 모두 충족시켜서 이번 PR은 여기서 머지하도록 할게요.
몇가지 피드백을 남겼는데, 2단계 구현시 반영 부탁드립니다.
고생하셨습니다👍🏻

Hyunta · 2024-02-23T14:52:59Z

src/main/java/view/OutputView.java

+    private void addLastName(List<String> names, StringBuilder stringBuilder) {
+        String lastPlayer = names.get(names.size() - 1);
+        stringBuilder.append(String.format("%5s", lastPlayer));
+    }
+
+    private void addMiddleNames(List<String> names, StringBuilder stringBuilder) {
+        for (String name : names.subList(1, names.size() - 1)) {
+            stringBuilder.append(String.format("%6s", name));
+        }
+    }
+
+    private void addFirstName(List<String> names, StringBuilder stringBuilder) {
+        stringBuilder.append(String.format("%s ", names.get(0)));
+    }


좋습니다 이전보다 훨씬 메서드를 통해서 의도를 잘 드러낼 수 있게 되었네요.👍🏻

세가지 기능은 전부 이름을 출력하고 비슷한 기능을 수행하고 있네요.
처음, 중간, 끝 사람을 출력하는 형태가 꼭 달라야 했는지를 잘 고민해보시고,
만약 레디의 생각에는 필수적인 조건이라고 한다면 메서드 갯수를 줄여볼 수 있을까요?

그리고 StringBuilder를 전달해서 값을 외부에서 append 하는 방식을 사용하고 있는데,
개인적으로 builder를 이동시켜가면서 조작하는건 좋아보이진 않습니다.
StringBuilder의 상태가 여러 메서드에 의해서 수정될 수 있기 때문에요,
저는 String을 반환하도록 하고, formatNames 에서 append하는 과정을 진행했을 것 같아요.

지금 출력 문이 조금 이상하게 나오고 있는 것 같은데 확인 부탁드려요

참여할 사람 이름을 입력하세요. (이름은 쉼표(,)로 구분하세요) a,aa,aaa,aaaa,aaaaa 최대 사다리 높이는 몇 개인가요? 10 실행결과 a aa aaa aaaaaaaaa |-----| |-----| | | | | | | |-----| |-----| | | | |-----| | | |-----| |-----| |-----| | | | | | |-----| | |-----| |-----| | |-----| | | | | |-----| |-----|

Hyunta · 2024-02-23T14:53:36Z

src/test/java/model/PlayerTest.java

+    @Test
+    @DisplayName("플레이어의 이름이 5자 초과여서 오류가 발생한다.")
+    void invalidNameLength() {
+        String name = "pobipobi";
+        Assertions.assertThatThrownBy(() -> new Player(name))
+                .isInstanceOf(IllegalArgumentException.class);
+    }


엣지케이스를 잡는 테스트로 구성하면 더 좋을 것 같아요!

Mingyum-Kim and others added 30 commits February 21, 2024 14:35

docs: 기능 구현 목록 생성

a4437ae

feat: n - 1번 true/false를 결정하여 사다리 라인의 유무를 결정한다.

6e77817

feat: 이전 라인에서 true인 경우 false로 설정하는 기능 구현

383aa70

feat: 최대 사다리 높이만큼 사다리 라인을 생성한다.

7d698a0

feat: 이름 입력 요청 메시지를 출력하고 이름을 입력받는다.

f2ae11c

feat: 쉼표(,)로 구분된다.

bb7d759

feat: 빈 문자열은 예외 처리한다.

ea231d2

feat: 이름은 최대 5글자이다.

eaae4db

feat: 최소 2명 이상 입력 받는다

83a3450

feat: 최대 사다리 높이 입력 요청 메시지를 출력한다.

b5b77f8

feat: 최대 사다리 높이를 입력 받는다.

b7e9e5b

feat: 1 이상의 숫자임을 검증한다.

5175d8e

docs: 기능 구현 목록 추가

578f221

feat:쉼표가 연속으로 오거나 양 옆에 오는 경우 예외처리한다.

6f65319

feat: 빈 문자열은 예외 처리한다.

caa325e

feat: 실행 결과를 출력한다.

3ab7d8f

feat: 사다리 요소 Enum 생성

5e0e3a2

feat: 참여 인원의 이름을 출력한다.

470b68d

feat: 사다리 출력을 위한 DTO 구현

eecb978

feat: 첫번째 플레이어 이름 길이만큼 공백을 출력한다.

23055c4

refactor: 사다리 요소를 가져오는 메서드 분리

d4007b1

fix: 사다리가 연속으로 true가 되는 문제 해결

1592c6c

fix: 사용자 이름 출력 간격 수정

5d7580f

feat: 사다리 높이 검증

517a1ef

refactor: Height 객체를 사용하여 Application 수정

4be4027

refactor: LadderResponse 미사용 로직 제거

18b8597

refactor: Controller 로직 분리

8961674

refactor: 불필요한 import 제거 및 테스트 코드 접근제한자 수정

aa95c18

refactor: 필드에 final 추가

0698d05

refactor: 실행결과 출력 메서드 수정

c5db814

Mingyum-Kim and others added 13 commits February 22, 2024 14:20

refactor: Formatter에서 출력 구문을 생성

8e64086

refactor: dto 대신 formatter로 변경

e148ad0

refactor: Formatter의 formatPlayers 메서드 분리

ce558f0

refactor: 상수를 정적 변수로 분리

45fa8c1

refactor: 문자열 상수를 정적 변수로 분리

7c410d2

refactor: 문자열을 상수로 변경

2a346a0

fix: 실행결과 메시지에 대한 개행 수정

7f92a1a

style: 자바 코드 컨벤션 적용

dfa81a3

feat: 예외 메시지를 Enum에서 관리하도록 구현

7599f25

style: 메서드를 분리하고 가독성 개선

4b1fb70

refactor: 메서드 네이밍 개선

213af7d

refactor: 테스트 메서드에서 Test 네이밍 제외

151bedf

docs: 구현 완료한 기능 목록 체크

31eb778

Hyunta requested changes Feb 22, 2024

View reviewed changes

reddevilmidzy commented Feb 22, 2024

View reviewed changes

reddevilmidzy added 13 commits February 23, 2024 10:41

style: todo 주석 제거

9940835

style: displayName 메시지의 형식을 통일

d6bf3f0

refactor: 메서드 분리하였던 것을 다시 병합

226ae31

refactor: 메서드 이름을 명확하게 수정

1130807

refactor: height 메서드의 접근제어자 default에서 private로 변경

0659c1a

refactor: Line을 생성하는 것을 캡슐화하여 전략패턴 적용

1d6ba05

refactor: isConnected 메서드명을 hasStep으로 변경

b561863

feat: Height의 equals 와 hashCode 정의

8b0ecb6

refactor: 이름사이 공백에서 String.format 활용

b008d3e

refactor: 이름 사이에 공백 추가하는 로직을 players로 이동 및 테스트 추가

ce55464

refactor: 사다리 출력 책임 위치 변경

fc67897

refactor: 사다리 생성 전략 클래스 및 인터페이스를 한 패키지로 묶음

c258890

refactor: 이름 포맷팅 메서드 분리

4977211

Hyunta approved these changes Feb 23, 2024

View reviewed changes

Hyunta merged commit ebcc564 into woowacourse:reddevilmidzy Feb 23, 2024

Conversation

reddevilmidzy commented Feb 22, 2024

Uh oh!

Hyunta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reddevilmidzy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hyunta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants