[LBP] 김준 사다리 5단계 제출합니다. #43

kjoon418 · 2025-03-14T04:32:42Z

사다리 미션의 5단계까지 구현했습니다.
사다리 게임의 메인 로직에 대한 테스트 코드도 작성하고 싶었지만 작성하지 못했습니다.
이와 관련해 질문 하나 드리고 싶습니다!

질문

Ladder의 getEndPoint()와 Line의 getNextPoint() 메서드에 대한 테스트 코드를 작성하지 못했습니다.
- 테스트 작성을 위해서 해당 메서드가 제대로 동작하는지 검증하는 메서드를 만들어야 했는데, 이를 위해선 사실상 Line 클래스 내부에 존재하는 private 메서드와 똑같은 메서드(shouldMoveToLeft(), shouldMoveToRight())를 테스트 클래스에 작성해야 했습니다.
- Line의 메서드를 그대로 복사해 오는 것은 의미가 없는 테스트 코드라 생각했기에 다른 방법을 고민하였으나, 결국 테스트 코드를 작성하지 못했습니다.
- Line의 기존 메서드를 그대로 가져와 작성해도 괜찮은 것일까요? 혹은 LinksGenerator의 테스트용 구현체를 만들어 해결할 수 있는 것일까요?
- 테스트하기 어렵게 설계된 것이 문제인지, 혹은 추가적인 해결책이 있는지가 궁금합니다.

피드백 항상 감사합니다 😄

kjoon418 · 2025-03-14T04:54:58Z

src/test/java/model/LadderTest.java

+    @ParameterizedTest
+    @DisplayName("생성자에 전달한 높이와 같은 수의 원소를 지닌 Line 컬렉션을 반환한다")
+    @ValueSource(ints = {5, 10, 200, 3000})
+    void returnLineCollectionWithSameNumberOfElementAsHeight(int height) {
+        Ladder ladder = new Ladder(DEFAULT_WIDTH, height);
+        List<Line> lines = ladder.getLines();
+
+        assertThat(lines.size()).isEqualTo(height);
+    }


제가 겪은 버그를 다른 스터디원과 공유하고자 코멘트 작성합니다.

Suggested change

@ParameterizedTest

@DisplayName("생성자에 전달한 높이와 같은 수의 원소를 지닌 Line 컬렉션을 반환한다")

@ValueSource(ints = {5, 10, 200, 3000})

void returnLineCollectionWithSameNumberOfElementAsHeight(int height) {

Ladder ladder = new Ladder(DEFAULT_WIDTH, height);

List<Line> lines = ladder.getLines();

assertThat(lines.size()).isEqualTo(height);

}

@ParameterizedTest

@DisplayName("생성자에 전달한 높이와 같은 수의 원소를 지닌 Line 컬렉션을 반환한다")

@ValueSource(ints = {5, 10, 200, 3000})

void returnLineCollectionWithSameNumberOfElementAsHeight(int height) {

Ladder ladder = new Ladder(DEFAULT_WIDTH, height);

List<Line> lines = ladder.getLines();

assertThat(lines.size()).isSameAs(height);

}

위의 테스트 코드는 int와 int를 비교하는 것이기에, 동일 비교(==)한다는 것을 명확히 하고자 isSameAs() 메서드를 사용했었습니다.

그러나 위 결과 화면과 같이, height의 값이 200, 3000일 때는 Expected와 Actual의 값이 같음에도 테스트가 실패하는 현상을 경험했습니다.
구글링을 통해서도 원인을 찾을 수 없어 Chat GPT에게 질문한 결과, Java의 Integer 캐싱 메커니즘에 의해 발생하는 버그라는 답변이 나왔습니다.

Chat GPT 답변 요약

isSameAs()는 객체의 참조(주소)가 같은지 확인하는 메서드인데, Java의 Integer 캐싱 메커니즘에 의해 isSameAs() 비교가 특정 값에서만 다르게 동작할 수 있습니다.

Java의 Integer 객체는 -128 ~ 127 범위에서 동일한 객체를 캐싱해서 재사용합니다.
5와 10과 같은 작은 정수는 캐싱이 적용되어 isSameAs() 비교가 성공하지만, 200이나 3000과 같은 정수는 캐싱 범위를 벗어나 새로운 Integer 객체가 생성되어 isSameAs() 비교가 실패합니다.

isSameAs() 대신 isEqualTo() 를 사용해 문제를 해결해야 합니다.

가능한 한 int와 int의 비교에도 isEqualTo()를 사용하고, 정말 주소를 비교해야 하는 상황(싱글톤 검증 등)에만 isSameAs()를 사용하는 것이 좋아 보이네요.

Chat GPT 답변에 틀린 부분이 있다면 정정 부탁드립니다. 😄

dd-jiyun

안녕하세요 준 👋

이번 미션도 고생 많으셨습니다!

객체를 역할 별로 나누고 불변성까지 요구사항을 지키려고 열심히 고민하신 것 같아요 👍🏻

특히 올려주신 isSameAs()와 isEqualTo() 보고 저도 배웠습니다!

마지막까지 화이팅 🔥

Ladder의 getEndPoint()와 Line의 getNextPoint() 메서드에 대한 테스트 코드를 작성하지 못했습니다.
테스트 작성을 위해서 해당 메서드가 제대로 동작하는지 검증하는 메서드를 만들어야 했는데, 이를 위해선 사실상 Line 클래스 내부에 존재하는 private 메서드와 똑같은 메서드(shouldMoveToLeft(), shouldMoveToRight())를 테스트 클래스에 작성해야 했습니다.
Line의 메서드를 그대로 복사해 오는 것은 의미가 없는 테스트 코드라 생각했기에 다른 방법을 고민하였으나, 결국 테스트 코드를 작성하지 못했습니다.
Line의 기존 메서드를 그대로 가져와 작성해도 괜찮은 것일까요? 혹은 LinksGenerator의 테스트용 구현체를 만들어 해결할 수 있는 것일까요?
테스트하기 어렵게 설계된 것이 문제인지, 혹은 추가적인 해결책이 있는지가 궁금합니다.

제가 보기에는 Ladder의 메서드가 Line과 의존성이 있어서 테스트가 복잡해지는 것 같아요.
구조적으로는 Ladder가 Line에 의존하는 건 자연스러울 수 있지만, 테스트할 때 동작을 제어해야 하는 불편함이 있을 수 있죠.

말씀해주신 것처럼 테스트용 구현체를 사용해서 테스트할 수 있겠지만, 먼저 클래스 간의 의존성을 다시 한 번 살펴보는 게 좋을 것 같아요 :)
예를 들면, 객체 주입 방법을 바꿔서 의존성을 줄이는 것도 하나의 방법일 수 있겠죠?!

dd-jiyun · 2025-03-17T05:56:30Z

src/main/java/dto/LadderResultDto.java

@@ -0,0 +1,4 @@
+package dto;
+
+public record LadderResultDto(String name, String resultValue) {


record 사용 👍🏻

dd-jiyun · 2025-03-17T06:02:58Z

src/test/java/model/LadderUserTest.java

+    void ifUseIllegalNameThenThrowException() {
+        for (String illegalName : ILLEGAL_NAMES) {
+            assertThatThrownBy(() -> new LadderUser(illegalName))
+                    .isInstanceOf(IllegalArgumentException.class);
+        }


예외 발생 테스트 좋아요!
추가로 예외 메시지까지 검증하면 의도한 예외가 발생하는지 더 정확하게 확인할 수 있을 것 같아요 🤭

예외 메시지를 수정하려 할 때 마다 테스트 코드를 수정해야 하는 번거로움이 있겠지만, 그만큼 확실한 테스트 코드를 짤 수 있겠군요!
장단점을 비교해가며 상황에 맞게 사용하도록 노력하겠습니다. 😄

dd-jiyun · 2025-03-17T06:05:50Z

src/test/java/model/LadderUsersTest.java

+        }
+    }
+
+    private List<LadderUser> createLadderUserCollection(String... names) {


가변 인자 사용 👍🏻

dd-jiyun · 2025-03-17T12:24:43Z

src/main/java/model/DetachedRandomLinksGenerator.java

+    private boolean isLeftNotPresent(int index, List<Link> links) {
+        if (isFirstIndex(index)) {
+            return true;
+        }
+
+        Link leftLink = links.get(index - 1);
+
+        return leftLink.getLinkstatus() != PRESENT;
+    }
+
+    private boolean isRightNotPresent(int index, List<Link> links) {
+        if (isLastIndex(index, links)) {
+            return true;
+        }
+
+        Link rightLink = links.get(index + 1);
+
+        return rightLink.getLinkstatus() != PRESENT;
+    }
+
+    private boolean isFirstIndex(int index) {
+        return index == 0;
+    }
+
+    private boolean isLastIndex(int index, List<Link> links) {
+        return index == links.size() - 1;
+    }
+
+    private int getNextIndex(int index, List<Link> linkStatuses) {
+        return (index + 1) % linkStatuses.size();
+    }


인덱스 관련 로직이 Line 객체에서도 유사하게 사용되는 것 같아요.
혹시 이렇게 따로 작성하신 이유가 있을까요?

맞습니다...! 유사한 로직이 Line과 DetachedRandomLinksGenerator 모두에 존재하고 있습니다.
제가 우선 현재처럼 구현하게 된 이유는 다음과 같습니다.

|-----|-----|이라는 라인이 있을 때, 해당 라인은 총 3개의 Point(|)와 2개의 Link(-----)를 갖습니다.
그렇기 때문에 마지막 Point인지 검사하기 위해선 links.size()와 같은지 검사해야 했고,
(Link)의 마지막 Index인지 검사하기 위해선 links.size() - 1과 같은지 검사해야 했습니다.

이 점 때문에 메서드 이름을 is...Index()와 is...Point()로 구분해 작성했지만, 제 코드를 읽는 다른 개발자들에게 직관적이지 않을 것 같다는 점이 계속 마음에 걸렸습니다.

전략 패턴을 처음 사용해보면서 놓친 것이 많은 것 같은데, 오래 고민한 후 해결해 보도록 하겠습니다!

dd-jiyun · 2025-03-17T12:44:15Z

src/main/java/model/Ladder.java

        }

-        this.lines = Collections.unmodifiableList(lines);
+        return Collections.unmodifiableList(lines);


준님이 이번 미션을 하면서 어떤 기준으로 unmodifiableList()와 copyOf()를 선택하셨는지 궁금합니다!

정말 부끄럽게도 unmodifiable...()과 copyOf()의 특징을 반대로 이해했습니다. 😭
원본 컬렉션과의 연결을 끊고자 unmodifiableList()를 사용한 건데, 반대로 사용했군요...!

아래는 저의 기준입니다...!

저는 둘의 차이점을 공부하며, unmodifiable...()는 기존 컬렉션과 똑같은 참조를 반환한다라는 점에 주목했습니다.
'넘겨받은 불변 컬렉션이 외부로 인해 변할 수 있다'는 점은 상당히 위험해 보였고, 의도적으로 그렇게 동작하도록 만들 경우가 아니면 unmodifiable...()를 사용하지 않아야겠다 판단했습니다.

dd-jiyun · 2025-03-17T13:00:42Z

src/main/java/model/LadderUsers.java

+        Optional<LadderUser> findUser = ladderUsers.stream()
+                .filter(ladderUser -> ladderUser.getName().equals(name))
+                .findAny();


Optional 활용 👍🏻

추가로 해당 메서드가 이름의 인덱스를 찾는 거 같아요. filter로 맞는 객체를 findAny로 찾고 그 후에 인덱스를 찾기 위해 또 리스트를 순회해요.
이 두 연산을 한 번에 처리할 수 있는 방법으로 IntStream을 활용해 볼 수 있을 거 같은데 적용해보시는 거 어떠세요?

로직을 작성하며 비효율적이라 생각했지만 다른 방법을 생각해내지 못했는데, IntStream을 활용해 해결할 수 있군요!

좋은 정보를 알려주셔서 감사합니다!
리팩터링하며 적용해 보도록 하겠습니다. 😊

dd-jiyun · 2025-03-17T13:06:14Z

src/main/java/model/LadderUsers.java

+
+    private void validateDuplicate(List<LadderUser> ladderUsers) {
+        boolean duplicated = ladderUsers.stream()
+                .collect(Collectors.groupingBy(Function.identity(), Collectors.counting()))


grouping과 counting을 사용해서 중복 검증을 하신 이유가 있나요?

실행 결과 예제를 보고 사용자 이름은 중복되면 안된다고 생각한 것 같습니다.

neo neo brie tommy |-----| |-----| | |-----| | |-----| | | | |-----| | |-----| |-----| 꽝 5000 꽝 3000 결과를 보고 싶은 사람은? neo 실행 결과 꽝, 3000

같은 이름이 여럿 있는 경우, 같은 사용자가 여러 번 참가했다고 가정하고 위와 같이 출력하는게 자연스러울까요?
리뷰어님의 의견이 궁금합니다!

dd-jiyun · 2025-03-17T13:21:32Z

src/main/java/controller/LadderController.java

 import dto.LineDto;
-import model.Ladder;
-import model.Line;
+import model.*;


*을 사용하신 이유가 있나요?

IntelliJ의 자동 임포트 기능을 활용하다보니 *가 사용된 것 같습니다.
IntelliJ가 제공해준 import 구문을 굳이 바꿀 필요 없다 생각해서 의식하지 않았는데, 이에 관한 리뷰어님의 의견이 궁금합니다.

리뷰어님은 *의 사용을 지양하시나요?

dd-jiyun · 2025-03-17T13:29:20Z

src/main/java/model/Ladder.java

+        if (height < MINIMUM_HEIGHT) {
+            throw new IllegalArgumentException("사다리의 높이는 " + MINIMUM_HEIGHT + "보다 짧을 수 없습니다. 전달된 값: " + height);
+        }


높이 입력값이 1일 때도 사다리가 생성이 되나요? 🧐

네이버 사다리 게임을 실행해 보았더니, 위 사진과 같은 케이스도 발생하는 것을 확인할 수 있었습니다.

사다리 결과 1 2 |-----| 3 4

그에 따라 위와 같이 높이가 1인 사다리도 문제 없다고 생각해 허용했는데, 다시 생각해보니 높이가 1인 사다리는 어색한 것 같네요.
사다리 높이가 2일 때 부터 생성되도록 수정하도록 하겠습니다!

dd-jiyun · 2025-03-17T13:31:17Z

src/main/java/view/LadderInputView.java

+    public String[] getResultValues() {
+        System.out.println("실행 결과를 입력하세요. (결과는 쉼표(,)로 구분하세요)");
+        String resultValues = scanner.nextLine();
+
+        System.out.println();
+
+        return resultValues.split(INPUT_SEPARATOR);
+    }


실행 결과가 공백으로 입력되었을 때도 실행이 돼요! 의도하신 걸까요?

참여자의 이름에 대한 예외 처리만 수행하고, 실행 결과에 대한 처리는 놓쳤군요!
실행 결과 역시 도메인으로 포장해서 자가 검증이 가능하도록 수정하겠습니다. 😄

피드백 감사합니다!

Kim Joon and others added 8 commits March 5, 2025 19:07

feat: 1단계 미션 구현

d02f1bd

refactor: 어색한 메서드의 이름 수정

f072a93

refactor: 불필요한 변수 제거

00f6903

refactor: 적절한 예외 클래스 사용

35edfcf

refactor: 피드백 반영

7059223

refactor: 중복되는 불변 컬렉션 생성 제거

2fde758

feat: 2 ~ 5단계 구현

63d9750

Merge branch 'kjoon418' into step2

f61159e

kjoon418 commented Mar 14, 2025

View reviewed changes

Kim Joon added 2 commits March 14, 2025 13:58

fix: isSameAs()를 isEqualTo()로 변경

857cfc9

Merge remote-tracking branch 'origin/step2' into step2

74d4322

dd-jiyun suggested changes Mar 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LBP] 김준 사다리 5단계 제출합니다. #43

[LBP] 김준 사다리 5단계 제출합니다. #43

kjoon418 commented Mar 14, 2025 •

edited

Loading

kjoon418 Mar 14, 2025

dd-jiyun left a comment •

edited

Loading

dd-jiyun Mar 17, 2025

dd-jiyun Mar 17, 2025

kjoon418 Mar 21, 2025

dd-jiyun Mar 17, 2025

dd-jiyun Mar 17, 2025

kjoon418 Mar 21, 2025 •

edited

Loading

dd-jiyun Mar 17, 2025

kjoon418 Mar 21, 2025

dd-jiyun Mar 17, 2025

kjoon418 Mar 21, 2025

dd-jiyun Mar 17, 2025

kjoon418 Mar 21, 2025

dd-jiyun Mar 17, 2025

kjoon418 Mar 21, 2025

dd-jiyun Mar 17, 2025

kjoon418 Mar 21, 2025

dd-jiyun Mar 17, 2025

kjoon418 Mar 21, 2025

		@@ -0,0 +1,4 @@
		package dto;

		public record LadderResultDto(String name, String resultValue) {

[LBP] 김준 사다리 5단계 제출합니다. #43

Are you sure you want to change the base?

[LBP] 김준 사다리 5단계 제출합니다. #43

Conversation

kjoon418 commented Mar 14, 2025 • edited Loading

질문

Choose a reason for hiding this comment

dd-jiyun left a comment • edited Loading

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

kjoon418 Mar 21, 2025 • edited Loading

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

kjoon418 commented Mar 14, 2025 •

edited

Loading

dd-jiyun left a comment •

edited

Loading

kjoon418 Mar 21, 2025 •

edited

Loading