2주차 미션/ 서버 2조 임제형 #36

kisusu115 · 2024-09-26T14:58:28Z

Test 코드를 작성하는 도중
기존에 private void 형태로 로직 내부에 사용되던 메서드를, 테스트를 위해 public에 객체를 반환하는 형태로 바꾸게 되었는데
이렇게 테스트 코드를 작성할 때 기존의 메서드를 변경 시켜가며 테스트 하는 것이 맞는지 궁금합니다!

구체적으로 말씀드리면
RandomLadderCreator의 makeRandomLine() 메서드를 변경시켜
LadderGameTest의 랜덤_사다리_생성_시_주위_노드_정상_여부_확인() 을 테스트 하였습니다.

…word in Row class

추상클래스인 WrapperLadderCreator 클래스를 추가하여 중복 코드 제거 RandomLadderCreator에서 null을 리턴하는 방식 대신 재귀호출로 로직 변경 Row에서 즉시 print 하는 것이 아닌 StringBuilder 객체 사용 LadderGameTest에 랜덤 라인 생성 및 주위 노드 정상 검증 테스트 구현

hamhyeongju

코드 잘 봤습니다!
전반적으로 코드를 잘 작성해 주신 부분은 좋았지만, 강의나 워크북에서 언급했던 내용이 잘 반영이 안돼있는듯한 느낌을 받았습니다. 미션 구현도 중요하지만 미션의 의도를 파악하고 그 부분을 신경쓰면서 미션 진행하시면 더 도움될 것 같습니다.
코드 리뷰와 2week-completed 코드 보시면서 본인의 코드와 비교해보는 시간을 가져보시면 좋을 것 같습니다. 수고 많으셨습니다!

hamhyeongju · 2024-09-28T13:38:09Z

src/test/java/ladder/LadderGameTest.java

+        LadderSize ladderSize = LadderSize.of(numberOfRow, numberOfPerson);
+        LadderGame ladderGame = LadderGameFactory.createCustomLadderGame(ladderSize);
+
+        LadderCreator ladderCreator = ladderGame.getLadderCreator();


테스트에서는 단위 테스트를 위해 특정 상황을 가정하는 경우가 많아서 항상 팩토리 패턴을 통해 객체를 생성할 필요는 없을 것 같습니다. 때문에 테스트만을 위해 getLadderCreator 를 사용하는건 적절하지 않아 보입니다.

hamhyeongju · 2024-09-28T13:41:20Z

src/main/java/ladder/LadderRunner.java

+        return lastColPosition.getValue();
+    }
+
+    private void moveTurnPrinting(LadderPosition ladderPosition){


move와 print는 각각의 기능으로 봐야할 것 같아요. 메서드가 여러 일을 하고 있는 것 같습니다!

hamhyeongju · 2024-09-28T13:55:14Z

src/main/java/ladder/LadderRunner.java

+
+    public int run(Position position) {
+        LadderPosition ladderPosition = LadderPosition.of(Position.from(0), position);
+        while(!ladderWrapper.isLadderPositionAtLastRow(ladderPosition)){


LadderSize 를 기반으로 반복문을 제어하는게 가독성이 좋을 것 같습니다! fori문을 사용하면 인덱스의 변화에 따른 코드 실행이 눈에 더 잘 들어올 것 같아요.

hamhyeongju · 2024-09-28T14:05:28Z

src/main/java/ladder/LadderWrapper.java

+
+    public boolean isLadderPositionAtLastRow(LadderPosition ladderPosition){
+        Position rowPosition = ladderPosition.getRowPos();
+        return rowPosition.getValue() == rows.length;


getValue로 비교하기보단 Position 내부 메서드로 처리하는게 어떨까요?

hamhyeongju · 2024-09-28T14:24:06Z

src/main/java/ladder/LadderWrapper.java

+    private void printRows(LadderPosition ladderPosition){
+        Position nowRowPosition = ladderPosition.getRowPos();
+        for(int rowIndex=0; rowIndex<rows.length; rowIndex++){
+            if(rowIndex != nowRowPosition.getValue()) { System.out.println(rows[rowIndex].buildRowString()); continue; }


반복문과 조건문이 겹쳐서 이중 들여쓰기가 되었습니다! 메서드 추출이 필요할 것 같아요!

hamhyeongju · 2024-09-28T14:27:41Z

src/main/java/ladder/Row.java

+        return stringBuilder;
+    }
+
+    public StringBuilder buildStarRowString(Position position){


buildRowString() 과 buildStarRowString() 은 로직이 매우 유사한 것 같아요. 하나의 메서드를 사용하되, * 를 출력하는 기능을 따로 메서드로 만들면 가독성도 좋고 해당 코드를 사용하는 개발자 입장에서도 사용법을 더 명확히 이해할 수 있을 것 같습니다.

hamhyeongju · 2024-09-28T14:29:07Z

src/main/java/ladder/Node.java

+    }
+
+    public int getDirectionValue(){
+        return direction.getValue();


Node 의 값을 외부로 노출하는 건 캡슐화의 위반이죠. 코드 전반적으로 의미없는 getter가 많이 보이는데 더 신경을 쓰시면 좋을 것 같습니다.

hamhyeongju · 2024-09-28T14:30:20Z

src/main/java/ladder/Row.java

+    }
+
+    public int getNodesSize(){
+        return nodes.length;


LadderSize가 담당할 수 있는 기능 아닐까요?

hamhyeongju · 2024-09-28T14:38:37Z

src/main/java/ladder/creator/WrapperLadderCreator.java

+
+import ladder.*;
+
+public abstract class WrapperLadderCreator implements LadderCreator {


LadderCreator를 추상 클래스를 도입해서 해결하려고 생각하신 부분이 돋보이네요.
하시만 상속을 통한 기능의 구현은 자칫하면 부모와의 강한 결합으로 캡슐화를 무너뜨리고, 유지보수를 힘들게 만드는 원인이 됩니다.
1, 2주 차 강의와 워크북 내용이자 미션 힌트에서 언급했듯이 조합 기능을 이용해서 구현해 보는게 목표였는데 아쉬움이 남습니다.
강의와 워크북 내용을 그냥 넘기지 말고 제대로 이해하면서 미션 진행하시면 좋을 것 같습니다.

hamhyeongju · 2024-09-28T14:40:12Z

src/main/java/ladder/creator/RandomLadderCreator.java

+        return makeRandomLadderCreator(ladderWrapper);
+    }
+
+    public static RandomLadderCreator emptyLadderFrom(LadderSize ladderSize) {


랜덤으로 사다리를 그리는게 목적인 클래스인데 빈 사다리를 가지는 인스턴스를 반환하는게 어떤 의미가 있을까요?

자세한 코드 리뷰 너무 감사합니다!! 나름 객체 지향적으로 작성해보고자 했는데 문제가 많았네요..
특히 getter를 줄이는 부분과 캡슐화에 대해선 생각을 많이 해봐야 할 것 같습니다
항상 시간 내서 상세한 코드 리뷰 해주셔서 너무 감사합니다!

랜덤으로 사다리를 그리는게 목적인 클래스인데 빈 사다리를 가지는 인스턴스를 반환하는게 어떤 의미가 있을까요?

Test에서 억지로 특정 상황을 구성하는 과정에서 로직 상으로 의미 없는 메서드가 나온 것 같습니다..
혹시 이미 테스트 코드 작성 후에, 추가 테스트를 하는 과정에서
기능 구현을 위한 로직과 별개로 새로 만들어지는 메서드나,
기존 메서드의 형태(반환 타입 등의 변경 : void->특정 객체)가 바뀌는 것에 대해 어떻게 생각하시나요??

TDD를 완벽히 했다면 고민 할 필요가 없겠지만 놓친 부분이 많아 검증하지 못한 부분에 대해 추가적으로 테스트를 하려 할 때 이런 문제가 발생하는 것 같은데 파트장님 께서는 어떻게 대응하시는지 궁금합니다!

음... 1주 차 강의에서도 언급 했듯이 신기하게도 객체 지향과 TDD 는 겹치는 부분이 많아요. 그래서 일단 질문에 대한 답을 드리자면, 좋지 않다고 생각합니다.
모든 상황에 통용되는 대답이 될 순 없겠지만, 테스트만을 위한 메서드가 존재하거나 비지니스 로직과는 상관없는 코드가 테스트를 위해 포함된다면, 그건 잘못 작성된 코드일 가능성이 높습니다. 테스트를 작성하기 위한 로직은 해당 객체가 가져야 하는 책임이라고 볼 수 있을까요? 무언가 잘못 설계된 코드에 대한 임시방편일 뿐이라고 생각합니다.

우선 억지로 특정 상황을 구성 부터가 무언가 코드의 책임이 모호하거나 여러 일을 동시에 할 수 있기 때문일 수 있습니다. 기본적으로 코드를 작성 할 때, 발생 가능한 여러 특정 상황에도 항상 의도한대로 작동하도록 만들어야 합니다. 그렇다면 그런 상황들에 대해서 테스트가 가능해야 하는데, 하지만 막상 그렇지 못한 경우가 많죠. 해당 기능을 테스트 하기 위해서 필요한 정보를 구성해주려는데 책임이 많거나 외부 모듈과의 강하게 의존하고 있다면 그런 일이 발생하기 쉽습니다. 때문에 작성한 코드가 충분히 객체 지향적인가,, 혹은 테스트를 하기 좋은 코드인가를 생각해보면 좋을 것 같습니다. 물론 반대로 테스트가 과하거나 해당 로직에서 테스트할 부분이 아닐 수도 있겠죠.

해당 미션에선 테스트하기 어려운 부분이라고 하면, Radom값을 다루는 부분 뿐입니다. 이 부분을 최대한 핸들링하기 위해, LadderCreator <- CustomLadderCreator <- RandomLadderCreator (상속) 구조로 작성하는 것이 아닌, LadderCreator <-(상속) CustomLadderCreator, RandomLadderCreator 으로 분리하고, CustomLadderCreator <-(의존) RandomLadderCreator 구조로 작성한 것입니다. 힌트에서 언급했던 조합을 이용해 랜덤 기능을 분리했고, 덕분에 랜덤값을 직접적으로 다루지 않아도, CustomLadderCreator 에 대한 유효성 검증 로직과 테스트를 잘 작성하여 랜덤으로 사다리 라인이 생성되더라도 그 결과를 신뢰할 수 있도록 만들어 준 것입니다.
만약 랜덤값을 직접 다루는 부분이 아닌 다른 부분에서 테스트에 문제가 있었다면, 구현부의 코드에서 문제가 있는지 다시 확인해 보는게 좋겠습니다.

이런 부분들은 솔직히 그냥 봐서는 알기 쉽지 않습니다. 어디까지가 객체 지향적인 코드인지, 해당 객체가 적절하게 분리된 책임을 가지고 있는지, 테스트를 작성하기 좋은 코드인지, 그렇다면 어떻게 리팩토링을 해야 앞의 문제를 해결할 수 있는지 등은 너무나 알기 힘듭니다. 그래서 제가 언급했듯이 코드를 작성하는건 훈련이 되어야 한다고 말씀드린거고 코드를 그냥 보기만 하는게 아니라 기계적으로 따라치고, 기계적으로 객체 지향 개발 원칙을 지키고 이런 부분을 강조한 겁니다. 이런 부분은 직접 머리 싸매면서 고민하고, 다시 엎어서 구현하고 해가며 스스로 터득하는게 중요하다고 생각합니다.

쓸데 없이 말이 길어진 것 같네요. 3줄 요약 찍고 마무리 하겠습니다.

테스트만을 위한 로직을 작성하는 건 바람직하지 않다.

잘 작성된 코드는 테스트가 쉽다. 반대로 테스트가 어려운 코드는 잘못 작성되었을 가능성이 높다.

테스트 작성이 어렵다면, 구현부의 코드를 다시 점검해보자.

늦은 시간까지 친절한 답변 너무 감사드립니다
말씀해 주신 부분 생각하며 코드 작성해 나가도록 하겠습니다!
정말 감사드립니다..

hamhyeongju and others added 8 commits September 17, 2024 11:10

feat : add basic code and test

adb3d07

move : move domain to ladder package

7c31908

refactor : change access modifier of row to private and add final key…

1b840fa

…word in Row class

2주차 기본 코드

ecb3b54

feat : 기능 구현 사항 2, 3 추가

b7a4b2e

refactor : 정적 팩토리 메서드로 Creator 생성 방식 변경

8c02b54

refactor : Row 클래스의 StringBuilder 반환 메서드에서 개행 문자 제거 및 메서드 이름 변경

67b8f56

hamhyeongju reviewed Sep 28, 2024

View reviewed changes


		import ladder.*;

		public abstract class WrapperLadderCreator implements LadderCreator {

2주차 미션/ 서버 2조 임제형 #36

Are you sure you want to change the base?

2주차 미션/ 서버 2조 임제형 #36

Uh oh!

Conversation

kisusu115 commented Sep 26, 2024

Uh oh!

hamhyeongju left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamhyeongju Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hamhyeongju Sep 29, 2024 •

edited

Loading