유태식 - 사다리 게임 #4
Open
유태식 - 사다리 게임 #4
Conversation
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
- InputScanner, LadderGameInput 클래스 추가 - LadderGameInput.getPlayerNamesFromInputScanner()에서 사람 이름을 쉼표 기준으로 분류
- InputValidator 클래스 추가 - validatePlayerNames() 메소드를 통해 이름이 5글자 이내인지 확인한 후 5자 초과일시 IllegalArgumentException을 throw함
- Ladder, Line, Point 클래스 추가, Point는 Line 객체를 구성하고 Line은 Ladder를 구성하게 됨 - LadderGenarator, LinesGenerator, PointsGenerator 클래스 추가
kwangilcho
requested review from
wbluke
and removed request for
Deocksoo,
kwangilcho and
wbluke
- LadderGameController안의 run()에서 LadderGameService의 메서드들을 호출 - LadderHeight는 새롭게 객체를 만들었지만, Players의 경우 기존의 LadderGamePlayers의 이름만 바꿈
- LadderGameService에서 Players를 생성 - Players는 Player 객체를 담고 있는 일급 객체 - Players는 중복된 참여자가 있는지 검사 - Player는 참여자의 이름이 5자 이내인지 검사
- LadderHeight에서 사다리 높이가 정수 1 이상인지 검사 - 그 밖에 Domain 패키지 안의 클래스들의 구조를 정리
- 팩토리 메서드 with()가 Ladder 객체를 생성 - 기존의 LadderGenerator 클래스는 삭제
- LadderLine에서 Point를 생성 - Point는 좌우의 다른 Point들과 연결되어있는지의 정보를 가진 enum객체 - 그 밖의 불필요한 테스트 파일 삭제
- Maunal과 Random Strategy가 해당 인터페이스를 implement - Point가 생성될 때 Strategy의 isConnectable()에서 boolean값을 반환받음
- LadderLine과 Point 객체 수정 - Point 객체에서 현재 Point의 connection에 따라 다음 Point가 가질 수 있는 connection을 지정
- 테스트 통과를 위해 Point의 generateLastPoint 메서드의 버그 수정 - 이어서 PointTest도 수정
- '참여자 이름 입력'에 '이름이 공백일 수 없음' 추가
- OutputView에서 사용하기 위함
- OutputView에서의 사용을 위해 일부 접근자 수정
- OutputView 객체 추가 - LadderGame과 LadderGameController에서 OutputView를 사용하도록 수정 - Players에 OutputView에 사용할 stream 메서드 추가
- build.gradle에 StringUtils 디펜던시 추가
- Outputview의 printResult 메서드 내의 추상화 수준을 맞추기 위해 printResultMessage 메서드로 코드를 분리 - Player의 이름을 출력을 위해 align하는 로직을 Players로 추출 - 추출한 메서드인 getAlignedPlayerNames 내의 로직을 정리하여 3개의 메서드로 쪼갬
- LineToString과 LineToConnection을 각각 LadderLine과 Point로 이동시킴 - 사다리를 표현하는 String들을 LadderLine의 static 변수로 추출
- 사다리게임 결과의 입출력 기능들을 목록에 추가
- InputView에 getGameResultFromConsole 메서드 추가 - controller에서 InputView가 받아온 문자열을 result 변수에 저장
- LadderGameService에 createResults 메서드가 Results 객체를 반환 - LadderGameService의 parsePlayerNames 메서드를 createResults에서도 재사용가능하도록 parse로 변경하고 이에 맞게 리팩토링 - createPlayers를 리팩토링하던 중 createPlayerList 메서드를 Players로 이주시킴
- Results의 createResultList에서 Result 객체를 생성 - Result의 생성자에서 name의 길이를 검사
- Results의 생성자에서 입력받은 객체의 수가 같은지를 체크
wbluke
requested changes
Jan 31, 2020
wbluke
left a comment
wbluke
left a comment
There was a problem hiding this comment.
안녕하세요 태식 선생님! 구현하신 코드 잘 봤습니다ㅎㅎ
일급컬렉션, 전략패턴, 테스트 등등 배우고 계신 것들 적용하려고 노력하신 부분들이 보이네요 👍
자동차게임에서 남겼던 리뷰와 동일한 부분들은 언급하지 않았습니다. 이전 리뷰 참고하셔서 적용해보시길 바랍니다.
더불어 LadderHeight, Player, Players 등의 클래스에 대해 테스트가 없는데요,
정상적인 상황, 예외적인 상황 둘 다 고려하셔서 한번 테스트 추가해 보시길 바랍니당ㅎㅎ
Comment on lines
+66
to
+71
| String alignedPlayerNames =""; | ||
|
|
||
| for (String playerName : playerNames) { | ||
| alignedPlayerNames += playerName; | ||
| } | ||
| return alignedPlayerNames; |
Comment on lines
+24
to
+27
| if (point.hasRightConnection()) { | ||
| return generatePoint(true, false); | ||
| } | ||
| return generatePoint(false, false); |
There was a problem hiding this comment.
return point.hasRightConnection() ? LEFT : NONE;요렇게 하면 훨씬 깔끔하지 않을까요?ㅎㅎ
다른 메소드에도 generatePoint() 를 거치지 않고도 간단히 할 수 있는 부분들이 있네요!
Comment on lines
+5
to
+9
| public enum Point { | ||
|
|
||
| LEFT(true, false), | ||
| RIGHT(false, true), | ||
| NONE(false, false); |
Comment on lines
+59
to
+68
| public String lineToString() { | ||
| List<Point> points = getPoints(); | ||
|
|
||
| String result = " "; | ||
| for (Point point : points) { | ||
| String connectionString = point.connectionToString(); | ||
| result += (POINT + connectionString); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
저도 옛날에 비슷하게 구현했던 적이 있었는데요,
한번 더 생각해보면 사다리의 출력(Output)이 어떻게 보일 것인지는 사다리의 책임이 아니라는 생각을 해볼 수 있습니다.
지금 코드를 기반으로 콘솔출력이 아니라 웹에다 표현하려고 하면 어떻게 될까요? :)
|
|
||
| public static LadderLine with(final int width, LadderCreationStrategy strategy) { | ||
| List<Point> points = new ArrayList<>(); | ||
| Point previousPoint = null; |
There was a problem hiding this comment.
저번 모임때도 전체적으로 이야기가 잠깐 나왔던거같은데, null 초기화는 굉장히 위험합니다.
NPE(NullPointerException)가 나올 수 있는 가능성은 원천 배제하는 것이 좋습니다!
정말 필요한 상황이라면 어쩔 수 없지만, 정말 필요한 상황이 정말 있을지는 의문이네요
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
현재 2단계 진행 중입니다.