Skip to content

[2단계 - 사다리 게임 실행] 오잉(이하늘) 미션 제출합니다.#213

Merged
choijy1705 merged 28 commits intowoowacourse:hanueleeefrom
hanueleee:step2
Feb 27, 2023
Merged

[2단계 - 사다리 게임 실행] 오잉(이하늘) 미션 제출합니다.#213
choijy1705 merged 28 commits intowoowacourse:hanueleeefrom
hanueleee:step2

Conversation

@hanueleee
Copy link

안녕하세요 영이!
2단계 미션 구현 완료했습니다. 잘 부탁드립니다!

BooleanGenerator를 LadderGame에서 생성하여 Line까지 계속 넣어줬던 이유

  1. 게임 규칙에 따라 사다리 생성 방식이 달라질 수 있다(ex. true/false 확률 설정 등)고 생각해 BooleanGenerator 인터페이스를 구현했습니다.
    또한 어떠한 BooleanGenerator을 쓰느냐를 결정하는 책임을 가지는 것은 LadderGame이라고 생각해 LadderGame의 인스턴스 변수로 BooleanGenerator를 위치시켰습니다.
    LadderGame에서 BooleanGenerator를 결정해 Ladder에게 사다리를 생성하도록 요청 -> Ladder는 전달 받은 BooleanGenerator를 사용해 각 Line에게 다리를 생성하도록 요청하는 방식으로 구현했습니다.
  2. 랜덤값을 사용한 프로그램이기 때문에 원하는 값으로 테스트 하기 위해 가장 바깥 쪽 도메인에 BooleanGenerator를 위치했습니다.

궁금한 점

하지만 영이님 말씀을 듣고보니 BooleanGenerator가 실제로 사용되는 곳은 Line인데, LadderGame에서 생성하여 Line까지 밀어주는 제 로직이 조금 이상하다는 생각이 들었습니다..! 그래서 Line에서 바로 BooleanGenerator를 사용해볼까 하였으나, 랜덤값이라 테스트하기가 어려웠습니다🥲
#69 (comment)
RandomGenerator 만으로 랜덤 boolean값을 return받고 그 값을 통해 bridge를 생성하는 로직을 line 클래스에서 처리한다면 좀 더 테스트가 쉽지않을까요?라는 리뷰를 반영하고자 했는데,
랜덤값을 어떻게 테스트해야할지를 모르겠습니다..! 영이님이라면 이러한 상황에서 어떻게 테스트를 하실건가요??

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 오잉!
2단계 잘 구현해주셨네요
질문 해주신부분에 대한 답변 남겼습니다!
요거 피드백 반영하면 테스트 코드가 크게 변할거 같아서 테스트 코드에 대해서는 크게 리뷰를 안남겼습니다

private Height requestLadderHeight() {
try {
return new Height(validateNumber(inputView.requestLadderHeight()));
return new Height(validateIntegerNumber(inputView.requestHeight()));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

controller에서 validate하는것 보다는 Height에서 String을 생성자 파라미터로 받고 검증후 int를 값을 저장해주면 어떨까요?

Comment on lines +67 to +69
String inputPrizes = inputView.requestPrize();
List<String> prizes = Arrays.stream(inputPrizes.split(DELIMITER))
.collect(Collectors.toList());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputView에서 List 으로 만들어서 반환할수도 있겠네요


private void showResult() {
String inputName = inputView.requestShowingName();
if (inputName.equals("all")) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (inputName.equals("all")) {
if ("all".equals(inputName)) {

이렇게 작성해주는게 좋아요!

public boolean isExist() {
return this == EXIST;
}
} No newline at end of file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개행 해주세요!

Comment on lines +16 to +20
if (height < MIN_HEIGHT) {
throw new IllegalArgumentException(ErrorCode.NUMBER_UNDER_2.getMessage());
}
if (height > MAX_HEIGHT) {
throw new IllegalArgumentException(ErrorCode.NUMBER_OVER_100.getMessage());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러메세지를 2~100까지 입력하라하고 if문을 통합하면 더 깔끔할거 같아요
제 생각입니다! 굳이 안바꾸셔도 될거 같아요

Comment on lines +33 to +38
public Bridge generateRandomBridge(BooleanGenerator booleanGenerator) {
if (booleanGenerator.generate()) {
return Bridge.EXIST;
}
return Bridge.EMPTY;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RandomBooleanGenerator 밖에 쓰이는 클래스가 없는데
일어나지 않을일을 대비하여 BooleanGenerator를 만든것 같아요!
다른 요구사항이 생겨 RandomIntegerGenerator가 필요하면 지금 만든 인터페이스는 아무것도 쓰이지 않고 지워져버릴거같아요
다른 요구사항이 있을때 그때 공통적인 부분이 있다면 인터페이스를 만드는게 적절한 시점이라고 생각합니다 저는!

우선 인터페이스를 제거하고 generate() 메서드를 static 메서드로 변경하고generateRandomBridge(RandomBooleanGenerator.generate()) 처럼 사용해보도록 변경해보는건 어떨까요?

Suggested change
public Bridge generateRandomBridge(BooleanGenerator booleanGenerator) {
if (booleanGenerator.generate()) {
return Bridge.EXIST;
}
return Bridge.EMPTY;
}
public Bridge generateRandomBridge(boolean randomBoolean) {
if (randomBoolean) {
return Bridge.EXIST;
}
return Bridge.EMPTY;
}

테스트가 어려운건 LadderTest에서의 어려움일까요??
랜덤한 값으로 line이 생성되는것을 LineTest에서 진행했다면
LadderTest에서 굳이 TestGenerator까지 만들어서 테스트를 하는건 비효율적이라고 생각해요!
비효율적일뿐만 아니라 실제 구현에서 사용하지 않는 Generator로 테스트 하는건 큰 의미도 없는것 같아요
LineTest에서 true면 Bridge.Exist가 반환, false면 empty가 반환되는지만 확인하면 충분하지 않을까요?
가장 작은 단위의 테스트가 성공한다면 그 이후 상위 레벨에서는 흐름만 체크하는 테스트가 되면 될것 같습니다!

Copy link
Author

@hanueleee hanueleee Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 요구사항이 새로 생겼을 때 공통점에 맞게 확장하면 되기 때문에 현재 요구사항에서는 인터페이스가 필요없다.
  2. 실제 구현에서 사용하지 않는 Generator로 테스트하는건 큰 의미가 없다.

라고 이해하면 될까요??

Copy link
Author

@hanueleee hanueleee Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestGenerator를 만들었던 이유는
이번 미션 핵심인 사다리 타기를 Ladder의 findFinalPosition이 담당하고 있는데
사다리가 랜덤으로 생성되기 때문에, 사다리를 제대로 타는지(findFinalPosition이 제대로 작동하는지)를 확인하기 위해서
테스트코드에서는 원하는대로 사다리를 생성해야한다고 생각했습니다!
(핵심로직이기 때문에 실제 구현에서 사용하지 않는 Generator로라도 랜덤값을 제어하여 테스트를 해야한다고 생각했습니다)

말씀하신 것처럼 true면 Bridge.Exist가 반환, false면 empty가 반환되는지만 확인한다면,
핵심로직에 대한 테스트는 어떻게 진행할 수 있나요??

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DisplayName("사다리 게임")
    class climbLadder {
        @Test
        @DisplayName("사다리를 타고난 후의 위치값을 제대로 반환하는지 테스트")
        void climbTest() {
               /**
             *  0     1     2
             *  |-----|     |
             *  |     |-----|
             *  0     1     2
             */
            
            
            Line line = new Line(List.of(Bridge.EXIST, Bridge.EMPTY), 2);
            Line line2 = new Line(List.of(Bridge.EMPTY, Bridge.EXIST), 2);
            Ladder ladder = new Ladder(List.of(line, line2));


            //then
            assertThat(ladder.findFinalPosition(0)).isEqualTo(2);
            assertThat(ladder.findFinalPosition(1)).isEqualTo(0);
            assertThat(ladder.findFinalPosition(2)).isEqualTo(1);
        }
    }

말로 설명하기가 너무 어려워서 수도코드로 예시를 보여드립니다 참고만해주세요!
정적팩토리 메서드를 사용할 경우는 분명히 적절한 시점이 있을겁니다
제 기준에서는 이번 미션에서는 정적 팩토리 메서드가 오히려 깔끔한 코드를 작성하는데 방해가 되는것 같아요

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line의 lineSize 필드도 굳이 필요없을것 같아요
birdges.size()를 활용해도 되지 않을까요?

public int getPosition() {
return position;
}
} No newline at end of file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개행해주세요!

public List<String> getAllPersonNames() {
private static Persons initializePerson(List<String> names) {
List<Person> persons = names.stream()
.map((name) -> new Person(name, names.indexOf(name)))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.map((name) -> new Person(name, names.indexOf(name)))
.map(name -> new Person(name, names.indexOf(name)))

Comment on lines 26 to 29
return names.size() != names.stream()
.map(Person::getName)
.distinct()
.count();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set.of(names).size()
를 활용해볼수도 있겠네요

Comment on lines +61 to +66
for (Person person : persons) {
if (position == person.getPosition()) {
return person.getName();
}
}
return null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stream으로 해보면 어떨까요?

@hanueleee
Copy link
Author

hanueleee commented Feb 24, 2023

안녕하세요 영이! 리뷰를 반영하여 리팩토링 해보았습니다!

궁금한 점

  1. 핵심로직 테스트(랜덤값 제어) 관련 질문
    [2단계 - 사다리 게임 실행] 오잉(이하늘) 미션 제출합니다. #213 (comment)

  2. controller

저는 개인적으로 controller는 view와 domain의 연결다리 역할만 하는게 좋다고 생각해서,
하단 예시처럼 inputView에서 받아온 입력값들을 그대로 LadderGame에 넘기고 LadderGame이 알아서 필요한 객체들을 생성하고 동작했으면 합니다.

   public void play() {
        List<Person> persons = requestPlayerName();
        List<String> prizes = requestLadderPrize(persons.getTotalPersonCount());
        int height = requestLadderHeight();

        ladderGame = new LadderGame(persons, height);
        ...
    }

하지만, 현재 제 코드는 도메인 관련 검증(이름은 중복이 안된다, 높이는 2이상이어야 한다 등)이 도메인 생성시 이루어지고 있기 때문에
잘못된 입력값이 들어왔을 경우 재입력을 받기위해 어쩔 수 없이 controller내부에서 도메인(Persons, Height)을 생성하여 LadderGame에 넘겨주게 되었습니다.

    public void play() {
        Persons persons = requestPlayerName();
        List<String> prizes = requestLadderPrize(persons.getTotalPersonCount());
        Height height = requestLadderHeight();

        ladderGame = new LadderGame(persons, height);
        ...
    }

혹시 이 상황을 해결할 수 있는 방법이 있을까요??

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 반영 빠르게 해주셨네요!
몇가지 질문에 대한 답변 남겼습니다
말로 설명하기가 너무 어려워서 코드예시를 남겼어요
이해가 안가는 부분있다면 코멘트 혹은 DM 주세요


public class LadderGameController {
private static final String NAME_DELIMITER = ",";
private static final String GAME_END_COMMAND = "all";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines 23 to 27
Persons persons = requestPlayerName();
List<String> prizes = requestLadderPrize(persons.getTotalPersonCount());
Height height = requestLadderHeight();

ladderGame = new LadderGame(persons, height);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LadderGame 에서 raw 한 string, int 값을 받아서 Person, Height 객체를 생성하는 방법이 제 기준에는 오히려 더 이상한것 같아요!
domain과 view의 연결다리 역할은 controller의 역할중 하나인거지
이것만 해야해 라는 규칙같은건 없을것 같아요!
좀더 가독성이 좋은 방법이 뭘지 생각해보면 좋을것 같아요

그리고 inputView에서도 필요하다면 어느정도의 검증로직은 있으면 좋을것 같아요.
예를들어 숫자만 입력받아야 하는 경우에 대한 것은
domain에서 검증하기보다는 inputView에서 확인하고 넘어오는게 더 적절한것 같다고 생각이 드네요!

Comment on lines +14 to +16
public static Height from(String height) {
return new Height(validate(height));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

검증말고는 별다른 로직이 없는것 같아서
생성자로만 처리해도 되지 않을까요?
불필요한 코드만 몇줄 추가되는 느낌이라서요!

앞서 코멘트 남겼던대로 입력값이 정수인지 검증하여 int를 return 받는 정도는 inputView에서 처리하면 더 깔끔해질것 같아요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성자는 생성하는 기능만 해야한다라는 말을 어디선가 들은적이 있는 것 같아서,
검증 로직을 정적 팩토리 메소드안에서 처리한 후 생성하고자 했습니다!

Copy link
Author

@hanueleee hanueleee Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#69 (comment)
#213 (comment)
이 두 리뷰를 보고 최종적으로
Height 생성자에 String을 통째로 인자로 넘겨서 검증하는 방식으로 이해했었는데,
정수가 아닌 경우(숫자가 아닌 문자, int범위가 넘어가는 값 등)에 대한 검증을 inputView, 도메인 관련 검증(높이는 2이상이어야 한다)에 대한 검증은 Height에서 하는게 좋겠다는 말씀이실까요??

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 최종적으로는

  • inputView에서는 int값이 제대로 들어오는지 검증
  • Height에서는 int값으로 생성하고 적절한 범위에 들어가는지 검증
    이렇게 두가지가 되겠네요
    Height 생성자에 String을 통째로 인자로 넘겨서 검증하는 방식 요 리뷰는 Controller에서 검증로직이 있어서 달았던 리뷰라고 생각하면 될것 같아요

Comment on lines +33 to +38
public Bridge generateRandomBridge(BooleanGenerator booleanGenerator) {
if (booleanGenerator.generate()) {
return Bridge.EXIST;
}
return Bridge.EMPTY;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DisplayName("사다리 게임")
    class climbLadder {
        @Test
        @DisplayName("사다리를 타고난 후의 위치값을 제대로 반환하는지 테스트")
        void climbTest() {
               /**
             *  0     1     2
             *  |-----|     |
             *  |     |-----|
             *  0     1     2
             */
            
            
            Line line = new Line(List.of(Bridge.EXIST, Bridge.EMPTY), 2);
            Line line2 = new Line(List.of(Bridge.EMPTY, Bridge.EXIST), 2);
            Ladder ladder = new Ladder(List.of(line, line2));


            //then
            assertThat(ladder.findFinalPosition(0)).isEqualTo(2);
            assertThat(ladder.findFinalPosition(1)).isEqualTo(0);
            assertThat(ladder.findFinalPosition(2)).isEqualTo(1);
        }
    }

말로 설명하기가 너무 어려워서 수도코드로 예시를 보여드립니다 참고만해주세요!
정적팩토리 메서드를 사용할 경우는 분명히 적절한 시점이 있을겁니다
제 기준에서는 이번 미션에서는 정적 팩토리 메서드가 오히려 깔끔한 코드를 작성하는데 방해가 되는것 같아요

Comment on lines +60 to +62
return persons.stream()
.filter(person -> person.getPosition() == position)
.findAny().get().getName();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return persons.stream()
.filter(person -> person.getPosition() == position)
.findAny().get().getName();
return persons.stream()
.filter(person -> person.getPosition() == position)
.map(Person::getName)
.findAny()
.orElse(null);

Optional로 반환되는경우 get()을 사용하기보다는 orElse(null) 혹은 orElseThrow() 사용해주는게 좋을것 같아요

public class LadderGame {
private final Persons persons;
private final Ladder ladder;
private Map<String, String> mappingResult = new HashMap<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

성능상의 이점이 있거나 하지 않을것 같아요 어차피 생성시점에 초기화가 될거라
가독성에서는 별로 좋아보이지 않아서요
다른 필드들은 생성자에서 초기화하고 해당 필드만 필드에서 초기화를해서요
보통 필드에서 초기화하는 경우는 static 변수일때 많이 사용하는데 다른 사람들이 보기에는 혼동을 줄 수도 있을것 같네요!

Copy link

@choijy1705 choijy1705 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2단계까지 진행한다고 고생하셨습니다👍
도메인을 딱 필요한 클래스만 생성해주셔서 코드를 이해하기 편했어요!
조금 더 개선해볼 만한 몇가지 코멘트 남겼는데 확인부탁드립니다!
이번단계는 여기서 merge하겠습니다

Comment on lines +14 to +16
public static Height from(String height) {
return new Height(validate(height));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 최종적으로는

  • inputView에서는 int값이 제대로 들어오는지 검증
  • Height에서는 int값으로 생성하고 적절한 범위에 들어가는지 검증
    이렇게 두가지가 되겠네요
    Height 생성자에 String을 통째로 인자로 넘겨서 검증하는 방식 요 리뷰는 Controller에서 검증로직이 있어서 달았던 리뷰라고 생각하면 될것 같아요

Line line = Line.from(5);

//when
line.generate(new TrueGenerator());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앞서 리뷰드렸지만 테스트를 위한 Generator를 만들어서 테스트 코드를 작성한다면
이 코드를 백프로 신뢰할순 없을것 같습니다!

Bridge가 Line보다 더 작은 단위인데 그 Bridge에서 원하는데로 잘 테스트 된다면
Line에서는 line의 size정도만 체킹해보면 되지 않을까 생각됩니다

Comment on lines +7 to 12
public static Bridge from(boolean status) {
if (status) {
return EXIST;
}
return EMPTY;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이코드는 전혀 활용이 안되고 있어요!
얘를 활용한다면 좀더 Line의 generate 코드를 명확하게 표현할수 있을것 같아요

Comment on lines +37 to +42
private Bridge generateRandomBridge(BooleanGenerator booleanGenerator) {
if (booleanGenerator.generate()) {
return Bridge.EXIST;
}
return Bridge.EMPTY;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bridge에서 review했는데
Bridge의 from을 활요하면 좋을것 같습니다

Comment on lines +18 to 20
List<Bridge> bridges = IntStream.range(0, personCount - 1)
.mapToObj(it -> Bridge.EMPTY)
.collect(Collectors.toList());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어차피 왼쪽에서 bridge를 생성하기 때문에 얘도 굳이 Empty로 미리 초기화할 필요는 없겠네요
왼쪽부터 순차적으로 생성하고 hasBridgeInLeft 만 체크해준다면
Line클래스를 좀더 경량화할수 있겠네요

}

return hasBridgeInLeft(bridgeIndex - 1) || hasBridgeInRight(bridgeIndex + 1);
return hasBridgeInLeft(bridgeIndex) || hasBridgeInRight(bridgeIndex);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for 문 index를 통해 bridge를 만들면 오른쪽은 굳이 체크하지 않아도 안되나요?

@choijy1705 choijy1705 merged commit 1984eb6 into woowacourse:hanueleee Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants