-
Notifications
You must be signed in to change notification settings - Fork 709
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
base: yumble
Are you sure you want to change the base?
Step2 PR 요청드립니다. #2015
Changes from all commits
78d3c9e
844fcd3
51d6378
01128ff
4e7450f
16783e9
e954912
931105b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(); | ||
} | ||
} |
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); | ||
} | ||
} |
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); | ||
} | ||
|
||
public boolean canCrossBridge() { | ||
return value; | ||
} | ||
|
||
public Bridge next() { | ||
if (this.value) { | ||
return of(false); | ||
} | ||
return of(ThreadLocalRandom.current().nextBoolean()); | ||
} | ||
|
||
public void compareToNextBridge(Bridge next) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 보통 compare는 비교 후 조건에 따라 True False를 내려주는 형태로 사용하시는 케이스가 많습니다. 만약 특정 조건을 만족하지 않으면 예외를 반환하는 메서드를 네이밍하시려는 경우 아래와 같은 형태가 좀 더 적절하지 않을까요?
|
||
if (this.value && next.value) { | ||
throw new IllegalArgumentException("연속해서 true인 Bridge가 있습니다."); | ||
} | ||
} | ||
} |
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이건 생성을 위한 책임이 Generator와 각 클래스에 나누어져 있어서 발생하는 문제인 것 같습니다. 클래스나 메서드의 크기가 커지면 분리를 고민해봐야겠지만 Bridge나 Line에 대한 생성이나 제약 조건 검증 책임은 한 곳에 일관되게 있는게 좋을 것 같아요.
생성자에서 불필요하게 책임을 많이 가져가는게 걱정되시면 정적 팩터리 메서드를 만든 뒤 내부에서 제약 조건 검증 등을 모두 수행해도 괜찮을 것 같네요. 그 과정에서 정적 팩터리의 책임이 너무 커진다면 제약 조건 검증이나 기타 책임들만 떼서 별도의 클래스로 만들면 지금 말씀하시는 문제는 해결될 것 같아요. |
||
|
||
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)))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 사다리의 한 Row를 Line이라는 것으로 잘 정의하셨는데요, Bidge와 관련해선 |
||
|
||
for (int i = 2; i < numbersOfPeople ; i++) { | ||
result = dp(result); | ||
} | ||
Comment on lines
+16
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
위의 객체지향 생활 체조 원칙을 어기고 있네요. 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; | ||
} | ||
} |
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; | ||
} | ||
} |
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
스트림 toList 혹은 Collections의 unmodifible 관련 메서드를 활용해서 외부에서 참조를 수정할 수 없도록 하는건 어떨까요? |
||
} | ||
} |
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; | ||
} | ||
} |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이정도 복잡도라면 Lines 내부 정적 팩터리로 책임을 옮기는건 어떨까요? 관리할 클래스가 늘어나면 복잡도가 더 올라갈 것 같습니다. |
||
} |
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; | ||
} | ||
} |
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; | ||
} | ||
} |
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()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중요한 부분은 아니나 참고 부탁드려요. 이펙티브 자바에서 정적 팩터리에 네이밍을 아래와 같이 가이드하고 있습니다.