Step2 - 사다리(생성) #1740
Step2 - 사다리(생성) #1740
Conversation
| private static LadderFactory ladderFactory() { | ||
| return new LadderFactory(lineFactory()); | ||
| } | ||
|
|
||
| private static LineFactory lineFactory() { | ||
| return new LineFactory(randomConnectionsFactory()); | ||
| } | ||
|
|
||
| private static ConnectionsFactory randomConnectionsFactory() { | ||
| return new RandomConnectionsFactory(); | ||
| } | ||
|
|
||
| private static int maximumLadderHeight() { | ||
| return ConsoleInputUtil.askMaximumLadderHeight(); | ||
| } | ||
|
|
||
| private static String participantNames() { | ||
| return ConsoleInputUtil.askParticipantNames(); | ||
| } |
There was a problem hiding this comment.
우선 해당 메서드 모두 이름이 명사형이에요!! 메서드는 행위를 하는 코드여서 이름은 행위를 �표현할 수 있는 동사형으로 작성되어야 적절합니다. 그래야지 해당 코드가 어떠한 일을 하는지 알 수 있어요. 동사형 이름을 보면 메서드겠구나! 라는 생각이 들 수 있어요 :)
추가로 해당 메서드들은 굳이 분리를 해야할 메서드인가?라는 생각은 듭니다. 메서드로 나누는 단위는 유의미한 단위가 되면 좋아요. 특정한 행위나 목적을 달성하기 위한 코드를 모아두는 곳으로 생각하면 좋은데 지금은 이미 그러한 메서드를 호출하는 역할만 하는, 단순히 wrapper 역할만 하는 메서드에요. 메서드의 행위나 존재 이유를 묻기에는 그 이유가 너무 부족한 것 같아요. 적절한 메서드 분리 기준을 어떻게 가져가면 좋을 지도 한번 고민해보면 좋을 것 같아요.
There was a problem hiding this comment.
리뷰어님, 안녕하세요.
우선 꼼꼼하게 리뷰해주셔서 감사합니다!
말씀해주신 내용을 바탕으로 조금씩 미션을 수정해보고 있는데,
여기서 해주신 피드백에 조금 의아한 부분이 있습니다.
교육 과정에서는 새로운 객체를 반환하는 빌더 메소드의 경우 이름을 명사로 짓으라고 가이드를 주셔서, 어떤 방향이 좋을지 판단이 어렵습니다 😭
private static LadderFactory ladderFactory() {
return new LadderFactory(lineFactory());
}
private static LineFactory lineFactory() {
return new LineFactory(randomConnectionsFactory());
}
private static ConnectionsFactory randomConnectionsFactory() {
return new RandomConnectionsFactory();
}There was a problem hiding this comment.
엇, 이 부분은 저도 처음 보네요!! 제가 교육을 받았을 때는 빌더의 이름에 베스트 케이스를 따로 학습한 적이 없었던 것 같은데 전달해주신 자료 저도 한번 참고하도록 하겠습니다 :) 그럼 우선은 교육에서 전달해주신 방향으로 진행해주시면 좋을 것 같아요 👍🏼
| private static final int ONE = 1; | ||
| private static final int ZERO = 0; |
There was a problem hiding this comment.
해당 상수는 어떠한 목적으로 작성하였을까요? 매직넘버는 상수로 표현하라는 말이 있는데 이는 상수가 단순히 숫자의 의미 그 이상을 가지는 경우를 말합니다. 예를 들어서 자동차 게임에서 숫자 4는 단순히 숫자 4를 의미하는 것이 아닌, 비즈니스 상에서 '자동차를 움직이게 할 수 있는 최소 숫자'를 의미해요. 그렇기에 우리는 이 숫자 4를 앞선 비즈니스 의미를 담을 수 있는 상수를 사용해서 대체하는 것이구요.
그러나 지금의 상수들은 단순한 숫자 1, 0의 의미를 그대로 옮기는 것에 불과해요. 이러한 숫자는 굳이 상수로 추출해서 관리하지 않아도 됩니다. 하드코딩으로 작성된 숫자가 단순한 숫자의 값 이상의 비즈니스적 의미를 지닐 경우, 그런 상황에서 상수를 사용한다면 더 유의미한 상수 활용이 가능할 것 같아요!!
| private final LineFactory lineFactory; | ||
|
|
||
| public LadderFactory(LineFactory lineFactory) { | ||
| this.lineFactory = lineFactory; | ||
| } |
There was a problem hiding this comment.
이 부분은 개인적인 방향일 수 있어서 참고만 해주세요. 저는 보통 객체로 만들 때는 객체로 만들어야만 하는 값의 경우 객체로 사용합니다. 지금의 LadderFactory나 LineFactory는 상태를 가지지 않고도 충분히 사용이 가능하기에 이런 경우는 그냥 클래스로 사용하곤 해요. 이 부분은 한번 참고만 해주시면 좋을 것 같아요.
| this.lineFactory = lineFactory; | ||
| } | ||
|
|
||
| public Ladder create(Participants participants, int height) { |
There was a problem hiding this comment.
해당 메서드의 파라미터도 한번 고민해보면 좋을 것 같아요!! LadderFactory에서 사다리를 만들기 위해 필요한 정보는 사실 높이와 사다리 개수입니다. 즉 participants 객체를 굳이 넘기지 않고 '만들 사다리의 사이즈'라는 관점의 파라미터만 넘겨받을 수 있도록 하면 해당 팩토리의 의미가 더 명확해질 것 같아요!!
퍼블릭 메서드를 작성할 때는 메서드 명, 파라미터, 리턴 값과 내부 로직을 잘 고려하고 선정해야 합니다. 퍼블릭 메서드는 어디서든 사용되고 호출될 수 있기에 어떤 상황에서 어떻게 호출되더라도 합당한 정보들로 구성되는 것이 좋아요. 그런 관점에서 사다리를 만드는 팩토리가 굳이 참가자들이라는 객체를 받아야 할까? 를 생각해본다면 그 방향보다는 아래의 create 메서드처럼 만들 라인 갯수를 파라미터로 받는게 더 적절할 것 같고 사용하는 곳에서도 그렇게 사용하면 더 적절한 코드가 될 수 있을 것 같아요!!
| if(height <= ZERO) { | ||
| throw new IllegalArgumentException("사다리의 높이는 0 이하일 수 없습니다: " + height); | ||
| } |
There was a problem hiding this comment.
앞선 상수와 관련된 피드백을 여기에서 상세하게 추가해본다면, 여기서 0은 사다리를 만들 수 없는 높이입니다. 제가 보기엔 오히려 다시 풀어서 비즈니스 로직을 말해본다면, 1은 사다리를 만들기 위한 최소한의 높이 값이에요. 그렇다면 if의 조건을 height < 1로 두고, 이때의 1을 MINIMUM_LADDER_HEIGHT라고 한다면 1은 '사다리를 만들기 위한 최소한의 높이'라는 비즈니스 정보를 가질 수 있게 됩니다.
만약 비즈니스 로직에서 '사다리를 만들기 위해서는 최소한 2이상의 높이가 필요합니다'라고 변경이 발생하면 어떻게 될까요? 지금과 같은 코드라면 ZERO라는 상수는 TWO란 상수명으로 이름도 변경되어야 하고, 그 값도 2로 변경되어야 할 것입니다. 그러나 앞선 예시처럼 MINIMUM_LADDER_HEIGHT를 상수로 사용한다면 상수명은 전혀 변경할 필요가 없이 여전히 비즈니스 로직을 나타내주고 있으며 단순히 해당 상수의 값을 2로만 바꾸면 됩니다.
이처럼 상수가 가지는 장점과 어떻게 사용해야지 이러한 장점을 누릴 수 있는지 한번 학습해보시면 앞으로 코드를 작성하는데 있어서 많은 도움이 될 것 같아요!! 👍🏼
|
|
||
| private Connection createConnection(Connection previousConnection) { | ||
| if(previousConnection.isConnected()) { | ||
| return new Connection(false); |
There was a problem hiding this comment.
false인 커넥션을 디폴트처럼 생성해주는 펙토리 메서드나 상수를 하나 둬보는 것은 어떨까요??
| private static final String COMMA = ","; | ||
| private final List<ParticipantName> names; | ||
|
|
||
| public Participants(String csvNames) { | ||
| this(csvNames.split(COMMA)); | ||
| } |
There was a problem hiding this comment.
해당 로직은 뷰의 로직이 함께 포함된 것으로 보여요! 만약 참가자가 이름이 ,가 아닌 /로 들어온다면 어떻게 될까요? 이는 뷰의 로직 변경인데 그 변경의 영향이 도메인 코드까지 도달하게 될 것 같아요.
| public Line(Connections connections) { | ||
| this.connections = connections; | ||
| } |
There was a problem hiding this comment.
저의 경우는 주 생성자를 가장 위에 두곤 합니다. 그렇게 된다면 코드를 읽을 때 가독성을 높일 수 있어요!!
| private static final String VERTICAL_BAR = "|"; | ||
| private final Line line; |
| private ConnectionsFactory evenConnectedConnectionsFactory() { | ||
| return numberOfConnections -> new Connections(evenElementTrueList(numberOfConnections)); | ||
| } | ||
|
|
||
| private List<Connection> evenElementTrueList(int size) { | ||
| return IntStream.range(0, size) | ||
| .mapToObj(index -> index % 2 == 0) | ||
| .map(Connection::new) | ||
| .collect(Collectors.toUnmodifiableList()); | ||
| } |
There was a problem hiding this comment.
반복되는 코드로 보여지는데 테스트 패키지 내에서 따로 테스트용 펙토리를 만들어 두는 것은 어떨까요?
리뷰어님, 안녕하십니까 🙇♂️
두번째 스텝 PR 드립니다. 리뷰 잘 부탁 드리겠습니다!!