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 - 사다리(생성) #1438

Open
wants to merge 2 commits into
base: 0319easy
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,11 @@
* 모든 피드백을 완료하면 다음 단계를 도전하고 앞의 과정을 반복한다.

## 온라인 코드 리뷰 과정
* [텍스트와 이미지로 살펴보는 온라인 코드 리뷰 과정](https://github.com/nextstep-step/nextstep-docs/tree/master/codereview)
* [텍스트와 이미지로 살펴보는 온라인 코드 리뷰 과정](https://github.com/nextstep-step/nextstep-docs/tree/master/codereview)

## step1 - 기능요구사항
* 사다리 게임에 참여하는 사람에 이름을 최대5글자까지 부여할 수 있다. 사다리를 출력할 때 사람 이름도 같이 출력한다.
* 사람 이름은 쉼표(,)를 기준으로 구분한다.
* 사람 이름을 5자 기준으로 출력하기 때문에 사다리 폭도 넓어져야 한다.
* 사다리 타기가 정상적으로 동작하려면 라인이 겹치지 않도록 해야 한다.
* |-----|-----| 모양과 같이 가로 라인이 겹치는 경우 어느 방향으로 이동할지 결정할 수 없다.
18 changes: 18 additions & 0 deletions src/main/java/ladder/LadderMain.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package ladder;

import java.util.List;
import ladder.domain.Ladder;
import ladder.ui.LadderScanner;
import ladder.ui.ResultView;

public class LadderMain {
public static void main(String[] args) {
String input = LadderScanner.insertParticipant();
List<String> participants = List.of(input.split(","));
Copy link

Choose a reason for hiding this comment

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

참가자 객체를 만들고 해당 객체에서 이름 길이에 대한 검증을 해보는건 어떨까요?

int height = LadderScanner.insertLadderHeight();

Ladder ladder = new Ladder(participants.size() - 1, height);
Copy link

Choose a reason for hiding this comment

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

participants.size() - 1 에서 -1 은 도메인 로직이라고 생각되는데요.
해당 로직은 도메인으로 이동시키는건 어떨까요?


ResultView.printResult(participants, ladder);
}
}
18 changes: 18 additions & 0 deletions src/main/java/ladder/domain/Ladder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package ladder.domain;

import java.util.ArrayList;
import java.util.List;

public class Ladder {
private List<Line> lines = new ArrayList<>();

public Ladder(int width, int height) {
Copy link

Choose a reason for hiding this comment

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

width, height은 1이상 이어야 하지 않나요?
검증이 필요하지 않을까요?

for (int i = 0; i < height; ++i) {
lines.add(Line.of(width));
}
}

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

import java.util.ArrayList;
import java.util.List;

public class Line {
Copy link

Choose a reason for hiding this comment

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

일급컬렉션 사용 좋습니다 👍

private List<Point> points;

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

static Line of(int size) {
Copy link

Choose a reason for hiding this comment

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

정적 팩토리 메소드를 사용해주셨네요 👍👍

추가적으로 전체적으로 접근제한자 default, public 를 혼용해서 사용하셨는데 이유가 있을까요?? :)

List<Point> points = new ArrayList<>();

points.add(new Point());
for (int i = 1; i < size; i++) {
points.add(points.get(i - 1).nextPoint());
}

return new Line(points);
}

public List<Point> getPoints() {
Copy link

Choose a reason for hiding this comment

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

내부 상태인 List를 그대로 반환하면
외부에서 List에 값을 추가하거나 제거하는 등 변형할 수 있는데요
이를 방지하기 위해선 어떻게 해야할까요?

return points;
}
}
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.Random;

public class Point {
private final boolean exist;

Point() {
this(new Random().nextBoolean());
Copy link

Choose a reason for hiding this comment

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

랜덤한 값이 있다면 테스트할 수 없고
Point를 테스트할 수 없으니 하니 상위 도메인 객체인 Line과 Ladder도 테스트할 수 없는 구조인데요 🥲

전략 패턴을 사용해서 테스트 가능한 구조로 변경해보는건 어떨까요? :)

}

Point(boolean exist) {
this.exist = exist;
}

public boolean isExist() {
return exist;
}

Point nextPoint() {
if (isExist()) {
Copy link

Choose a reason for hiding this comment

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

멤버 변수에 대한 접근인데 불필요한 getter 사용아닐까요?

return new Point(false);
}

return new Point();
}
}
11 changes: 11 additions & 0 deletions src/main/java/ladder/ui/InputView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package ladder.ui;

public class InputView {
public static void printInsertParticipantPhrase() {
System.out.println("참여할 사람 이름을 입력하세요. (이름은 쉼표(,)로 구분하세요)");
}

public static void printInsertLadderHeightPhrase() {
System.out.println("최대 사다리 높이는 몇 개인가요?");
}
}
17 changes: 17 additions & 0 deletions src/main/java/ladder/ui/LadderScanner.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package ladder.ui;

import java.util.Scanner;

public class LadderScanner {
Copy link

Choose a reason for hiding this comment

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

LadderScanner 가 하는 일을 InputView에서 해도 좋을 것 같은데 별도의 클래스로 분리해주신 이유가 있나요?

public static Scanner scanner = new Scanner(System.in);
Copy link

Choose a reason for hiding this comment

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

자바 상수 컨벤션에 맞게 수정해볼까요?
접근제한자는 private이 적절하지 않을까요?


public static String insertParticipant() {
InputView.printInsertParticipantPhrase();
return scanner.nextLine();
}

public static int insertLadderHeight() {
InputView.printInsertLadderHeightPhrase();
return Integer.parseInt(scanner.nextLine());
}
}
34 changes: 34 additions & 0 deletions src/main/java/ladder/ui/ResultView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package ladder.ui;

import java.util.List;
import ladder.domain.Ladder;
import ladder.domain.Line;
import ladder.domain.Point;

public class ResultView {
public static void printResult(List<String> participants, Ladder ladder) {
participants.forEach(it -> {
it = " " + it;
it = it.substring(it.length() - 6);
System.out.print(it);
});
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.

스트림을 활용해주셨네요 👍

Suggested change
participants.forEach(it -> {
it = " " + it;
it = it.substring(it.length() - 6);
System.out.print(it);
});
participants.forEach(ResultView::printParticipant);

11 ~ 13 라인을 메소드로 추출한다면 좀 더 간결한 코드를 작성할 수 있을 것 같아요 :)

System.out.println();

List<Line> lines = ladder.getLines();

lines.forEach(it -> printLine(it.getPoints()));
}

private static void printLine(List<Point> points) {
System.out.print(" ");
points.forEach(it -> {
Copy link

Choose a reason for hiding this comment

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

it 대신 적절하게 point 로 네이밍을 변경하는 것도 좋을 것 같아요 :)

if (it.isExist()) {
System.out.print("|-----");
return;
}

System.out.print("| ");
});
System.out.println("|");
}
}
7 changes: 4 additions & 3 deletions src/main/java/nextstep/optional/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,10 @@ public static boolean ageIsInRange1(User user) {

public static boolean ageIsInRange2(User user) {
return Optional.ofNullable(user)
.filter(o -> o.age != null)
.filter(o -> o.age >= 30)
.filter(o -> o.age <= 45)
.map(User::getAge)
.filter(o -> o != null)
.filter(o -> o >= 30)
.filter(o -> o <= 45)
.isPresent();
}

Expand Down