Open
Conversation
javajigi
requested changes
Dec 19, 2023
Contributor
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
전체적인 객체 설계와 구현 잘 했네요. 💯
소소한 피드백 몇 개 남겼어요.
특히 LineState 생성과 관련해 좀 더 개선해 보면 어떨까요?
| LineGenerator lineGenerator = new RandomLineGenerator(); | ||
| ArrayList<Boolean> line = new ArrayList<>(); | ||
| line.add(false); // 사다리 라인의 맨 왼쪽은 생성될 수 없다. | ||
| private static ArrayList<LineState> generateRandomLine(int countOfParticipant) { |
Contributor
There was a problem hiding this comment.
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; |
| return new LineState(previous, lineGenerator.generateLine()); | ||
| } | ||
|
|
||
| public LineState(boolean previous, boolean current) { |
Contributor
There was a problem hiding this comment.
생성자는 클래스 메서드 앞에 위치하는 것이 관례임
| } | ||
|
|
||
| public LineState(boolean previous, boolean current) { | ||
| checkForConsecutiveTrue(previous, current); |
Contributor
There was a problem hiding this comment.
extract method를 통해 분리한 private 메서드는 가능하면 바로 아래 위치하도록 구현하는 것을 추천
| this.previous = previous; | ||
| this.current = current; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
next LineState를 생성하기 위해 다음과 같이 구현하는 것은 어떨까?
public LineState next() {
return new LineState(current, lineGenerator.generateLine())
}
|
|
||
| assertThrows(IllegalArgumentException.class, () -> new ResultInfo("a", participants.count())); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
LineState에 대한 단위 테스트도 추가해 보면 어떨까?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
안녕하세요 :)
사다리 3단계 리뷰 요청드립니다!