Skip to content
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

Open
wants to merge 17 commits into
base: seriouskang
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
dfd56b6
feat(Line): 사다리의 가로 라인에 대한 도메인 객체 신규
seriouskang May 13, 2023
c707bc4
feat(Line): 가로 연결선이 null일 경우에 대한 검증 로직 추가
seriouskang May 13, 2023
69ed35b
feat(Connections): 가로 라인 연결선 포장 객체 신규
seriouskang May 13, 2023
3216b24
feat(LineFactory, ConnectionsFactory): 라인 팩토리 및 Connections 팩토리 신규
seriouskang May 13, 2023
db7d380
feat(Ladder, LadderFactory): 사다리 및 사다리 팩토리 클래스 신규
seriouskang May 13, 2023
c80cf45
feat(Name): 사다리 게임 참여자 이름 클래스 신규
seriouskang May 14, 2023
e158913
feat(Participants): 참여자 클래스 신규
seriouskang May 14, 2023
14318fc
feat(ConsoleInputUtil): 사용자 콘솔 입력용 유틸 클래스 신규
seriouskang May 14, 2023
7f4ac45
feat(ConnectionResultView): 연결선 출력을 위한 클래스 신규
seriouskang May 14, 2023
b3c5cba
feat(LineResultView): 사다리의 한 라인을 출력하기 위한 클래스 신규
seriouskang May 14, 2023
9621f60
feat(LadderResultView); 사다리 출력을 위한 클래스 신규
seriouskang May 14, 2023
98c638a
feat(ParticipantResultView): 참가자 출력을 위한 클래스 신규
seriouskang May 14, 2023
e04869a
feat(LadderGameResultView): 사다리 게임 결과 출력을 위한 클래스 신규
seriouskang May 14, 2023
c407e71
feat(RandomConnectionsFactory): 랜덤한 연결선 생성을 위한 팩토리 클래스 신규
seriouskang May 14, 2023
27e4a27
feat(Connection): 연결 여부 클래스 신규
seriouskang May 14, 2023
d96a635
feat(LadderApplication): 사다리 게임 메인 클래스 신규
seriouskang May 14, 2023
a485e1e
docs(README.md): 프로그래밍 및 기능 요구 사항 추가
seriouskang May 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* [텍스트와 이미지로 살펴보는 온라인 코드 리뷰 과정](https://github.com/nextstep-step/nextstep-docs/tree/master/codereview)


## step1(1단계) - 스트림, 람다, Optional
## step1 - 스트림, 람다, Optional
### 람다 실습
- [x] 익명 클래스를 람다로 전환
> `CarTest`
Expand All @@ -31,3 +31,19 @@
> 메소드 인자로 받은 `User`를 `Optional`로 생성하여 메서드 구현
- [x] `Users` 클래스의 `getUser()` 메서드를 `stream`과 `Optional`을 활용해 리팩토링
- [x] `Expression`의 `of` 메서드를 `stream` 기반으로 리팩토링


## step2 - 사다리(생성)
### 프로그래밍 요구 사항
- 자바 8의 스트림과 람다를 적용해 프로그래밍
- 규칙 6: 모든 엔티티를 작게 유지

### 기능 요구 사항
- [x] 사다리 게임에 참여하는 사람의 이름을 최대 5글자까지 부여
> 참여 최소 인원은 1명
- [x] 사람 이름은 쉼표(`,`)를 기준으로 구분
- [x] 사다리 출력 시, 사람 이름을 5자 기준으로 함께 출력
- [x] 사다리 타기가 정상적으로 동작하려면 라인이 겹치지 않도록 해야 한다.
> 가로 라인이 겹치는 경우(ex. `|-----|-----|`) 어느 방향으로 이동할지 결정할 수 없다.
> 최소 사다리 높이는 1
> 최소 라인 연결수는 0개
35 changes: 35 additions & 0 deletions src/main/java/nextstep/ladder/LadderApplication.java
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();
}
Comment on lines +16 to +34
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우선 해당 메서드 모두 이름이 명사형이에요!! 메서드는 행위를 하는 코드여서 이름은 행위를 �표현할 수 있는 동사형으로 작성되어야 적절합니다. 그래야지 해당 코드가 어떠한 일을 하는지 알 수 있어요. 동사형 이름을 보면 메서드겠구나! 라는 생각이 들 수 있어요 :)

추가로 해당 메서드들은 굳이 분리를 해야할 메서드인가?라는 생각은 듭니다. 메서드로 나누는 단위는 유의미한 단위가 되면 좋아요. 특정한 행위나 목적을 달성하기 위한 코드를 모아두는 곳으로 생각하면 좋은데 지금은 이미 그러한 메서드를 호출하는 역할만 하는, 단순히 wrapper 역할만 하는 메서드에요. 메서드의 행위나 존재 이유를 묻기에는 그 이유가 너무 부족한 것 같아요. 적절한 메서드 분리 기준을 어떻게 가져가면 좋을 지도 한번 고민해보면 좋을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰어님, 안녕하세요.
우선 꼼꼼하게 리뷰해주셔서 감사합니다!

말씀해주신 내용을 바탕으로 조금씩 미션을 수정해보고 있는데,
여기서 해주신 피드백에 조금 의아한 부분이 있습니다.

교육 과정에서는 새로운 객체를 반환하는 빌더 메소드의 경우 이름을 명사로 짓으라고 가이드를 주셔서, 어떤 방향이 좋을지 판단이 어렵습니다 😭

private static LadderFactory ladderFactory() {
    return new LadderFactory(lineFactory());
}

private static LineFactory lineFactory() {
    return new LineFactory(randomConnectionsFactory());
}

private static ConnectionsFactory randomConnectionsFactory() {
    return new RandomConnectionsFactory();
}

Copy link

@lxxjn0 lxxjn0 May 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엇, 이 부분은 저도 처음 보네요!! 제가 교육을 받았을 때는 빌더의 이름에 베스트 케이스를 따로 학습한 적이 없었던 것 같은데 전달해주신 자료 저도 한번 참고하도록 하겠습니다 :) 그럼 우선은 교육에서 전달해주신 방향으로 진행해주시면 좋을 것 같아요 👍🏼

}
28 changes: 28 additions & 0 deletions src/main/java/nextstep/ladder/domain/Connection.java
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);
}
}
57 changes: 57 additions & 0 deletions src/main/java/nextstep/ladder/domain/Connections.java
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);
}
}
5 changes: 5 additions & 0 deletions src/main/java/nextstep/ladder/domain/ConnectionsFactory.java
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);
}
35 changes: 35 additions & 0 deletions src/main/java/nextstep/ladder/domain/Ladder.java
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);
}
}
40 changes: 40 additions & 0 deletions src/main/java/nextstep/ladder/domain/LadderFactory.java
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
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 개인적인 방향일 수 있어서 참고만 해주세요. 저는 보통 객체로 만들 때는 객체로 만들어야만 하는 값의 경우 객체로 사용합니다. 지금의 LadderFactory나 LineFactory는 상태를 가지지 않고도 충분히 사용이 가능하기에 이런 경우는 그냥 클래스로 사용하곤 해요. 이 부분은 한번 참고만 해주시면 좋을 것 같아요.


public Ladder create(Participants participants, int height) {
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앞선 상수와 관련된 피드백을 여기에서 상세하게 추가해본다면, 여기서 0은 사다리를 만들 수 없는 높이입니다. 제가 보기엔 오히려 다시 풀어서 비즈니스 로직을 말해본다면, 1은 사다리를 만들기 위한 최소한의 높이 값이에요. 그렇다면 if의 조건을 height < 1로 두고, 이때의 1을 MINIMUM_LADDER_HEIGHT라고 한다면 1은 '사다리를 만들기 위한 최소한의 높이'라는 비즈니스 정보를 가질 수 있게 됩니다.

만약 비즈니스 로직에서 '사다리를 만들기 위해서는 최소한 2이상의 높이가 필요합니다'라고 변경이 발생하면 어떻게 될까요? 지금과 같은 코드라면 ZERO라는 상수는 TWO란 상수명으로 이름도 변경되어야 하고, 그 값도 2로 변경되어야 할 것입니다. 그러나 앞선 예시처럼 MINIMUM_LADDER_HEIGHT를 상수로 사용한다면 상수명은 전혀 변경할 필요가 없이 여전히 비즈니스 로직을 나타내주고 있으며 단순히 해당 상수의 값을 2로만 바꾸면 됩니다.

이처럼 상수가 가지는 장점과 어떻게 사용해야지 이러한 장점을 누릴 수 있는지 한번 학습해보시면 앞으로 코드를 작성하는데 있어서 많은 도움이 될 것 같아요!! 👍🏼

}
}
41 changes: 41 additions & 0 deletions src/main/java/nextstep/ladder/domain/Line.java
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
Copy link

Choose a reason for hiding this comment

The 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);
}
}
25 changes: 25 additions & 0 deletions src/main/java/nextstep/ladder/domain/LineFactory.java
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);
}
}
}
40 changes: 40 additions & 0 deletions src/main/java/nextstep/ladder/domain/ParticipantName.java
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);
}
}
53 changes: 53 additions & 0 deletions src/main/java/nextstep/ladder/domain/Participants.java
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
Copy link

Choose a reason for hiding this comment

The 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);
}
}
Loading