[사다리 - 1단계] 김성환 미션 제출합니다. #8
Conversation
There was a problem hiding this comment.
성환님 먼저 사다리 1단계가 조금 어려웠을 수도 있는데 객체를 잘 분리해서 구현주셨네요👍👍
수정했으면 하는 부분에 리뷰 남겼으니 확인 부탁드립니다.
1차적으로 요구사항대로 구현을 했는데, 사다리에서 23번 열 사이에 연결된 곳이 없어서 12번/34번 이렇게 서로 독립되는 케이스가 발생합니다. 해당사항이 요구조건에 존재하지않는데 올바른 방향인지 조금 고민됩니다!
해당 부분은 랜덤으로 출력되다보니 서로 독립되어 나타나는 경우도 발생할 수 있습니다!
여기는 잘 나타나고 있습니다!
전반적으로 구현하신 코드에 대해서 테스트도 추가했으면 좋겠습니다.
이번 단계에서 핵심으로 테스트를 해야할 점은 사다리 내부에 연결된 Point가 모두 연결되어있지 않아야 하는 것이 될 것 같아요
| public class LadderService { | ||
| public Ladder generateLadder(int height, int width){ | ||
| return new Ladder(height, width); | ||
| } |
There was a problem hiding this comment.
현재 service의 메소드를 여러 객체가 협업을 하기보다는 단순하게 객체 생성을 하는 역할만 하고 있는 것 같습니다.
이와 같은 상황이라면 현재 상황에서는 굳이 service가 필요 없을 것 같습니다! 오히려 불필요한 계층만 생기는 것 같습니다!
| public Ladder createLadder() { | ||
| int height = 4; | ||
| int width = 4; | ||
| return ladderService.generateLadder(height, width); |
There was a problem hiding this comment.
앞선 리뷰에 이어서 하자면 여기서 바로 new Ladder(height, width)를 해주시면 될 것 같습니다!
| public class Point { | ||
| private boolean isConnected; | ||
| private final BooleanGenerator booleanGenerator; |
There was a problem hiding this comment.
이번에 Point 객체는 사다리의 연결 여부를 나타내는 객체인 것 같아요!
자동차 미션과 같이 Point의 상태의 변화를 줄 수 있는 전략(BooleanGenerator)을 필드로 두셨네요!
저는 이번 미션에서 Point는 자동차 객체와는 조금 성격이 다르다고 생각하는데요.
그 이유는 자동차 미션의 경우 레이스를 거치면서 자동차의 위치 값이 변경되어야 하는 상황이 발생하지만 이번 미션의 Point의 경우는 처음 생성되고 나서 부터는 상태가 변경될 필요가 없는 것으로 보입니다.
그래서 Point 객체에서는 굳이 BooleanGenrator를 필드로 둘 필요가 없이 isConnected만 가지면 될 것 같습니다!
혹시 다른 의견이 있으시면 남겨주세요!
| import java.util.List; | ||
|
|
||
| public class Line { | ||
| private final Size lineSize; |
There was a problem hiding this comment.
현재 해당 필드는 활용되는 곳이 없는데 필드로 가지고 있는 이유가 있으신가요?
| this.points = generatePoints(lineSize); | ||
| } | ||
|
|
||
| public List<Point> generatePoints(int lineSize) { |
There was a problem hiding this comment.
현재 이 메소드는 외부에서 이용하는 것이 아니기에 접근제어자를 private로 수정해주시면 좋을 것 같습니다!
| return size; | ||
| } | ||
|
|
||
| public void validateSize(int size) { |
There was a problem hiding this comment.
이 메소드도 외부에서 이용되는 것이 아니기에 접근 제어자를 private로 수정해주세요!
| package domain; | ||
|
|
||
| public class Size { | ||
| private int size; |
| @@ -0,0 +1,30 @@ | |||
| package domain; | |||
|
|
|||
| public enum PointString { | |||
There was a problem hiding this comment.
해당 객체 네이밍으로 보았을 때 해당 객체의 역할은 Point(연결 여부)를 출력형태의 문자열로 변경해주는 역할을 하고 있는 것 같습니다!
해당 객체는 domain 패키지 내부에 있기 보다는 view 패키지에 있는 것이 좋을 것 같습니다!
그 이유는 현재 이 객체는 화면 출력 요구사항 변화시 변경되는 객체라고 판단이 되기 때문입니다!
| import java.util.List; | ||
|
|
||
| public class Ladder { | ||
| private final Size ladderSize; |
There was a problem hiding this comment.
이 부분도 현재 따로 이용되지 활용되는 곳이 없는데 필드로 가지시는 이유가 있나요?
| this.lines = generateLines(lineSize); | ||
| } | ||
|
|
||
| public List<Line> generateLines(int lineSize) { |
There was a problem hiding this comment.
현재 해당 메소드도 외부에서 이용되는 것이 아니어서 접근 제어자를 private로 변경해주면 좋을 것 같습니다!
1차적으로 요구사항대로 구현을 했는데,
사다리에서 23번 열 사이에 연결된 곳이 없어서 12번/34번 이렇게 서로 독립되는 케이스가 발생합니다.
해당사항이 요구조건에 존재하지않는데 올바른 방향인지 조금 고민됩니다!