Step2 (사다리 생성) : 리뷰 요청드립니다. #413
Merged
Conversation
This file contains 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
ssosso
requested changes
Jun 7, 2020
Comment on lines
5
to
7
| public static final String INVALID_PLAYER_NAME = "게임 참가자의 이름은 빈 문자열이 아닌 1 ~ 5자리의 문자만 가능합니다."; | ||
| public static final String INVALID_PLAYER_COUNTS = "게임 참가자는 1명 이상이어야 합니다."; | ||
| public static final String INVALID_LADDER_HEIGHT = "사다리 높이는 1 이상이어야 합니다."; |
There was a problem hiding this comment.
이보다는 RuntimeException을 상속하여 에러를 정의한 클래스를 만들어서 사용해 보면 어떨까요?
Comment on lines
4
to
5
|
|
||
| private static final int MINIMUM_LADDER_HEIGHT = 1; |
| public class LadderFactory { | ||
|
|
||
| private static final int MINIMUM_LADDER_HEIGHT = 1; | ||
| private final int ladderHeight; |
Comment on lines
8
to
9
| private static final int MINIMUM_PLAYER_COUNTS = 1; | ||
| private final List<Point> points; |
| } | ||
| } | ||
|
|
||
| private static List<Point> drawLineWithStrategy(int playerCounts, |
|
|
||
| public static Line drawLine(int playerCounts) { | ||
| validatePlayerCounts(playerCounts); | ||
| List<Point> points = drawLineWithStrategy(playerCounts, new RandomDrawingLineStrategy()); |
There was a problem hiding this comment.
인터페이스를 생성하신 부분 칭찬드립니다 👍
다만 전략 인터페이스는 조건 + 전략의 조합으로 이루어집니다. 때문에 지금 활용하신 부분은 인터페이스의 활용 정도로 생각해 주세요ㅎㅎ
There was a problem hiding this comment.
인터페이스를 활용해 주셨는데 인터페이스의 확장성은 열려있지 않습니다. 외부에서 어떠한 인터페이스를 사용할지 결정시켜 주어야 하는데 파라미터 혹은 생성자 파라미터로 의존성을 열어 주세요.
이와 같은 원리는 스프링의 DI와 일맥상통하니 참고하시기 바랍니다.
| private static final int LINE_WIDTH_CONSTANT = 1; | ||
| private static final int INDEX_CONSTANT = 1; | ||
| private static final int RANDOM_NUMBER_RANGE = 10; | ||
| private static final Random random = new Random(); |
ssosso
approved these changes
Jun 7, 2020
|
|
||
| private final List<Line> lines; | ||
|
|
||
| private Ladder(List<Line> lines) { |
There was a problem hiding this comment.
생성자에서 DrawingLineStrategy를 받도록 처리해 주세요. 이렇게 처리해야 확장성이 높아집니다.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
안녕하세요. 피드백을 다음과 같이 반영했습니다.
상수와 인스턴스 필드의 개행 관련 & 상수 대문자화 : 2f232ec
커스텀 예외 구현 : df6f938
Line 관련 불필요한 메서드 삭제 및 인터페이스 주입 : cc1bfa7