Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Step2 #1830

Open
wants to merge 8 commits into
base: devcat36
Choose a base branch
from
Open

Step2 #1830

wants to merge 8 commits into from

Conversation

devcat36
Copy link

리뷰 부탁드립니다.

Copy link

@sah3122 sah3122 left a comment

Choose a reason for hiding this comment

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

개인적인 사정으로 인해 리뷰가 늦어져서 죄송합니다 😢
사다리 그리기 미션 구현 잘해주셨네요 👍
몇가지 고민거리를 남겨두었는데 확인 부탁드려요 🙏

@@ -0,0 +1,65 @@
package nextstep.ladder;
Copy link

Choose a reason for hiding this comment

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

도메인과 ui 의 패키지를 분리해보는것이 어떨까요 ?

}

private void placeLegWithStrategy(GenerationStrategy strategy, Position position) {
if (isLegPlaceableOnPosition(position) && strategy.shouldPlace()) {
Copy link

Choose a reason for hiding this comment

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

strategy는 매번 random boolean 값을 리턴하고, 현재 위치 기준으로 왼쪽 사다리 연결 선이 존재하는지 검증 👍

import java.util.stream.LongStream;

public class Legs {
private final Set<Position> legLeftPositions;
Copy link

Choose a reason for hiding this comment

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

Legs일급 컬렉션이 사다리 연결선의 정보를 가지는 Position을 가지는것 보다 Leg라는 클래스를 도출하여 사다리 가로 한줄과 연결선에 대한 정보를 가지는 Position의 구현은 어떻게 생각하시나요 ?
Leg 객체에 사다리를 그릴수 있는지에 대한 메시지를 던져보는 구현도 좋은 구현이 될수 있을것 같습니다 😄

@@ -0,0 +1,20 @@
package nextstep.ladder;

public class NaturalInput {
Copy link

Choose a reason for hiding this comment

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

NaturalInput을 활용하여 Natural객체를 생성하는것보단 Natural에 다양한 생성자를 정의해보는건 어떨까요 ?


import java.util.Arrays;

public class UsersInput {
Copy link

Choose a reason for hiding this comment

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

NatualInput 코멘트와 동일합니다 😄

import java.util.Iterator;
import java.util.List;

public class Users implements Iterable<String> {
Copy link

Choose a reason for hiding this comment

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

사다리 게임에 참여하는 유저가 1명 이하인경우 정상적인 게임이 동작할수 있을까요 ?

import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;

public class LadderTest {
@Test
Copy link

Choose a reason for hiding this comment

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

@DisplayName 을 활용하여 테스트의 의도를 명확하게 나타내보는건 어떠세요 ?


import java.util.Scanner;

public class LadderView {
Copy link

Choose a reason for hiding this comment

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

Input / Output 클래스를 분리하는것도 좋아보입니다 😄

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.

3 participants