Skip to content

[LBP] 윤준석 사다리 1단계 제출합니다. #39

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 2 commits into
base: junseok0304
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
3 changes: 3 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ dependencies {
testImplementation platform('org.assertj:assertj-bom:3.25.1')
testImplementation('org.junit.jupiter:junit-jupiter')
testImplementation('org.assertj:assertj-core')
implementation 'ch.qos.logback:logback-classic:1.5.6'
implementation 'ch.qos.logback:logback-core:1.5.6'
implementation 'org.slf4j:slf4j-api:2.1.0-alpha1'
}

test {
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/Application.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import model.Ladder;
import utils.ExceptionHandler;
import view.LadderView;

public class Application {
public static void main(String[] args) {
try {
Ladder ladder = Ladder.of(4, 4);
LadderView.printLadder(ladder);
} catch (Exception e) {
ExceptionHandler.handleException(e);
}
}
}
17 changes: 17 additions & 0 deletions src/main/java/model/Ladder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package model;

public class Ladder {
private final Lines lines;

public Ladder(Lines lines) {
this.lines = lines;
}

public static Ladder of(int width, int height) {
return new Ladder(LadderGenerator.generate(width, height));
}
Comment on lines +3 to +12

Choose a reason for hiding this comment

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

Ladder는 생성자로도 생성이 가능하고 정적 팩터리 메서드 of()로도 생성이 가능한 것이 의도된 것인가요?

Copy link
Author

Choose a reason for hiding this comment

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

사다리 생성과 커스텀 생성이 둘 다 가능한 의도된 로직이나 아직 제대로 활용은 못한 것 같습니다!


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

public class LadderGenerator {
public static Lines generate(int width, int height) {
return LineGenerator.generate(width, height);
}
}
Comment on lines +3 to +7

Choose a reason for hiding this comment

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

LadderGenerator.generate()의 반환 타입은 Ladder라고 기대할 것 같은데 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

맞습니다,, 래더를 반환할 것 같지만 라인스를 반환 하므로
반환타입을 변경하거나, 직접 호출해주는 방법으로 고치는 것이 좋겠습니다.

33 changes: 33 additions & 0 deletions src/main/java/model/LadderValidator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package model;

import java.util.*;
import java.util.stream.*;

public class LadderValidator {
private static final Random RANDOM = new Random();

public static void validate(List<Line> lines, int width) {
IntStream.rangeClosed(0, width)
.forEach(i -> validateColumn(lines, i, width));
}

private static void validateColumn(List<Line> lines, int col, int width) {
boolean emptyColumn = lines.stream().noneMatch(line -> hasBridgeAt(line, col, width));
if (emptyColumn) {
connect(lines.get(RANDOM.nextInt(lines.size())), col, width);
}
}

private static boolean hasBridgeAt(Line line, int col, int width) {
if (col == 0) return line.hasBridgeAt(col);
if (col == width) return line.hasBridgeAt(col - 1);
return line.hasBridgeAt(col - 1) || line.hasBridgeAt(col);
}


private static void connect(Line line, int col, int width) {
if (col == width) line.setBridgeAt(col - 1);
else line.setBridgeAt(col);
}

}
Comment on lines +31 to +33

Choose a reason for hiding this comment

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

클래스 내의 마지막 멤버와 클래스를 닫는 괄호 사이에 줄바꿈이 있을 때도 있고 없을 때도 있는 것 같아요.
팀 내 규칙을 만들고 일관성을 지키면 좋을 것 같아요! 😄

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분을 미처 확인하지 못한 것 같습니다!
다음 번 커밋때에는 조금 더 집중해서 확인하겠습니다.

23 changes: 23 additions & 0 deletions src/main/java/model/Line.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package model;

import java.util.*;

public class Line {
private final List<Boolean> points;

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

public List<Boolean> getPoints() {
return points;
}
Comment on lines +12 to +14

Choose a reason for hiding this comment

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

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 List getPoints() {
return new ArrayList<>(points);
}

Choose a reason for hiding this comment

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

좋습니다! 이제 외부로 반환된 리스트가 변경되더라도 Line의 원본 데이터에는 변경이 전파되지 않겠네요!

그렇다면 조금 더 나아가서 아래와 같은 경우가 생긴다해도 괜찮을까요?

List<Boolean> points = line.getPoints(); // 복사본을 반환
// 어떤 개발자가 반환된 points를 변경함
points.add(true);

이때도 points가 line이 반환하려는 값을 나타낸다고 할 수 있을까요?
변경이 전파되지는 않지만, line이 가진 정보를 사용하려고 할 때(출력 등) 어디선가 변경된다면 어떻게 될까요?

변경 전파를 막는 것과 함께 불변까지 가져간다면 더욱 안전한 데이터 전달이 될 것 같은데 어떻게 생각하시나요?

변경 전파를 막는 것과 더불어 Line이 반환하려는 값이 훼손되지 않게 한다면 더욱 안전할 것 같다는 의견입니다!


public boolean hasBridgeAt(int index) {
return points.get(index);
}

public void setBridgeAt(int index) {
points.set(index, true);
}
}
27 changes: 27 additions & 0 deletions src/main/java/model/LineGenerator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package model;

import java.util.*;

public class LineGenerator {

public static Lines generate(int width, int height) {
List<Set<Integer>> reserved = ReservedPositionGenerator.generate(width - 1, height);
List<Line> lines = new ArrayList<>();

for (int row = 0; row < height; row++) {
Line previous = getPreviousLine(lines, row);
Line line = SingleLineGenerator.generate(width - 1, reserved.get(row), previous);
lines.add(line);
}

LadderValidator.validate(lines, width - 1);
return new Lines(lines);
}

private static Line getPreviousLine(List<Line> lines, int row) {
if (row == 0) {
return null;
}
return lines.get(row - 1);
}
}
15 changes: 15 additions & 0 deletions src/main/java/model/Lines.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package 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;
}
}
Comment on lines +5 to +15

Choose a reason for hiding this comment

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

Lines | Line의 일급 컬렉션

일급 컬렉션이라고 말씀하셨는데,
Lines는 단순히 List을 가지면서 반환하는 책임만 가지도록 설계된 것이 맞는건가요?

준석님이 생각하시는 일급 컬렉션이란 무엇인가요?

Copy link
Author

Choose a reason for hiding this comment

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

지적해주신대로, 일급 컬렉션의 특징을 가지지 못하고 있는 것 같습니다.
(데이터의 무결성 보장이나 컬렉션과 관련된 비즈니스 로직을 수행하고 있지 않음)
일급컬렉션의 정의에 대해서 다시 한 번 점검해보겠습니다.

19 changes: 19 additions & 0 deletions src/main/java/model/ReservedPositionGenerator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package model;

import java.util.*;
import java.util.stream.*;

public class ReservedPositionGenerator {
private static final Random RANDOM = new Random();

public static List<Set<Integer>> generate(int width, int height) {
List<Set<Integer>> reserved = IntStream.range(0, height)
.mapToObj(i -> new HashSet<Integer>())
.collect(Collectors.toList());

IntStream.range(0, width).forEach(i ->
reserved.get(RANDOM.nextInt(height)).add(i));

return reserved;
}
Comment on lines +9 to +18

Choose a reason for hiding this comment

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

IntStream을 사용하신 특별한 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

가독성을 위해서 사용했던 것 같습니다. for 루프를 이용해도 문제없을 것 같습니다.

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

import java.util.*;
import java.util.stream.*;

public class SingleLineGenerator {
private static final Random RANDOM = new Random();

public static Line generate(int width, Set<Integer> reserved, Line prev) {
List<Boolean> points = new ArrayList<>(Collections.nCopies(width, false));

reserved.forEach(i -> {
if (isNotOverlap(points, prev, i)) {
points.set(i, true);
}
});

IntStream.range(0, width).forEach(i -> {
if (!points.get(i) && isNotOverlap(points, prev, i)) {
points.set(i, RANDOM.nextBoolean());
}
});
Comment on lines +12 to +22

Choose a reason for hiding this comment

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

현재 코드에서는 Stream의 forEach() 내부에서 리스트(points)의 상태를 변경하고 있어요.
하지만 스트림 연산은 원칙적으로 불변성을 유지하는 것을 권장해요.
(특히 병렬 스트림을 사용할 경우 예상치 못한 동작이 발생할 수 있어요.)

forEach() 내부에서 points.set(i, true); 또는 points.set(i, RANDOM.nextBoolean());처럼
리스트의 상태를 직접 변경하는 대신, 새로운 리스트를 생성하여 변화를 적용하는 방식이 더 안전하고 가독성이 좋을 것 같아요.

아래 공식 문서에서도 스트림 내 부작용(Side Effects)은 피해야 한다고 명시되어 있어요 😄
🔗 Java 공식 문서 - StreamOps: Side Effects

Choose a reason for hiding this comment

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

추가로 어떤 작업을 하는지 이해하기 어려웠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

스트림의 사용 원칙을 위배한 코드가 되었군요 ㅜㅜ
map을 사용해 불변성을 유지하면서 해결 할 수 있을 것 같습니다!


ensureOneBridge(points, prev);
return new Line(points);
}

private static boolean isNotOverlap(List<Boolean> points, Line prev, int i) {
if (prev != null && prev.hasBridgeAt(i)) return false;
if (i > 0 && points.get(i - 1)) return false;
return true;
}
Comment on lines +28 to +32

Choose a reason for hiding this comment

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

라인 수가 1줄이라서 {}를 안쓰신 건가요?

Copy link
Author

Choose a reason for hiding this comment

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

그렇습니다! { }를 사용하지 않았을 때의 side effect가 있을까요?

Choose a reason for hiding this comment

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

성능 측면에선 없겠지만, 읽을 때 일관성이 떨어진다는 느낌을 받았어요.

저는 꼭 {}를 추가해요. 만약 처음에 작성하지 않는다면,
추후 로직이 복잡해지는 경우(1줄에서 3줄되는 예시)에는 다시 중괄호를 추가해줘야 하기도 하고,
2줄이었다가 1줄 된다고 가정하면 그때 또 다시 일관성을 위해 중괄호를 지워야 하는 과정이 유지보수하기 어려운 규칙이라고 생각해요.


private static void ensureOneBridge(List<Boolean> points, Line prev) {
if (points.contains(true)) return;

IntStream.range(0, points.size())
.filter(i -> canSetBridge(points, prev, i))
.findFirst()
.ifPresent(i -> points.set(i, true));
}

private static boolean canSetBridge(List<Boolean> points, Line prev, int index) {
return (prev == null || !prev.hasBridgeAt(index))
&& (index == 0 || !points.get(index - 1));
}
}
12 changes: 12 additions & 0 deletions src/main/java/utils/ExceptionHandler.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package utils;

import org.slf4j.*;

public class ExceptionHandler {
private static final Logger logger = LoggerFactory.getLogger(ExceptionHandler.class);

public static void handleException(Exception e) {
System.err.println("시스템 오류 : " + e.getMessage());
logger.error("시스템 오류 : ", e);
}
}
31 changes: 31 additions & 0 deletions src/main/java/view/LadderView.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package view;

import model.*;

public class LadderView {
private static final String BRIDGE = "-----|";
private static final String SPACE = " |";
private static final String BAR = "|";

public static void printLadder(Ladder ladder) {
printLines(ladder.getLines());
}

private static void printLines(Lines lines) {
lines.getLines().forEach(LadderView::printLine);
}

private static void printLine(Line line) {
System.out.print(BAR);
line.getPoints().forEach(LadderView::printPoint);
System.out.println();
}

private static void printPoint(Boolean point) {
if (point) {
System.out.print(BRIDGE);
return;
}
System.out.print(SPACE);
}
}