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

2단계 - 사다리(생성) #2238

Open
wants to merge 12 commits into
base: owljoa
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
13 changes: 12 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,15 @@
* 모든 피드백을 완료하면 다음 단계를 도전하고 앞의 과정을 반복한다.

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

## 기능 목록 (TODO)

### 2단계 - 사다리(생성)

- [x] 참여할 사람 이름 입력받기 - 콤마(,)로 구분
- [x] 최대 사다리 높이 입력받기
- [x] 참여자 추가 - 이름은 최대 5글자까지 허용
- [x] 가로선을 무작위로 생성 - 사다리의 가로선이 연속되면 안됨
- [x] 사다리 생성 - 사다리 최대 높이만큼 무작위 가로선 생성 반복
- [x] 사다리 출력 - 사람 이름도 함께 출력
11 changes: 4 additions & 7 deletions src/main/java/nextstep/fp/Lambda.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,9 @@ public static int sumAllOverThree(List<Integer> numbers) {
}

private static int sumAll(List<Integer> numbers, SumCondition sumCondition) {
int total = 0;
for (int number : numbers) {
if (sumCondition.isSummable(number)) {
total += number;
}
}
return total;
return numbers.stream()
.filter(sumCondition::isSummable)
.mapToInt(Integer::valueOf)
.sum();
}
}
9 changes: 9 additions & 0 deletions src/main/java/nextstep/ladder/Main.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package nextstep.ladder;

import nextstep.ladder.controller.LadderController;

public class Main {
public static void main(String[] args) {
LadderController.startLadderGame();
}
}
19 changes: 19 additions & 0 deletions src/main/java/nextstep/ladder/controller/LadderController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package nextstep.ladder.controller;

import nextstep.ladder.domain.Ladder;
import nextstep.ladder.domain.User;
import nextstep.ladder.view.InputView;
import nextstep.ladder.view.ResultView;

import java.util.List;

public class LadderController {
public static void startLadderGame() {
List<User> users = InputView.inputUsers();
int ladderMaxHeight = InputView.inputLadderMaxHeight();

Ladder ladder = new Ladder(users.size(), ladderMaxHeight);

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

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

public class HorizontalLine {
private final List<Boolean> hasLineOrNotList;
Copy link
Member

Choose a reason for hiding this comment

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

변수명이 쉽지 않네요 😅
조금 더 간단하게 표현하면 어떨까요?


private HorizontalLine(List<Boolean> hasLineOrNotList) {
this.hasLineOrNotList = hasLineOrNotList;
}
Comment on lines +10 to +12
Copy link
Member

Choose a reason for hiding this comment

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

가로라인은 겹칠 수 없다는 제약을 지키기 위해 할당하기 전에 검증하면 어떨까요?


public HorizontalLine(int userCount) {
this(generateLines(userCount));
}
Comment on lines +14 to +16
Copy link
Member

Choose a reason for hiding this comment

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

관례적으로 부 생성자는 주 생성자보다 상위에 위치합니다.
로직이 필요하다면 생성자가 아닌 정적 팩토리 메서드를 활용하면 어떨까요?


private static List<Boolean> generateLines(int userCount) {
Random random = new Random();
Copy link
Member

Choose a reason for hiding this comment

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

이전 단계에서도 경험하셨겠지만 random은 테스트하기 어려운 구조로 만들게 됩니다.
랜덤에 직접 의존하지 않는 구조로 변경해 보시면 좋을 것 같아요.

List<Boolean> lines = new ArrayList<>();
Boolean isPreviousLineExist = null;
int maxBooleanCountForLine = userCount - 1;

while (lines.size() < maxBooleanCountForLine) {
boolean isLineExist = generateIsLineExist(random, isPreviousLineExist);
lines.add(isLineExist);
isPreviousLineExist = isLineExist;
}

return lines;
}

private static boolean generateIsLineExist(Random random, Boolean isPreviousLineExist) {
boolean isLineExist = random.nextBoolean();
while (isPreviousLineExist != null && isLineExist == isPreviousLineExist) {
isLineExist = random.nextBoolean();
}
return isLineExist;
}

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

public Boolean hasLineOnIndex(int index) {
return hasLineOrNotList.get(index);
}
}
34 changes: 34 additions & 0 deletions src/main/java/nextstep/ladder/domain/Ladder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package nextstep.ladder.domain;

import java.util.LinkedList;
import java.util.List;

public class Ladder {
private final List<HorizontalLine> horizontalLines;

public Ladder(List<HorizontalLine> horizontalLines) {
this.horizontalLines = horizontalLines;
}

public Ladder(int userCount, int ladderMaxHeight) {
this(generate(userCount, ladderMaxHeight));
}

private static List<HorizontalLine> generate(int userCount, int ladderMaxHeight) {
List<HorizontalLine> horizontalLines = new LinkedList<>();

for (int i = 0; i < ladderMaxHeight; i++) {
horizontalLines.add(new HorizontalLine(userCount));
}

return horizontalLines;
}

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

public HorizontalLine getByIndex(int index) {
return horizontalLines.get(index);
}
}
19 changes: 19 additions & 0 deletions src/main/java/nextstep/ladder/domain/User.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package nextstep.ladder.domain;

public class User {
private static final int MAX_NAME_LENGTH = 5;

private final String name;

public User(String name) {
if (name == null || name.length() > MAX_NAME_LENGTH) {
throw new IllegalArgumentException("참여자의 이름은 최대 5글자입니다");
Comment on lines +9 to +10
Copy link
Member

Choose a reason for hiding this comment

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

null인 경우는 메시지가 달라야 하지 않을까요?
빈 값인 경우에 대한 예외처리도 추가하면 좋을 것 같아요.

}

this.name = name;
}

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

import nextstep.ladder.domain.User;

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

public class InputView {
private static final String DELIMITER = ",";

public static List<User> inputUsers() {
System.out.println("참여할 사람 이름을 입력하세요. (이름은 쉼표(,)로 구분하세요)");
String userNames = getInputString();
return parseUsers(userNames);
}

public static int inputLadderMaxHeight() {
System.out.println("최대 사다리 높이는 몇 개인가요?");
return getInputInteger();
}

private static List<User> parseUsers(String userNames) {
return Arrays.stream(userNames.split(DELIMITER))
.map(User::new)
.collect(Collectors.toList());
}

private static int getInputInteger() {
try {
Scanner scanner = new Scanner(System.in);
return scanner.nextInt();
} catch (Exception e) {
return 0;
}
}
Comment on lines +30 to +37
Copy link
Member

Choose a reason for hiding this comment

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

임의의 값으로 리턴하기 보다 재입력을 받아 정상 동작하도록 유도하면 어떨까요?


private static String getInputString() {
Scanner scanner = new Scanner(System.in);
return scanner.nextLine();
}
}
41 changes: 41 additions & 0 deletions src/main/java/nextstep/ladder/view/ResultView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package nextstep.ladder.view;

import nextstep.ladder.domain.HorizontalLine;
import nextstep.ladder.domain.Ladder;
import nextstep.ladder.domain.User;

import java.util.List;

public class ResultView {
public static void printResult(Ladder ladder, List<User> users) {
System.out.println("실행결과");
users.forEach(user -> System.out.printf("%-6s", user.getName()));
System.out.println();
printLadder(ladder);
}

private static void printLadder(Ladder ladder) {
for (int index = 0; index < ladder.size(); index++) {
System.out.print("|");
HorizontalLine horizontalLine = ladder.getByIndex(index);
printHorizontalLine(horizontalLine);
System.out.println();
}
}

private static void printHorizontalLine(HorizontalLine horizontalLine) {
for (int i = 0; i < horizontalLine.size(); i++) {
Boolean hasLineOnIndex = horizontalLine.hasLineOnIndex(i);
printHorizontalLineOrBlank(hasLineOnIndex);
System.out.print("|");
}
Comment on lines +27 to +31
Copy link
Member

Choose a reason for hiding this comment

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

size(), hasLineOnIndex() 는 출력에 필요한 메서드라고 생각되는데요.
스트림, 람다 미션이니 index로 접근하기 보다 불변 컬렉션을 리턴해서 스트림을 최대한 활용해 보시면 좋을 것 같아요.

}

private static void printHorizontalLineOrBlank(boolean hasLine) {
if (hasLine) {
System.out.print("-----");
return;
}
System.out.print(" ");
}
}
4 changes: 2 additions & 2 deletions src/main/java/nextstep/optional/ComputerStore.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ public static String getVersion(Computer computer) {
}

public static String getVersionOptional(Computer computer) {
Optional<Computer> computerOptional = Optional.ofNullable(computer);
return computerOptional.map(Computer::getSoundcard)
return Optional.ofNullable(computer)
.map(Computer::getSoundcard)
.map(Soundcard::getUsb)
.map(USB::getVersion)
.orElse(UNKNOWN_VERSION);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/nextstep/optional/Users.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public class Users {

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

import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;

public class HorizontalLineTest {
@Test
void 생성된_가로선은_연속되지_않아야_한다() {
// given
int userCount = 4;

// when
HorizontalLine horizontalLine = new HorizontalLine(userCount);

// then
for (int i = 0; i < horizontalLine.size() - 1; i++) {
boolean currentGeneratedLine = horizontalLine.hasLineOnIndex(i);
boolean nextGeneratedLine = horizontalLine.hasLineOnIndex(i + 1);
Assertions.assertThat(currentGeneratedLine).isNotEqualTo(nextGeneratedLine);
}
Comment on lines +8 to +20
Copy link
Member

Choose a reason for hiding this comment

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

모든 값을 꺼내어서 결과를 검증하다보니 테스트 코드가 복잡해질 수 밖에 없을 것 같아요.
'연속된 가로선을 가질 수 없다'는 책임을 생성 시점에 검증하면 어떨까요?
new HorizontalLine(List.of(true, true)); // exception throw
혹은 위 책임을 수행할 별도의 객체를 도출해 보셔도 좋을 것 같아요.

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

import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;

public class LadderTest {

@Test
void 사다리_생성() {
int userCount = 4;
int ladderMaxHeight = 5;

// when
Ladder ladder = new Ladder(userCount, ladderMaxHeight);

// then
Assertions.assertThat(ladder.size()).isEqualTo(ladderMaxHeight);
Comment on lines +13 to +17
Copy link
Member

Choose a reason for hiding this comment

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

size 검증만으로 정상 사다리라고 보기 어렵지 않을까요.
userCount는 어떤 역할을 하는지 테스트 코드를 아무리 봐도 알 수가 없는데요.
사다리를 생성한 다음 예상되는 사다리 객체를 하드코딩해서 객체끼리 비교하면 어떨까요?
생성 전략을 외부에서 주입한다면 원하는 형태의 사다리를 만들 수 있으니 객체간의 비교도 가능할 것 같아요.

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

import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;

public class UserTest {
@Test
void 참여자의_이름은_최대_5글자() {
String name = "michael";

Assertions.assertThatExceptionOfType(IllegalArgumentException.class)
.isThrownBy(() -> new User(name))
.withMessage("참여자의 이름은 최대 5글자입니다");

}
}