3주차 - Step4 사다리(리팩토링) 리뷰요청 드립니다..! by Integerous · Pull Request #221 · next-step/java-ladder

Integerous · 2019-07-08T14:50:40Z

질문에 빠른 답변 감사합니다!! 👍 👍 👍 👍 👍

Bar객체가 방향(Direction)을 가지게 해보라는 피드백을 주셨었는데요,
이에 대한 질문을 피드백에 댓글 로 남겼습니다..!

조금 멍청한 질문일지 모르겠지만
사다리를 구현하면서 가장 고민을 많이 한 부분이라
바쁘시겠지만 조언 부탁드리겠습니다..!!

나머지 피드백은 모두 반영했습니다-!

…지 보내기

wotjd243

피드백을 잘 반영해 주셨습니다. 👍
설계한 것을 소리 내어 읽어 보고 말이 되는지 확인해 보는 것이 많은 도움이 된답니다.
그 외에도 몇 가지 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
진행 과정에서 궁금한 점은 댓글이나 Slack을 통해 물어보세요.

wotjd243 · 2019-07-09T07:13:45Z

src/main/java/ladder/Application.java

+
        Height height = Height.from(InputView.askHeight());

        Ladder ladder = Ladder.from(players, height);


GameInfo와 Ladder를 가진 LadderGame 클래스를 만들어 보면 어떨까요?

wotjd243 · 2019-07-09T07:32:33Z

src/main/java/ladder/Application.java

+        OutputView.drawLadder(ladder, gameInfo);

-        GameResult gameResult = GameResult.of(players, ladder, prizes);
+        GameResult gameResult = GameResult.of(ladder, gameInfo);


"LadderGame에서 GameResult를 생성한다."라는 설계는 어떨까요? 이렇게 하면 어떤 다른 곳에서 해당 객체를 생성할 때 생산자의 정보를 필요로 하는 것을 줄일 수 있어요. 아울러 생산자와 생성된 객체 사이의 특별한 관계를 전해주기도 합니다.

LadderGame에 gameInfo랑 ladder가 있으니까 LadderGame에서 GameResult를 생성하는것이 맞겠네요! 피드백 감사합니다!!

wotjd243 · 2019-07-09T07:57:12Z

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

-        return position;
-    } //TODO: 이 로직을 Bar 객체로 위임하고 싶은데, Bar 2개를 사용해서 위치를 갱신하는 방식이라 이 곳에서 처리하는 상태
+        return position; // 위의 경우가 아니면 위치 유지(패스)
+    } //TODO: 이 부분에 대한 질문을 https://github.com/next-step/java-ladder/pull/207 피드백 주신 이 곳에 남겼습니다.


질문에 대한 답변은 Slack을 통해 남겼어요. 👍

wotjd243

피드백을 전부 잘 반영해 주셨습니다. 💯
간단한 생각거리를 남겼으니 코드에 반영해 주셔도 좋습니다.
진행 과정에서 궁금한 점은 댓글이나 Slack을 통해 물어보세요.

wotjd243 · 2019-07-11T08:30:05Z

src/main/java/ladder/domain/Bar.java

@@ -2,17 +2,29 @@

 public class Bar {


아래의 질문에 답해 보면 좋을 것 같아요.

Bar 인스턴스가 만들어지는 경우의 수는 몇 가지 일까요?

미리 생성하여 필요할 때마다 사용해 볼 수 있을까요?

위의 방식은 어떤 장점이 있을까요?

Point로 바꾸었다고 가정했을 때, 아래 3가지 경우에 point를 생성할 수 있습니다.

왼쪽 point가 없고, 오른쪽 point가 있을 때, (생성되면 왼쪽으로 연결)

왼쪽 point가 있고, 오른쪽 point가 없을 때, (생성되면 오른쪽으로 연결)

양쪽 point가 모두 없을 때 (아래로 연결)

Point 인스턴스가 생성되기 위해서는 위의 3가지 조건에만 가능하고, 각 생성 조건은 결국 연결되는 방향을 결정합니다. 그러므로 생성조건 3가지를 '방향'이라는 Enum에 각각 정의 해두고 사용하면 될 것 같습니다.

장점

어떤 조건에서 point가 생성되고, 어느 방향으로 연결되는지 명확하게 표현할 수 있습니다.

불가능한 생성 조건 (true, true)를 사전에 방지할 수 있습니다.

게임의 룰이 바뀌었을 때 Enum만 수정하면 되므로 결합도를 낮출 수 있습니다.

주신 질문에 답을 작성하다보니 구현 방향이 정해지네요! 감사합니다ㅎㅎ
혹시나 제 답변에서 잘못된 부분이나 추가해주실 부분이 있으면 조언부탁드립니다!

wotjd243 · 2019-07-11T08:30:18Z

src/main/java/ladder/domain/Bar.java


-    private Bar(boolean bar) {
-        this.bar = bar;
+    private Bar(boolean left, boolean right) {


"가로 라인은 겹치지 않는다."라는 요구 사항을 코드로 표현해 보면 어떨까요?

Direction Enum이 추가됌에 따라 Direction에 (true, true)를 검증하는 메서드를 만들었습니닷.. 올바르게 개선한 것인지 궁금합니다..!

wotjd243 · 2019-07-11T08:40:48Z

src/main/java/ladder/domain/Position.java

+
    private void validationMovableToLeft() {
-        if (unmovableToLeft()) {
+        if (!isMovableToLeft()) {


아래의 글을 읽고 코드에 반영하면 어떨까요?
https://schneide.blog/2014/08/03/dont-ever-not-avoid-negative-logic

wotjd243 · 2019-07-11T08:56:21Z

src/main/java/ladder/Application.java

        Height height = Height.from(InputView.askHeight());

+        GameInfo gameInfo = GameInfo.of(players, prizes);
        Ladder ladder = Ladder.from(players, height);


Ladder가 Players에 의존하는 구조를 바꿔 볼 수 있을까요?

wotjd243

피드백을 전부 잘 반영해 주셨습니다. 💯
질문에 대한 답변은 코멘트로 남겼어요.
간단한 생각거리를 남겼으니 코드에 반영해 주셔도 좋습니다.
다음 리뷰 요청 때는 Merge 하도록 하겠습니다.

wotjd243 · 2019-07-14T05:50:37Z

src/main/java/ladder/domain/Direction.java

+    private boolean right;
+
+    Direction(boolean left, boolean right) {
+        validationDirection(left, right);


이미 요구 사항을 Direction으로 잘 표현하였어요. 👍

Suggested change

validationDirection(left, right);

wotjd243 · 2019-07-14T05:51:00Z

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

+        return Collections.unmodifiableList(points);
+    }
+
+


공백 라인을 의미 있게 사용하는 것이 좋아 보이며, 문맥을 분리하는 부분에 사용하는 것이 좋습니다. 과도한 공백은 다른 개발자에게 의문을 줄 수 있어요.

wotjd243 · 2019-07-14T05:51:12Z

src/main/java/ladder/domain/PointGenerator.java

+@FunctionalInterface
+public interface PointGenerator {
+
+    boolean generatePoint();


"PointGenerator는 boolean을 생성한다."라는 설계는 자연스러울까요?

wotjd243

수많은 피드백을 반영하고자 대단히 고생 많으셨습니다. 💯
"인내는 쓰고 열매는 달다."라는 말처럼 정말 달콤한 열매를 수확했길 바랍니다.
이만 Merge 하겠습니다.

Integerous added 12 commits July 8, 2019 21:08

[step4] refactor : 매직넘버를 상수로 치환

6e9f9e1

[step4] refactor : 데이터를 가진 객체에 메세지 던지도록 수정

36b16d6

[step4] refactor : 인터페이스에 상수 선언 대신 상수 유틸리티 클래스 사용

67d2cab

[step4] refactor : 필요없어진 NameFormatter 인터페이스와 구현체 제거

517b95b

[step4] refactor : 의미 모호한 메서드명 수정

9f986da

[step4] feat : Players와 Prizes를 가진 GameInfo 클래스 생성

3f1cd73

[step4] refactor : 상품수와 플레이어수 유효성 체크를 GameInfo에서 처리

7007291

[step4] refactor : OutputView로 넘기는 인자들(Players, Prizes)을 GameInfo로 변경

4b480c7

[step4] refactor : GameResult로 넘기는 인자들(Players, Prizes)을 GameInfo로 변경

b2ca7a0

[step4] refactor : 필요없어진 NameConstants(상수 유틸리티 클래스) 제거

523ab5d

[step4] refactor : 데이터를 꺼내지 않고 isMovableToLeft() 메서드로 Position 객체에 메세…

4e73f72

…지 보내기

[step4] docs : Bar 객체가 방향 갖도록 하는 것에 대한 질문 추가

7095da7

wotjd243 requested changes Jul 9, 2019

View reviewed changes

Integerous added 4 commits July 9, 2019 22:25

[step4] feat : GameInfo와 Ladder를 가진 LadderGame 클래스 생성

6eecba5

[step4] refactor : OutputView에서 LadderGame 사용

ec65edf

[step4] refactor : LadderGame에서 GameResult 생성하도록 개선

f022beb

[step4] refactor : Bar가 2개의 방향을 가지도록 수정

e01a085

wotjd243 requested changes Jul 11, 2019

View reviewed changes

한정수 and others added 12 commits July 12, 2019 14:49

[step4] refactor : if문 조건을 부정에서 긍정으로 변경

0710847

[step4] feat : Point 인스턴스의 생성조건에 따라 연결 방향을 표현하는 Enum 추가

e816327

[step4] feat : Point 객체 생성

dd61845

[step4] refactor : Line 생성 로직을 bars에서 points로 변경

af8fcc9

[step4] refactor : 중복 제거 (방향 선택 로직)

bf0128d

[step4] refactor : Point 이동 로직을 Direction에 위임, Bar 객체 삭제

e95d7f1

[step4] refactor : Point에서 다음 index 생성을 Position에서 하도록 변경

9c8d496

[step4] refactor : 남아있던 Bar 네이밍들 제거

5991053

[step4] fix : Direction이 무조건 PASS로 결정되는 문제 해결

a9a41cc

[step4] refactor : Ladder가 Players에 의존하지 않도록 수정

d13b90a

[step4] test : 테스트 추가 및 변경

67490e5

[step4] refactor : 가로라인 중복 불가 명시

19296be

wotjd243 requested changes Jul 14, 2019

View reviewed changes

Integerous added 3 commits July 14, 2019 19:16

[step4] refactor : 가로라인 중복 체크 로직 제거

0de28b6

[step4] refactor : 의미 없는 공백 제거

fa0bee8

[step4] refactor : 네이밍 변경 (PointGenerator -> PointIndicator)

075030b

wotjd243 approved these changes Jul 14, 2019

View reviewed changes

wotjd243 merged commit 5b48b38 into next-step:Integerous Jul 14, 2019


		Height height = Height.from(InputView.askHeight());

		Ladder ladder = Ladder.from(players, height);

Conversation

Integerous commented Jul 8, 2019

Uh oh!

wotjd243 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!

wotjd243 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Integerous Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

wotjd243 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!

wotjd243 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Integerous Jul 12, 2019 •

edited

Loading