Skip to content

Eunjeong#3

Open
eunjeongS2 wants to merge 14 commits intoOOP-study-media:eunjeonsS2from
eunjeongS2:eunjeonsS2
Open

Eunjeong#3
eunjeongS2 wants to merge 14 commits intoOOP-study-media:eunjeonsS2from
eunjeongS2:eunjeonsS2

Conversation

@eunjeongS2
Copy link

No description provided.

@ghost ghost changed the base branch from master to eunjeonsS2 May 28, 2020 13:56
List<String> resultNames = DataProvider.getResultNames(names, resultName);
List<Integer> nameNumbers = DataProvider.getResultNameNumbers(names, resultNames);
List<Integer> matchNumbers = DataProvider.getMatchNumbers(ladder, nameNumbers);
List<String> matchItems = DataProvider.getMatchItems(items, matchNumbers);
Copy link

Choose a reason for hiding this comment

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

DataProvider는 사다리 타기를 진행하고 게임결과를 생성하는 역할을 하고 있네요
컨트롤러에서 DataProvider의 각 메서드를 호출하기보다는 사다리 타기를 진행하는 객체에게 역할을 위임하면 어떨까요

}

private static int move(Line line, int current) {
if (current == 0 && line.connected(current)) {
Copy link

Choose a reason for hiding this comment

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

current == 0 을 메서드로 추출하면 더 의미를 이해하는데 도움이 될 것 같네요

line.isFirstLocation(current)

expectedItems.add("꽝4");
expectedItems.add("꽝1");
expectedItems.add("꽝3");
assertThat(matchItems).isEqualTo(expectedItems);
Copy link

Choose a reason for hiding this comment

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

List<String> expectedItems = new ArrayList<>();
        expectedItems.add("꽝2");
        expectedItems.add("꽝4");
        expectedItems.add("꽝1");
        expectedItems.add("꽝3");

이 부분도 given에 있으면 좋을것같아요


assertThat(firstConnected).isTrue();
assertThat(secondConnected).isFalse();
assertThat(thirdConnected).isTrue();
Copy link

Choose a reason for hiding this comment

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

비교할 Ladder를 만들어서 검증하도록 해봐요:)

List<Line> testLadder = ... ;

assertThat(ladder).isEqualTo(testLadder);

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.

1 participant