-
Notifications
You must be signed in to change notification settings - Fork 33
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
[LBP] 박세은 사다리 1단계 제출합니다. #36
base: seun0123
Are you sure you want to change the base?
Conversation
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.
사다리 1단계 미션 수고하셨어요 세은😁😁
사다리 미션이 많이 어려웠던 것 같아요ㅠㅠ
저도 처음에 정말 많이 고생했던 기억이 있어요
- 100번 실행중 약 2-3회 연속된 가로줄이 발생하는 상황은 제가 명시적으로 코드를 보고 바로 파악하기 어려운 문제인 것 같아요, 전반적인 코드의 구조를 피드백을 통해서 수정하면 아마 깊게 숨어있는 정답을 찾기 쉬울거라고 생각해요
- 코멘트에 남겨두었으니 확인해보시면 좋을 것 같아요
- 요구사항에 대한 부분은 코멘트에 남겨두었어요
사다리 1단계 미션 고생 많이하셨어요
아마 많은 리팩토링에는 많은 시간이 소요될 것 같아요 🥲
힘들겠지만 마지막까지 집중 놓치지 않고 완주했으면 좋겠어요! 🔥🔥
늦게 리팩토링을 완료하게 되어 죄송합니다. 🙇🙇 |
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.
사다리 1단계 미션 고생 많았어요!
나머지 단계도 수행하고 있죠?? 받은 리뷰를 바탕으로 2,3,4,5단계도 개선해봅시다!
src/main/java/domain/Line.java
Outdated
public List<Boolean> getPoints() { | ||
return points; | ||
} |
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.
저번 리뷰와 동일합니다 외부에서 변경되면 어떡하죠..? 🤔
public Line setBridgeAt(int index) { | ||
List<Boolean> newPoints = new ArrayList<>(points); | ||
newPoints.set(index, true); | ||
return new Line(newPoints); | ||
} |
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.
생성자에서도 생성할 수 있고 setBridgeAt()
을 통해서도 Line이 생성될 수 있는데
Line
을 2가지 방법으로 생성할 수 있게 되는데 이렇게 작성하신 이유가 있을까요?
public static void applyBridges(List<Boolean> points, Set<Integer> reserved, Line prev, boolean isReserved) { | ||
IntStream.range(0, points.size()).forEach(i -> { | ||
boolean shouldAddBridge = (isReserved && reserved.contains(i)) || (!isReserved && RandomUtil.nextBoolean()); | ||
|
||
if (shouldAddBridge && isValidBridgePosition(points, prev, i)) { | ||
points.set(i, true); | ||
} | ||
}); | ||
} |
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.
static
메서드가 매개변수로 참조값을 받고 있어요, 이는 위험한 설계가 될 수 있을 것 같아요
해당 리뷰를 참고하시면 도움이 될 수 있을 것 같아요
public class Lines { | ||
private final List<Line> lines; | ||
|
||
public Lines(List<Line> lines) { | ||
this.lines = List.copyOf(lines); | ||
} | ||
|
||
public List<Line> getLines() { | ||
return lines; | ||
} |
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.
아직 dto의 요구사항을 수행하고 있는 것 같은 모델이 보여요 🤔🤔
public LadderDto(List<List<Boolean>> ladderData) { | ||
this.ladderData = ladderData; | ||
} | ||
|
||
public static LadderDto from(Ladder ladder) { | ||
return new LadderDto( | ||
ladder.getLines().getLines().stream() | ||
.map(Line::getPoints) | ||
.collect(Collectors.toList()) | ||
); | ||
} |
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.
마찬가지로 해당 dto는 정적 팩토리 메서드를 도입했지만 생성자를 활용해서도 사용이 가능한것으로 보여요 🥲
public List<List<Boolean>> getLadderData() { | ||
return ladderData; | ||
} |
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.
해당 중첩 리스트도 외부에서 호출하여 변경 가능성에 취약한 것 같아요 개선해봅시다!
public static void handleException(Exception e) { | ||
System.err.println("시스템 오류 : " + e.getMessage()); | ||
logger.error("시스템 오류 : ", e); | ||
} |
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.
ExceptionHandler
를 통해서 모든 예외를 "시스템 오류"라고 단정하는 것으로 보여요.
"시스템 오류"라고 한 이유가 있을까요??
|
||
import domain.Lines; | ||
|
||
public final class LadderGenerator { |
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.
public final class로 작성하신 이유가 궁금합니다
private static Line getPreviousLine(List<Line> lines, int row) { | ||
if (row == 0) { | ||
return null; | ||
} | ||
return lines.get(row - 1); | ||
} |
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.
위의 리뷰와 마찬가지로 static메서드가 참조값을 받아오고 있네요
왜 static 메서드가 인스턴스의 상태를 가지면 안될까요?
public static Line generate(int width, Set<Integer> reserved, Line prev) { | ||
List<Boolean> points = new ArrayList<>(Collections.nCopies(width, false)); | ||
|
||
Line.applyBridges(points, reserved, prev, true); | ||
Line.applyBridges(points, null, prev, false); | ||
Line.ensureOneBridge(points, prev); | ||
|
||
return new Line(points); | ||
} |
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.
여기도!
전체 구조
사다리 타기 생성 기준
사다리 타기 생성 기준 관련 메서드
SingleLineGenerator.ensureOneBridge()
SingleLineGenerator.isNotOverlap()
SingleLineGenerator.canConnectedBridge()
LadderValidator.validate()
LadderValidator.validateColumn()
LadderValidator.connected()
LadderValidator.connect()
ReservedPositionGenerator.generate()
ReservedPositionGenerator.reserveRandomPosition()
SingleLineGenerator.generate()
SingleLineGenerator.generatePoint()
질문
앞서 설정한 기준을 적용하여 세로와 가로에 빈 줄이 없도록 하고, 연속된 가로줄이 생성되지 않도록 구현했습니다.
그러나 100번 실행 중 약 2-3회(2-3%) 정도의 확률로 연속된 가로줄이 발생하는 상황입니다.
현재 코드에서 놓친 부분이 있다면 피드백 부탁드리겠습니다!
문제를 발생할 때마다 조건을 추가하는 방식으로 구현하다보니 코드가 상당히 복잡해졌다고 느껴집니다.
중복된 조건이 있거나 불필요하게 복잡한 방식으로 구현한 부분, 혹은 더 간결하게 표현할 수 있는 부분이 있다면 피드백 부탁드리겠습니다!
그 외에도 요구사항을 충족하지 못한 부분이나 코드 컨벤션과 관련하여 개선이 필요한 점이 있다면 피드백 부탁드리겠습니다. 😊