Step3 #1853
Step3 #1853
Conversation
| public List<ExecuteResult> value() { | ||
| return executeResults; | ||
| } |
There was a problem hiding this comment.
지금과 같은 복잡도와 혼자 작업하는 상황에서라면 상관없겠지만.
이렇게 List의 참조가 외부에 노출되게 되면 외부에서 의도하지 않게 값을 추가하거나 삭제할 수 있게 됩니다.
Collections.unmodifiableList와 같은걸로 감싸서 반환하는 것을 고려해보셔도 좋겠네요.
| @@ -0,0 +1,23 @@ | |||
| package nextstep.ladder.domain; | |||
|
|
|||
| public class InputOutput { | |||
There was a problem hiding this comment.
InputOutput의 이름만 보면 어떤 의도를 가진 클래스인지 알기 어려워보여요.
사다리 게임의 결과가 담겨있다는 의미를 나타낼 수 있는 클래스명으로 변경해보시면 어떨까요?
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class ExecuteResults { |
There was a problem hiding this comment.
일급 컬렉션이나 클래스 분리, 제약 등을 깔끔하게 잘 정의해주셨네요 👍
| public String[][] result() { | ||
| List<Line> lines = this.lines.value(); | ||
|
|
||
| int width = verticalLineCount * 2 - 1; | ||
| int height = this.lines.value().size(); | ||
|
|
||
| String[][] ladder = new String[height][width]; | ||
|
|
||
| IntStream.range(BEGIN_INDEX, height) | ||
| .forEach(i -> IntStream.range(BEGIN_INDEX, width) | ||
| .forEach(j -> { | ||
| if (isVerticalLine(j)) { | ||
| ladder[i][j] = "v"; | ||
| return; | ||
| } | ||
| if (isHorizontalLine(lines, i, j)) { | ||
| ladder[i][j] = "h"; | ||
| } | ||
| })); | ||
| return ladder; | ||
| } |
There was a problem hiding this comment.
Collection에 여러 편의 메서드들이 존재하는데 다시 이차원 배열을 활용하는게 좋은 선택일지 고민이 필요할 것 같아요.
vertical인지 horizontal인지는 메서드로 검증할 수 있으므로 v, h가 기록된 String[][] 타입의 이차원 배열을 만들 필요가 있을까요?
| .reduce(currentIdx, (idx, i) -> { | ||
| String[] row = ladder[i]; | ||
| if (isMovableToLeft(idx, row)) { | ||
| return idx - 2; | ||
| } | ||
| if (isMovableToRight(idx, row, columnLength)) { | ||
| return idx + 2; | ||
| } | ||
| return idx; | ||
| }); |
There was a problem hiding this comment.
전체적으로 클래스도, 책임도 잘 나누어 주셨는데 이 부분의 복잡도가 높은 것 같습니다.
메서드 추출 혹은 리팩토링으로 조금 더 로직을 가독성 좋게 만들어보시면 좋겠습니다.
| (v1, v2) -> v1, LinkedHashMap::new))); | ||
| } | ||
|
|
||
| int findOutputIdx(int startIdx) { |
There was a problem hiding this comment.
여기에 접근 제한자가 빠진 이유가 있을까요~?
makeResult에서만 활용하는거라면 private 메서드로 만들어주시면 좋겠습니다.
| People people = ladder.people(); | ||
| Lines lines = ladder.lines(); | ||
| private static String generateLine(String result) { | ||
| if (isVerticalLine(result)) { |
There was a problem hiding this comment.
뷰 쪽의 isVerticalLine 로직을 도메인, 핵심 비즈니스 로직에서 활용하고 있는 형태인데요.
의존이 도메인 -> 뷰를 향하고 있기 때문에 좋은 형태로 보이지 않습니다. 개선해보시면 어떨까요?
리뷰 잘 부탁드립니다 ^^