Step2 - 사다리(생성) #985

lsj8367 · 2021-08-04T03:45:08Z

이번 리뷰도 잘 부탁드립니다.

JunilHwang

안녕하세요 승재님! 사다리 생성 미션 잘 진행해주셨네요 👍
다만 지금은 객체지향보단 절차지향에 유사한 설계 같아요
사다리 미션에서 추출할 수 있는 객체들에 대해 조금 더 고민이 필요할 것 같아요!
이 외에도 몇 가지 리뷰 남겼으니 확인 후 피드백 요청 부탁드립니다 🙇‍♂️

src/main/java/nextstep/ladder/HumanMaker.java

src/main/java/nextstep/ladder/Line.java

src/main/java/nextstep/ladder/Validation.java

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

JunilHwang

안녕하세요 승재님!

원시값 포장객체와 일급컬렉션이 혼용되서 사용중인 것 같아요. 역할을 명확하게 분리해주세요
사다리 생성과 관련하여 힌트 하나 남겼으니 참고해주세요

일단 지금 상태는.. 이전과 크게 다를게 없는 것 같아요. 조금 더 객체 분리에 대해 신경써주시면 좋을 것 같습니다.

src/main/java/nextstep/ladder/LadderName.java

src/main/java/nextstep/ladder/Line.java

src/main/java/nextstep/ladder/domain/Lines.java

JunilHwang

안녕하세요! 피드백 반영 잘 해주셨네요.
몇 가지 리뷰 남겼는데 정리해보자면 다음과 같습니다.

View와 Domain을 명확하게 구분해주세요. 이번 미션을 진행하면서 반복적으로 강조하고 있는 것 같아요.
이름을 명확하게 지어주세요(ex: LineStrategy -> LineCreationStrategy)
for/if 중첩을 제거해주세요
사다리 생성 전략이 두 개로 분리되어 있는 것 같아요. 한 개로 합쳐주세요
이상입니다!

조금만 더 힘내주세요 💪

src/main/java/nextstep/ladder/domain/AddLines.java

src/main/java/nextstep/ladder/domain/IsOrNoneAddLines.java

src/main/java/nextstep/ladder/domain/LadderLineStrategy.java

src/main/java/nextstep/ladder/domain/LadderName.java

src/main/java/nextstep/ladder/domain/LadderNames.java

src/main/java/nextstep/ladder/domain/Lines.java

src/main/java/nextstep/ladder/domain/LineStrategy.java

src/main/java/nextstep/ladder/domain/Lines.java

JunilHwang

안녕하세요 승재님. 피드백 내용이 거의 전혀 반영되지 않은 것 같아요 ㅠㅠ

커밋 로그도 작업 단위로 나눠서 관리해주세요! 지금 한 개의 커밋에 모든 내용이 들어가있네요
View / Domain 이 아직 결합된 상태입니다. 분리해주세요
LadderName의 역할이 뭔지 다시 곰곰히 생각해보시면 좋을 것 같아요
Stream 미션인데 Stream에 대한 활용이 제대로 되고 있지 않은 것 같아요. Stream에 대해 조금 더 공부해보시면 어떨까요?

잘 모르겠다면 다른 분들이 어떻게 미션을 진행하고 있는지 참고해보는 것도 하나의 방법입니다!
어차피 미션 종료까지 아직 시간이 많기 때문에 천천히 고민해보셔도 좋을 것 같아요.
몇 차례 피드백 요청 보내주시고, 피드백을 하고 있는데 의미있는 개선이 거의 보이지 않는 것 같아요.. ㅠㅠ

src/main/java/nextstep/ladder/domain/LadderLineCreationStrategy.java

src/main/java/nextstep/ladder/domain/LadderName.java

src/main/java/nextstep/ladder/domain/LadderNames.java

JunilHwang · 2021-08-07T12:41:12Z

src/main/java/nextstep/ladder/domain/LadderNames.java

+    public int size() {
+        return this.ladderNames.size();
+    }


Suggested change

public int size() {

return this.ladderNames.size();

}

public int size() {

return ladderNames.size();

}

this는 생략해도 괜찮을 것 같아요.

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

src/main/java/nextstep/ladder/domain/LineCreationStrategy.java

JunilHwang · 2021-08-07T12:42:47Z

src/main/java/nextstep/ladder/domain/Lines.java

+    public Lines(List<Line> lines, int maxNum, LineCreationStrategy lineCreationStrategy) {
+        this.lines = lines;
+        this.lines = drawLine(maxNum, lineCreationStrategy);
+    }
+
+    public static Lines of (List<Line> lines, int maxNum, LineCreationStrategy lineCreationStrategy) {
+        return new Lines(lines, maxNum, lineCreationStrategy);
+    }
+
+    private List<Line> drawLine(int maxNum, LineCreationStrategy lineCreationStrategy) {
+
+        for (int i = 1; i < maxNum; i++) {
+            drawLine(lineCreationStrategy);
+        }
+
+        return lines;
+    }


LadderNames에 남겨놓은 피드백을 참고로해서 이부분도 개선해주세요

JunilHwang

안녕하세요 승재님! 피드백 반영 잘 해주셨네요 👍
몇 가지 피드백 남겼으니 확인해보시면 될 것 같습니다.

일단 핵심적인 이야기만 몇 가지 하자면,

테스트코드가 너무 빈약해요.
- 도메인 객체가 매우 많은데 지금은 두 개의 도메인에 대해서만 테스트하고 있네요.
- 모든 도메인 객체의 메소드에 대해 테스트할 수 있도록 작성해보세요!
항상 네이밍에 신경써주세요!
- 객체를 정확하게 묘사하고 있는지 고민해보세요!

Step2는 여기서 마무리하겠습니다.
피드백은 Step3 진행하면서 참고해보시면 좋을 것 같아요.
고생하셨습니다!

JunilHwang · 2021-08-09T10:11:06Z

src/main/java/nextstep/ladder/domain/LadderLineCreationStrategy.java

+public class LadderLineCreationStrategy implements LineCreationStrategy {
+    @Override
+    public boolean createLine() {
+        return getRandomNum();
+    }
+
+    private boolean getRandomNum() {
+        Random random = new Random();
+        return random.nextBoolean();
+    }
+}


LadderLineCreationStrategy은 너무 포괄적인 이름이 아닐까요? 행위를 구체적으로 묘사해주세요!

JunilHwang · 2021-08-09T10:11:36Z

src/main/java/nextstep/ladder/domain/LadderName.java

+    public LadderName(String ladderName) {
+        this.laddername = ladderName;
+    }
+
+    public static LadderName of(String ladderName) {
+        lengthValidation(ladderName);
+        return new LadderName(ladderName);
+    }


Suggested change

public LadderName(String ladderName) {

this.laddername = ladderName;

}

public static LadderName of(String ladderName) {

lengthValidation(ladderName);

return new LadderName(ladderName);

}

private LadderName(String ladderName) {

this.laddername = ladderName;

}

public static LadderName of(String ladderName) {

lengthValidation(ladderName);

return new LadderName(ladderName);

}

어차피 of가 똑같은 역할을 하고 있기 때문에 생성자는 private으로 지정해주세요!
무엇보다 생성자에 검증로직이 없기 때문에 of를 통해서만 인스턴스를 만들 수 있도록 해야합니다.

JunilHwang · 2021-08-09T10:12:38Z

src/main/java/nextstep/ladder/domain/LadderName.java

+
+    private static void lengthValidation(String name) {
+        if(name.length() > LETTER_LIMIT) {
+            throw new StringLengthException();


StringLengthException이란 이름 또한 굉장히 포괄적인 이름이 아닐까요?

JunilHwang · 2021-08-09T10:13:24Z

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

+    INITLINE(false),
+    NONELINE(false),
+    ISLINE(true);


Suggested change

INITLINE(false),

NONELINE(false),

ISLINE(true);

IN_IT_LINE(false),

NONE_LINE(false),

IS_LINE(true);

이렇게 구분해줘야 읽기 쉬울 것 같아요!

JunilHwang · 2021-08-09T10:14:57Z

src/main/java/nextstep/ladder/domain/Lines.java

+    private List<Line> lines;
+
+    public Lines(List<Line> lines, int maxNum, LineCreationStrategy lineCreationStrategy) {
+        this.lines = lines;
+        this.lines = drawLine(maxNum, lineCreationStrategy);
+    }


Suggested change

private List<Line> lines;

public Lines(List<Line> lines, int maxNum, LineCreationStrategy lineCreationStrategy) {

this.lines = lines;

this.lines = drawLine(maxNum, lineCreationStrategy);

}

private final List<Line> lines;

private Lines(List<Line> lines) {

this.lines = lines;

}

이전 리뷰에도 남겼는데 이렇게 표현할 수 있도록 개선해보세요!
지금은 코드가 무척 부자연스러워보이네요

JunilHwang · 2021-08-09T10:15:32Z

src/main/java/nextstep/ladder/domain/Lines.java

+    private void previousCheck(LineCreationStrategy lineCreationStrategy) {
+
+        if (lineCreationStrategy.createLine()) {
+            lines.add(Line.NONELINE);
+            return;
+        }
+        lines.add(Line.ISLINE);
+    }


이 로직은 생성로직입니다.
이 부분 또한 Strategy에게 위임해보세요.

JunilHwang · 2021-08-09T10:16:37Z

src/main/java/nextstep/ladder/view/ResultView.java

+public class ResultView {
+    private static final int LETTER_LIMIT = 5;
+    private static final String SPACE = " ";
+    private static final String REPLACE_STRING =  "\\[|\\]";
+    private static final String INIT_LINE = "    |";
+    private static final String NONE_LINE = "     |";
+    private static final String IS_LINE = "-----|";
+
+    public void print(LadderNames ladderNames, int maxLadderHeight) {
+        printLadderName(ladderNames);
+        printLadderLines(ladderNames, maxLadderHeight);
+    }
+
+    private static List<String> makeLine(List<Line> lines) {
+        return lines.stream()
+                .map(ResultView::drawLine)
+                .collect(Collectors.toList());
+    }
+
+    private static String drawLine(Line line) {
+        if (line == Line.INITLINE) {
+            return INIT_LINE;
+        }
+
+        if(line.isExist()) {
+            return IS_LINE;
+        }
+
+        return NONE_LINE;
+    }
+
+    public void printLadderLines(LadderNames ladderNames, int maxLadderHeight) {
+        for(int i = 0; i < maxLadderHeight; i++) {
+            printLines(Lines.of(initLine(), ladderNames.size(), new LadderLineCreationStrategy()));
+        }
+    }
+
+    private void printLines(Lines lines) {
+        System.out.println(makeLine(lines.getLines())
+                               .toString()
+                               .replaceAll(REPLACE_STRING, "")
+                               .replaceAll(", ", ""));
+    }
+
+    public void printLadderName(LadderNames ladderNames) {
+        Object[] names = Arrays.stream(ladderNames.toString().split(","))
+                               .map(this::fitLength)
+                               .toArray();
+
+        System.out.println(Arrays.toString(names).replaceAll(REPLACE_STRING, "").replaceAll(",", ""));
+    }
+
+    private String fitLength(String name) {
+        if(name.length() < LETTER_LIMIT) {
+            name = fitLength(SPACE.concat(name));
+        }
+
+        return name;
+    }
+
+    private List<Line> initLine() {
+        List<Line> lines = new ArrayList<>();
+        lines.add(Line.INITLINE);
+        return lines;
+    }
+}


어차피 멤버변수가 없기 때문에 전부 static method로 표현해도 무방할 것 같아요.
혹은 싱글톤으로 관리해주면 더 좋을 것 같습니다.

JunilHwang · 2021-08-09T10:17:10Z

src/main/java/nextstep/ladder/view/ResultView.java

+    public void printLadderLines(LadderNames ladderNames, int maxLadderHeight) {
+        for(int i = 0; i < maxLadderHeight; i++) {
+            printLines(Lines.of(initLine(), ladderNames.size(), new LadderLineCreationStrategy()));
+        }
+    }
+
+    private void printLines(Lines lines) {
+        System.out.println(makeLine(lines.getLines())
+                               .toString()
+                               .replaceAll(REPLACE_STRING, "")
+                               .replaceAll(", ", ""));
+    }
+
+    public void printLadderName(LadderNames ladderNames) {
+        Object[] names = Arrays.stream(ladderNames.toString().split(","))
+                               .map(this::fitLength)
+                               .toArray();
+
+        System.out.println(Arrays.toString(names).replaceAll(REPLACE_STRING, "").replaceAll(",", ""));
+    }


상수로 표현할 수 있는 것들이 많은 것 같아요!

JunilHwang · 2021-08-09T10:17:26Z

src/test/java/nextstep/ladder/LadderNamesTest.java

+    @Test
+    @DisplayName("이름 5글자 예외처리")
+    void ladderExceptionTest() {
+        assertThatThrownBy(() -> LadderNames.of("pobiaaaaa,honux,crong,jk"))
+                .hasMessageContaining("5글자")
+                .isInstanceOf(StringLengthException.class);
+    }


이건 LadderName에서 테스트해야하지 않을까요?

JunilHwang · 2021-08-09T10:17:43Z

src/test/java/nextstep/ladder/LinesTest.java

+public class LinesTest {
+    List<Line> list;
+
+    @BeforeEach
+    void setUp() {
+        list = new ArrayList<>();
+        list.add(Line.INITLINE);
+    }
+
+    @Test
+    @DisplayName("사다리 줄 생성 true")
+    void makeLineTest() {
+        int height = 5;
+
+        for(int i = 0; i < height; i++) {
+            Lines lines = Lines.of(list, 4, () -> true);
+            assertThat(lines).isEqualTo(Lines.of(list, 4, () -> true));
+        }
+    }
+
+    @Test
+    @DisplayName("사다리 줄 생성 false")
+    void makeLineFalseTest() {
+        int height = 5;
+
+        for(int i = 0; i < height; i++) {
+            Lines lines = Lines.of(list, 3, () -> false);
+            assertThat(lines).isEqualTo(Lines.of(list, 3, () -> false));
+        }
+    }
+
+}


Line에 대한 테스트 코드가 없네요 ㅠ
테스트 코드가 전체적으로 빈약합니다.

lsj8367 added 6 commits August 4, 2021 12:25

test: 사다리, 사람 생성 테스트

d182850

fix: 클래스이름 변경

1fa2267

test: 테스트 이름 변경

5939d5b

feat: 공백 예외처리 추가

f0e98a9

feat: 사다리 생성 로직 구현

9d42286

feat: 사다리 게임 구현

1ab5111

JunilHwang requested changes Aug 4, 2021

View reviewed changes

lsj8367 added 6 commits August 4, 2021 23:47

feat: 피드백 반영

019bf8e

remove: 불필요한 테스트 삭제

7ff9cbd

feat: 주석 추가 및 기능 삭제

68ed3d2

remove: 필요없는 테스트, 클래스 삭제

52851ad

test: 테스트 수정

4d4c5de

refactor: 피드백 반영

0141b72

JunilHwang requested changes Aug 5, 2021

View reviewed changes

lsj8367 added 6 commits August 5, 2021 21:58

fix: 피드백 반영

9202b70

fix: 피드백 반영 - 커스텀 exception

eec7c7a

fix: 패키지 이동 및 파일 삭제

a28d17e

test: 테스트 수정

f8736a7

fix: 패키지 이동

56c6a91

fix: 피드백 반영 수정

136ea23

lsj8367 commented Aug 5, 2021

View reviewed changes

src/main/java/nextstep/ladder/domain/Lines.java Outdated Show resolved Hide resolved

JunilHwang requested changes Aug 6, 2021

View reviewed changes

fix: 피드백 반영 (8/6)

dbeaf1f

JunilHwang requested changes Aug 7, 2021

View reviewed changes

lsj8367 added 6 commits August 8, 2021 13:45

fix: LadderNames 검증로직 수정

7aa5e45

fix: LadderName 길이 검증로직 추가

81eb1af

fix: 인터페이스 수정

3473d8a

fix: InitLine 예외처리

085828d

fix: line, ResultView 분리

d83baea

fix: 오타 수정

c80a4ff

fix: 예외 제거

7568c9f

JunilHwang approved these changes Aug 9, 2021

View reviewed changes

JunilHwang merged commit 774d817 into next-step:lsj8367 Aug 9, 2021

lsj8367 deleted the step2 branch August 9, 2021 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step2 - 사다리(생성) #985

Step2 - 사다리(생성) #985

lsj8367 commented Aug 4, 2021

JunilHwang left a comment

JunilHwang left a comment

JunilHwang left a comment

JunilHwang left a comment

JunilHwang Aug 7, 2021

JunilHwang Aug 7, 2021

JunilHwang left a comment •

edited

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

JunilHwang Aug 9, 2021

Step2 - 사다리(생성) #985

Step2 - 사다리(생성) #985

Conversation

lsj8367 commented Aug 4, 2021

JunilHwang left a comment

Choose a reason for hiding this comment

JunilHwang left a comment

Choose a reason for hiding this comment

JunilHwang left a comment

Choose a reason for hiding this comment

JunilHwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunilHwang left a comment • edited

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

JunilHwang left a comment •

edited