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, 3 #1813

Open
wants to merge 3 commits into
base: 201411167
Choose a base branch
from
Open

Step2, 3 #1813

wants to merge 3 commits into from

Conversation

201411167
Copy link

사다리 생성 & 게임실행 관련 PR입니다.

Copy link

@seondongpyo seondongpyo left a comment

Choose a reason for hiding this comment

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

2단계와 3단계를 같이 진행해주셨네요!
다만 하나의 커밋에 너무 많은 변경사항이 포함되어 있는 것으로 보여요 😢
이번 과정의 주요 학습 목표가 TDD인 만큼, TDD 사이클에 따라 기능을 하나씩 구현해가면서
커밋 단위를 작게 가져가는 습관을 들여보시는 걸 추천 드립니다.
몇 가지 코멘트 드렸는데, 확인 후 다시 리뷰 요청을 부탁 드리겠습니다 🔥


public class LadderApplication {

public static void main(String[] args) {

Choose a reason for hiding this comment

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

메서드 라인 수가 길어 보이는데, 자동차 경주 미션 때처럼 메서드의 라인 수를 줄여보면 어떨까요?
이 같은 원칙 아래에서 메소드의 라인 수를 15라인이 넘지 않도록 구현한다.

Comment on lines +35 to +39
if (name.equals("all")) {
OutputView.announceAllUsers(usersAfter, ladderResults);
return;
}
OutputView.announceUser(name, usersAfter, ladderResults);

Choose a reason for hiding this comment

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

해당 로직은 OutputView 내부로 옮겨보면 어떨까요?


@FunctionalInterface
public interface BridgeStrategy {
Boolean bridgeBuild();

Choose a reason for hiding this comment

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

  • wrapper 클래스를 사용하신 이유가 있으실까요?
  • BridgeStrategy라는 이름을 통해 Bridge를 생성하는 전략임을 유추할 수 있기 때문에,
    메서드 이름은 build() 정도로 지어도 충분히 의미를 전달할 수 있을 것 같습니다.

Comment on lines +12 to +21
public static Ladder createLadder(int countOfUsers, int height, BridgeStrategy strategy) {
return new Ladder(countOfUsers, height, strategy);
}

private Ladder(int countOfUsers, int height, BridgeStrategy strategy) {
for (int i = 0; i < height; i++) {
Line line = Line.createLine(countOfUsers, strategy);
lines.add(line);
}
}

Choose a reason for hiding this comment

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

다음 클래스 작성 순서 가이드를 참고해보시면 좋을 것 같습니다.
https://www.oracle.com/java/technologies/javase/codeconventions-fileorganization.html

return strategy.bridgeBuild();
}

private Boolean decide(Optional<Integer> prevIndex, Boolean expected) {

Choose a reason for hiding this comment

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

매개변수의 타입으로 Optional이 사용되고 있는데, 다음 링크를 참고해보시면 좋을 것 같습니다.
https://yeonyeon.tistory.com/224

Comment on lines +11 to +12
private static final Scanner scanner = new Scanner(in);
public static final String ASKING_NAMES = "참여할 사람 이름을 입력하세요. (이름은 쉼표(,)로 구분하세요)";

Choose a reason for hiding this comment

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

접근 제어자 순서는 public부터 private 순으로 작성해보면 좋을 것 같아요.
https://www.oracle.com/java/technologies/javase/codeconventions-fileorganization.html

public static List<String> inputUserNames() {
System.out.println(ASKING_NAMES);
String next = scanner.next();
return Arrays.stream(next.split(","))

Choose a reason for hiding this comment

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

문자 ,은 상수로 선언하여 의미를 부여해보면 어떨까요? 아래 32라인에서도 재활용이 가능할 것 같습니다.


public class OutputView {

public static void display(Users users, Ladder ladder, LadderResults ladderResults) {

Choose a reason for hiding this comment

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

참여자 이름과 실행 결과를 출발 지점과 도착 지점 위치에 맞게 출력하여 가독성을 높여보면 어떨까요?

image

public class OutputView {

public static void display(Users users, Ladder ladder, LadderResults ladderResults) {
System.out.println("사다리 결과\n");

Choose a reason for hiding this comment

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

플랫폼에 독립적인 줄바꿈 문자를 사용해보면 어떨까요?
https://www.baeldung.com/java-string-newline#2-using-platform-independent-line-separators

assertThat(ladder.findLastPosition(3)).isEqualTo(2);
assertThat(ladder.findLastPosition(4)).isEqualTo(4);
}
}

Choose a reason for hiding this comment

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

파일의 끝에는 개행을 추가해보면 어떨까요?
https://velog.io/@doondoony/posix-eol

image

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