3주차 - Step3 사다리(게임 실행) 리뷰 요청드립니다. #207

Integerous · 2019-07-06T07:08:51Z

안녕하세요-!
2단계에서 빠른 피드백 주셔서 감사했습니다!
2단계에서 피드백 주신 내용들을 반영하였습니다.(커밋 메세지에 (피드백 반영) 표시)

저는 사다리 게임을 2개의 point가 아니라 1개의 bar를 사용해서 구현했는데요,
(플레이어의 위치와 bar의 위치를 비교해서 플레이어의 위치를 변화시키는 방식)

point대신 bar를 사용하니 사다리에서 이동하는 로직이 이해하기 쉬워졌지만,
수업시간에 배운대로 point 객체에까지 역할을 위임하기 힘들어졌습니다.(저의 경우는 bar 객체)
제가 구현한 방식에서 bar 객체에 역할을 더 위임하려면 어떻게 하면 좋을지 조언주시면 감사하겠습니다-!

그리고 2단계에서 String.format()을 사용해보라고 조언주신 부분을 구현하다가
코드 중복이 발생해서 인터페이스로 빼봤는데 올바르게 구현한건지 궁금합니다..!

그 외에도 문제가 많을 것으로 생각됩니다..!
바쁘신 와중에 좋은 피드백 감사드립니다 👍 👍

…제거해서 두 view를 분리

…가하여 사용

…시적 메서드들을 만들어 위임

wotjd243

피드백을 잘 반영해 주셨습니다. 👍
질문에 대한 답변은 코멘트로 남겼어요.
그 외에도 몇 가지 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
원활한 4단계 진행을 위해 3단계는 빠르게 Merge 할게요.

wotjd243 · 2019-07-08T02:45:18Z

src/main/java/ladder/Application.java

 public class Application {

    public static void main(String[] args) {
        Players players = Players.of(InputView.askPlayers());
+        Prizes prizes = Prizes.from(InputView.askPrizes(), players.numberOfPlayers());


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

wotjd243 · 2019-07-08T02:55:45Z

src/main/java/ladder/domain/Prizes.java

+    }
+
+    private void validationPrizes(List<Prize> prizes, int numberOfPlayers) {
+        if (prizes.size() != numberOfPlayers) {


"참여할 사람과 실행 결과의 수는 같아야 한다."라는 요구 사항을 GameInfo에 표현해 보면 어떨까요?

wotjd243 · 2019-07-08T02:56:08Z

src/main/java/ladder/Application.java


-        OutputView.printResult(players, ladder);
+        if ("all".equals(wantedPlayer)) {


매직 넘버란 코드 안에 작성된 구체적인 숫자 혹은 문자열을 말합니다. 그리고 매직 넘버의 단점은 아래와 같습니다.

의미를 이해하기 어렵다.

복수의 장소에서 사용된다면 변경하기 힘들다.

그런 매직 넘버는 의미를 나타낼 수 있는 상수로 치환하여 코드의 가독성을 높여 보는 것은 어떨까요?

wotjd243 · 2019-07-08T04:27:46Z

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

@@ -0,0 +1,18 @@
+package ladder.domain;
+
+public class Bar {


"Bar는 방향을 표현하는 Direction을 가진다."라는 설계는 어떨까요?

질문이 있습니다..!!

이 부분은 아직 제대로 이해가 안되서 바로 Bar 객체를 뜯어고치기보다
먼저 구현한 방법이 어떤 문제를 가지고 있는지, 개선이 필요한 이유를 여쭙고 싶은데요..!

저의 얕은 시야로는 Bar에 방향을 표현하는 Direction을 가지게 하는 것이
Bar 객체의 성질(?)과는 어울리지 않게 팔 다리를 하나씩 더 붙이는 느낌이 들었습니다..

Bar는 Point 2개를 사용하는 방법과는 조금 다르게 Bar들을 배열로 생각하고
Bar의 index를 플레이어의 index와 비교해서 (플레이어의 위치도 고정된 배열이므로)
같으면 플레이어의 위치+1
Bar의 index가 플레이어의 index보다 1만큼 왼쪽에 있으면 플레이어의 위치-1
이 방법인데요..

Point 2개를 사용하면 사다리를 타서 플레이어의 위치를 변화시킨다는 핵심 로직이
true와 false로 표현되기 때문에 더 복잡하고 명시적이지 않은 것 같아서 Bar를 사용했고,
아래와 같이 구현했었습니다.

Position travel(Position position) { if (bars.get(position.getPosition()).isExist()) { return position.moveToRight(); // 사용자와 같은 위치에 bar가 있으면 사용자 위치 1 증가 } if (position.isMovableToLeft() && bars.get(position.getLeftPosition()).isExist()) { return position.moveToLeft(); // 사용자의 왼쪽에 bar가 있으면 사용자 위치 1 감소 } return position; // 위의 경우가 아니면 위치 유지(패스) }

당연히 문제가 있기 때문에 Bar 객체가 방향을 가지도록 조언을 주셨을 것 같은데요!
위와 같이 구현할 경우에 어떤 문제가 있을지 궁금합니다..!

(수업시간에 강사님께서 Point 2개로 구현하시는 것을 배우긴했지만..
나중에 연습 삼아 다시 구현해도 Point 2개보다는 Bar 1개로 구현하고 싶을 것 같아서
문제점을 알고 제 생각을 바로 잡고 싶습니다..)

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

wotjd243 · 2019-07-08T04:29:37Z

src/main/java/ladder/domain/GameResult.java

+        int numberOfPlayers = players.numberOfPlayers();
+
+        for (int i = 0; i < numberOfPlayers; i++) {
+            Player player = players.getPlayers().get(i);


Players에 메시지를 던지도록 구조를 바꿔 보면 어떨까요? 객체에 메시지를 보내 데이터를 가지는 객체가 일하도록 해보세요.

wotjd243 · 2019-07-08T04:54:56Z

src/main/java/ladder/formatter/NameFormatter.java

+
+public interface NameFormatter<T> {
+    int SPACE_FOR_NAME = 5;
+    String BLANK_TO_FILL_THE_NAME_SPACE = " "; //TODO: 질문입니다. 인터페이스에 이런 상수들을 둬도 되는건지 궁금합니다.


인터페이스는 정의하는 용도로만 사용하는 것이 좋아요. 아래의 글을 참고하세요.
https://leedo1982.github.io/wiki/ITEM22

wotjd243 · 2019-07-08T05:09:48Z

src/main/java/ladder/formatter/PlayersPrintFormatter.java

+    @Override
+    public String dataPrintFormat(Players players) {
+        return players.getPlayers().stream()
+                .map(player -> String.format(playerNameFormatter.nameFormat(player), player.getName()))


아래의 코드로 개선해 보면 어떨까요?

Suggested change

.map(player -> String.format(playerNameFormatter.nameFormat(player), player.getName()))

.map(player -> String.format("%6s", player.getName()))

기초적인 것을 잊고 어렵게 구현하고 있었네요... 조언 감사합니다!! 👍 👍

wotjd243 · 2019-07-08T05:32:52Z

src/main/java/ladder/domain/LineMaker.java

    }

-    private void addBlankBar() {
-        randomBars.add(false);
+    private void generateLastBar() {


메서드 이름은 메서드의 정확한 동작을 구체적으로 나타내는 것이 좋겠죠?

마지막 Bar는 모두 비어있는 Bar라서 generateLastBar()로 이름지었었는데요,
비어있는 바를 생성하는 메서드니까 generateEmptyBar()로 고치면 조언주신 의도에 부합할까요..?

네, 맞습니다. 👍

한정수 and others added 29 commits July 5, 2019 13:56

[step3] refactor : (피드백 반영) @FunctionalInterface 추가

680d774

[step3] refactor : (피드백 반영) BarGeneratorImpl 명칭 변경

e361f56

[step3] feat : (피드백 반영) List<Boolean>의 Boolean을 포장하기 위한 Bar 객체 생성

daa0bf8

[step3] refactor : (피드백 반영) List<Boolean>의 Boolean을 Bar 객체로 포장해서 사용

9a7128f

[step3] refactor : (피드백 반영) OutputView 내부에 InputView에서 import받은 메서드를 …

af9ca67

…제거해서 두 view를 분리

[step3] refactor : (피드백 반영) String.format() 사용

2ef101a

[step3] feat : 지네릭 인터페이스(NameFormatter)와 구현클래스(PlayerNameFormatter) 추…

6047e63

…가하여 사용

[step3] refactor : (피드백 반영) LineMaker 클래스의 generateBars() 메서드의 기능들을 명…

f53137a

…시적 메서드들을 만들어 위임

[step3] refactor : 상수 사용해서 의미 명시

cc0def7

[step3] refactor : (피드백 반영) generateBars() 메서드가 재사용될 수 있도록 수정

d459ab6

[step3] docs : 구현 기능 목록 업데이트

f4cec19

[step3] feat : Prize(상품; 사다리의 결과) 객체 추가

4b01e7c

[step3] feat : Prize(상품) 이름 예외처리 추가

3b0ebe1

[step3] feat : 상품들(Prizes) 객체 추가 및 예외처리

2c6b1ac

[step3] refactor : 예외처리 메서드 분리

ae7057d

[step3] refactor : 생성시에 새로운 컬렉션에 담기 및 예외처리 메서드 분리

8365d36

[step3] feat : 사다리 게임 실행 로직 작성중 (미완성)

a0f0ec7

[step3] feat : 위치 데이터를 가지는 Position 객체 생성

a06e462

[step3] feat : 사다리 이동 로직에 Position 객체 사용 및 테스트 추가

2fece83

[step3] refactor : Position이 0보다 작거나 같은 경우 왼쪽으로 이동 못하는 예외 처리

2176f90

[step3] feat : 실행 결과들(상품들)을 입력받아 Prizes 생성

e0e3115

[step3] feat : PrizeNameFormatter 생성해서 입력받은 상품들 출력

cf6667f

[step3] refactor : formatter들을 패키지 생성해서 분리

0cafc8f

[step3] feat : DataPrintFormatter 인터페이스와 구현체 생성하여 코드 중복 제거

3babbd1

[step3] feat : 사다리 게임 실행 로직과 결과를 GameResult 객체로 분리

86a3441

[step3] feat : 개별 결과 입출력 및 개별 결과 찾는 로직 추가

7586d14

[step3] feat : 플레이어 이름 비교 메서드 추가, 전체 결과 출력 로직 추가

f345856

[step3] docs : 구현 기능 목록 업데이트

828993f

[step3] style : 코드 컨벤션 점검

a3ebeed

wotjd243 approved these changes Jul 8, 2019

View reviewed changes

wotjd243 merged commit bfeff7e into next-step:Integerous Jul 8, 2019

Integerous mentioned this pull request Jul 8, 2019

3주차 - Step4 사다리(리팩토링) 리뷰요청 드립니다..! #221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

3주차 - Step3 사다리(게임 실행) 리뷰 요청드립니다. #207

3주차 - Step3 사다리(게임 실행) 리뷰 요청드립니다. #207

Uh oh!

Integerous commented Jul 6, 2019 •

edited

Loading

Uh oh!

wotjd243 left a comment

Uh oh!

wotjd243 Jul 8, 2019

Uh oh!

wotjd243 Jul 8, 2019

Uh oh!

wotjd243 Jul 8, 2019

Uh oh!

wotjd243 Jul 8, 2019

Uh oh!

Integerous Jul 8, 2019 •

edited

Loading

Uh oh!

wotjd243 Jul 9, 2019

Uh oh!

wotjd243 Jul 8, 2019

Uh oh!

wotjd243 Jul 8, 2019 •

edited

Loading

Uh oh!

wotjd243 Jul 8, 2019

Uh oh!

Integerous Jul 8, 2019

Uh oh!

wotjd243 Jul 8, 2019

Uh oh!

Integerous Jul 8, 2019

Uh oh!

wotjd243 Jul 9, 2019

Uh oh!

Uh oh!


		OutputView.printResult(players, ladder);
		if ("all".equals(wantedPlayer)) {

	.map(player -> String.format(playerNameFormatter.nameFormat(player), player.getName()))
	.map(player -> String.format("%6s", player.getName()))

3주차 - Step3 사다리(게임 실행) 리뷰 요청드립니다. #207

3주차 - Step3 사다리(게임 실행) 리뷰 요청드립니다. #207

Uh oh!

Conversation

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

Uh oh!

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!

Integerous Jul 8, 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!

wotjd243 Jul 8, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Integerous commented Jul 6, 2019 •

edited

Loading

Integerous Jul 8, 2019 •

edited

Loading

wotjd243 Jul 8, 2019 •

edited

Loading