[1, 2단계 - 체스] 히이로(문제웅) 미션 제출합니다. #522

MoonJeWoong · 2023-03-17T08:52:50Z

안녕하세요 영이! 히이로입니다.

이번 체스미션을 페어와 진행하며 집중한 부분은 다음과 같습니다.

객체를 설계할 때 적절한 책임과 역할이 부여 되었는지 확인하며 설계하려고 노력했습니다.
출력을 위한 용도 외 getter를 최대한 사용하지 않기 위해 값 객체간 연산이 가능하도록 하려고 노력했습니다.

그리고 미션을 진행하며 들었던 궁금했던 부분은 다음과 같습니다.

Position 객체 간 이동 방향을 판단하는 역할을 수행하기 위해 Direction 클래스를 설계했습니다.
- Direction 클래스 작성 시 함수형 인터페이스 적용을 해봤는데 구현 방식 상 어색한 부분은 없었는지, 개선할 부분은 없는지 궁금합니다.

Position객체가 다른 Position 간의 연산을 책임지도록 했습니다. 그러다 보니 서로 다른 두 position 객체 간에 다다를 수 있는 직선 상 경로를 계산하는 책임도 지게 되었습니다. 미션을 제출하며 다시 돌아보니 Position의 역할이 너무 과도한가라는 생각이 들었습니다.
- 한 객체에 역할과 책임이 과도하게 부여되었다고 판단되었을 때 어떤 방식으로 객체를 잘 분리할 수 있을지 고민이 되는데 혹시 객체 책임 분리에 대한 좋은 사고방식이 있다면 한 번 의견을 구하고 싶습니다.

Position 클래스에서 move 메서드를 작성할 때 여덟 방향에 대해 모두 작성해야 한다고 생각했습니다. 그런데 구현 완료 후 다시 보니 left right은 테스트 메서드에서만 사용하게 된 것을 확인했습니다. 이런 경우 해당 메서드들을 삭제하는 것이 좋을지, 아니면 객체 설계 직관성 상 유지하는 것이 좋은지 질문드립니다.

이번 요구사항 중 getter를 지양하라, 단 dto를 위한 getter는 허용한다고 되어 있었습니다. 그런데 출력을 위해 getter를 여는 것과 dto를 구현해서 전달하는 것의 차이가 아직 와닿지는 않는 것 같아요. 당장 생각해봤을 때는 getter 오용의 방지를 위함이라는 이유가 생각나기는 하는데 이 부분에 대해 영이는 어떻게 생각하는지 궁금합니다.

위 질문드린 내용과 비슷한 것이기는 한데 개인적으로 원시타입 wrapping 객체의 내부 값을 가져오는 책임은 view에서 져야 한다고 생각했었습니다. 그 이유 중 하나는 도메인 단에서 객체가 상태값으로 가진 wrapping 객체의 내부 값을 외부로 반환하여 로직을 수행하게끔 하면 안된다고 생각했기 때문입니다.
그런데 MVC 패턴을 바라보는 관점 중 Model과 View는 상호 독립적으로 존재해야 한다고 주장하는 관점에 대해 알게 되었습니다. 이 경우라면 view에서 wrapping 객체를 unwrap해주는 기능도 결국 view가 도메인에게 의존하는 것이기 때문에 하면 안됩니다.
그렇다면 이번 미션의 요구사항과 이러한 MVC 관점들은 dto의 사용을 권장하는 것일까요? 이런 제 생각에 대한 영이의 의견이 궁금합니다.

PR 메세지를 작성하며 확인해보니 상수 분리가 안된 리터럴 값들이 많네요... 해당 부분은 이후 리팩토링하며 정리할 수 있도록 하겠습니다.

리뷰 잘 부탁드립니다! 😄

세부사항 - PieceName - Color

세부사항 - 체스말은 색깔에 따라 다른 이름을 가진다.

세부사항 - 살아있는 말의 현황 관리

세부사항 - 출발 좌표 기준으로 8방향 직선 상에 도착 좌표가 존재하면 경로를 계산한다. - 출발 좌표 기준으로 8방향 직선 상에 도착 좌표가 존재하지 않으면 도착 좌표만 경로에 넣어준다.

세부사항 - 내부 상태값 타입 변경

세부사항 - 폰 이외의 말은 기존 방식으로 진행 - 폰은 전진하는 방향의 대각선에 상대편 말이 존재하면 잡고 이동할 수 있다. - 폰은 전진하는 경로에 다른 말이 존재하면 이동하지 못한다.

choijy1705

안녕하세요 히이로
체스미션 엄청 잘구현해주셨네요
테스트코드는 진짜 깔끔해서 읽기 좋은것 같아요
질문에 대한 답변과 몇가지 코멘트 남겼습니다!

choijy1705 · 2023-03-18T02:25:46Z

src/main/java/controller/ChessController.java

+    }
+
+    private GameCommand receiveStartOrEndCommand(GameCommand gameCommand) {
+        while (!gameCommand.equals(GameCommand.START) && !gameCommand.equals(GameCommand.END)) {


move인지 체크가 더 명확하지 않을까요?

그리고 enum 비교는 == 으로 해주는게 좋을것 같아요

해당 부분은 확실히 move 이외 명령어를 받지 않기에 move인지 체크해 주는 것이 더 명확한 의미를 전달하겠네요! 수정했습니다.

그리고 enum 간 비교는 동등성이 아닌 동일성 비교를 해줘야 하는 것에 동의합니다. 해당 부분도 수정했습니다. 그런데 리팩토링하면서 들었던 한 가지 궁금한 점이 있습니다. enum처럼 동일한 단일 객체에 대해 비교를 해줄 때 반드시 동일성으로 비교해줘야만 하는 이유가 있는지 여쭤보고 싶습니다.

동등성과 동일성 비교 연산간 뚜렷한 성능차이가 있는 것인지, 그게 아니면 코드를 보는 입장에서 직관적으로 더 의미가 와닿을 수 있도록 명시하기 위함인지 질문드립니다!

성능 차이는 없을겁니다 equlas 내부적으로도 == 을 사용할거에요
다만 타입이 다른경우라도 equals는 컴파일에러를 내지 않고 == 은 컴파일 에러가 뜨기 때문에 타입 오류를 예방할 수 있고
equals는 nullPointerException이 날 수 있지만 == 은 NullPointerException이 나지 않는 장점이 있을것 같아요

choijy1705 · 2023-03-18T02:27:47Z

src/main/java/controller/ChessController.java

+        }
+    }
+
+    private GameCommand receiveStartOrEndCommand(GameCommand gameCommand) {


굳이 파라미터로 받기보다 이 메서드 내부에서 receiveGameCommand 를 호출해주는건 어떤가요?

영이의 의견에 동의합니다. run 메서드 내부에서 기능 구현을 하고 인텔리제이 메서드 분리 기능을 이용해 분리를 진행하다보니 인지하지 못했네요... 수정했습니다!

choijy1705 · 2023-03-18T02:30:18Z

src/main/java/controller/ChessController.java

+        List<String> userInput = InputView.readUserInput();
+        GameCommand gameCommand = GameCommand.of(userInput.get(0));
+
+        while (!gameCommand.equals(GameCommand.END)) {


move인지 체크해야는거 아닌가요?

해당 부분은 게임이 start 된 상태에서 command를 입력받아 게임을 진행하고 있습니다. 이미 start가 된 상태에서는 end command가 입력된 게 아닌 이상 게임이 진행되어야 한다고 판단해 이렇게 구현했습니다. 즉 start 명령도 예외처리를 해주고 재입력을 하도록 의도했기에 이렇게 구현했습니다. 혹시 좀 더 다른 방식으로 구현할 방법이 있다면 알려주시면 감사하겠습니다! (리팩토링 진행 후 다시 코멘트 확인한 후 올리기)

choijy1705 · 2023-03-18T02:32:47Z

src/main/java/controller/ChessController.java

+    private Position inputToPosition(String input) {
+        int row = RowToNumber.of(input.charAt(1));
+        int column = ColumnToNumber.of(input.charAt(0));
+
+        return Position.of(row, column);
+    }


position객체에서 string을 받아 position을 생성할수 있도록 만들면 어떨까요?

앗 그렇게 하면 훨씬 더 깔끔한 책임 분배가 되겠네요! 기존 inputToPosition 기능은 Position 내부에서 of 메서드를 이용해 처리할 수 있도록 리팩토링 했습니다.

choijy1705 · 2023-03-18T02:37:06Z

src/main/java/domain/GameCommand.java

+    public static GameCommand of(String inputCommand) {
+        return Arrays.stream(values())
+                .filter(gameCommand -> gameCommand.getCommand().equals(inputCommand))
+                .findFirst()
+                .orElseThrow(() -> new IllegalArgumentException("[ERROR] 사용할 수 없는 명령어를 입력했습니다."));
+    }


enum 내장 기능인 valueOf를 활용할수 있도록 해보면 어떨까요?

GameCommand.valueOf("START")
하면 START가 반환됩니다

stream으로 검증해서 필요했던 여러 라인을 줄일 수 있겠네요. 리뷰주신 방향대로 수정했습니다.

다만 예외 메세지를 위해서 valueOf 메서드에서 발생하는 IllegalArgumentException을 try catch로 잡아서 예외 메세지를 담은 IllegalArgumentException을 동일하게 throw하도록 구현했습니다. 기존에 넣어줬던 예외처리 메세지를 보여주기 위해 해당 방식을 사용해도 괜찮은 지 확신은 없었는데 영이의 의견이 궁금합니다!

저는 괜찮은것 같습니다!

choijy1705 · 2023-03-18T03:20:41Z

src/main/java/domain/position/Position.java

+    public List<Position> getPathTo(Position end) {
+        if (Direction.of(this, end) != Direction.OTHER) {
+            return calculatePath(calculateRowGap(end), calculateColumnGap(end));
+        }
+        return new ArrayList<>(List.of(end));
+    }


일급컬렉션에 대해 많이 배우고 연습해보셨을것 같은데
요런건 PathPositions 같은 클래스를 만들어서 반환하도록 하는건 어떨까요?

영이가 리뷰해주신 대로 Path라는 List 클래스를 작성해서 기존 Position이 서로 다른 Position 간 경로를 계산하는 책임을 이관시킬 수 있도록 리팩토링 했습니다.

Position이 책임지던 로직들이 훨씬 가벼워지는 게 느껴지네요! 👍

choijy1705 · 2023-03-18T03:24:13Z

src/main/java/domain/position/Position.java

+    public int calculateRowGap(Position other) {
+        return other.row - this.row;
+    }
+
+    public int calculateColumnGap(Position other) {
+        return other.column - this.column;
+    }


저도 이런 방법은 조금 별로더라고요
저는 보통 getter를 통해 값비교를 했었는데 요구사항이 getter를 쓰지말라니...
position의 책임이 너무 많아져서 걱정이라면
계산하는로직은 따로 util클래스를 만들어서 분리하는 방법이 있을것 같네요

getter를 쓰지 말라는 이유는 여러가지 있겠지만
getter를 통해서 값이 변경되면 실제 원본에도 영향을 미치는 문제점 때문이 아닐까 싶네요
setter없는 개발은 많이 해봤지만 getter없는 개발은 저도 조금 당황스럽습니다 ㅠ

요건 히이로가 크루, 코치들과 이야기해보고 방향을 정해보면 좋을것 같습니다!
미션 도중 방향을 정한다면 저한테도 공유해주시면 좋을것 같네요!

미션 요구사항으로 getter를 사용하지 말라고 했던 이유에 대해 생각해봤습니다. 그리고 내린 결론은 무지성적인 getter를 열지 말고 최대한 객체를 객체답게 사용할 수 있도록 설계를 고려하라는 의미라는 것이었습니다.

그래서 출력을 위한 getter와 객체 책임 분리로부터 파생된 불가피한 getter 사용은 허용해 리팩토링을 진행했습니다. 다만 최대한 getter를 사용하지 않아도 객체 자체에서 수행가능한 로직들은 유지하려고 노력했습니다!

choijy1705 · 2023-03-18T03:26:36Z

src/test/java/domain/chessGame/ChessBoardTest.java

+        Map<Position, Piece> setupBoard = Map.of(
+                Position.of(2, 2), new Rook(Color.WHITE),
+                Position.of(5, 2), new Rook(Color.BLACK));


setup을 활용해보면 어떤가요

영이의 리뷰를 보고 BeforeAll 어노테이션을 사용해 수정해보려 했습니다. 그런데 Inner Class에서는 static선언이 자바 11에서는 불가능 하다고 합니다. 그래서 BeforeAll 어노테이션 사용이 불가능한데 다른 방식으로라도 변경하는 것이 좋을지 질문드립니다!

beforeEach는 사용가능할것 같아요
그리고 평소에도 beforeAll보다는 beforeEach 를 사용하면 좋을것 같아요
독립된 테스트를 보장하기 위해서요

choijy1705 · 2023-03-18T03:32:01Z

src/main/java/domain/position/Position.java

+    public Position moveLeft() {
+        return Position.of(row, column - 1);
+    }
+
+    public Position moveRight() {
+        return Position.of(row, column + 1);
+    }


메인 코드에서 안쓰이면 지우는게 좋을것 같습니다.
up, down, upLeft 요런 로직들도 pawn에서만 쓰이네요
pawn의 로직을 변경해서 나머지도 없애는 방향도 고민해보면 좋겠네요!

전체적인 폰의 설계를 다시 살펴보고 해당 로직을 calculateRowGap, calculateColumnGap 두 개의 메서드를 통해 폰 내부에서 구현 가능할 수 있다고 판단했습니다. 리팩토링 결과 Position 내 move 메서드 사용을 없앨 수 있었어서 삭제했습니다!

choijy1705 · 2023-03-18T03:38:41Z

src/main/java/view/OutputView.java

+        System.out.println(START_MESSAGE);
+    }
+
+    public static void printChessBoard(List<Position> allPosition, Map<Position, Piece> chessBoard) {


model이 직접적으로 view로 전달되고 있는데요
dto를 사용하는것도 하나의 방법중 하나지만 꼭 dto를 사용해라 이건 아닌것 같아요!

예를들어 회원아이디, 비밀번호, 나이 등등의 신상정보가 있는 user 도메인을 view로 그대로 넘겨버리면
개발자도구에서 비밀번호같은 중요정보를 볼수있어요 그래서 view와 domain을 분리하는 이유라고 생각합니다
만약에 user의 이름만 view에서 사용한다면
원시타임으로 string name 만을 view로 전달해주면 될것 같고요
모든 값들이 다 들어가서 하나하나 파라미터로 전달하기 어려우면 그때 dto를 고려해볼수 있을것 같네요

지금까지 미션을 진행하면서 느낀 부분들, 그리고 영이가 주신 의견도 함께 고려하면서 Dto에 대한 개인적인 의견을 정리해 보았습니다.

https://makemepositive.tistory.com/25
내용이 많이 길어져서 정리해 둔 개인 포스팅을 같이 참고합니다.
view에게 domain을 전달하는 것을 지양하기 위해 다른 방법을 고려해보고 싶은데 위와 같은 이유로 그 중 Dto도 이제는 고려해볼 수 있을 것 같습니다.

다만 제가 이전 미션에서 밀린 것들을 처리하면서 진행하느라 생각보다 시간이 많이 소요되었던 터라 어떤 방식으로 수정해보는 것이 좋을지 고민할 여유가 없었습니다. 그래서 해당 부분은 view가 domain에 의존하는 것으로 놔두고 우선순위를 뒤로 미루는 것으로 결정했습니다...! 😭

좋습니다!
요건 이번미션에 핵심이 아니니 여유가 되신다면 챙겨주세요
포스팅은 시간될때 한번 읽어보겠습니다!

세부사항 - 폰의 최종 목적지 이동 가능 여부를 검증하는 기능에서 색이 같은 말인지 체크하는 중복 로직 삭제

세부사항 - isMoveDistance 메서드에서 항상 true를 반환하도록 구현한 부분 수정 - UnsupportedOperationException 던지도록 수정 - 단순 contains 용도로 사용되는 List를 Set으로 변경

세부사항 - 기존 Position클래스의 move 메서드를 사용하던 코드를 calculateRowGap, calculateColumnGap을 사용해서 수행하는 것으로 수정 - Pawn 내부에서 Color에 따른 로직 리팩토링을 위한 추상 클래스화 - 추상 클래스 Pawn의 구현체인 BlackPawn, WhitePawn 작성

세부사항 - Position 내부에서 position 객체 간 경로를 계산해 반환하는 기능 제거 - 해당 기능을 Path 클래스로 이관 - Path 관련 도메인 코드 수정 - Path 관련 기존 테스트 코드 수정

세부사항 - 불필요한 검증 조건 삭제 - 검증 로직 직관성 향상을 위한 네이밍 변경 - 관련 테스트 코드 수정

MoonJeWoong

안녕하세요 영이, 히이로입니다!

정말 빠르게 리뷰를 주셨는데 제가 이전 미션에서 밀렸던 것들을 처리하고 진행하고 싶었던 터라 재요청이 많이 늦어졌네요... 죄송합니다 😭

영이가 주신 리뷰들 덕분에 페어와 설계한 구조에서 좀 더 객체간에 책임을 분리하거나 명확하게 할 수 있도록 고민하는 시간을 가질 수 있었습니다. 다만 아쉬운 부분은 좀 더 고민해보고 고쳐보고 싶은 부분들이 아직 있는 것 같은데 시간 상의 이유로 우선순위를 조절해야 하는 것이 많이 아쉽네요... 하지만 해당 부분들은 후순위로 남겨두고 남은 3, 4 단계를 빠르게 구현해봐야 할 것 같다고 판단했습니다.

양질의 리뷰 해주셔서 감사했습니다 영이!

MoonJeWoong · 2023-03-22T05:01:30Z

src/main/java/controller/ChessController.java

+        List<String> userInput = InputView.readUserInput();
+        GameCommand gameCommand = GameCommand.of(userInput.get(0));
+
+        while (!gameCommand.equals(GameCommand.END)) {


해당 부분은 게임이 start 된 상태에서 command를 입력받아 게임을 진행하고 있습니다. 이미 start가 된 상태에서는 end command가 입력된 게 아닌 이상 게임이 진행되어야 한다고 판단해 이렇게 구현했습니다. 즉 start 명령도 예외처리를 해주고 재입력을 하도록 의도했기에 이렇게 구현했습니다. 혹시 좀 더 다른 방식으로 구현할 방법이 있다면 알려주시면 감사하겠습니다! (리팩토링 진행 후 다시 코멘트 확인한 후 올리기)

MoonJeWoong · 2023-03-22T06:46:07Z

src/main/java/controller/ChessController.java

+    }
+
+    private GameCommand receiveStartOrEndCommand(GameCommand gameCommand) {
+        while (!gameCommand.equals(GameCommand.START) && !gameCommand.equals(GameCommand.END)) {


해당 부분은 확실히 move 이외 명령어를 받지 않기에 move인지 체크해 주는 것이 더 명확한 의미를 전달하겠네요! 수정했습니다.

그리고 enum 간 비교는 동등성이 아닌 동일성 비교를 해줘야 하는 것에 동의합니다. 해당 부분도 수정했습니다. 그런데 리팩토링하면서 들었던 한 가지 궁금한 점이 있습니다. enum처럼 동일한 단일 객체에 대해 비교를 해줄 때 반드시 동일성으로 비교해줘야만 하는 이유가 있는지 여쭤보고 싶습니다.

동등성과 동일성 비교 연산간 뚜렷한 성능차이가 있는 것인지, 그게 아니면 코드를 보는 입장에서 직관적으로 더 의미가 와닿을 수 있도록 명시하기 위함인지 질문드립니다!

MoonJeWoong · 2023-03-22T07:00:11Z

src/main/java/controller/ChessController.java

+        }
+    }
+
+    private GameCommand receiveStartOrEndCommand(GameCommand gameCommand) {


영이의 의견에 동의합니다. run 메서드 내부에서 기능 구현을 하고 인텔리제이 메서드 분리 기능을 이용해 분리를 진행하다보니 인지하지 못했네요... 수정했습니다!

MoonJeWoong · 2023-03-22T07:35:02Z

src/main/java/controller/ChessController.java

+    private Position inputToPosition(String input) {
+        int row = RowToNumber.of(input.charAt(1));
+        int column = ColumnToNumber.of(input.charAt(0));
+
+        return Position.of(row, column);
+    }


앗 그렇게 하면 훨씬 더 깔끔한 책임 분배가 되겠네요! 기존 inputToPosition 기능은 Position 내부에서 of 메서드를 이용해 처리할 수 있도록 리팩토링 했습니다.

MoonJeWoong · 2023-03-22T08:13:38Z

src/main/java/domain/chessGame/ChessBoard.java

+        if (isPieceMovable(startPosition, endPosition)) {
+            chessBoard.put(endPosition, chessBoard.get(startPosition));
+            chessBoard.remove(startPosition);
+            return;
+        }
+        throw new IllegalArgumentException("[ERROR] 선택한 말은 목표 좌표로 이동이 불가능합니다.");


동의합니다. 해당 부분처럼 if문과 외부 수행 코드가 어색하게 반대로 되어 있는 부분은 전체적으로 검토해서 수정하도록 했습니다!

MoonJeWoong · 2023-03-23T13:22:36Z

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

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


말씀해주신 부분은 Pawn을 추상 클래스화하고 BlackPawn, WhitePawn을 각각 구현체로 작성하여 반영했습니다!

말씀해주신대로 훨씬 가독성이 향상되었네요!

MoonJeWoong · 2023-03-24T06:28:16Z

src/main/java/domain/chessGame/ChessBoard.java

+        if (startPiece.isPawn()) {
+            considerPawnCase(startPosition, endPosition);
+        }


영이가 리뷰를 주셔서 오랜 시간 고민해봤습니다.
결과적으로 불필요한 조건 검사문을 제거하고 중복되는 로직은 묶어볼 수 있었던 것 같아요.

그런데 chessBoard에서 pawn에 대해 판단하는 로직은 경로 상에 다른 말 존재 여부에 따라 달라지는 판단을 수행하고 있습니다. 이를 Pawn 내부에서 판단하게 리팩토링 하려면 chessBoard 에서 저장하고 있는 말의 현황을 다시 pawn에게 넘겨줘야 했습니다. 개인적으로 이번 미션에서 Piece는 자신이 기물로써 갈 수 있는 경로를 알고있고 자신의 시작점과 목적지 position 그리고 path만 주어졌을 때 적절한지 판단하는 책임만 지도록 했습니다.

이런 사고과정을 거치니 Pawn 내부로 해당 로직을 이관시키는 방법은 찾아내기 어려웠던 것 같습니다. 혹시 처음에 생각하셨던 방향과 너무 다르게 제가 생각했다면 알려주시면 감사하겠습니다!

MoonJeWoong · 2023-03-24T06:29:07Z

src/main/java/domain/chessGame/ChessBoard.java

+    private boolean isSameColorPiece(Piece startPiece, Piece endPiece) {
+        return startPiece.isBlack() == endPiece.isBlack();
+    }


해당 메서드는 제거했습니다! 😃

MoonJeWoong · 2023-03-24T06:32:34Z

src/main/java/domain/position/Position.java

+    public int calculateRowGap(Position other) {
+        return other.row - this.row;
+    }
+
+    public int calculateColumnGap(Position other) {
+        return other.column - this.column;
+    }


미션 요구사항으로 getter를 사용하지 말라고 했던 이유에 대해 생각해봤습니다. 그리고 내린 결론은 무지성적인 getter를 열지 말고 최대한 객체를 객체답게 사용할 수 있도록 설계를 고려하라는 의미라는 것이었습니다.

그래서 출력을 위한 getter와 객체 책임 분리로부터 파생된 불가피한 getter 사용은 허용해 리팩토링을 진행했습니다. 다만 최대한 getter를 사용하지 않아도 객체 자체에서 수행가능한 로직들은 유지하려고 노력했습니다!

MoonJeWoong · 2023-03-24T06:52:56Z

src/main/java/view/OutputView.java

+        System.out.println(START_MESSAGE);
+    }
+
+    public static void printChessBoard(List<Position> allPosition, Map<Position, Piece> chessBoard) {


지금까지 미션을 진행하면서 느낀 부분들, 그리고 영이가 주신 의견도 함께 고려하면서 Dto에 대한 개인적인 의견을 정리해 보았습니다.

https://makemepositive.tistory.com/25
내용이 많이 길어져서 정리해 둔 개인 포스팅을 같이 참고합니다.
view에게 domain을 전달하는 것을 지양하기 위해 다른 방법을 고려해보고 싶은데 위와 같은 이유로 그 중 Dto도 이제는 고려해볼 수 있을 것 같습니다.

다만 제가 이전 미션에서 밀린 것들을 처리하면서 진행하느라 생각보다 시간이 많이 소요되었던 터라 어떤 방식으로 수정해보는 것이 좋을지 고민할 여유가 없었습니다. 그래서 해당 부분은 view가 domain에 의존하는 것으로 놔두고 우선순위를 뒤로 미루는 것으로 결정했습니다...! 😭

choijy1705

안녕하세요 히이로
1, 2단계는 머지해도 될것 같습니다
코멘트 남긴거는 다음단계 진행하면서
반영 고려해주세요!

choijy1705 · 2023-03-24T09:17:10Z

src/main/java/controller/ChessController.java

+        while (gameCommand != GameCommand.END) {
+            executePlayingCommand(chessBoard, userInput);
+            userInput = InputView.readUserInput();
+            gameCommand = GameCommand.of(userInput.get(0));
+        }


start 한 게임에서 start 하면 에러 메세지를 띄우고 다시 게임할 수 있도록 한다고 했는데
실제로 error를 캐치해주지 않아 종료되고 있습니다!

choijy1705 · 2023-03-24T09:21:08Z

src/main/java/domain/chessGame/ChessBoard.java

+    private boolean validatePieceMovable(Position startPosition, Position endPosition) {
+        Path path = new Path(startPosition, endPosition);
+        Piece startPiece = chessBoard.get(startPosition);
+
+        if (startPiece.isPawn() && startPiece.isMovablePath(startPosition, path)) {
+            validatePawnMovable(startPosition, endPosition);
+        }
+        return startPiece.isMovablePath(startPosition, path) &&
+                validatePassablePath(path) &&
+                validateMovableEndPosition(endPosition, startPiece);
+    }


validate로 시작하는 메서드면 예외를 던지고
is로 시작하는 메서드는 boolean을 return 받으면 좋을것 같습니다!
지금 같으면 isPieceMovable이 좋을것 같네요

다른 부분들도 체크해주세요

choijy1705 · 2023-03-24T09:32:27Z

src/main/java/domain/chessGame/ChessBoard.java

+
+    private void validatePawnMovable(Position startPosition, Position endPosition) {
+        Path path = new Path(startPosition, endPosition);
+        Pawn selectedPawn = (Pawn) chessBoard.get(startPosition);


validatePieceMovable에서 startPiece를 찾았는데 여기서 또 찾고 있어요
한번만 조회해오면 좋을것 같습니다
지금은 비용이 거의 안들지만 DB에서 조회한다고 생각하면 꽤 비용이드는 중복일것 같아요

choijy1705 · 2023-03-24T09:34:54Z

src/main/java/domain/chessGame/ChessBoard.java

+        if (!chessBoard.containsKey(endPosition)) {
+            throw new IllegalArgumentException("[ERROR] 폰은 대각선 이동 경로에 상대 말이 없으면 이동이 불가능합니다.");
+        }
+        validateMovableEndPosition(endPosition, selectedPawn);


요로직도 여기 있으면 pawn입장에서는 두번 중복 체킹일것 같네요

choijy1705 · 2023-03-24T09:39:35Z

src/main/java/domain/piece/Queen.java

+    @Override
+    protected boolean isMovableDirection(Position start, Position nextPosition) {
+        Direction nextDirection = Direction.of(start, nextPosition);
+        return QUEEN_MOVABLE_DIRECTIONS.contains(nextDirection);
+    }


폰 나이트 에서만 구현해주고
나머지는 따로 구현을 안해주면 되지 않을까요? piece에서 로직 그대로 사용할 수 있도록요

choijy1705 · 2023-03-24T09:45:29Z

src/main/java/domain/position/Path.java

+    public void makePathTo(Position start, Position end) {
+        if (Direction.of(start, end) == Direction.OTHER) {
+            positions.add(end);
+            return;
+        }
+        calculatePath(start, end);
+    }
+
+    private void calculatePath(Position start, Position end) {
+        int rowGap = start.calculateRowGap(end);
+        int columnGap = start.calculateColumnGap(end);
+
+        int unit = Math.max(Math.abs(rowGap), Math.abs(columnGap));
+        int rowCoefficient = rowGap / unit;
+        int columnCoefficient = columnGap / unit;
+        for (int i = 1; i <= unit; i++) {
+            positions.add(start.moveByGap(rowCoefficient * i, columnCoefficient * i));
+        }


path를 생성하는 로직이 너무 복잡한것 같아요
이런 부분은 정적팩토리 메서드를 활용해보는게 좋을것 같네요

choijy1705 · 2023-03-24T09:51:04Z

src/main/java/domain/position/ColumnToNumber.java

+    public static int of(char alphabet) {
+        try {
+            return convertAlphabetToColumnNumber(alphabet).getNumber();
+        } catch (IllegalArgumentException e) {
+            throw new IllegalArgumentException("[ERROR] 사용할 수 없는 명령어를 입력했습니다.");
+        }
+    }


이것두 굳이 메서드 분리할 필요가 없을것 같네요!

choijy1705 · 2023-03-24T09:52:56Z

src/main/java/domain/position/RowToNumber.java

+
+import java.util.Arrays;
+
+public enum RowToNumber {


position이 이 값들을 필드로 가지면 어떨까요?
RowToNumber라는 enum명도 일반적인 명칭은 아닌것 같아서요

MoonJeWoong added 30 commits March 14, 2023 16:16

docs: 기능 요구사항 초안 작성

3602c77

chore: JDBC 관련 gradle 설정

cb8a348

feat: piece 클래스 상태 값 enum 작성

fb471e5

세부사항 - PieceName - Color

feat: 체스 piece 생성 기능 구현

b5183f8

세부사항 - 체스말은 색깔에 따라 다른 이름을 가진다.

feat: 체스판에서 좌표를 관리하는 Position 기능 구현

1aecc6b

feat: 체스판 초기화 기능 구현

88e266e

세부사항 - 살아있는 말의 현황 관리

feat: View 구현

158fafc

feat: controller 구현

7f27ca0

docs: step2 기능 요구 사항 작성

6cff352

feat: 명령어 안내 문구 출력 기능 구현

fa6ccb0

feat: 말의 기본적인 이동은 8방향으로 수행하는 기능 구현

91a3c1b

feat: 출발 좌표부터 목표 좌표까지의 이동 경로를 반환하는 기능 구현

baba02c

feat: piece 이동 규칙 판단 추상 메서드 작성

536dc5c

feat: rook 이동 경로 판단 기능 구현

2fe7be2

feat: bishop 이동 경로 판단 기능 구현

26b4e81

feat: queen 이동 경로 판단 기능 구현

0302881

feat: king 이동 경로 판단 기능 구현

c0ceefa

feat: pawn 이동 경로 판단 기능 구현

38e33da

refactor: 다른 position과의 행,열 좌표 값 차이 계산 기능 분리

cb7e26a

feat: knight 이동 경로 판단 기능 구현

e3d6f97

refactor: 체스판 초기화 기능 분리

339415a

fix: 체스판 초기화 행, 열 좌표 반전 버그 수정

a2ff5c5

fix: position 캐싱 값에 padding position 추가

4f7fe72

docs: 누락된 기능 요구사항 반영

99dc9db

feat: 체스판에서 입력받은 위치로 말이 이동할 수 있는지 판단하는 기능 구현

7bdf8e2

refactor: 행, 열 좌표값을 관리하는 enum 수정

d16e708

세부사항 - 내부 상태값 타입 변경

feat: 사용자의 입력을 받아 체스 게임을 진행시키는 기능 구현

c8f1f9a

refactor: piece 진행 방향 판단 기능을 position이 수행하도록 분리

8af3895

MoonJeWoong added 3 commits March 17, 2023 16:33

refactor: 기존 방향 판단 기능을 Direction에게 위임

aa3053c

style: 미사용 import문 삭제 및 코드 포맷팅 정리

4e59000

refactor: static view 클래스에 private 생성자 추가

51a3646

choijy1705 requested changes Mar 18, 2023

View reviewed changes

MoonJeWoong added 19 commits March 22, 2023 15:54

chore: 람다/스트림 미션 완료 코드 커밋

3b4b296

refactor: comamnd Enum 객체 간 비교 시 동일성 비교하도록 수정

8f92824

refactor: 기존 사용자 입력으로 좌표 값을 계산하는 기능 position으로 이관

fe8a5da

refactor: 불필요한 매개 변수 삭제 및 메서드 내부로 이관

e6ab7e7

refactor: if문 내부와 외부가 반대로 작성되어 어색한 부분 수정

48e4c91

refactor: 검증 로직 실행후 예외를 던지는 메서드 네이밍 수정

57a19d4

refactor: 불필요한 stream 사용 제거

56a44f4

refactor: piece 내 불필요한 동등성 비교를 동일성 비교로 수정

912156f

refactor: 불필요하게 stream을 사용한 검증 로직 수정

9caa661

refactor: 불필요한 인터페이스 사용 코드 삭제

cc82040

refactor: chessBoard에서 폰 이동 가능 여부 검증 로직 수정

cc9e106

세부사항 - 폰의 최종 목적지 이동 가능 여부를 검증하는 기능에서 색이 같은 말인지 체크하는 중복 로직 삭제

refactor: chessBoardGenerator 내 불필요한 메서드 분리 삭제

620b1db

refactor: Enum 내 정적 팩토리메서드 stream 사용 부분 valueOf로 수정

2ee63cd

refactor: Direction 로직 내 불필요한 함수형 인터페이스 사용 수정

68c2bf7

refactor: position 초기화 로직 수정 및 상수화

58b68bd

refactor: chessBoard 내 pawn 이동 판단 로직 수정

acabfa4

세부사항 - 불필요한 검증 조건 삭제 - 검증 로직 직관성 향상을 위한 네이밍 변경 - 관련 테스트 코드 수정

MoonJeWoong commented Mar 24, 2023

View reviewed changes

MoonJeWoong added 4 commits March 24, 2023 15:58

refactor: 도메인 로직에 어색한 outputview 메서드 명 수정

2575d7f

style: 사용하지 않는 import문 삭제

c8a18f7

test: Pawn 테스트에서 변수 타입 변경

b4bdae3

docs: 후순위 Todo 정리

d671518

choijy1705 requested changes Mar 24, 2023

View reviewed changes

choijy1705 merged commit a6a90fc into woowacourse:moonjewoong Mar 24, 2023


		import java.util.List;

		public final class Pawn extends Piece {

[1, 2단계 - 체스] 히이로(문제웅) 미션 제출합니다. #522

[1, 2단계 - 체스] 히이로(문제웅) 미션 제출합니다. #522

Conversation

MoonJeWoong commented Mar 17, 2023 • edited Loading

choijy1705 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

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

MoonJeWoong 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

choijy1705 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

MoonJeWoong commented Mar 17, 2023 •

edited

Loading