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

3단계 - 사다리(게임 실행) #2034

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

Conversation

hvoiunq
Copy link

@hvoiunq hvoiunq commented Dec 18, 2023

안녕하세요 :)
사다리 3단계 리뷰 요청드립니다!

Copy link
Contributor

@javajigi javajigi left a comment

Choose a reason for hiding this comment

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

전체적인 객체 설계와 구현 잘 했네요. 💯
소소한 피드백 몇 개 남겼어요.
특히 LineState 생성과 관련해 좀 더 개선해 보면 어떨까요?

LineGenerator lineGenerator = new RandomLineGenerator();
ArrayList<Boolean> line = new ArrayList<>();
line.add(false); // 사다리 라인의 맨 왼쪽은 생성될 수 없다.
private static ArrayList<LineState> generateRandomLine(int countOfParticipant) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static ArrayList<LineState> generateRandomLine(int countOfParticipant) {
private static List<LineState> generateRandomLine(int countOfParticipant) {

가능하면 구현체보다 인터페이스 사용할 것을 추천
인터페이스로 구현하는 것이 왜 좋은지 고민해 보면 좋겠다.

Comment on lines +6 to +7
private final boolean previous;
private final boolean current;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return new LineState(previous, lineGenerator.generateLine());
}

public LineState(boolean previous, boolean current) {
Copy link
Contributor

Choose a reason for hiding this comment

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

생성자는 클래스 메서드 앞에 위치하는 것이 관례임

}

public LineState(boolean previous, boolean current) {
checkForConsecutiveTrue(previous, current);
Copy link
Contributor

Choose a reason for hiding this comment

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

extract method를 통해 분리한 private 메서드는 가능하면 바로 아래 위치하도록 구현하는 것을 추천

this.previous = previous;
this.current = current;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

next LineState를 생성하기 위해 다음과 같이 구현하는 것은 어떨까?

    public LineState next() {
        return new LineState(current, lineGenerator.generateLine())
    }


assertThrows(IllegalArgumentException.class, () -> new ResultInfo("a", participants.count()));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

LineState에 대한 단위 테스트도 추가해 보면 어떨까?

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