Step2 PR 요청드립니다. #2015
Step2 PR 요청드립니다. #2015
Conversation
- refactoring 사항
- validation은 generator에서 객체로 이동
- 생성자에서 필드를 초기화하는 것이 아닌, 컨트롤러에서 필드 값들을
만들고 넘겨주는 것으로 변경
모든 Line 후보를 만들어놓고 shuffle을 통해 하나의 후보를 꺼내쓰는 전략
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class BridgesGeneratorByCreatingCandidates implements BridgesGenerator { |
There was a problem hiding this comment.
이때, generator를 생성할 때, 값들을 검증해주어야할지 아니면 우선 Generator에서 값들을 만들고 그 값을 Line, Lines를 생성할때 생성자에서 검증해야할지 고민이었습니다. 제가 생각한 바로는, Generator에서 한다면 Generator에서 만든 List들은 확실히 검증된 값을 배출한다는 장점이 있지만, 다른 개발자가 Generator를 거치지 않고, new Line(), new Lines()를 호출해버린다면, 해당 객체는 유효성체크가 되지않는 객체이기에 문제가 있다고 판단했습니다.
하지만, 생성자에서 유효성체크를 한다면, Generator에서 만드는 값들은 미리 체크할 수가 없어 아쉬운 점이 있었던 것 같습니다. 이 부분에서 리뷰어님 생각이 궁금합니다..!
이건 생성을 위한 책임이 Generator와 각 클래스에 나누어져 있어서 발생하는 문제인 것 같습니다. 클래스나 메서드의 크기가 커지면 분리를 고민해봐야겠지만 Bridge나 Line에 대한 생성이나 제약 조건 검증 책임은 한 곳에 일관되게 있는게 좋을 것 같아요.
추가적으로, 위의 방법을 고민하면서, 다른 방법으로 생성자에서 Generator를 호출해서 값을 만들고, validation도 하면 되지 않을까라는 생각도 가지게 되었습니다. 서치를 하다보니, 생성자는 최소한의 일만 해야 테스트가 쉬울거라는 어떤 블로그의 글을 봐서 해당 방법은 이용하지않았습니다.
생성자에서 불필요하게 책임을 많이 가져가는게 걱정되시면 정적 팩터리 메서드를 만든 뒤 내부에서 제약 조건 검증 등을 모두 수행해도 괜찮을 것 같네요. 그 과정에서 정적 팩터리의 책임이 너무 커진다면 제약 조건 검증이나 기타 책임들만 떼서 별도의 클래스로 만들면 지금 말씀하시는 문제는 해결될 것 같아요.
| .flatMap(ladder -> { | ||
| List<Bridge> withFalse = new ArrayList<>(ladder); | ||
| withFalse.add(Bridge.of(false)); | ||
| List<Bridge> withTrue = new ArrayList<>(ladder); | ||
| if (!ladder.get(ladder.size() - 1).canCrossBridge()) { | ||
| withTrue.add(Bridge.of(true)); | ||
| return Stream.of(withFalse, withTrue); | ||
| } | ||
| return Stream.of(withFalse); | ||
| }) |
There was a problem hiding this comment.
규칙 1: 한 메서드에 오직 한 단계의 들여쓰기(indent)만 한다.
위의 객체지향 생활 체조 원칙을 어기고 있네요.
Stream의 연산 단계에서 이렇게 복잡한 연산이 적용되면 문제가 생겼을 때 확인이 어렵습니다. 메서드를 별도로 분리해 가독성을 높이거나 복잡도가 높아 개선이 어려운 경우라면 아예 Stream을 제거하는것도 방법일 것 같네요.
| return of(ThreadLocalRandom.current().nextBoolean()); | ||
| } | ||
|
|
||
| public void compareToNextBridge(Bridge next) { |
There was a problem hiding this comment.
보통 compare는 비교 후 조건에 따라 True False를 내려주는 형태로 사용하시는 케이스가 많습니다.
만약 특정 조건을 만족하지 않으면 예외를 반환하는 메서드를 네이밍하시려는 경우 아래와 같은 형태가 좀 더 적절하지 않을까요?
- requireXX
- checkXX
- validateXX
| public static Bridge of(boolean value) { | ||
| return cacheBridge.get(value); | ||
| } |
There was a problem hiding this comment.
중요한 부분은 아니나 참고 부탁드려요. 이펙티브 자바에서 정적 팩터리에 네이밍을 아래와 같이 가이드하고 있습니다.
- 매개변수가 하나일 때 from
- 매개변수가 여러개 일 때 of
| private static List<List<Bridge>> candidates = new ArrayList<>(); | ||
|
|
||
| static void generateLadders(int numbersOfPeople) { | ||
| List<List<Bridge>> result = new ArrayList<>(List.of(List.of(Bridge.of(false)), List.of(Bridge.of(true)))); |
There was a problem hiding this comment.
사다리의 한 Row를 Line이라는 것으로 잘 정의하셨는데요, Bidge와 관련해선 List<List<<>> 형태로 정의되고 있어 복잡도 높고 가독성이 떨어지는 것 같습니다. 이와 관련해서도 Bridges와 같은 일급 컬렉션을 추출해 가독성을 높이고 책임을 옮겨볼 수 있을까요?
| for (int i = 2; i < numbersOfPeople ; i++) { | ||
| result = dp(result); | ||
| } |
There was a problem hiding this comment.
이쪽 로직이 다소 복잡한데요, 이 2가 어떤 의미를 갖는건지 처음 보는 사람도 바로 이해할 수 있도록 상수 등으로 추출하는건 어떨까요?
| public class LinesGenerator { | ||
|
|
||
| private final BridgesGenerator bridgesGenerator = new BridgesGeneratorBySequentialRandom(); | ||
|
|
||
| public List<Line> generatorLines(Integer numbersOfPeople, Integer height) { | ||
| return IntStream.range(0, height) | ||
| .mapToObj(index -> new Line(numbersOfPeople, bridgesGenerator.generateBridges(numbersOfPeople))) | ||
| .collect(Collectors.toList()); | ||
| } |
There was a problem hiding this comment.
이정도 복잡도라면 Lines 내부 정적 팩터리로 책임을 옮기는건 어떨까요? 관리할 클래스가 늘어나면 복잡도가 더 올라갈 것 같습니다.
| } | ||
|
|
||
| public List<Bridge> getBridges() { | ||
| return bridges; |
There was a problem hiding this comment.
List<Bidge>의 참조가 외부에 노출되면 컬렉션이 수정되거나 할 수 있지 않을까요?
스트림 toList 혹은 Collections의 unmodifible 관련 메서드를 활용해서 외부에서 참조를 수정할 수 없도록 하는건 어떨까요?
규남님 안녕하세요!!
사다리타기 2단계 리뷰 요청드립니다..!!
BridgesGeneratorBySequentialRandom: 맨 첫 컬럼과 두번째 컬럼을 연결하는 Bridge를 랜덤으로 생성하고 그 다음 Bridge는 앞의 Bridge의 여부를 보고 생성하는 전략BridgesGeneratorByCreatingCandidates: 사람 수에 따라 나올 수 있는 모든List<Bridge>를 미리 만들어놓고, 그 중 하나를 뽑아쓰는 전략 (DP Bottom up 전략에서 생각한 방식입니다)진행하면서 궁금했던 점은 밑에 남겨놓을게요.
validation을 어디서 검증해야하는가? -> Generator Or 생성자
Line은List<Bridge>를 가지고 있고,BridgeGenerator에서List<Bridge>를 만들고Line생성자를 호출할때 Generator를 통해 만든List<Bridge>를 넘겨주고있습니다.Lines은List<Line>을 가지고 있고,LinesGenerator에서List<Line>을 만들고Lines생성자를 호출할때List<Line>을 넘겨주고있습니다.new Line(),new Lines()를 호출해버린다면, 해당 객체는 유효성체크가 되지않는 객체이기에 문제가 있다고 판단했습니다.추가적으로, 위의 방법을 고민하면서, 다른 방법으로 생성자에서 Generator를 호출해서 값을 만들고, validation도 하면 되지 않을까라는 생각도 가지게 되었습니다.