-
Notifications
You must be signed in to change notification settings - Fork 708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Step2 - 사다리(생성) #1740
base: seriouskang
Are you sure you want to change the base?
Step2 - 사다리(생성) #1740
Changes from all commits
dfd56b6
c707bc4
69ed35b
3216b24
db7d380
c80cf45
e158913
14318fc
7f4ac45
b3c5cba
9621f60
98c638a
e04869a
c407e71
27e4a27
d96a635
a485e1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package nextstep.ladder; | ||
|
||
import nextstep.ladder.domain.*; | ||
import nextstep.ladder.presentation.LadderGameResultView; | ||
import nextstep.ladder.presentation.util.ConsoleInputUtil; | ||
|
||
public class LadderApplication { | ||
public static void main(String[] args) { | ||
Participants participants = new Participants(participantNames()); | ||
Ladder ladder = ladderFactory().create(participants, maximumLadderHeight()); | ||
|
||
LadderGameResultView ladderGameResultView = new LadderGameResultView(participants, ladder); | ||
ladderGameResultView.printExecuteResult(); | ||
} | ||
|
||
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(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package nextstep.ladder.domain; | ||
|
||
import java.util.Objects; | ||
|
||
public class Connection { | ||
private final boolean connection; | ||
|
||
public Connection(boolean connection) { | ||
this.connection = connection; | ||
} | ||
|
||
public boolean isConnected() { | ||
return connection; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (!(o instanceof Connection)) return false; | ||
Connection that = (Connection) o; | ||
return connection == that.connection; | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(connection); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
package nextstep.ladder.domain; | ||
|
||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.stream.IntStream; | ||
|
||
public class Connections { | ||
private static final int START_INDEX_OF_CONNECTIONS = 0; | ||
private static final int ONE = 1; | ||
private final List<Connection> connections; | ||
|
||
public Connections(List<Connection> connections) { | ||
validateConnections(connections); | ||
this.connections = connections; | ||
} | ||
|
||
public List<Connection> connections() { | ||
return Collections.unmodifiableList(connections); | ||
} | ||
|
||
private void validateConnections(List<Connection> connections) { | ||
if(connections == null) { | ||
throw new IllegalArgumentException("가로 라인의 연결선은 null이 될 수 없습니다."); | ||
} | ||
|
||
if(containsConsecutiveConnections(connections)) { | ||
throw new IllegalArgumentException("가로 라인의 연결선은 연속해서 존재할 수 없습니다."); | ||
} | ||
} | ||
|
||
private boolean containsConsecutiveConnections(List<Connection> connections) { | ||
return IntStream.range(START_INDEX_OF_CONNECTIONS, beforeLastIndexOf(connections)) | ||
.anyMatch(index -> isConsecutiveConnections(connections, index)); | ||
} | ||
|
||
private int beforeLastIndexOf(List<Connection> connections) { | ||
return connections.size() - ONE; | ||
} | ||
|
||
private boolean isConsecutiveConnections(List<Connection> connections, int index) { | ||
return connections.get(index).isConnected() && connections.get(index + ONE).isConnected(); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (!(o instanceof Connections)) return false; | ||
Connections that = (Connections) o; | ||
return Objects.equals(connections, that.connections); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(connections); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package nextstep.ladder.domain; | ||
|
||
public interface ConnectionsFactory { | ||
Connections create(int numberOfConnections); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
package nextstep.ladder.domain; | ||
|
||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Objects; | ||
|
||
public class Ladder { | ||
private final List<Line> lines; | ||
|
||
public Ladder(Line...lines) { | ||
this(Arrays.asList(lines)); | ||
} | ||
|
||
public Ladder(List<Line> lines) { | ||
this.lines = lines; | ||
} | ||
|
||
public List<Line> lines() { | ||
return Collections.unmodifiableList(lines); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (!(o instanceof Ladder)) return false; | ||
Ladder ladder = (Ladder) o; | ||
return Objects.equals(lines, ladder.lines); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(lines); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package nextstep.ladder.domain; | ||
|
||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.IntStream; | ||
|
||
public class LadderFactory { | ||
private static final int ONE = 1; | ||
private static final int ZERO = 0; | ||
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 상수는 어떠한 목적으로 작성하였을까요? 매직넘버는 상수로 표현하라는 말이 있는데 이는 상수가 단순히 숫자의 의미 그 이상을 가지는 경우를 말합니다. 예를 들어서 자동차 게임에서 숫자 4는 단순히 숫자 4를 의미하는 것이 아닌, 비즈니스 상에서 '자동차를 움직이게 할 수 있는 최소 숫자'를 의미해요. 그렇기에 우리는 이 숫자 4를 앞선 비즈니스 의미를 담을 수 있는 상수를 사용해서 대체하는 것이구요. 그러나 지금의 상수들은 단순한 숫자 1, 0의 의미를 그대로 옮기는 것에 불과해요. 이러한 숫자는 굳이 상수로 추출해서 관리하지 않아도 됩니다. 하드코딩으로 작성된 숫자가 단순한 숫자의 값 이상의 비즈니스적 의미를 지닐 경우, 그런 상황에서 상수를 사용한다면 더 유의미한 상수 활용이 가능할 것 같아요!! |
||
private final LineFactory lineFactory; | ||
|
||
public LadderFactory(LineFactory lineFactory) { | ||
this.lineFactory = lineFactory; | ||
} | ||
Comment on lines
+10
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 개인적인 방향일 수 있어서 참고만 해주세요. 저는 보통 객체로 만들 때는 객체로 만들어야만 하는 값의 경우 객체로 사용합니다. 지금의 LadderFactory나 LineFactory는 상태를 가지지 않고도 충분히 사용이 가능하기에 이런 경우는 그냥 클래스로 사용하곤 해요. 이 부분은 한번 참고만 해주시면 좋을 것 같아요. |
||
|
||
public Ladder create(Participants participants, int height) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 메서드의 파라미터도 한번 고민해보면 좋을 것 같아요!! LadderFactory에서 사다리를 만들기 위해 필요한 정보는 사실 높이와 사다리 개수입니다. 즉 participants 객체를 굳이 넘기지 않고 '만들 사다리의 사이즈'라는 관점의 파라미터만 넘겨받을 수 있도록 하면 해당 팩토리의 의미가 더 명확해질 것 같아요!! 퍼블릭 메서드를 작성할 때는 메서드 명, 파라미터, 리턴 값과 내부 로직을 잘 고려하고 선정해야 합니다. 퍼블릭 메서드는 어디서든 사용되고 호출될 수 있기에 어떤 상황에서 어떻게 호출되더라도 합당한 정보들로 구성되는 것이 좋아요. 그런 관점에서 사다리를 만드는 팩토리가 굳이 참가자들이라는 객체를 받아야 할까? 를 생각해본다면 그 방향보다는 아래의 create 메서드처럼 만들 라인 갯수를 파라미터로 받는게 더 적절할 것 같고 사용하는 곳에서도 그렇게 사용하면 더 적절한 코드가 될 수 있을 것 같아요!! |
||
return create(participants.size(), height); | ||
} | ||
|
||
public Ladder create(int numberOfLine, int height) { | ||
validateHeight(height); | ||
return new Ladder(lines(numberOfLine, height)); | ||
} | ||
|
||
private List<Line> lines(int numberOfLine, int height) { | ||
return IntStream.range(ZERO, height) | ||
.mapToObj(index -> lineFactory.create(numberOfConnections(numberOfLine))) | ||
.collect(Collectors.toUnmodifiableList()); | ||
} | ||
|
||
private int numberOfConnections(int numberOfLine) { | ||
return numberOfLine - ONE; | ||
} | ||
|
||
private void validateHeight(int height) { | ||
if(height <= ZERO) { | ||
throw new IllegalArgumentException("사다리의 높이는 0 이하일 수 없습니다: " + height); | ||
} | ||
Comment on lines
+36
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 앞선 상수와 관련된 피드백을 여기에서 상세하게 추가해본다면, 여기서 0은 사다리를 만들 수 없는 높이입니다. 제가 보기엔 오히려 다시 풀어서 비즈니스 로직을 말해본다면, 1은 사다리를 만들기 위한 최소한의 높이 값이에요. 그렇다면 if의 조건을 만약 비즈니스 로직에서 '사다리를 만들기 위해서는 최소한 2이상의 높이가 필요합니다'라고 변경이 발생하면 어떻게 될까요? 지금과 같은 코드라면 이처럼 상수가 가지는 장점과 어떻게 사용해야지 이러한 장점을 누릴 수 있는지 한번 학습해보시면 앞으로 코드를 작성하는데 있어서 많은 도움이 될 것 같아요!! 👍🏼 |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
package nextstep.ladder.domain; | ||
|
||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.stream.Collectors; | ||
|
||
public class Line { | ||
private final Connections connections; | ||
|
||
public Line(Boolean...connections) { | ||
this(Arrays.stream(connections) | ||
.map(Connection::new) | ||
.collect(Collectors.toUnmodifiableList())); | ||
} | ||
|
||
public Line(List<Connection> connections) { | ||
this(new Connections(connections)); | ||
} | ||
|
||
public Line(Connections connections) { | ||
this.connections = connections; | ||
} | ||
Comment on lines
+21
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저의 경우는 주 생성자를 가장 위에 두곤 합니다. 그렇게 된다면 코드를 읽을 때 가독성을 높일 수 있어요!! |
||
|
||
public List<Connection> connections() { | ||
return connections.connections(); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (!(o instanceof Line)) return false; | ||
Line line = (Line) o; | ||
return Objects.equals(connections, line.connections); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(connections); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
package nextstep.ladder.domain; | ||
|
||
public class LineFactory { | ||
private static final int ZERO = 0; | ||
private final ConnectionsFactory connectionsFactory; | ||
|
||
public LineFactory(ConnectionsFactory connectionsFactory) { | ||
this.connectionsFactory = connectionsFactory; | ||
} | ||
|
||
public Line create(int numberOfConnections) { | ||
validateNumberOfConnections(numberOfConnections); | ||
return new Line(connections(numberOfConnections)); | ||
} | ||
|
||
private Connections connections(int numberOfConnections) { | ||
return connectionsFactory.create(numberOfConnections); | ||
} | ||
|
||
private void validateNumberOfConnections(int numberOfConnections) { | ||
if(numberOfConnections < ZERO) { | ||
throw new IllegalArgumentException("라인 연결수는 0 미만이 될 수 없습니다: " + numberOfConnections); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package nextstep.ladder.domain; | ||
|
||
import java.util.Objects; | ||
|
||
public class ParticipantName { | ||
private static final int MAX_NAME_LENGTH = 5; | ||
private final String name; | ||
|
||
public ParticipantName(String name) { | ||
validateName(name); | ||
this.name = name; | ||
} | ||
|
||
public String name() { | ||
return name; | ||
} | ||
|
||
private void validateName(String name) { | ||
if(name == null || name.isBlank()) { | ||
throw new IllegalArgumentException("이름은 null이거나 공백일 수 없습니다: " + name); | ||
} | ||
|
||
if(name.length() > MAX_NAME_LENGTH) { | ||
throw new IllegalArgumentException("이름의 길이는 5를 초과할 수 없습니다: " + name); | ||
} | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (!(o instanceof ParticipantName)) return false; | ||
ParticipantName that = (ParticipantName) o; | ||
return Objects.equals(name, that.name); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(name); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package nextstep.ladder.domain; | ||
|
||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.stream.Collectors; | ||
|
||
public class Participants { | ||
private static final String COMMA = ","; | ||
private final List<ParticipantName> names; | ||
|
||
public Participants(String csvNames) { | ||
this(csvNames.split(COMMA)); | ||
} | ||
Comment on lines
+9
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 로직은 뷰의 로직이 함께 포함된 것으로 보여요! 만약 참가자가 이름이 ,가 아닌 /로 들어온다면 어떻게 될까요? 이는 뷰의 로직 변경인데 그 변경의 영향이 도메인 코드까지 도달하게 될 것 같아요. |
||
|
||
public Participants(String...names) { | ||
this(participantsNames(names)); | ||
} | ||
|
||
public Participants(List<ParticipantName> names) { | ||
this.names = names; | ||
} | ||
|
||
public int size() { | ||
return names.size(); | ||
} | ||
|
||
public List<String> names() { | ||
return names.stream() | ||
.map(ParticipantName::name) | ||
.collect(Collectors.toUnmodifiableList()); | ||
} | ||
|
||
private static List<ParticipantName> participantsNames(String[] names) { | ||
return Arrays.stream(names) | ||
.map(String::trim) | ||
.map(ParticipantName::new) | ||
.collect(Collectors.toUnmodifiableList()); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (!(o instanceof Participants)) return false; | ||
Participants that = (Participants) o; | ||
return Objects.equals(names, that.names); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(names); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 해당 메서드 모두 이름이 명사형이에요!! 메서드는 행위를 하는 코드여서 이름은 행위를 �표현할 수 있는 동사형으로 작성되어야 적절합니다. 그래야지 해당 코드가 어떠한 일을 하는지 알 수 있어요. 동사형 이름을 보면 메서드겠구나! 라는 생각이 들 수 있어요 :)
추가로 해당 메서드들은 굳이 분리를 해야할 메서드인가?라는 생각은 듭니다. 메서드로 나누는 단위는 유의미한 단위가 되면 좋아요. 특정한 행위나 목적을 달성하기 위한 코드를 모아두는 곳으로 생각하면 좋은데 지금은 이미 그러한 메서드를 호출하는 역할만 하는, 단순히 wrapper 역할만 하는 메서드에요. 메서드의 행위나 존재 이유를 묻기에는 그 이유가 너무 부족한 것 같아요. 적절한 메서드 분리 기준을 어떻게 가져가면 좋을 지도 한번 고민해보면 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰어님, 안녕하세요.
우선 꼼꼼하게 리뷰해주셔서 감사합니다!
말씀해주신 내용을 바탕으로 조금씩 미션을 수정해보고 있는데,
여기서 해주신 피드백에 조금 의아한 부분이 있습니다.
교육 과정에서는 새로운 객체를 반환하는 빌더 메소드의 경우 이름을 명사로 짓으라고 가이드를 주셔서, 어떤 방향이 좋을지 판단이 어렵습니다 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇, 이 부분은 저도 처음 보네요!! 제가 교육을 받았을 때는 빌더의 이름에 베스트 케이스를 따로 학습한 적이 없었던 것 같은데 전달해주신 자료 저도 한번 참고하도록 하겠습니다 :) 그럼 우선은 교육에서 전달해주신 방향으로 진행해주시면 좋을 것 같아요 👍🏼