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 PR 요청드립니다. #2015

Open
wants to merge 8 commits into
base: yumble
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 @@ -23,4 +23,15 @@
* map(String::toLowerCase) 적용 -> forEach(System.out::println) 으로 출
* [X] Optional 실습 1 : Optional을 활용해 조건에 따른 반환
* [X] Optional 실습 2 : Optional에서 값을 반환
* [X] Optional 실습 3 : Optional에서 exception 처리
* [X] Optional 실습 3 : Optional에서 exception 처리

# STEP 2
* [X] 참여할 사람 명단 입력받기
* [X] 쉼표 구분자를 파싱하기
* [X] 최대 사다리 높이 입력받기
* [X] 실행결과 출력하기
* [X] 사다리 만들기
* [X] 사다리 한 줄(Line) 만들기
* [X] 사다리 다리 구성하기 (Bridge)
* [X] 캐시 전략 사용하기
* [X] 사다리 다리 리스트 구성하기
11 changes: 11 additions & 0 deletions src/main/java/nextstep/ladder/LadderApplication.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package nextstep.ladder;

import nextstep.ladder.controller.LadderController;

public class LadderApplication {

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

import nextstep.ladder.model.Ladder;
import nextstep.ladder.model.Lines;
import nextstep.ladder.model.LinesGenerator;
import nextstep.ladder.model.Participants;
import nextstep.ladder.view.InputView;
import nextstep.ladder.view.OutputView;

public class LadderController {

private final LinesGenerator linesGenerator = new LinesGenerator();

public void game() {
Participants participants = new Participants(InputView.inputGameParticipants());
int height = InputView.inputLadderHeight();
Lines lines = new Lines(linesGenerator.generatorLines(participants.getNumbersOfParticipants(), height));

Ladder ladder = new Ladder(participants, lines, height);
OutputView.printLadder(ladder);
}
}
41 changes: 41 additions & 0 deletions src/main/java/nextstep/ladder/model/Bridge.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package nextstep.ladder.model;

import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ThreadLocalRandom;

public class Bridge {
private final boolean value;

private static final Map<Boolean, Bridge> cacheBridge = new HashMap<>();

static {
cacheBridge.put(Boolean.TRUE, new Bridge(true));
cacheBridge.put(Boolean.FALSE, new Bridge(false));
}

private Bridge(boolean value) {
this.value = value;
}

public static Bridge of(boolean value) {
return cacheBridge.get(value);
}
Comment on lines +21 to +23

Choose a reason for hiding this comment

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

중요한 부분은 아니나 참고 부탁드려요. 이펙티브 자바에서 정적 팩터리에 네이밍을 아래와 같이 가이드하고 있습니다.

  • 매개변수가 하나일 때 from
  • 매개변수가 여러개 일 때 of


public boolean canCrossBridge() {
return value;
}

public Bridge next() {
if (this.value) {
return of(false);
}
return of(ThreadLocalRandom.current().nextBoolean());
}

public void compareToNextBridge(Bridge next) {

Choose a reason for hiding this comment

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

보통 compare는 비교 후 조건에 따라 True False를 내려주는 형태로 사용하시는 케이스가 많습니다.

만약 특정 조건을 만족하지 않으면 예외를 반환하는 메서드를 네이밍하시려는 경우 아래와 같은 형태가 좀 더 적절하지 않을까요?

  • requireXX
  • checkXX
  • validateXX

if (this.value && next.value) {
throw new IllegalArgumentException("연속해서 true인 Bridge가 있습니다.");
}
}
}
7 changes: 7 additions & 0 deletions src/main/java/nextstep/ladder/model/BridgesGenerator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package nextstep.ladder.model;

import java.util.List;

public interface BridgesGenerator {
List<Bridge> generateBridges(Integer numbersOfPeople);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package nextstep.ladder.model;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class BridgesGeneratorByCreatingCandidates implements BridgesGenerator {

Choose a reason for hiding this comment

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

이때, generator를 생성할 때, 값들을 검증해주어야할지 아니면 우선 Generator에서 값들을 만들고 그 값을 Line, Lines를 생성할때 생성자에서 검증해야할지 고민이었습니다. 제가 생각한 바로는, Generator에서 한다면 Generator에서 만든 List들은 확실히 검증된 값을 배출한다는 장점이 있지만, 다른 개발자가 Generator를 거치지 않고, new Line(), new Lines()를 호출해버린다면, 해당 객체는 유효성체크가 되지않는 객체이기에 문제가 있다고 판단했습니다.
하지만, 생성자에서 유효성체크를 한다면, Generator에서 만드는 값들은 미리 체크할 수가 없어 아쉬운 점이 있었던 것 같습니다. 이 부분에서 리뷰어님 생각이 궁금합니다..!

이건 생성을 위한 책임이 Generator와 각 클래스에 나누어져 있어서 발생하는 문제인 것 같습니다. 클래스나 메서드의 크기가 커지면 분리를 고민해봐야겠지만 Bridge나 Line에 대한 생성이나 제약 조건 검증 책임은 한 곳에 일관되게 있는게 좋을 것 같아요.

추가적으로, 위의 방법을 고민하면서, 다른 방법으로 생성자에서 Generator를 호출해서 값을 만들고, validation도 하면 되지 않을까라는 생각도 가지게 되었습니다. 서치를 하다보니, 생성자는 최소한의 일만 해야 테스트가 쉬울거라는 어떤 블로그의 글을 봐서 해당 방법은 이용하지않았습니다.

생성자에서 불필요하게 책임을 많이 가져가는게 걱정되시면 정적 팩터리 메서드를 만든 뒤 내부에서 제약 조건 검증 등을 모두 수행해도 괜찮을 것 같네요. 그 과정에서 정적 팩터리의 책임이 너무 커진다면 제약 조건 검증이나 기타 책임들만 떼서 별도의 클래스로 만들면 지금 말씀하시는 문제는 해결될 것 같아요.


private static List<List<Bridge>> candidates = new ArrayList<>();

static void generateLadders(int numbersOfPeople) {
List<List<Bridge>> result = new ArrayList<>(List.of(List.of(Bridge.of(false)), List.of(Bridge.of(true))));

Choose a reason for hiding this comment

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

사다리의 한 Row를 Line이라는 것으로 잘 정의하셨는데요, Bidge와 관련해선 List<List<<>> 형태로 정의되고 있어 복잡도 높고 가독성이 떨어지는 것 같습니다. 이와 관련해서도 Bridges와 같은 일급 컬렉션을 추출해 가독성을 높이고 책임을 옮겨볼 수 있을까요?


for (int i = 2; i < numbersOfPeople ; i++) {
result = dp(result);
}
Comment on lines +16 to +18

Choose a reason for hiding this comment

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

이쪽 로직이 다소 복잡한데요, 이 2가 어떤 의미를 갖는건지 처음 보는 사람도 바로 이해할 수 있도록 상수 등으로 추출하는건 어떨까요?


candidates = result;
}

static List<List<Bridge>> dp(List<List<Bridge>> result) {
return result.stream()
.flatMap(ladder -> {
List<Bridge> withFalse = new ArrayList<>(ladder);
withFalse.add(Bridge.of(false));
List<Bridge> withTrue = new ArrayList<>(ladder);
if (!ladder.get(ladder.size() - 1).canCrossBridge()) {
withTrue.add(Bridge.of(true));
return Stream.of(withFalse, withTrue);
}
return Stream.of(withFalse);
})
Comment on lines +25 to +34

Choose a reason for hiding this comment

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

규칙 1: 한 메서드에 오직 한 단계의 들여쓰기(indent)만 한다.

위의 객체지향 생활 체조 원칙을 어기고 있네요.

Stream의 연산 단계에서 이렇게 복잡한 연산이 적용되면 문제가 생겼을 때 확인이 어렵습니다. 메서드를 별도로 분리해 가독성을 높이거나 복잡도가 높아 개선이 어려운 경우라면 아예 Stream을 제거하는것도 방법일 것 같네요.

.collect(Collectors.toList());
}

@Override
public List<Bridge> generateBridges(Integer numbersOfPeople) {
if (candidates.isEmpty()) {
generateLadders(numbersOfPeople);
}
Collections.shuffle(candidates);
return candidates.get(0);
}



}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package nextstep.ladder.model;

import java.util.LinkedList;
import java.util.List;
import java.util.concurrent.ThreadLocalRandom;

public class BridgesGeneratorBySequentialRandom implements BridgesGenerator {

@Override
public List<Bridge> generateBridges(Integer numbersOfPeople) {
List<Bridge> bridges = new LinkedList<>();
Bridge currentBridge = Bridge.of(ThreadLocalRandom.current().nextBoolean());
for (int i = 0; i < numbersOfPeople - 1; i++) {
bridges.add(currentBridge);
currentBridge = currentBridge.next();
}
return bridges;
}
}
31 changes: 31 additions & 0 deletions src/main/java/nextstep/ladder/model/Ladder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package nextstep.ladder.model;

public class Ladder {

private static final int MIN_HEIGHT = 1;

private final Participants participants;
private final Lines lines;
private final Integer height;

public Ladder(Participants participants, Lines lines, Integer height) {
validateHeight(height);
this.participants = participants;
this.lines = lines;
this.height = height;
}

private void validateHeight(Integer height) {
if (height < MIN_HEIGHT) {
throw new IllegalArgumentException("높이는 최소 1 이상이어야 합니다.");
}
}

public Participants getParticipants() {
return participants;
}

public Lines getLines() {
return lines;
}
}
45 changes: 45 additions & 0 deletions src/main/java/nextstep/ladder/model/Line.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package nextstep.ladder.model;

import java.util.List;

public class Line {

private static final Integer MIN_NUMBERS_OF_PEOPLE = 2;

private final List<Bridge> bridges;

public Line(Integer numbersOfPeople, List<Bridge> bridges) {
validateLine(numbersOfPeople, bridges);
this.bridges = bridges;
}

private void validateLine(Integer numbersOfPeople, List<Bridge> bridges) {
validateBridgeCount(numbersOfPeople, bridges);
validateContinuousTrueBridge(bridges);
validateNumbersOfPeople(numbersOfPeople);
}

private void validateBridgeCount(Integer numbersOfPeople, List<Bridge> bridges) {
if (bridges.size() != numbersOfPeople - 1) {
throw new IllegalArgumentException("다리의 개수는 사람의 수보다 1 작아야합니다");
}
}

private void validateContinuousTrueBridge(List<Bridge> bridges) {
bridges.stream()
.reduce((prevBridge, currentBridge) -> {
prevBridge.compareToNextBridge(currentBridge);
return currentBridge;
});
}

private void validateNumbersOfPeople(Integer numbersOfPeople) {
if (numbersOfPeople < MIN_NUMBERS_OF_PEOPLE) {
throw new IllegalArgumentException("사람은 최소 2명 이상이어야 합니다.");
}
}

public List<Bridge> getBridges() {
return bridges;

Choose a reason for hiding this comment

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

List<Bidge>의 참조가 외부에 노출되면 컬렉션이 수정되거나 할 수 있지 않을까요?

스트림 toList 혹은 Collections의 unmodifible 관련 메서드를 활용해서 외부에서 참조를 수정할 수 없도록 하는건 어떨까요?

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

import java.util.List;

public class Lines {

private final List<Line> lines;

public Lines(List<Line> lines) {
this.lines = lines;
}

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

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

public class LinesGenerator {

private final BridgesGenerator bridgesGenerator = new BridgesGeneratorBySequentialRandom();

public List<Line> generatorLines(Integer numbersOfPeople, Integer height) {
return IntStream.range(0, height)
.mapToObj(index -> new Line(numbersOfPeople, bridgesGenerator.generateBridges(numbersOfPeople)))
.collect(Collectors.toList());
}
Comment on lines +7 to +15

Choose a reason for hiding this comment

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

이정도 복잡도라면 Lines 내부 정적 팩터리로 책임을 옮기는건 어떨까요? 관리할 클래스가 늘어나면 복잡도가 더 올라갈 것 같습니다.

}
24 changes: 24 additions & 0 deletions src/main/java/nextstep/ladder/model/Participant.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package nextstep.ladder.model;

public class Participant {

private static final Integer MIN_NAME_LENGTH = 1;
private static final Integer MAX_NAME_LENGTH = 5;

private final String name;

public Participant(String name) {
validateNameLength(name);
this.name = name;
}

private void validateNameLength(String name) {
if (name.length() < MIN_NAME_LENGTH || name.length() > MAX_NAME_LENGTH) {
throw new IllegalArgumentException("0글자 이하이거나 5글자를 초과하는 참가자 이름이 포함되어 있습니다.");
}
}

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

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

public class Participants {

private static final String DELIMITER = ",";

private final List<Participant> participants;

public Participants(String inputParticipants) {
this.participants = createParticipants(parseInputParticipants(inputParticipants));
}

private List<String> parseInputParticipants(String inputParticipants) {
return List.of(inputParticipants.split(DELIMITER));
}

private List<Participant> createParticipants(List<String> inputParticipants) {
return inputParticipants.stream()
.map(Participant::new)
.collect(Collectors.toList());
}

public Integer getNumbersOfParticipants() {
return participants.size();
}

public List<Participant> getParticipants() {
return participants;
}
}
17 changes: 17 additions & 0 deletions src/main/java/nextstep/ladder/view/InputView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package nextstep.ladder.view;

import java.util.Scanner;

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

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

public static int inputLadderHeight() {
System.out.println("최대 사다리 높이는 몇 개인가요?");
return Integer.parseInt(scanner.nextLine());
}
}
Loading