-
Notifications
You must be signed in to change notification settings - Fork 709
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
base: hvoiunq
Are you sure you want to change the base?
3단계 - 사다리(게임 실행) #2034
Conversation
There was a problem hiding this 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static ArrayList<LineState> generateRandomLine(int countOfParticipant) { | |
private static List<LineState> generateRandomLine(int countOfParticipant) { |
가능하면 구현체보다 인터페이스 사용할 것을 추천
인터페이스로 구현하는 것이 왜 좋은지 고민해 보면 좋겠다.
private final boolean previous; | ||
private final boolean current; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; | ||
} | ||
|
There was a problem hiding this comment.
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())); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LineState에 대한 단위 테스트도 추가해 보면 어떨까?
안녕하세요 :)
사다리 3단계 리뷰 요청드립니다!