[LBP] 정상화 사다리 1단계 제출합니다. #41

borntocode2 · 2025-03-07T15:25:26Z

작성자 : 정상화
페어 : 윤준석, 박세은, 정상화

안녕하세요. 리건님 !
탈이 많았던 사다리 1단계 미션을 제출합니다.
사다리 타기의 기능 요구사항은 모두 만족하였지만,
간결한 메서드 라인, 원시값 포장, SRP준수, 객체지향 설계 등에 있어 부족한 점이 본인에게도 보이는 코드입니다.
리건님의 리뷰를 잘 참고하여 꼭 리팩토링하겠습니다.
시간 내어 주셔서 감사합니다.

들어가기

Ladder는 LadderGenerator에 의해 너비와 높이를 인자로 Ladder를 생성합니다. - Ladder.class

     public static Ladder of(int width, int height) {
        return new Ladder(LadderGenerator.generate(width, height));
    }

generate메서드에 의해 Line을 생성합니다.

  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 line = SingleLineGenerator.generate(width - 1, reserved.get(row));
            lines.add(line);
        }

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

Line은 이렇게 생성된다.

미리 초안 그리기(랜덤 활용)
List<Set<Integer>> reserved = ReservedPositionGenerator.generate(width - 1, height);
초안을 가지고 Line 구현

  public static Line generate(int width, Set<Integer> reserved) {
        List<Boolean> points = new ArrayList<>(Collections.nCopies(width, false));
       // 초안 대로 points 세팅해주기
        reserved.forEach(i -> points.set(i, true));

        IntStream.range(0, width).forEach(i -> {
            if (!points.get(i) && isNotOverlap(points, i)) {
                points.set(i, RANDOM.nextBoolean());
            }
        });

        return new Line(points);
    }

초안은 사다리의 세로 줄 기준으로 최소 한 개의 이동이 보장?된 사다리입니다.
미리 설정한 초안이긴 하지만, 랜덤으로 구현하였기 때문에 중복을 피할 수 있습니다.
초안으로 인해 세로 줄이 뚫린 터널 현상을 방지할 수 있습니다.

조건들을 가지고 Line 생성

   private static boolean isNotOverlap(List<Boolean> points, int i) {
        // 맨 마지막 point가 아니라면, 오른쪽 칸이 true면 Overlap
        if (i < points.size() - 1 && points.get(i + 1)) return false;
        // 맨 처음 point가 아니라면, 왼쪽 칸이 true면 Overlap
        if (i > 0 && points.get(i - 1)) return false;

        return true;
    }

한 라인마다, 옆으로 넘어가면서 왼쪽, 오른쪽의 칸을 비교합니다.
그려진 초안이 있기 때문에 오른쪽의 칸을 잘 주시해야 합니다.

랜덤으로 boolean값을 add

     IntStream.range(0, width).forEach(i -> {
            if (!points.get(i) && isNotOverlap(points, i)) {
                points.set(i, RANDOM.nextBoolean());
            }
        });

조건을 만족한 해당 point는 랜덤으로 boolean값이 그려지게 됩니다.

실행 모습

랜덤한 position을 가지고 있는 Set(Integer) 입니다.
랜덤 position을 이용하여 Line에 boolean값을 세팅합니다.
2번 메서드로 라인이 구현되면서 랜덤 position이 기록된 reserved에 추가로 랜덤 boolean이 세팅됩니다.
(해당 이미지는 랜덤으로 인해 초안이외의 아무런 boolean값이 세팅되지 않았습니다. 보통은 추가로 그려지게 됩니다.)

리건님의 소중한 리뷰 기다리겠습니다. 감사합니다 (꾸벅) !

hangillee

안녕하세요, 상화! 👋
벌써 마지막 미션이네요..

다른 모든 코멘트보다 선행되어야 할 것이 있어 알려드립니다!
현재 PR의 merge 대상이 main 브랜치입니다. 상화의 개인 브랜치로 변경 부탁드립니다.

요구 사항을 만족한 것은 확인했으나, 확인해주셔야 하는 부분이 있어 코멘트 남겼습니다. 또한, 현재 단위 테스트가 전무하여 모든 경우에 기능이 정상 동작하는지 확인이 어렵습니다. 테스트 작성 부탁드려요! 😁

변경 사항 적용해주시고 RC 남겨주세요! 파이팅! 💪

hangillee · 2025-03-08T11:14:26Z

build.gradle

+    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'


로깅 라이브러리를 활용했네요!
로그가 필요했던 이유는 무엇인가요? 🤔

hangillee · 2025-03-08T11:15:52Z

src/main/java/Application.java

+        try {
+            Ladder ladder = Ladder.of(4, 4);


4라는 원시값은 상수화하기에 충분한 근거가 있지 않았나요? 🤔
원시값 그대로 둔 이유가 궁금합니다!

리뷰 감사합니다 !

4라는 원시값은 상수화하기에 충분한 근거가 있지 않았나요? 🤔 원시값 그대로 둔 이유가 궁금합니다!

원시값을 그대로 둔 이유는 없습니다. 말씀대로 상수화하기에 충분히 근거가 있었고, 단순 실수입니다. 마지막 미션임에도 불구하고 커밋 전에 다시 한번 살펴보는게 미흡했던 것 같습니다...ㅜ 신경쓰도록 하겠습니다 !!

hangillee · 2025-03-08T11:26:50Z

src/main/java/model/Ladder.java

+    public Ladder(Lines lines) {
+        this.lines = lines;
+    }
+
+    public static Ladder of(int width, int height) {
+        return new Ladder(LadderGenerator.generate(width, height));
+    }


정적 팩토리 메소드 활용 좋습니다! 👍 다만, 정적 팩토리 메소드를 활용한 이상, 생성자는 private으로 닫아두어야 합니다.

리뷰 감사합니다 !

정적 팩토리 메소드 활용 좋습니다! 👍 다만, 정적 팩토리 메소드를 활용한 이상, 생성자는 private으로 닫아두어야 합니다.

정적 팩토리 메소드에 대해서 잘 몰랐었네요..
외부에서 생성자를 직접 호출 할 일 없이 정적 팩토리 메소드를 통해 간접적으로 객체를 생성하는데, 그럼 Ladder생성자가 외부에서 직접 생성될 일이 없을텐데요.. private이 당연하겠군요.. 알려주셔서 감사합니다 ^-^

hangillee · 2025-03-08T11:36:21Z

src/main/java/model/LadderGenerator.java

다른 클래스의 메소드를 호출하고 그 반환값을 그대로 반환하는 메소드만 있는 클래스가 필요한가요?
이 클래스의 역할이 무엇인가요? 🤔

리뷰 감사합니다 !

다른 클래스의 메소드를 호출하고 그 반환값을 그대로 반환하는 메소드만 있는 클래스가 필요한가요? 이 클래스의 역할이 무엇인가요? 🤔

Ladder 생성 -> LadderGenerator -> LineGenerator -> SingleLineGenerator 와 같은 단계로 Ladder가 구현됩니다.
중요한 것은 Lines와 Line입니다.

Ladder안에 Lines, Lines를 채우는 Line으로 그릴 수 있을 것 같은데요.
Lines를 생성하기 위해 Line을 생성하는 도메인 과정과, List을 일급컬렉션으로 관리하는 과정에서 LadderGenerator라는 필요 없는 클래스가 중간에 등장했습니다. Lines를 생성하는 것은 Ladder의 역할이라고 생각했거든요 !

이 답변을 작성하면서 코드를 다시 살펴보니, Ladder에서 Lines를 작성하는 것이 아니라, 이미 LineGenerator에서 Lines를 생성, 반환하고 있으므로 원래 계획인 도메인 절차가 이상했었습니다. LineGenerator를 LinesGenerator클래스명으로 변경하고, LadderGenerator를 삭제하여 필요없는 클래스를 정리하고 도메인 단계를 다시 설정 해주었습니다 !

hangillee · 2025-03-08T11:39:45Z

src/main/java/model/LadderValidator.java

+        IntStream.rangeClosed(0, width)
+                .forEach(i -> validateColumn(lines, i, width));


forEach를 활용하려고 stream을 도입하는 것보다 for-loop가 훨씬 가독성이 높지 않을까요?
관련 아티클입니다! 😁

리뷰 감사합니다 !

forEach를 활용하려고 stream을 도입하는 것보다 for-loop가 훨씬 가독성이 높지 않을까요?

말씀해주신 가독성은 for-loop가 훨씬 좋은 것 같습니다.

알려주신 아티클에서

pieces.keySet() .forEach( positionKey -> model.addAttribute( positionKey,pieces.get(positionKey)));

의 코드에서 혹시, forEach 내부에 로직이 하나라도 더 추가된다면 동시성 보장과 가독성 측면에서 문제가 생길 수 있다는 글을 발견했는데요.

for (String positionKey: pieces.keySet()) { model.addAttribute(positionKey, pieces.get(positionKey)); }

위의 코드 처럼, for-loop를 이용하여 수정한 후에, 로직이 추가된다면 forEach내부를 손봐야 하는 것이 아니라 filter나 map을 활용하거나 for-loop를 활용하는 것이 올바른 방향이라고 나와있기에 가독성 뿐만 아니라, 동시성과 성능면에서도 더 올바른 선택을 할 수 있을 것 같네요.

알려주셔서 감사합니다.

hangillee · 2025-03-08T12:05:23Z

src/main/java/model/LineGenerator.java

Generator라는 클래스들이 등장하게 된 계기가 궁금합니다.
여기서 하고 있는 역할들은 일급 컬렉션들이 직접 해도 되지 않나요? 🤔

리뷰 감사합니다 !

Generator라는 클래스들이 등장하게 된 계기가 궁금합니다. 여기서 하고 있는 역할들은 일급 컬렉션들이 직접 해도 되지 않나요? 🤔

말씀대로 일급 컬렉션들이 직접 해도 될 것 같아요. 원래, List<Line> lines = new ArrayList<>();

라는 코드를 Ladder클래스에 작성 했을 때에
일급컬렉션으로 따로 만들어야지
라고 생각해서...
Lines 클래스, Generator클래스 생성되고 정작 일급컬렉션 클래스에서는 get메서드만 있거나 도메인으로서 역할을 하지 못하고 있었네요..

멤버 변수 하나만 있으면 이미 일급 컬렉션이기에 도메인으로서 역할을 하지 못하는 클래스는 삭제하고 일급 컬렉션의 역할로 리팩토링 하였습니다.

감사합니다 !

hangillee · 2025-03-08T12:07:15Z

src/main/java/model/Lines.java

getter만 존재하는 도메인(모델)이 도메인으로서 역할을 제대로 수행하고 있나요?
지금 이 클래스가 DTO와 무엇이 다른가요? 🤔

리뷰 감사합니다 !

getter만 존재하는 도메인(모델)이 도메인으로서 역할을 제대로 수행하고 있나요? 지금 이 클래스가 DTO와 무엇이 다른가요? 🤔

맞습니다. 삭제하고 Lines도메인을 Ladder 멤버 변수로 옮겨줘서 쓸데 없는 도메인을 정리하였습니다 !

hangillee · 2025-03-08T12:09:05Z

src/main/java/model/ReservedPositionGenerator.java

+
+
+


무의미한 공백이 있네요.. 😢

무의미한 공백이 있네요.. 😢

앗.. 커밋 전 공백 및 라인 컨벤션에 더 신경쓰도록 하겠습니다 .... 😢

hangillee · 2025-03-08T12:19:03Z

src/main/java/model/ReservedPositionGenerator.java

+        List<Integer> positions = IntStream.range(0, width)
+                .boxed()
+                .collect(Collectors.toList());


IntStream이 필요한 로직이었나요? 🤔

리뷰 감사합니다 !

IntStream이 필요한 로직이었나요? 🤔

필요 없습니다 ! depth를 신경쓰다보니 무조건 for, while 반복문 대신에 Stream을 사용하게 되었습니다.

forEach를 활용하려고 stream을 도입하는 것보다 for-loop가 훨씬 가독성이 높지 않을까요?
관련 아티클입니다! 😁

위 리뷰도 참고하여, 잘못된 습관을 가지고 있는 것을 알게되었습니다.
감사합니다 😆

hangillee · 2025-03-08T12:33:20Z

src/main/java/model/SingleLineGenerator.java

+        IntStream.range(0, width).forEach(i -> {
+            if (!points.get(i) && isNotOverlap(points, i)) {
+                points.set(i, RANDOM.nextBoolean());
+            }
+        });


Package java.util.stream Java 8 Stream 공식 문서에서는 다음과 같이 설명하고 있습니다!

Note also that attempting to access mutable state from behavioral parameters presents you with a bad choice with respect to safety and performance; if you do not synchronize access to that state, you have a data race and therefore your code is broken, but if you do synchronize access to that state, you risk having contention undermine the parallelism you are seeking to benefit from. The best approach is to avoid stateful behavioral parameters to stream operations entirely; there is usually a way to restructure the stream pipeline to avoid statefulness.

요약하자면, 병렬 Stream 사용 시 컬렉션의 상태를 동기화 하지 않으면 데이터 경합이 발생하고 그렇다고 상태를 동기화하면 병렬 처리의 이점을 모두 포기해야 합니다. 즉, Stream 연산 활용 시에는 상태를 가져선 안됩니다. 그런데, 현재 이 연산은 points의 상태를 변경시키고 있습니다. 병렬 Stream 연산에서는 문제가 생길 가능성이 높겠죠.. 😢

borntocode2 · 2025-03-08T12:39:19Z

안녕하세요, 상화! 👋 벌써 마지막 미션이네요..

다른 모든 코멘트보다 선행되어야 할 것이 있어 알려드립니다! 현재 PR의 merge 대상이 main 브랜치입니다. 상화의 개인 브랜치로 변경 부탁드립니다.

요구 사항을 만족한 것은 확인했으나, 확인해주셔야 하는 부분이 있어 코멘트 남겼습니다. 또한, 현재 단위 테스트가 전무하여 모든 경우에 기능이 정상 동작하는지 확인이 어렵습니다. 테스트 작성 부탁드려요! 😁

변경 사항 적용해주시고 RC 남겨주세요! 파이팅! 💪

안녕하세요 리건님 !
제출 기한에 임박해서 급하게 제출하다보니 merge대상을 수정하는 것을 깜빡했어요 !! 해당 리뷰 확인하여 수정 후에, 브랜치 변경하여 다시 PR하겠습니다. !

borntocode2 added 4 commits March 7, 2025 00:34

made by Joshua

2803607

Pair.add(Joshua)

0fd7cbe

[edit] space convention

12055bd

[edit] 주석 삭제

2157ad1

hangillee suggested changes Mar 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LBP] 정상화 사다리 1단계 제출합니다. #41

[LBP] 정상화 사다리 1단계 제출합니다. #41

borntocode2 commented Mar 7, 2025

hangillee left a comment

hangillee Mar 8, 2025

hangillee Mar 8, 2025

borntocode2 Mar 10, 2025

hangillee Mar 8, 2025

borntocode2 Mar 10, 2025

hangillee Mar 8, 2025

borntocode2 Mar 10, 2025

hangillee Mar 8, 2025

borntocode2 Mar 10, 2025

hangillee Mar 8, 2025

borntocode2 Mar 12, 2025

hangillee Mar 8, 2025

borntocode2 Mar 12, 2025

hangillee Mar 8, 2025

borntocode2 Mar 10, 2025

hangillee Mar 8, 2025

borntocode2 Mar 10, 2025

hangillee Mar 8, 2025

borntocode2 commented Mar 8, 2025

		IntStream.rangeClosed(0, width)
		.forEach(i -> validateColumn(lines, i, width));

[LBP] 정상화 사다리 1단계 제출합니다. #41

Are you sure you want to change the base?

[LBP] 정상화 사다리 1단계 제출합니다. #41

Conversation

borntocode2 commented Mar 7, 2025

들어가기

Line은 이렇게 생성된다.

실행 모습

hangillee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borntocode2 commented Mar 8, 2025