Skip to content

Step2 #2324

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

Open
wants to merge 9 commits into
base: bb-worm
Choose a base branch
from
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,15 @@
- [x] 요구사항 1 - Optional을 활용해 조건에 따른 반환
- [x] 요구사항 2 - Optional에서 값을 반환
- [x] 요구사항 3 - Optional에서 exception 처리
- [x] 요구사항 3 - Optional에서 exception 처리
- 리뷰
- [x] 전체 테스트 코드 통과

### 사다리(생성)
- [x] 라인 길이를 전달받아 사다리 라인을 생성한다.
- [x] 각 사다리 라인은 이웃한 곳과 이어져있을 수 있다.
- [x] 각 사다리 라인은 겹치지 않게 이어져 있어야 한다.
- ~~[x] 라인의 각 포인트는 왼쪽/오른쪽 연결 유무를 갖는다.~~
- [x] 라인의 각 포인트는 오른쪽 연결 유무만 갖는다.
- [x] 사다리 높이와 라인 길이를 전달받아 사다리를 생성한다.
- [x] 사람 이름은 최대 5글자까지만 가능하다.
21 changes: 21 additions & 0 deletions src/main/java/ladder/Main.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package ladder;

import ladder.domain.Ladder;
import ladder.domain.People;
import ladder.view.InputView;
import ladder.view.ResultView;

import java.util.Random;

public class Main {
private static void makeLadder() {
People people = InputView.inputPeople();
int ladderHeight = InputView.inputLadderHeight();
Ladder ladder = new Ladder(ladderHeight, people, (idx, preConnected) -> !preConnected && new Random().nextBoolean());
Copy link
Author

Choose a reason for hiding this comment

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

일부러 람다 사용해보려고 ConnectStrategy를 람다로 표현했습니다. 그런데 오히려 코드 읽기가 어려워지는 거 같아서.. 이런 경우엔 따로 클래스로 빼는 게 나을까요?

Copy link
Member

Choose a reason for hiding this comment

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

클래스도 좋고 변수로 선언해도 좋을 것 같아요

Suggested change
Ladder ladder = new Ladder(ladderHeight, people, (idx, preConnected) -> !preConnected && new Random().nextBoolean());
final ConnectStrategy defaultStrategy = (idx, preConnected) -> !preConnected && RANDOM.nextBoolean();
final Ladder ladder = new Ladder(ladderHeight, people, defaultStrategy);

Optional을 어디에 써야할지 감이 잘 오지 않습니다. 적용하기 좋은 곳이 있을까요?

null을 활용한 설계가 아니라면 억지로 사용하지 않으셔도 됩니다 😃
기능 목록을 정의하실 때 마지막 포인트는 null 이다. 라는 내용이 있었는데요.
실제로 null을 활용했다면 Optional을 활용할 수 있는 지점이 아닐까 싶어요.

ResultView.printLadder(people, ladder);
}

public static void main(String[] args) {
makeLadder();
}
}
5 changes: 5 additions & 0 deletions src/main/java/ladder/domain/ConnectStrategy.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package ladder.domain;

public interface ConnectStrategy {
boolean connect(int idx, boolean preConnected);
}
19 changes: 19 additions & 0 deletions src/main/java/ladder/domain/Ladder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package ladder.domain;

import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class Ladder {
private final List<Line> lines;

public Ladder(int height, People people, ConnectStrategy connectStrategy) {
this.lines = IntStream.range(0, height)
.mapToObj(i -> new Line(people.size(), connectStrategy))
.collect(Collectors.toList());
}
Comment on lines +10 to +14
Copy link
Member

Choose a reason for hiding this comment

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

생성자는 유효성 검증과 변수 초기화에 집중하고,
생성 로직은 정적 팩토리 메서드나 별도의 클래스로 분리하면 어떨까요?

    public Ladder(List<Line> lines) {
        // validate
        this.lines = lines;
    }

    public static Ladder create(int height, People people, ConnectStrategy connectStrategy) {
        final List<Line> lines = ...;
        return new Ladder(lines);
    }

Copy link
Author

Choose a reason for hiding this comment

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

매번 생성자가 복잡해질 때마다 고민이 있었는데 말씀하신 것처럼 분리해보겠습니다


public List<Line> lines() {
return lines;
}
}
32 changes: 32 additions & 0 deletions src/main/java/ladder/domain/Line.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package ladder.domain;

import java.util.List;
import java.util.Objects;

public class Line {
Copy link
Member

Choose a reason for hiding this comment

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

Line 객체는 특별한 기능이 없는 것은 같아요
Points와 어떤 차이가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

엇 그러게요.. 작업하면서도 뭔가 이상하다 싶었는데 의미없이 사용하고 있었네요.
합쳐놓겠습니다!

private final Points points;

public Line(List<Point> points) {
this.points = new Points(points);
}

public Line(int size, ConnectStrategy connectStrategy) {
this.points = new Points(size, connectStrategy);
}
Comment on lines +9 to +15
Copy link
Member

Choose a reason for hiding this comment

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

주 생성자를 두어 호출하면 어떨까요?

        public Line(Points points) {
            this.points = points;
        }
        
        public Line(List<Points> points) {
            this(new Points(points));
        }

Copy link
Author

Choose a reason for hiding this comment

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

좋습니다!


public Points points() {
return points;
}

@Override
public boolean equals(Object o) {
if (o == null || getClass() != o.getClass()) return false;
Line line = (Line) o;
return Objects.equals(points, line.points);
}

@Override
public int hashCode() {
return Objects.hashCode(points);
}
}
25 changes: 25 additions & 0 deletions src/main/java/ladder/domain/People.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package ladder.domain;

import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;

public class People {
private final static String DELIMITER = ",";
Copy link
Member

Choose a reason for hiding this comment

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

파싱과 관련된 로직은 view 또는 controller로 이동하면 어떨까요?
People 이라는 도메인은 구분기호에 따라 역할이 달라지지 않을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

말씀하신 것처럼 People이 구분자를 가지고 있을 필요가 없네요. 변경해두겠습니다.


private final List<Person> people;

public People(String input) {
this.people = Arrays.stream(input.split(DELIMITER))
.map(Person::new)
.collect(Collectors.toList());
}

public int size() {
return people.size();
}

public List<Person> values() {
return people;
}
}
18 changes: 18 additions & 0 deletions src/main/java/ladder/domain/Person.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package ladder.domain;

public class Person {
private static final int NAME_MAX_LENGTH = 5;

private final String name;

public Person(String name) {
if (name.length() > 5) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (name.length() > 5) {
if (name.length() > NAME_MAX_LENGTH) {

throw new IllegalArgumentException("이름 길이는 최대 " + NAME_MAX_LENGTH + "자입니다.");
}
this.name = name;
}

public String name() {
return name;
}
}
27 changes: 27 additions & 0 deletions src/main/java/ladder/domain/Point.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package ladder.domain;

import java.util.Objects;

public class Point {
private final boolean rightConnected;
Comment on lines +5 to +6
Copy link
Member

Choose a reason for hiding this comment

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

  • Point 클래스를 만들어서 이웃한 Point와의 연결 유무를 가지도록 했습니다. 원래는 좌우로 연결 유무를 가지도록 하려고 했는데 너무 복잡해지는 거 같아서.. 일단 오른쪽 연결 유무만 갖도록 했습니다. 그런데 사용자가 이를 알고 사용해야 하니 뭔가 찜찜하더라고요. 더 좋은 방법이 있을까요?
  • 그리고 위처럼 Point가 오른쪽 연결 유무를 갖도록 하니, 마지막 Point는 오른쪽에 이웃한 Point가 없어도 연결되는 경우가 있습니다. 처음에는 생성될 때 막으려고 했는데요, 그러면 필드 값을 set 하는 것 같아서 좀 아닌 거 같더라고요. 아직 구현하지는 않았지만 나중에 실행 단계에서는 마지막 위치인지 확인하는 조건절을 넣을 생각인데요. 애초에 생성 단계에서 막아주는 게 나을까요?

좌우연결을 가진 상태로 끝까지 구현해 보셨어도 좋았을 것 같아요.

마지막 질문에 대한 답변을 드리자면 생성 단계에서 막아주는게 좋지 않을까 생각이 드는데요 😃
질문 내용으로 보아 여러 시도와 많은 고민 하신 것 같아 약간의 참고용 힌트를 드리면 어떨까 싶어요.
(힌트일뿐 정답이 아닙니다)

Point first = Point.first();
Point second = first.next();
Point third = second.next();
Point forth = third.last();

Point가 처음, 중간, 마지막에 대한 값을 스스로 결정할 수 있다면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

설명 감사합니다!
좌우연결을 가진 상태로 한 번 더 구현해보겠습니다..!


public Point(boolean rightConnected) {
this.rightConnected = rightConnected;
}

public boolean connected() {
return rightConnected;
}

@Override
public boolean equals(Object o) {
if (o == null || getClass() != o.getClass()) return false;
Point point = (Point) o;
return rightConnected == point.rightConnected;
}

@Override
public int hashCode() {
return Objects.hashCode(rightConnected);
}
}
55 changes: 55 additions & 0 deletions src/main/java/ladder/domain/Points.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package ladder.domain;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.stream.IntStream;

public class Points {
private final List<Point> points;

public Points(List<Point> points) {
this.points = points;
validate();
}

public Points(int size, ConnectStrategy connectStrategy) {
this(generatePoints(size, connectStrategy));
}

public List<Point> values() {
return points;
}

private static List<Point> generatePoints(int size, ConnectStrategy connectStrategy) {
List<Point> points = new ArrayList<>(size);
IntStream.range(0, size)
.forEach(i -> points.add(new Point(connectStrategy.connect(i, i != 0 && points.get(i - 1).connected()))));
return points;
}

public void validate() {
IntStream.range(0, points.size()-1)
.filter(this::continuousConnected)
.findFirst()
.ifPresent(i -> {
throw new IllegalArgumentException("각 포인트는 연속적으로 연결될 수 없음");
});
}
Comment on lines +31 to +38
Copy link
Member

Choose a reason for hiding this comment

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

Point가 좌우연결을 가진 구조라면 유효성 검증도 단순화할 수 있을 것 같아요

new Point(true, true); // 생성 불가


private boolean continuousConnected(int idx) {
return points.get(idx).connected() && points.get(idx+1).connected();
}

@Override
public boolean equals(Object o) {
if (o == null || getClass() != o.getClass()) return false;
Points points1 = (Points) o;
return Objects.equals(points, points1.points);
}

@Override
public int hashCode() {
return Objects.hashCode(points);
}
}
19 changes: 19 additions & 0 deletions src/main/java/ladder/view/InputView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package ladder.view;

import ladder.domain.People;

import java.util.Scanner;

public class InputView {
private final static Scanner scanner = new Scanner(System.in);

public static People inputPeople() {
System.out.println("참여할 사람 이름을 입력하세요. (이름은 쉼표(,)로 구분하세요)");
return new People(scanner.nextLine());
}

public static int inputLadderHeight() {
System.out.println("최대 사다리 높이는 몇인가요?");
return scanner.nextInt();
}
}
27 changes: 27 additions & 0 deletions src/main/java/ladder/view/ResultView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package ladder.view;

import ladder.domain.Ladder;
import ladder.domain.People;

public class ResultView {
public static void printLadder(People people, Ladder ladder) {
System.out.println("실행결과");
printPeople(people);
printLadder(ladder);
}

private static void printPeople(People people) {
people.values().forEach(v -> System.out.printf("%-6s", v.name()));
System.out.println();
}

private static void printLadder(Ladder ladder) {
ladder.lines().forEach(l -> {
l.points().values().forEach(p -> {
System.out.print("|");
System.out.printf("%5s", p.connected() ? "-----" : "");
});
System.out.println();
});
}
}
8 changes: 7 additions & 1 deletion src/main/java/nextstep/optional/ComputerStore.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import nextstep.optional.Computer.Soundcard;
import nextstep.optional.Computer.USB;

import java.util.Optional;

public class ComputerStore {
public static final String UNKNOWN_VERSION = "UNKNOWN";

Expand All @@ -21,6 +23,10 @@ public static String getVersion(Computer computer) {
}

public static String getVersionOptional(Computer computer) {
return null;
return Optional.ofNullable(computer)
.map(Computer::getSoundcard)
.map(Soundcard:: getUsb)
.map(USB::getVersion)
.orElse(UNKNOWN_VERSION);
}
}
3 changes: 1 addition & 2 deletions src/main/java/nextstep/optional/Users.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import java.util.Arrays;
import java.util.List;
import java.util.Optional;

public class Users {
static final User DEFAULT_USER = new User("codesquad", 100);
Expand All @@ -15,7 +14,7 @@ public class Users {

User getUser(String name) {
return users.stream()
.filter(user -> user.getName().equals(name))
.filter(user -> user.matchName(name))
.findFirst()
.orElse(DEFAULT_USER);
}
Expand Down
38 changes: 38 additions & 0 deletions src/test/java/ladder/LineTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package ladder;

import ladder.domain.Line;
import ladder.domain.Point;
import org.junit.jupiter.api.Test;

import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

public class LineTest {
Point notConnected = new Point(false);
Point connected = new Point(true);

@Test
void 라인_생성() {
Line line = new Line(4, (idx, preConnected) -> false);
assertThat(line).isEqualTo(new Line(List.of(notConnected, notConnected, notConnected, notConnected)));
}

@Test
void 처음에만_연결() {
Line line = new Line(4, (idx, preConnected) -> idx == 0);
assertThat(line).isEqualTo(new Line(List.of(connected, notConnected, notConnected, notConnected)));
}

@Test
void 홀수번째만_연결() {
Line line = new Line(4, (idx, preConnected) -> idx % 2 != 0);
assertThat(line).isEqualTo(new Line(List.of(notConnected, connected, notConnected, connected)));
}

@Test
void 연속적으로_연결되면_예외() {
assertThatThrownBy(() -> new Line(4, (idx, preConnected) -> true)).isInstanceOf(IllegalArgumentException.class);
Comment on lines +34 to +36
Copy link
Member

Choose a reason for hiding this comment

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

예외 테스트 👍
예외 메시지까지 함께 검증하면 좋을 것 같아요.

Point, Points, Ladder, Person 에 대한 단위 테스트도 추가하면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

보완해보겠습니다!

}
}
13 changes: 13 additions & 0 deletions src/test/java/ladder/PeopleTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package ladder;

import ladder.domain.People;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
public class PeopleTest {
@Test
void 이름_길이_예외() {
assertThatThrownBy(() -> new People("finnnnnn,foo"))
.isInstanceOf(IllegalArgumentException.class);
}
}