[3단계 - 체스] 테오(최우성) 미션 제출합니다. #536

woosung1223 · 2023-03-23T13:32:24Z

안녕하세요 카프카 반갑습니다!

저번 리뷰가 정말 도움이 많이 되었습니다 ㅎㅎ

이번에는 코드 구조의 변화가 좀 있었는데요,
기존 구조에 3단계 기능을 얹으려고 하니 추상화 단계가 불안정한 것 같다고 판단했었던 것 같아요!

그리고 저번 리뷰때 말씀하신 피드백은 모두 반영했습니다.

하지만 몇번의 개선 작업이 있었음에도 추상화 레벨이 잘 지켜진 것 같지 않고,
Board나 ChessGame의 크기가 방대해 진 것 같아 그런 부분은 아쉬움이 남습니다.

좋은 설계를 하고 싶은 마음은 큰데, 아직 쉽지 않은 것 같습니다 😅

이번에도 카프카의 피드백 잘 새겨듣도록 하겠습니다! 감사합니다 👍

include42 · 2023-03-23T15:49:16Z

안녕하세요 테오, 리뷰어 카프카입니다.
코드 간단히 살펴보니 변경사항이 많네요! 좋은 방향으로 잘 수정해주셨으리라 생각됩니다. 😄
제가 내일 업무를 좀 일찍 시작하게 될 듯해서, 빠르면 내일 정오, 늦어도 내일 저녁 7시 전에 리뷰 진행 예정입니다.
혹시 리뷰 전에 질문사항이 있으면 편하게 코멘트 달아주세요.

고생 많으셨습니다. 한주 마무리까지 화이팅입니다 💪

woosung1223

안녕하세요 카프카 😃

주로 고민했던 부분은 코멘트로 남겨두었습니다.

다른 부분들도 부족한 점이 많으니, 이번에도 좋은 피드백 부탁드립니다..!
설계와 객체 자체(역할, 책임, 협력)에 대한 관심도 많아서 그런 부분으로도 많이 피드백해주시면 감사할 것 같아요 🥹

바쁘신데 항상 감사합니다!

woosung1223 · 2023-03-24T03:32:10Z

src/main/java/domain/board/Board.java

-import java.util.ArrayList;
-import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;

 public final class Board {



Board의 책임이 방대하다고 생각이 듭니다. (100줄이 넘어가네요 ㅠ)

하지만 저는 상태와 행위를 한 곳에서 관리해야 한다는 입장에서 더 이상 책임을 쪼개서 다른 객체에게 할당하기는 어려울 것 같은데
카프카는 어떻게 생각하시나요?!

코드를 한번 보니 방대하긴 한데, 적어주신 대로 어느정도 책임을 관리하는 것도 맞아서 난감하긴 하네요.
제가 생각하는 개선 가능한 방향은 다음과 같습니다.

전반적인 메소드명 등을 수정할 필요가 있어 보여요. 현재 Square 관련 클래스들이 삭제된 것으로 보이는데, 관련된 메소드명이 그대로 존재해서 읽을때 혼란스러운 부분이 있었습니다.

예를 들어 findSquare 메소드는 Piece를 반환하고 있던데, 그렇다면 메소드명을 findPiece 로 바꿔주면 더 좋지 않을까요?

그리고 아래에도 코멘트 남겨두었지만, 폰일때 대응하는 로직이 너무 많은 느낌입니다. 물론 점수 계산의 경우 불가피해 보이지만(굳이 정리하자면, 유틸 클래스를 하나 만들어서 점수계산만 따로 하게 분리할수는 있겠네요) 이동 로직에 폰 관련 검사 로직이 붙으면서 코드가 한눈에 안들어온다는 느낌을 받았습니다.

저는 이전에 레벨2 진행하시면서 Square, ConcreteSquare, Rank 클래스를 둔 것이 오버엔지니어링이라고 생각되었는데, 이번에 해당 부분이 개선되면서 Board 코드를 보는게 더 편해졌다고 느꼈습니다.
마찬가지로 Pawn에 있어서도, 최대한 로직을 단순하게 하고 불필요한 클래스를 모두 삭제해본 다음, 거기서 다시 아쉬운 부분을 리팩토링해 나가는 것이 좋아보입니다. 때때로 코드의 책임 분리를 과하게 수행할 경우, 가독성이 떨어지기도 하니까요.

가독성과 코드 구조 둘 다 선택할 수 있다면 최고의 상황이겠지만, 때때로 그 중 하나에 우선점을 두고 진행해야 할 때도 있다고 생각합니다. 이부분은 좀 더 가독성을 높이는 방향으로 리팩토링을 진행해 봅시다. 💪

안녕하세요 카프카 고민한 부분에 대해 답변해주셔서 감사합니다!

우선 메소드명 부분은 제가 캐치를 못했어서, 빠르게 수정하도록 하겠습니다.

그리고 말씀해주신 것처럼 폰만을 위한 로직이 많다고 느껴집니다.

특별한 이동규칙에 대해 로직이 많다는 것은
만약 체스게임에서 또 다른 기물이 추가되어 폰같은 특별한 이동규칙을 가지게 된다면,
Board 클래스가 또 한번 변경에 영향을 받을 것을 의미하는 것이라 생각합니다.

마찬가지로 현재 Pawn이 수정되어도 Board가 변경에 영향을 받을 것이구요.

그렇기에 Pawn의 움직임을 Board에서 분리해낼 필요가 있다고 생각이 드네요!

우선 가독성을 높이고 난 뒤, 구조 개선을 진행해보겠습니다 👍👍

woosung1223 · 2023-03-24T04:21:40Z

src/main/java/domain/board/Board.java

+            squareLocations.put(target, new BlackPawn(Color.BLACK));
+            return;
+        }
+        squareLocations.put(target, new WhitePawn(Color.WHITE));
    }


여기서 추상화 단계가 무너진 것 같습니다.. ㅠㅠ
폰의 움직임을 결국 Board가 알아야 되는 상황이 되었는데요,

이러한 국소적인 추상화 레벨의 파괴가 좋은 설계라고는 생각되지 않습니다.

하지만 도메인이 복잡하면 이런 부분은 존재하게 되는건지, 아니면 좋은 설계를 통해 제거할 수 있는 부분인지 궁금합니다.

이부분은 InitPawn과 Pawn 클래스의 변환을 위해 존재하는 로직일까요?
코드를 보면서 이부분이 잘 이해가 가지 않았습니다.
점수 계산이나 이동 특수 연산 등 Pawn 관련된 로직이 많기는 하나, 일단 본 메소드를 사용하지 않아도 되도록 수정해본다면 Board의 로직이 많이 정리될 것으로 보입니다.

당연히 도메인이 복잡해지면 이러한 부분이 생기게 됩니다. 이를 리팩토링을 통해 고치는 것이 옳고요.
그러나 좋은 설계를 수행했다면, 이러한 문제가 생기지 않았을 것이라고 생각합니다.
정답은 없지만, 더 좋은 방향으로 조금씩 고쳐가는 것을 지향해 봅시다 💯

맞습니다.

만약 처음 움직임을 수행했다면 다음에는 InitPawn -> Pawn으로 변환해서 2칸 이상 움직이지 못하게 구현했는데
가독성이 떨어지는 것 같다는 생각이 들긴 합니다.

그리고 폰이 움직일 때마다 수행되기에 불필요한 Pawn -> Pawn 교체도 생기구요.

이 부분은 일단 개선해보겠습니다 👍👍
설계란 참 어렵네요! ㅋㅋ

woosung1223 · 2023-03-24T04:26:28Z

src/main/java/domain/board/ChessGame.java

-        this.board = new Board();
+        this(new Board());
+    }
+


여기서 부생성자 도입을 통해 유연성을 증가시켰습니다.

하지만 현재로서는 테스트에서만 사용되고 있는데요,
저는 개인적으로 public 메소드가 많아지면 문제가 되지만

생성자가 많아지는 것은 의존성을 주입하는 방식만 변화하는 것이기 때문에 괜찮다고 생각합니다.

따라서 테스트를 위해서 생성자를 만드는 것은, 물론 프로덕션 코드에서 사용되지 않기에 코드가 늘어난다는 단점이 있겠지만

테스트에서 가져갈 수 있는 이득이 더 크다고 생각하는데요,

카프카는 어떻게 생각하시나요?

저는 테스트에서만 사용되는 코드가 있는 것에 대해서는 대체로 반대하는 편입니다.
협업하는 과정에서 해당 코드의 목적을 잘못 이해할 수 있기 때문입니다.
다만 생성자에 대해서는 조금 여유롭게 생각하는 편인데, 위의 생성자는 어차피 4단계에서 저장/불러오기 구현시 필요한 생성자니까요.
현재 구조 유지해도 괜찮을 것으로 생각됩니다. 😄

그리고 해당 생성자가 없다면 테스트가 정말 불가한 것일까요?
ChessGameTest의 경우 테스트가 어렵겠다고 이해가 가긴 하는데, PawnTest에서 ChessGame과 Board를 바탕으로 테스트하는걸 보니 그부분은 개선이 필요해 보입니다.

Pawn의 메소드를 통해 Pawn이 movable한 공간을 어떻게 찾는지 테스트해야 하는데, ChessGame과 Board가 테스트에 필요하다는건 Pawn의 책임이 잘못 관리되고 있다는 의미일 수 있습니다.

감사합니다 카프카!

그렇다면 ChessGame 테스트와 같이 통합 테스트의 성격을 갖고 있는 경우
테스트가 어려운 것은 불가피한 것일까요?

생성자 체이닝을 사용하기 전 코드는 굉장히 테스트 메소드마다 내용이 길었는데,
이런 부분은 불가피한 부분인지,

혹은 이런 부분 때문에 ChessGame의 move 메소드를 잘게 나눠야 하는건지.. 궁금합니다!

현재로서는 통합 테스트가 어려울 수 있습니다.
이에 대응하기 위해 필요한 도메인 객체를 세팅해주는 테스트용 정적 클래스를 만들기도 하고,
외부 서버와의 통신을 mocking하거나 특정 서비스를 도커에 올려 테스트하는 경우도 존재합니다.

현재 단계에서는 단위 테스트를 하면서 다른 레이어에 대한 부분을 mocking하는 법을 배우지 않아 더 복잡할 수 있습니다.
레벨2에서 이에 대해 배우게 될 텐데, 그 과정에서 꼼꼼히 정리해두고 고민해보는 과정이 필요하다고 생각합니다. 💪

다만 이러한 부분 때문에 우리가 TDD를 하고자 노력한다는 생각도 들어요.
물론 테스트 기술의 부족도 있겠으나..우리가 테스트하기 어려운 코드를 만드는 경우를 보면,
해당 메소드가 여러 가지 일을 수행하거나, 혹은 입력에 대한 검증과 기대값이 명확하지 않아 테스트를 짜기 어려운 경우도 존재하기 때문입니다.
ChessGame의 경우 명세는 명확해서 이에 해당한다고 생각하지는 않고요, 다만 아래에서 리뷰한 폰의 경우 테스트에서 다른 클래스를 빌려야 하는 것이 이러한 이유에서 기인한다고 생각되어 추가로 코멘트 적어두었습니다.

woosung1223 · 2023-03-24T05:04:21Z

src/main/java/domain/piece/pawn/Pawn.java

+        }
+        return isMovableWhenMovingVariates(start, end);
+    }
+


여기서 isMovable 메소드를 오버라이딩해서 상위 타입의 기능을 덮어씌우고 있는데요,

사실 이것은 상속을 잘못 사용하고 있는 것이라 생각합니다.
캡슐화를 저해시키고 있으니까요.

개인적으로 이 역시도 좋은 구조라고는 생각되지 않는 것 같습니다.
하지만 폰의 움직임을 공통 인터페이스로 다루고 싶어 이런 구조가 나왔는데요,

이런 부분은 기능에 따라 어느정도 감안이 가능한 부분일까요?

캡슐화를 저해시킨다는 말이 정확히 어떤 의미인지 말씀해주실 수 있으실까요?
저는 상속을 통해서 기능이 변경되는 것이 충분히 가능하다고 생각합니다.

다만 고민되는 부분이, 여기서 Situation이 꼭 있어야 할까요?
Pawn이 아니면 해당 클래스는 크게 필요해 보이지는 않는데... (대각선 이동때문에 추가한 것으로 보여서요)
end 포지션에 있는 Piece를 전달해줘서 알아서 판단하게 하는게 더 나을수도 있고요.

제가 캡슐화를 저해시킨다고 말씀드린 이유는,

상위 메소드 isMovable 은 이미 Piece 객체에서 구현이 완료된 상태고

Piece 객체만 보았을 때는 하위 클래스에서 구현을 할 것이라는 보장을 어디서도 얻을 수 없습니다.

반면 Pawn 클래스에서는 이 isMovable 메소드를 오버라이딩해 행위를 바꾸고 있는데요,

이렇다면 상위 클래스(Piece)에서 isMovable를 정의하는 이유는 사실 없긴 합니다. Pawn에 한해서는요.

만약 이런 상황에서 모든 기물들의 움직임 규칙이 수정되어야 한다고 하겠습니다.
(예를 들어, 같은 팀 기물이 있는 곳으로도 이동할 수 있다고 해볼게요. 그럴 일은 없겠지만요)

그렇다면 Piece의 isMovable 클래스만 수정하면 될 것 같지만, (모든 기물들의 공통구현처럼 보이므로)
사실 Pawn까지 변경되어야합니다. 기능 자체를 덮어씌우고 있으니까요!!

따라서 모든 기물들의 움직임을 Is-A 관계로 정의하는 것이 아니므로 변경을 격리시킬 수 없고
이런 부분에 있어서 캡슐화를 저해시킨다. 라고 말했던 것 같습니다!

즉, 요약하자면

추상 메소드가 아닌 메소드를 오버라이딩하고 있으므로 Piece 내부 구현의 변경이 Pawn의 변경으로 이어질 수 있다.

가 될 것 같습니다. 카프카의 생각은 어떠실까요?!

그리고 말씀해주신 Situation 부분은 Piece를 사실 처음에 전달하려고는 했었는데요!

Piece를 상속받는 Pawn이 Piece를 인자로 받아도 되나.. 라는 생각이 들었었습니다.
추상화 레벨을 거스르는게 아닐까? 라고 생각했었거든요.

그런데 글을 쓰면서 생각해보니까
파라미터로 받게 되는 객체는 Piece 객체가 아니고 Piece를 구현하는 Pawn, Knight.. 등의 객체이므로
동등한 추상화 레벨이겠구나 라는 생각이 드네요

카프카의 말씀대로 수정해보겠습니다!

저는 상위 클래스에서 어떤 동작을 구현하더라도, 하위 클래스에서 충분히 그 동작을 override할 수 있다고 생각합니다.
물론 말씀해주신 것처럼 수정이 필요할 경우, 각 클래스를 수정해줘야 하는 불편함이 있겠지만요.
생각해보면, 이 케이스에서는 수정이 필요한 경우 각 클래스를 모두 살펴보는게 맞는게 아닐까요?

예를 들어 getScore 메소드의 점수 저장 방식이 Int에서 Double로 변경되었다면, Piece에서만 수정해주는게 옳다고 충분히 생각할 수 있습니다.

그러나 isMovable의 경우, 메소드 특성상 각 클래스별로 충분히 변경이 있을 수 있다고 추론됩니다. (제 생각일 뿐이므로, 동의하지 않을 수도 있습니다. 다만 기물별로 해당 로직이 달라질 수 있으니까요)

그리고 이러한 부분이 걱정된다면, 각 상속받은 클래스별로 꼼꼼하게 테스트를 작성해두면 됩니다. 테스트 실패 여부를 통해 올바르게 리팩토링이 되었는지 확인할 수 있다고 생각합니다.

위에 저렇게 적기는 했지만, 어쩌면 제일 이상적인 건 Piece에서 구현을 수행하지 않고 하위 클래스별로 로직을 참고하는 것일 수도 있겠습니다. 이 부분은 고민이 많이 되는데, 추후 리뷰하면서 코드 다시한번 꼼꼼히 살펴보겠습니다. 😄

include42

안녕하세요 테오, 리뷰어 카프카입니다.

3단계 진행 잘 해주셨습니다! 이전보다 코드도 많이 좋아졌고, 새로운 명세들도 잘 적용되었네요.
기능상에는 크게 문제가 없었지만, 구조를 좀 더 단순화하면 좋겠다고 생각되는 지점들이 중간중간 존재했습니다. 해당 부분들은 테오도 질문을 주셨던 부분이니, 고민해서 개선해보면 좋겠습니다.

해당 리팩토링 작업은 4단계 작업과 함께 진행되면 더 좋을 것 같아요.
DB를 적용하게 되면 구조의 개선이 필요할텐데, 함께 진행하면서 더 좋은 방향을 고민해보기를 바랍니다.
이 미션은 approve 하겠습니다. 고생하셨습니다 👍

include42 · 2023-03-24T10:18:43Z

src/main/java/controller/ChessController.java

+        commander.put(Command.START, this::start);
+        commander.put(Command.END, this::end);
+        commander.put(Command.MOVE, this::move);
+        commander.put(Command.STATUS, this::status);


이부분을 독특하게 작성해주셨네요!
commander 클래스를 별도로 만들어줘야 한다는 점에서 조금 걸리긴 하나,
구조 개선에 있어서는 역할을 분명히 한다는 생각도 듭니다. 일단 유지해 봅시다 👍

include42 · 2023-03-24T10:20:36Z

src/main/java/controller/ChessController.java

+        Coordinate startCoordinate = CoordinateAdapter.convert(arguments.getArgumentOf(START_COORDINATE_INDEX));
+        Coordinate endCoordinate = CoordinateAdapter.convert(arguments.getArgumentOf(END_COORDINATE_INDEX));


arguments를 별도의 일급 컬렉션으로 뺀 부분이 매우 좋았습니다.
다만 그럼에도 참조하는 번지수는 Controller에서 상수로 지정해주는 부분이 아쉽네요.
ComamndArguments 내에 getStartCoordinateInfo, getEndCoordinateInfo 와 같은 메소드가 있어도 괜찮지 않을까요?
이 경우 아예 getArgumentOf를 삭제하고, 위에 적어둔 메소드로만 접근하게 해서 외부 참조 동작을 통제할 수 있다는 생각이 들어서요.

물론 이부분은 이견이 있을 수 있으므로, 자유롭게 의견 적어주시고 반영 여부 결정해주세요 👍

동의합니다 👍👍

좌표계 변환 자체를 완전히 Arguments의 책임으로 변경해볼게요!

include42 · 2023-03-24T10:21:38Z

src/main/java/controller/adapter/inward/CoordinateAdapter.java

@@ -22,17 +22,16 @@ private static void validateSize(final String frontCoordinate) {

    private static int convertToRow(final String frontCoordinate) {
        char pureRow = frontCoordinate.charAt(1);
-        if (pureRow >='0' && pureRow <= '9') {
+        if (Character.isDigit(pureRow)) {


👍 👍 👍

include42 · 2023-03-24T10:24:34Z

src/main/java/controller/adapter/outward/RenderingAdapter.java

@@ -0,0 +1,53 @@
+package controller.adapter.outward;


혹시 패키징이나 네이밍을 결정하면서 참고한 구조 내지 자료가 있을까요?
따로 학습 진행해서 적용하신 느낌인데, 어떤 식으로 적용하게 되었는지 궁금해서요 😄

사실 따로 학습한 부분은 없고, 제가 정의한 것인데요!

이전 미션까지는 데이터의 흐름이
Input -> Controller -> Domain - 입력 시
Domain -> Controller -> Output 출력 시

이렇게 구성되었는데,
과연 InputView에서 받는 데이터나, outputView로 보내주는 데이터나
Domain에서 반환하거나 InputView에서 반환하는 데이터를 그대로 쓸 일이 있을까?
싶어서 Layer를 하나씩 정의한 느낌입니다.

즉,
Input -> inward패키지 -> Controller -> Domain - 입력 시
Domain -> Controller -> outward패키지 -> Output 출력 시

이런 그림을 원해서 위와 같이 패키지, 클래스를 만들었습니다!
저번 리뷰때 OutputView에서 도메인 의존성을 분리하라는 카프카의 말씀을 저만의 방식으로 접목시킨 것 같습니다!!

괜찮은 방법일까요?
사실 이걸 추상화하면 어댑터 패턴이 될 것 같기는 한데.. 좋은 방법인지 궁금합니다.

충분히 잘 해주셨다고 생각됩니다.
물론 현재 구조를 모든 프로젝트에 적용할 수 있는지는 고민이나, 현재 구조에서는 가독성좋게 잘 구현되었다고 생각해요.
사실 이 구조는 제가 클린 아키텍처를 공부하면서 본 헥사고날 아키텍처와 유사한 점이 있어서, 그 부분을 참고하셨나 싶어서 물어본 것이었습니다. 해당 구조를 지금 적용하는 것은 너무 선행학습이라고 생각해서요 😄 그런데 직접 고민해서 유사한 결론까지 이르렀다니, 성취도가 높다고 칭찬을 드리고 싶습니다 💯

이번 미션에서의 설계를 기억해두시고, 이후 레벨2에서 구조를 작성할 때 참고해보면 도움이 많이 될 것으로 생각됩니다. 👍

include42 · 2023-03-24T10:39:56Z

src/main/java/domain/board/Board.java

+            squareLocations.put(target, new BlackPawn(Color.BLACK));
+            return;
+        }
+        squareLocations.put(target, new WhitePawn(Color.WHITE));
    }


이부분은 InitPawn과 Pawn 클래스의 변환을 위해 존재하는 로직일까요?
코드를 보면서 이부분이 잘 이해가 가지 않았습니다.
점수 계산이나 이동 특수 연산 등 Pawn 관련된 로직이 많기는 하나, 일단 본 메소드를 사용하지 않아도 되도록 수정해본다면 Board의 로직이 많이 정리될 것으로 보입니다.

당연히 도메인이 복잡해지면 이러한 부분이 생기게 됩니다. 이를 리팩토링을 통해 고치는 것이 옳고요.
그러나 좋은 설계를 수행했다면, 이러한 문제가 생기지 않았을 것이라고 생각합니다.
정답은 없지만, 더 좋은 방향으로 조금씩 고쳐가는 것을 지향해 봅시다 💯

include42 · 2023-03-24T10:53:27Z

src/main/java/domain/board/ChessGame.java

-        this.board = new Board();
+        this(new Board());
+    }
+


저는 테스트에서만 사용되는 코드가 있는 것에 대해서는 대체로 반대하는 편입니다.
협업하는 과정에서 해당 코드의 목적을 잘못 이해할 수 있기 때문입니다.
다만 생성자에 대해서는 조금 여유롭게 생각하는 편인데, 위의 생성자는 어차피 4단계에서 저장/불러오기 구현시 필요한 생성자니까요.
현재 구조 유지해도 괜찮을 것으로 생각됩니다. 😄

그리고 해당 생성자가 없다면 테스트가 정말 불가한 것일까요?
ChessGameTest의 경우 테스트가 어렵겠다고 이해가 가긴 하는데, PawnTest에서 ChessGame과 Board를 바탕으로 테스트하는걸 보니 그부분은 개선이 필요해 보입니다.

Pawn의 메소드를 통해 Pawn이 movable한 공간을 어떻게 찾는지 테스트해야 하는데, ChessGame과 Board가 테스트에 필요하다는건 Pawn의 책임이 잘못 관리되고 있다는 의미일 수 있습니다.

include42 · 2023-03-24T10:59:34Z

src/main/java/domain/piece/pawn/BlackInitPawn.java

+
+import java.util.List;
+
+public final class BlackInitPawn extends Pawn {


InitPawn을 만들어준 것이 좋은 아이디이이긴 하나, 오히려 코드 복잡도를 많이 증가시킨다고 생각됩니다.
InitPawn과 일반 Pawn의 차이가 이동 외에 어떤 것이 있을지 잘 모르겠고, 결국 중복되는 로직을 늘린다고 생각합니다.
일단 이번 미션에서 이정도까지 복잡한 구조를 작성해주시지는 않아도 괜찮을 것 같으니,
Pawn 관련 클래스 5개를 합쳐서 하나의 Pawn으로 구현하는게 가능할까요?

도전해보겠습니다 👍👍

include42 · 2023-03-24T11:08:27Z

src/main/java/domain/piece/pawn/Pawn.java

+        }
+        return isMovableWhenMovingVariates(start, end);
+    }
+


캡슐화를 저해시킨다는 말이 정확히 어떤 의미인지 말씀해주실 수 있으실까요?
저는 상속을 통해서 기능이 변경되는 것이 충분히 가능하다고 생각합니다.

다만 고민되는 부분이, 여기서 Situation이 꼭 있어야 할까요?
Pawn이 아니면 해당 클래스는 크게 필요해 보이지는 않는데... (대각선 이동때문에 추가한 것으로 보여서요)
end 포지션에 있는 Piece를 전달해줘서 알아서 판단하게 하는게 더 나을수도 있고요.

include42 · 2023-03-24T11:09:47Z

src/test/java/domain/board/BoardTest.java

+    }
+
+    @Test
+    @DisplayName("폰은 점수가 1점이다")


각 기물의 점수 테스트는 해당 기물 테스트에서 수행해도 충분할 것으로 보입니다.

include42 · 2023-03-24T11:14:47Z

src/main/java/domain/board/Board.java

-import java.util.ArrayList;
-import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;

 public final class Board {



저는 이전에 레벨2 진행하시면서 Square, ConcreteSquare, Rank 클래스를 둔 것이 오버엔지니어링이라고 생각되었는데, 이번에 해당 부분이 개선되면서 Board 코드를 보는게 더 편해졌다고 느꼈습니다.
마찬가지로 Pawn에 있어서도, 최대한 로직을 단순하게 하고 불필요한 클래스를 모두 삭제해본 다음, 거기서 다시 아쉬운 부분을 리팩토링해 나가는 것이 좋아보입니다. 때때로 코드의 책임 분리를 과하게 수행할 경우, 가독성이 떨어지기도 하니까요.

가독성과 코드 구조 둘 다 선택할 수 있다면 최고의 상황이겠지만, 때때로 그 중 하나에 우선점을 두고 진행해야 할 때도 있다고 생각합니다. 이부분은 좀 더 가독성을 높이는 방향으로 리팩토링을 진행해 봅시다. 💪

woosung1223 added 30 commits March 20, 2023 17:44

refactor: 추상메소드가 아닌 람다를 사용해 방향을 찾도록 변경

8a20fd1

refactor: 불필요한 생성자에 대한 테스트는 제거

c0f2674

test: 예외 케이스뿐 아니라, 정상 흐름에 대한 테스트도 생성

844e12b

docs: 구현에 맞게 기능 목록 최신화

96d2a03

docs: 3단계 기능 목록 추가

a675aba

refactor: 메소드 네이밍 수정

397db84

fix: 보드 이미지에서 캐싱 된 객체가 가변이기에 새로운 객체로복사해서 반환하도록 수정

ace2935

refactor: 불필요한 Boolean Wrapping 제거

56c6248

feat: 올바른 턴이 아니라면 기물의 움직임을 제한할 수 있다

c379d07

refactor: Turn에서 getter 삭제, 상태 검사 메소드를 도메인 친화적인 용어로 변경

47a7e0b

chore: 코드 포맷팅을 통한 가독성 개선

adb7b9e

fix: 게임 진행 의도와 다르게 턴이 행동하던 버그 수정

e72e2d7

fix: 턴에서 getter 삭제로 인한 테스트 변경

6ebb81b

refactor: 칸들을 2중 리스트 형태가 아닌 Map으로 관리하도록 변경

a82b2da

docs: 구현 변경에 따른 기능목록 변경

50f7bb8

refactor: 캐싱된 칸을 반환할 때 가변 객체이므로 clone해서 반환

53b948c

refactor: 도메인 내부 구현 변경에 따른 출력 구현방식 변경

afc0124

feat: 기물은 다른 진영의 기물을 잡을 수 있다

f8f5a5e

fix: 테스트 클래스가 도메인 클래스명과 다른 문제 해결

f8eb8da

feat: 킹이 잡히면 게임이 종료된다

8142fff

docs: 완성된 기능 체크 및 고민사항 기재

7a2e345

refactor: 불필요한 추상화 단계 압축

85ecc60

chore: 패키지 추가 분리

a9b00ec

refactor: 도메인 용어에 맞도록 Camp -> Color 네이밍 변경

22407a2

chore: 상속가능성이 없는 클래스 final화

fe990f7

refactor: 첫번째 움직임인지는 기물에서 알도록 변경

170367d

chore: 메소드 네이밍을 클라이언트의 기준에서 이해할 수 있도록 변경

98e8ba6

refactor: pawn의 초기 움직임을 상태가 아닌 객체로 표현

d09048d

refactor: 칸은 불변객체가 되었기에 Cloneable 구현 삭제

a5809eb

chore: 변수명 통일성 있게 변경, 코드 포맷팅

aaee1bb

woosung1223 added 25 commits March 22, 2023 16:18

refactor: 메소드명을 보다 이해하기 쉽도록 수정

8ff6b7e

fix: 폰이 연속적으로 두칸 갈 수 있던 버그 수정

ea1de33

feat: 각 진영의 최종 점수를 계산할 수 있다

5bec05f

feat: 게임 결과 출력을 위한 흐름제어 생성

f417116

refactor: 중복해서 검사하는 예외처리 기능 삭제

5e6f178

refactor: 사용하지 않는 메소드 제거

d9ba41a

refactor: 검증 메소드 병합

1147a41

refactor: Square, Piece -> Piece 통합

b9c93f2

refactor: Color에 대한 getter 삭제, 상태검사 메소드로 변경

400ce9a

refactor: 메소드명을 보다 이해하기 쉽도록 변경

92b8542

refactor: Pawn 움직임 체크에 대한 중복 코드 제거

a385625

refactor: 부생성자가 주생성자를 이용하도록 변경

37cce48

refactor: Character.isDigit을 사용함으로써 가독성 개선

114b89b

refactor: 뷰에서 도메인 패키지 의존성 분리

2bee1e3

refactor: 컨트롤러 단 패키지 분리

6fd7258

feat: 명령 파라미터를 객체로 분리

f226eb4

refactor: status 입력 시 게임이 끝나지 않도록 수정

46a4228

refactor: 출력 메세지 조정

efe7f23

feat: 공동 우승 기능 추가

98ebd72

refactor: 메소드 추가 분리

f4be3bc

fix: 킹을 잡고 게임이 끝나면 점수가 출력되지 않던 문제 해결

707270b

docs: 기능목록 및 다이어그램 최신화

a8b9850

test: 컨트롤러 레이어 테스트 보충

01b786f

refactor: 테스트 코드 리팩토링 및 보완

12c7e7a

test: 폰 대각 공격 테스트케이스 추가

6fac6f6

fix: 폰이 같은 팀을 공격할 수 있던 버그 수정

5e9f40f

woosung1223 commented Mar 24, 2023

View reviewed changes

include42 approved these changes Mar 24, 2023

View reviewed changes

include42 merged commit fa29ccd into woowacourse:woosung1223 Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3단계 - 체스] 테오(최우성) 미션 제출합니다. #536

[3단계 - 체스] 테오(최우성) 미션 제출합니다. #536

woosung1223 commented Mar 23, 2023 •

edited

Loading

include42 commented Mar 23, 2023

woosung1223 left a comment

woosung1223 Mar 24, 2023

include42 Mar 24, 2023

include42 Mar 24, 2023

woosung1223 Mar 25, 2023

woosung1223 Mar 24, 2023

include42 Mar 24, 2023

woosung1223 Mar 25, 2023

woosung1223 Mar 24, 2023

include42 Mar 24, 2023

woosung1223 Mar 25, 2023

include42 Mar 26, 2023

woosung1223 Mar 24, 2023

include42 Mar 24, 2023

woosung1223 Mar 25, 2023 •

edited

Loading

woosung1223 Mar 25, 2023

include42 Mar 26, 2023

include42 left a comment

include42 Mar 24, 2023

include42 Mar 24, 2023

woosung1223 Mar 25, 2023

include42 Mar 24, 2023

include42 Mar 24, 2023

woosung1223 Mar 25, 2023 •

edited

Loading

include42 Mar 26, 2023

include42 Mar 24, 2023

include42 Mar 24, 2023

include42 Mar 24, 2023

woosung1223 Mar 25, 2023

include42 Mar 24, 2023

include42 Mar 24, 2023

include42 Mar 24, 2023

		Coordinate startCoordinate = CoordinateAdapter.convert(arguments.getArgumentOf(START_COORDINATE_INDEX));
		Coordinate endCoordinate = CoordinateAdapter.convert(arguments.getArgumentOf(END_COORDINATE_INDEX));


		import java.util.List;

		public final class BlackInitPawn extends Pawn {

[3단계 - 체스] 테오(최우성) 미션 제출합니다. #536

[3단계 - 체스] 테오(최우성) 미션 제출합니다. #536

Conversation

woosung1223 commented Mar 23, 2023 • edited Loading

include42 commented Mar 23, 2023

woosung1223 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

woosung1223 Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

include42 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

woosung1223 Mar 25, 2023 • 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

woosung1223 commented Mar 23, 2023 •

edited

Loading

woosung1223 Mar 25, 2023 •

edited

Loading

woosung1223 Mar 25, 2023 •

edited

Loading