[1,2,3단계 - 체스] 크로플(이동환) 미션 제출합니다. by perenok · Pull Request #178 · woowacourse/java-chess

perenok · 2021-03-22T06:00:58Z

안녕하세요 지노!
체스 첫번째 풀리퀘입니다!! 정말이지 힘든 한 주였어요ㅠㅠ
난이도가 지난번에 비해 많이 올랐다고 느껴지는 미션이었어요ㅋㅋㅋ
이번 미션은 페어 프로그래밍이 저에게 큰 장점으로 다가왔어요! 페어 덕분에 기간 내에 잘 마무리 할 수 있었다고 생각합니다ㅋㅋㅋ
가장 오래 고민했던 것은 Piece가 자신의 현재 좌표를 가지고 있느냐 아니면 Board에서 좌표별로 해당 Piece를 가지고 있냐에 대한 것이었어요. 두번의 reset 끝에 결론은 Piece에게 현재 좌표를 주고 이를 통해 이동 가능한 위치들을 각 Piece 별로 체크하게 만들었어요.
이걸 이용해서 체크메이트까지 구현을 할 수 있을 줄 알았는데... 체크는 충분히 가능한데 체크메이트는 생각보다 더 많은 경우의 수가 필요하더라고요ㅋㅋㅋ 최대한 구현해 보려 하다 실패하고 일단 Todo로 유배 보내놨습니다ㅋㅋㅋ 처음에 생각했던 체크메이트는 king이 움직일 수 있는 위치들이 모두 적들의 공격범위 안에 들면 체크메이트!라고 구현을 했는데 실제로 돌려보니 왕이 움직이지 못하더라도 다른 기물이 경로에 들어오거나, 아니면 체크중인 상대방 기물을 잡을 수 있을 때도 체크메이트로 선언해버리고 게임을 끝내더라고요ㅋㅋㅋ 그래서 좀 더 고민해보고 설계를 손본 뒤에 기능을 추가하겠습니다ㅠㅠ
페어 덕분에 BiFunction을 처음 써봤어요! 입력에 따라 행동을 해줄 ChessAction과 입력인 String값을 받아 GameStatus를 반환하도록 구현해 보았습니다! 또한 포비의 엘레강트 수업에서 controller도 잘못된 네이밍이라는 의견이 있다고 해서 이름을 ChessAction으로 변경했습니다ㅋㅋㅋ
많은 피드백 부탁드려요~~

궁금한 점

Piece에서 instanceof 대신 isKing과 isPawn 메소드를 통해 King과 Pawn인지 구분하고 있습니다. 나머지 Piece에선 false를 반환하는 식으로 이렇게 메소드를 통해 구분하는게 instanceof를 사용하는 것보다 괜찮은 걸까요? instanceof가 안티패턴이라고 지난 블랙잭 미션에서 피드백을 받아서 바꿔 구현했는데 King이나 Pawn이 아닌 클래스까지 코드들이 추가되니 현재 코드가 더 나은지 확신이 안 서네요ㅠㅠ

초기 배치에 필요한 Position과 Piece 클래스 생성 한번 사용한 Position을 캐싱하여 재활용

Map을 통해 Position과 Piece를 한번에 관리하는 대신 기물의 리스트와 위치의 리스트로 분리하여 관리 공백을 따로 Piece 객체로 선언하기 위해 필요했던 상수 삭제 Position 객체에서 위치값들을 바로 캐싱해서 사용

movable 조건에 필요한 position 연산 메소드 추가

Piece에 currentPostion과 movablePositions를 필드로 주고 이를 통해 이동 가능한 Position들을 가지고 있음 move 때 movablePositions과 targetPosition을 비교해서 가능하면 이동

…로 List 되도록 수정

…위한 BoardDto 추가

moveDirections와 killDirections, 그리고 한칸 이상 갈 수 있는지 판단하는 boolean값을 가짐

현재 위치와 이동 가능한 위치들을 List로 가짐 또한 움직였는지 처음 위치인지 판단하는 moved를 필드변수로 가짐

List를 탐색해서 조건에 맞는 반환값을 가지는 메소드들을 모아놓아 chessGame의 코드를 줄임

hyunssooo

안녕하세요, 크로플! 지노입니다.🙇‍♂️
정말 어려운 체스 미션 진행하신다고 수고 많으셨습니다.👏👏👏
몇 가지 코멘트 남겼어요! 확인 부탁 드립니다!

hyunssooo · 2021-03-22T17:26:14Z

src/main/java/chess/domain/Score.java

+
+public class Score {
+


점수도 클래스로 만드셨네요! 👍

hyunssooo · 2021-03-22T17:28:15Z

src/main/java/chess/domain/Score.java

+    public Score addedScore(Score score) {
+        return from(this.value + score.value);
+    }
+
+    public Score minus(Score score) {
+        return Score.from(value - score.value);
+    }


minus처럼 addedScore 메서드명에 Score라는 이름은 빠져도 괜찮을 것 같아요. 🙂

수정했습니다!

hyunssooo · 2021-03-22T17:45:01Z

src/main/java/chess/domain/piece/direction/PieceDirections.java

+            if (currentPosition.invalidGo(killDirection)) return;
+            currentPosition = currentPosition.go(killDirection);


Suggested change

if (currentPosition.invalidGo(killDirection)) return;

currentPosition = currentPosition.go(killDirection);

if (currentPosition.invalidGo(killDirection)) {

return;

}

currentPosition = currentPosition.go(killDirection);

if 문 중괄호를 생략하지 마세요 :) 바로 아래줄이 if문에 들어간다고 읽을 수도 있습니다!
이 클래스내에 전체적으로 수정해보면 좋을 것 같아요

hyunssooo · 2021-03-22T18:14:45Z

src/main/java/chess/domain/piece/Pieces.java

+    public Optional<Piece> pieceByPosition(final Position position) {
+        return pieces.stream()
+                .filter(piece -> piece.isSamePosition(position))
+                .findAny();
+    }


optional에 관해서는 아래 글을 읽어 보면 좋을 것 같아요 :)
http://homoefficio.github.io/2019/10/03/Java-Optional-바르게-쓰기/
또한, optional을 사용하지 않는 방법은 없는지 고민해 보면 좋을 것 같아요 :)

optional을 반환하는 메소드를 일반 객체를 반환하도록 변경했습니다ㅎㅎ

hyunssooo · 2021-03-22T18:18:35Z

src/main/java/chess/domain/piece/moving/BishopMoving.java

+    @Override
+    public int row() {
+        return currentPosition.row();
+
+    }
+
+
+}


의미 없는 개행은 지워주세요 :)

수정했습니다!

hyunssooo · 2021-03-23T07:28:02Z

src/main/java/chess/domain/Position.java

+                .collect(toMap(position -> "" + position.x + position.y, Function.identity()));
+    }


positions의 key값에 ""이 들어가는 이유는 뭘까요? (다 작성하고 보니 int 값을 String으로 변환하기 위해서네요.)
of() 에서도 사용하던데 상수로 분리도 가능할 것 같아요!
혹은 키 값을 integer로 변경할 수도 있을 것 같아요!
8 * 8 총 64개 이니 1 ~ 64까지의(혹은 0~63) 키 값으로 position을 캐싱하고 정적 팩터리 메서드에서 입력 받은 x, y 값을 계산해서 찾아줄수도 있을 것 같네요!

String으로 Key를 만드는 것보다 integer로 받는게 더 자연스러울 거 같아 변경해봤어요!

hyunssooo · 2021-03-23T08:24:23Z

src/main/java/chess/domain/ChessGame.java

+    public void move(final Position currentPosition, final Position targetPosition) throws PieceNotFoundException, ImpossibleMoveException {
+        Piece currentPiece = piece(currentPosition).orElseThrow(PieceNotFoundException::new);
+        if (currentPiece.notSameColor(currentColor)) {
+            throw new ImpossibleMoveException(currentColor + "팀 차례 입니다.");
+        }
+
+        Optional<Piece> targetPiece = piece(targetPosition);
+
+        currentPiece.move(targetPosition);
+        targetPiece.ifPresent(pieces::remove);
+        updateMovablePositions();
+        currentColor = currentColor.reverse();
+    }


저는 이 부분이 조금 어색하게 느껴지는데요
pieces에서 currentPiece를 꺼내고 targetPiece를 찾아서 pieces에게 targetPiece를 삭제해라
이 부분은 pieces의 책임이 아닐까요?
pieces에게 currentPosition, targetPosition 만 넘겨주는 형태로 바뀌는 것은 어떨까요?
pieces는 두 Position을 받고 내부에서 판단을 하고 움직임이 가능하다면 piece를 움직이는 방법이 있을 것 같아요 :)
만약 두 Position을 받는다면 Position이 piece안에 존재하는 것이 어울리는지도 함께 고민해보면 좋을 것 같아요.

Pieces에 메세지를 던져 move 로직을 수행하는 방식으로 변경했습니다!
Position이 Piece에 있어야 하나로 페어와 많은 고민을 했습니다. 처음에는 Map<Position, Piece> board를 ChessGame에서 가지는 식으로 만들었어요. 페어와 토론 중에 체크메이트를 계산하기 위해 각 Piece별로 움직일 수 있는 Position들을 알 수 있어야 하지 않겠나라는 얘기가 나와서 최종 결론을 Piece에 Position을 주는 것으로 정했거든요. 기존 방식대로 Map을 이용하면 각 Piece들이 움직일 수 있는 위치들을 저장하는 게 애매하다고 생각했어요. 다른 방법으로는 movablePositions만 Piece가 필드로 가지고 chessGame에서 계산한 뒤 Piece에게 값을 전해주면 되지 않을까 생각이 드네요ㅎㅎ

hyunssooo · 2021-03-23T09:03:06Z

src/main/java/chess/domain/piece/Pieces.java

+    public void updateMovablePositions() {
+        pieces.forEach(piece -> piece.updateMovablePositions(
+                existPiecePositions(),
+                positionsByTeamColor(piece.enemyColor())
+                )
+        );
+    }


piece가 움직일 때 마다 전체 piece를 움직임 가능한 경로를 계산해서 가지고 있는데 그러면 연산이 너무 많지 않을까요?
piece가 움직이려고 시도할 때 해당 piece에서만 가능한지 여부를 판단하는 방법은 어떨까요??

이 메소드는 왕이 체크가 됐을 때 이동가능한 경로들이 상대방이 공격할 수 있는 범위에 드는지 확인하고 맞다면 움직이지 못하도록 하기 위해 만들었던 메소드에요. 매 턴마다 상대방이 공격 가능한 위치를 검사해야 왕이 못 가게 만들 수 있다고 생각해서 매 턴마다 update를 진행했습니다. 만약 움직이는 말 하나만 update를 하게 되면 그 말로 인해 갈 수 있던 곳이 변경된 다른 말들의 이전 movablePositions를 그대로 사용하여 체크를 계산하게 되니 잘못된 정보가 나오지 않을까 생각했습니다. 다만 현재는 체크메이트도 미구현이고 과도하게 여러곳에서 update를 진행하는 것 같아 check 검사할 때만 하고 piece 하나가 움직였을 때는 그 piece의 movablePostions만 update하도록 변경했습니다!

hyunssooo · 2021-03-23T09:12:50Z

src/main/java/chess/domain/piece/Piece.java

+    public void updateMovablePositions(List<Position> existPiecePositions, List<Position> enemiesPositions) {
+        pieceMoving.updateMovablePositions(existPiecePositions, enemiesPositions);
+    }
+
+    public void move(Position targetPosition) throws ImpossibleMoveException {
+        pieceMoving.move(targetPosition);
+    }
+


상속 받는 클래스에서 재정의 못하게 하려면 final 키워드를 붙여주면 됩니다 :)

hyunssooo · 2021-03-23T09:16:51Z

src/main/java/chess/domain/piece/Piece.java

+    public abstract boolean isPawn();
+
+    public abstract boolean isKing();


Suggested change

public abstract boolean isPawn();

public abstract boolean isKing();

public boolean isPawn() {

return false;

}

public abstract boolean isKing();

부모 클래스에서 정의하고 필요한 클래스에서만 재정의 하는 방법도 있습니다 :)

public class Pawn extends Piece { .. @Override public boolean isPawn() { return true; } }

질문의 메서드가 여기네요. 비즈니스에 필요하니 필요한 메서드라고 생각이 들어요. 또한 각 객체에게 묻는 편이 instanceof 보다는 훨씬 나은것 같습니다 👍

오 이 방법이 훨씬 좋아보이네요 부모 클래스에서 정의하고 필요한 클래스인 King과 Pawn에서만 override하도록 변경했습니다!

…로 변경

하나의 Piece에 대해서만 updateMovablePositions를 수행하는 메소드 추가

Optional을 반환하던 메소드를 변경

hyunssooo

안녕하세요 크로플
step1 진행하신다고 수고 많으셨습니다.
몇 가지 코멘트 남겼어요. step2 진행하시면서 참고 부탁드릴게요!
step2 진행해주세요!

hyunssooo · 2021-03-25T17:27:08Z

src/main/java/chess/controller/ChessAction.java

+public class ChessAction {
+
+    private ChessGame chessGame;
+


ChessAction이 컨트롤러로 보이네요. 보통 컨트롤러는 인스턴스를 하나만 생성합니다.
현재 구조는 게임당 컨트롤러를 생성해야될 거 같은 구조네요

hyunssooo · 2021-03-25T17:29:37Z

src/main/java/chess/domain/Position.java

+
+    private static final Map<Integer, Position> positions;
+


List를 사용해도 괜찮을 것 같아요 :) index를 사용하면 됩니다!

hyunssooo · 2021-03-25T17:43:09Z

src/main/java/chess/domain/piece/direction/Direction.java

+    public int getY() {
+        return y;
+    }
+}


Suggested change

}

}

마지막 줄은 개행으로 끝납니다 :)

hyunssooo · 2021-03-25T17:50:15Z

src/main/java/chess/exception/ImpossibleMoveException.java

+
+public class ImpossibleMoveException extends ChessException {
+    public ImpossibleMoveException() {
+        this("이동할 수 없는 위치입니다.");
+    }
+


custom exception 패키지 위치도 고민해보면 좋을 것 같아요
보통 해당 레이어의 exception은 해당 레이어에 패키지를 별도로 분리합니다.
domain 객체에 대한 exception은 domain 패키지 아래에 생성하면 좋을 것 같아요 :)
또한, 이펙티브 자바 아이템 72. 표준 예외를 사용하라. 를 읽어보시고 정말 필요할때만 custom exception을 만들면 좋을 것 같아요 :)

hyunssooo · 2021-03-25T17:50:33Z

src/main/java/chess/util/MessagePositionConverter.java

+    private MessagePositionConverter() {
+    }
+


불필요한 객체 생성 방지 👍

perenok · 2021-04-19T15:11:55Z

학습 로그

Optional

메서드가 반환할 결과값이 ‘없음’을 명백하게 표현할 필요가 있고, null을 반환하면 에러를 유발할 가능성이 높은 상황의 반환 타입
orElse, orElseGet을 통해 해당 메소드가 null을 반환할 때 행동을 정해서 null이 반환되는 것을 방지할 수 있다.
비싼 연산이므로 단순 값을 얻을 때는 null 비교, 컬렉션을 반환 할 때는 빈 컬렉션을 반환하는 것이 좋다.

		if (currentPosition.invalidGo(killDirection)) return;
		currentPosition = currentPosition.go(killDirection);

		.collect(toMap(position -> "" + position.x + position.y, Function.identity()));
		}

		public abstract boolean isPawn();

		public abstract boolean isKing();

Conversation

perenok commented Mar 22, 2021

궁금한 점

Uh oh!

hyunssooo 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!

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!

hyunssooo 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!

perenok commented Apr 19, 2021

학습 로그

Optional

태그

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants