-
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단계 제출합니다. #39
base: junseok0304
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단계 미션 고생하셨습니다! 👍
전체적으로 Stream 연산과 static 메서드, 값만 가지는 객체가 자주 보였던 것 같아요.
- Stream 연산 내에서 상태를 변경하는 작업을 수행하거나, 여러번 Stream 연산을 수행해요.
- 생성 로직을 Generator가 가졌어야 하는 이유? + 테스트를 한다면 랜덤은 어떻게 할까? 등
- 검증, 사다리 생성 등의 대부분의 작업을 static 메서드로 작성되어 있어요.
객체지향 보다는 절차 지향에 가깝다고 느꼈습니다.
질문 답변
현재의 방식 외에 다른 방법으로 사다리 문제를 조금 더 효율적으로
해결할 수 있는 방안
이 있는지 궁금합니다. 숫자로 만든 다음 치환하는 방법 등
사다리라는 개념 자체가 랜덤하게 생성이 되어야 하는데 만들어진 사다리의 모양이 잘못되면 안되니까 이를 검증한 뒤에 다시 랜덤을 부여해서
완성시키는 방법으로 현재 수행되고 있는데도 불구하고 터널현상이 종종 등장합니다. (100번당 2-3회 수준) 이러한 경우에
해결방안
으로 적용해볼 수 있는 다른게 있을까요?
동적 프로그래밍(dp)의 개념이 적용되어야 하는 미션
이라고 생각했는데 아직 동적프로그래밍의 개념을 잘 모르겠습니다,,,ㅠㅠ
동적 프로그래밍과 관련해 참고할 만 한 좋은 아티클이 있을까요?
모든 질문이 해결 방안(정답)에 초점이 맞춰진 것 같아요.
항상 정답은 없다고 생각해요.
정답을 찾는 것보다 중요한 것은 근거를 가지고 더 나은 방법을 선택하는 것 같아요.
오히려 모든 질문에 아래와 같이 물어볼 수 있을 것 같아요.
- 왜 지금의 구조가 비효율적이라고 생각하는지? (근거)
- 왜 그런 문제가 발생하는 것 같은지? 나아가 왜 해결해야 하는지?
- 왜 DP 개념이 필요하다고 느꼈는지? 어떤 점이 개선되는지?
문제를 해결하는 방법은 많아요. 상황에 따라 가장 나은 방법이 바뀔 수도 있구요.
근거를 기반으로 더 나은 방법을 선택하고 왜 그랬는지 자신있게 설명할 수 있게 된다면 더 좋을 것 같네요.
고생하셨습니다.
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.
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 List getPoints() {
return new ArrayList<>(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.
좋습니다! 이제 외부로 반환된 리스트가 변경되더라도 Line의 원본 데이터에는 변경이 전파되지 않겠네요!
그렇다면 조금 더 나아가서 아래와 같은 경우가 생긴다해도 괜찮을까요?
List<Boolean> points = line.getPoints(); // 복사본을 반환
// 어떤 개발자가 반환된 points를 변경함
points.add(true);
이때도 points가 line이 반환하려는 값을 나타낸다고 할 수 있을까요?
변경이 전파되지는 않지만, line이 가진 정보를 사용하려고 할 때(출력 등) 어디선가 변경된다면 어떻게 될까요?
변경 전파를 막는 것과 함께 불변까지 가져간다면 더욱 안전한 데이터 전달이 될 것 같은데 어떻게 생각하시나요?
변경 전파를 막는 것과 더불어
Line이 반환하려는 값
이 훼손되지 않게 한다면 더욱 안전할 것 같다는 의견입니다!
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; | ||
} |
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.
IntStream을 사용하신 특별한 이유가 있으신가요?
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.
가독성을 위해서 사용했던 것 같습니다. for 루프를 이용해도 문제없을 것 같습니다.
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()); | ||
} | ||
}); |
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.
현재 코드에서는 Stream의 forEach() 내부에서 리스트(points)의 상태를 변경하고 있어요.
하지만 스트림 연산은 원칙적으로 불변성을 유지하는 것을 권장해요.
(특히 병렬 스트림을 사용할 경우 예상치 못한 동작이 발생할 수 있어요.)
forEach() 내부에서 points.set(i, true);
또는 points.set(i, RANDOM.nextBoolean());
처럼
리스트의 상태를 직접 변경하는 대신, 새로운 리스트를 생성하여 변화를 적용하는 방식이 더 안전하고 가독성이 좋을 것 같아요.
아래 공식 문서에서도 스트림 내 부작용(Side Effects)은 피해야 한다고 명시되어 있어요 😄
🔗 Java 공식 문서 - StreamOps: Side Effects
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.
추가로 어떤 작업을 하는지 이해하기 어려웠습니다.
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.
스트림의 사용 원칙을 위배한 코드가 되었군요 ㅜㅜ
map을 사용해 불변성을 유지하면서 해결 할 수 있을 것 같습니다!
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; | ||
} |
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줄이라서 {}
를 안쓰신 건가요?
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.
그렇습니다! { }를 사용하지 않았을 때의 side effect가 있을까요?
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줄에서 3줄되는 예시)에는 다시 중괄호를 추가해줘야 하기도 하고,
2줄이었다가 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.
클래스 내의 마지막 멤버와 클래스를 닫는 괄호 사이에 줄바꿈이 있을 때도 있고 없을 때도 있는 것 같아요.
팀 내 규칙을 만들고 일관성을 지키면 좋을 것 같아요! 😄
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 class Lines { | ||
private final List<Line> lines; | ||
|
||
public Lines(List<Line> lines) { | ||
this.lines = 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.
Lines | Line의 일급 컬렉션
일급 컬렉션이라고 말씀하셨는데,
Lines는 단순히 List을 가지면서 반환하는 책임만 가지도록 설계된 것이 맞는건가요?
준석님이 생각하시는 일급 컬렉션이란 무엇인가요?
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 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)); | ||
} |
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.
Ladder는 생성자로도 생성이 가능하고 정적 팩터리 메서드 of()
로도 생성이 가능한 것이 의도된 것인가요?
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 class LadderGenerator { | ||
public static Lines generate(int width, int height) { | ||
return LineGenerator.generate(width, height); | ||
} | ||
} |
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.
LadderGenerator.generate()
의 반환 타입은 Ladder
라고 기대할 것 같은데 어떻게 생각하시나요?
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.
맞습니다,, 래더를 반환할 것 같지만 라인스를 반환 하므로
반환타입을 변경하거나, 직접 호출해주는 방법으로 고치는 것이 좋겠습니다.
왜 지금의 구조가 비효율적이라고 생각하는지? (근거)
왜 그런 문제가 발생하는 것 같은지? 나아가 왜 해결해야 하는지?
왜 DP 개념이 필요하다고 느꼈는지? 어떤 점이 개선되는지?
숫자로 먼저 만든 다음 시각화 하는 방식이나, 여러 부분에서 다양하게 생각해볼 수 있었던 좋은 시간이 된 것 같습니다. 리뷰 감사드립니다! |
조금이라도 더 효율적인 방법을 찾으려는 자세와 적극적으로 고민하는 것은 잘 하고 계신 것 같습니다. 그러나 성능 저하를 느끼지 못했고, 찾은 방법이 더욱 복잡한 로직이라면, 항상 성능이 최고인 코드만 작성할 수는 없다고 생각해요. |
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.
코드에는 반영이 안되어있는 것 같아서 RC 남기겠습니다!
반영 후 리뷰 재요청
주세요! 😄
스터디 정보
질문
현재의 방식 외에 다른 방법으로 사다리 문제를 조금 더 효율적으로 해결할 수 있는 방안이 있는지 궁금합니다.
숫자로 만든 다음 치환하는 방법 등
사다리라는 개념 자체가 랜덤하게 생성이 되어야 하는데 만들어진 사다리의 모양이 잘못되면 안되니까 이를 검증한 뒤에 다시 랜덤을 부여해서
완성시키는 방법으로 현재 수행되고 있는데도 불구하고 터널현상이 종종 등장합니다. (100번당 2-3회 수준) 이러한 경우에
해결방안으로 적용해볼 수 있는 다른게 있을까요?
동적 프로그래밍(dp)의 개념이 적용되어야 하는 미션이라고 생각했는데 아직 동적프로그래밍의 개념을 잘 모르겠습니다,,,ㅠㅠ
동적 프로그래밍과 관련해 참고할 만 한 좋은 아티클이 있을까요?
< 구조 >
Ladder | 사다리 전체 구조 관리
Lines | Line의 일급 컬렉션
Line | 가로줄(Bridge)의 유무 관리
LadderGenerator | 전체 사다리 생성 로직 관리
LineGenerator | 각 줄(Line) 반복 생성 및 관리
SingleLineGenerator | 개별 줄(Line) 생성 로직
ReservedPositionGenerator | 미리 반드시 채워야 할 위치를 예약하는 로직
LadderValidator | 세로 방향에 대한 사다리의 유효성 검증 및 보장
LineView | View 관련 로직
가로(Bridge)줄 생성 기준
한 라인에 최소 한 개의 가로줄은 반드시 존재한다. -> 빈줄 금지
연속된 가로줄 금지 -> |-----|-----| 형태
이전 줄과 같은 위치에 가로줄을 생성하지 않는다. -> 세로로 같은 위치 반복 금지
세로(Column) 연결 기준
모든 세로줄은 최소한 하나의 가로줄로 연결된다.
결과상 세로줄 전체가 빈 세로줄이 하나라도 나오면 안된다. (개인적으론 터널 이라고 이름 지었습니다)
모든 출발 지점이 반드시 목적지까지 도착 해야 한다.
미리 예약된 위치
각 column은 예약을 통해 반드시 한 번은 가로줄을 가진다. -> 모든 세로줄 연결 보장