Step1 : 체스 게임 1단계 리뷰 요청 드립니다! #11

seongbeenkim · 2021-08-20T18:32:12Z

안녕하세요 우빈 선생님
체스 게임 1단계 리뷰 요청드립니디!

각 기물이 가진 이동 규칙 특수성(폰, 나이트 등)에 의해 어떻게 역할, 책임을 분리해야할 지 어려움을 많이 겪었던 미션이었습니다ㅠㅠ
이러한 특수성을 가지는 도메인 요구사항이 있을 경우에는 어떻게 공통적인 로직 + 특수성을 모두 챙기면서 객체지향적으로 코드를 작성할 수 있을 지 궁금합니다!

wbluke

깔끔하게 잘 구현해 주셨네요 👍
많은 고민을 하시고 잘 녹여주신 것 같아요 ㅎㅎ
몇 가지 코멘트 남겼으니 확인 부탁드려요 🙂

wbluke · 2021-08-22T11:16:05Z

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

+    private static final Map<String, Position> POSITIONS = createPositions();
+
+    private static Map<String, Position> createPositions() {
+        Map<String, Position> positions = new LinkedHashMap<>();


오 HashMap이 아니라 LinkedHashMap을 사용한 이유가 있으실까요? :)

초기 체스판 각 기물들이 위치가 순서대로 나열되어 있는 이미지를 그리면서 구현하여 LinkedHashMap으로 해주었습니다.
Position는 캐싱 값을 가지고 반환해주는 역할만 가지고 있기 때문에 HashMap으로 해도 되네요.
수정해주었습니다!

wbluke · 2021-08-22T11:17:01Z

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

+        Arrays.stream(File.values())
+                .forEach(file -> Arrays.stream(Rank.values()).forEach(rank -> positions.put(createKey(file, rank), new Position(file, rank))));


가독성 측면에서 어떤 생성을 하고 있는지 한 눈에 알아보기가 어렵네요 ㅎㅎ
메서드를 추출하여 가독성을 개선해보면 어떨까요? :)

넵넵 제가 작성한 코드임에도 리딩하는데 시간이 오래 걸리네요..😂
피드백 주신 방법대로 메소드 추출하는 방식으로 변경해주었습니다!

wbluke · 2021-08-22T11:54:47Z

src/main/java/chess/domain/piece/pattern/MovePattern.java

+    }
+
+    public static MovePattern queenPattern() {
+        return new MovePattern(DIAGONAL_COORDINATES, CARDINAL_COORDINATES);


페어나눔 때 이야기하셨지만 퀸은 전 방향 모두 여러 칸 이동 가능합니다 :)

넵넵 퀸은 모두 이동할 수 있게 수정해줬습니다 :)

wbluke · 2021-08-22T12:01:59Z

src/main/java/chess/domain/piece/pattern/MovePattern.java

+    private final Collection<MoveCoordinate> infiniteMoveCoordinates;
+    private final Collection<MoveCoordinate> finiteMoveCoordinates;


음 무한좌표와 유한좌표로 나눠서 관리할 필요가 있을까 하는 의문이 드는데요 ㅎㅎ
source, target이 주어졌을 때, 어떤 말이 움직일 수 있는 단위 방향만 가지고 있다면
source-target의 단위 방향을 구하고(findMoveCoordinate)
그 말이 단위방향 1칸으로만 움직일 수 있는 말인지, 단위방향 여러 칸으로 움직일 수 있는 말인지에 따라 로직을 전개하면 될 것 같아요 :)

넵넵 피드백주신 것처럼 단위 방향을 통해서 기물을 움직이도록 수정하였습니다 :)

wbluke · 2021-08-22T12:02:51Z

src/main/java/chess/domain/piece/pattern/MovePattern.java

+    private static final Collection<MoveCoordinate> CARDINAL_COORDINATES = Collections.unmodifiableList(Arrays.asList(
+            NORTH, SOUTH, WEST, EAST
+    ));
+
+    private static final Collection<MoveCoordinate> DIAGONAL_COORDINATES = Collections.unmodifiableList(Arrays.asList(
+            NORTH_EAST, NORTH_WEST, SOUTH_EAST, SOUTH_WEST
+    ));
+
+    private static final Collection<MoveCoordinate> WHITE_PAWN_COORDINATES = Collections.unmodifiableList(Arrays.asList(
+            WHITE_PAWN_INITIAL_NORTH, NORTH_EAST, NORTH_WEST, NORTH
+    ));
+
+    private static final Collection<MoveCoordinate> WHITE_PAWN_ATTACK_COORDINATES = Collections.unmodifiableList(Arrays.asList(
+            NORTH_EAST, NORTH_WEST
+    ));
+
+    private static final Collection<MoveCoordinate> BLACK_PAWN_COORDINATES = Collections.unmodifiableList(Arrays.asList(
+            BLACK_PAWN_INITIAL_SOUTH, SOUTH_EAST, SOUTH_WEST, SOUTH
+    ));
+
+    private static final Collection<MoveCoordinate> BLACK_PAWN_ATTACK_COORDINATES = Collections.unmodifiableList(Arrays.asList(
+            SOUTH_EAST, SOUTH_WEST
+    ));
+
+    private static final Collection<MoveCoordinate> KNIGHT_COORDINATES = Collections.unmodifiableList(Arrays.asList(
+            NORTH_EAST_LEFT, NORTH_EAST_RIGHT, NORTH_WEST_LEFT, NORTH_WEST_RIGHT,
+            SOUTH_EAST_LEFT, SOUTH_EAST_RIGHT, SOUTH_WEST_LEFT, SOUTH_WEST_RIGHT
+    ));


상수가 엄청 많아졌는데요, Enum을 고려해보면 어떨까요? :)

해당 부분 Enum으로 수정해주니 코드가 더 깔끔해졌네요 감사합니다 :)

wbluke · 2021-08-22T12:16:58Z

src/main/java/chess/domain/command/Command.java

+    private static final String START = "start";
+    private static final String END = "end";
+    private static final String MOVE = "move";


Command도 Enum으로 관리해보면 어떨까요? :)

상수들이기 때문에 Enum으로 관리하는 편이 더 좋겠네요!
Enum 객체로 수정하였습니다 :)

wbluke · 2021-08-22T13:15:10Z

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

+    public Set<Position> findPassingPositions(Position target, MoveCoordinate moveCoordinate) {
+        Set<Position> positions = new HashSet<>();
+        Position current = this;
+
+        while (!target.equals(current)) {
+            current = current.move(moveCoordinate);
+            positions.add(current);
+        }
+
+        positions.remove(target);
+        return positions;
+    }
+
+    public Collection<Position> findAvailablePositions(final MoveCoordinate moveCoordinate, final boolean isFinite) {
+        if (isFinite) {
+            return getFinitePositions(moveCoordinate);
+        }
+        return getInfinitePositions(moveCoordinate);
+    }


저는 이 기능들이 Position의 책임일까? 하는 생각이 드는데요 ㅎㅎ
메서드 내용을 살펴보면 Position 인스턴스 this를 current에 할당한 뒤
move 하면서 원하는 Collection<Position>을 추출하고 있는데요,

this를 새로운 current에 할당한다는 점부터 고민해보았을 때,
Position의 인스턴스 메서드로 다루는 것이 과연 맞을까,
이런 책임은 Position이 아니라 Position을 다루는 제 3자 객체가 담당해야 자연스럽지 않을까, 하는 생각이 들어요.

어떻게 보면 Position은 다뤄지는 최소 단위, 좌표를 나타내기 도구인 것이고
그 도구를 어떻게 다룰지는 도구가 결정하는 것이 아니라 도구를 사용하는 쪽에서 결정해야 한다고 생각해요 :)

넵넵 피드백 주신 부분에 대해서 동의합니다!
전체 캐싱된 Position을 가지고 있다고 하더라도 내부에서 this를 새로운 current에 할당하고 계속 변경이 일어나기 때문에
자기 자신에 대한 상태만 알고 있어야 하는 하나의 Position 객체가 알 필요 없는 다른 Position 객체에 대한 정보도 알 수 있게 되네요😂

해당 로직을 호출하는 상위 객체인 Piece에게 위임하는 방식으로 수정하였습니다!

wbluke · 2021-08-22T13:21:50Z

src/main/java/chess/domain/player/AttackPositions.java

+public class AttackPositions {
+
+    private static final int EMPTY = 0;
+
+    private final Map<Position, Integer> counts = new HashMap<>();
+
+    public AttackPositions(final Map<Position, Piece> pieces) {
+        pieces.keySet()
+                .forEach(position -> {
+                    Piece piece = pieces.get(position);
+                    Collection<Position> positions = piece.findAvailableAttackPositions(position);
+                    positions.forEach(this::increase);
+                });
+    }


저는 이 AttackPositions, Map<Position, Integer>의 의도나 활용이 잘 이해가 가지 않는데요,
관련해서 설명해주실 수 있으신가요? :)

AttackPositions은 각 Position(위치)로 공격할 수 있는 기물이 몇 개인지를 표시한 객체입니다.

예를 들어 게임 초기 시 흰색 플레이어
Position c3에는 b1 나이트, b2 폰이 공격 할 수 있기 때문에 (c3, 2)
Position b3에는 a2 폰, c2 폰이 공격 할 수 있기 때문에 (b3, 2) 이런식으로 저장이 됩니다.

킹의 스스로 체크 위치로 (잡힐 수 있는 위치) 이동할 수 없습니다라는 이동 규칙 때문에 만들어주었습니다.

각 플레이어는 자신이 공격할 수 있는 위치에 대한 AttackPositions를 가지고 있으며 각 기물 이동 시 마다 갱신됩니다.
킹을 선택하여 이동할 시, 상대방의 AttackPositions을 통해서 자신이 이동할 수 있는 위치를 확인할 수 있게 됩니다.

현재 경로에 상대방 또는 자신의 기물이 존재하여 공격할 수 없는 위치임에도 카운트가 되는 문제가 있어 해결중에 있습니다.... 빠르게 해결하겠습니다..

wbluke · 2021-08-22T13:37:13Z

src/main/java/chess/domain/player/Player.java

+        double pawnScores = calculatePawnScores(pawnPositions);
+        double sum = pieces.values()
+                .stream()
+                .filter(piece -> !piece.isPawn())


저는 부정연산자(!)를 웬만하면 피하는데요 ㅎㅎ
가독성의 문제도 있고, 읽는 사람이 한번 더 해석을 해야하기 때문에 메서드를 제공하는 쪽에서 한번 더 추상화시키면 좋을 것 같아요 ㅎㅎ
piece.isNotPawn()

넵넵! 해당 피드백 읽은 후 부정연산자 사용하는 곳 모두 수정해주었습니다 :)

wbluke · 2021-08-22T13:38:38Z

src/main/java/chess/domain/player/Player.java

+    }
+
+    public void attacked(final Position target) {
+        if (!hasPieceOn(target)) {


위에도 부정연산자 피드백을 남겼는데요, 여기도 한번더 추상화시키면 좋을 것 같아요 ㅎㅎ
전체적으로 한번 더 체크해봐도 좋겠네요 :)

seongbeenkim and others added 22 commits August 16, 2021 20:15

feat: 피스 생성 테스트 및 구현

3a12a76

feat: 피스 위치 캐싱 테스트 및 구현

667d30a

feat: 체스판 32개의 피스 초기화, 생성 테스트 및 구현

05e7c1d

feat: 기물 종류에 따라 이름을 매핑하는 기물 이름 매퍼 테스트 및 구현

2d33803

feat: 체스판 전체 출력하는 기능 구현

cc54492

feat: 체스 게임 시작&종료 명령어 입력 테스트 및 구현

8bc0bd5

feat: 기물 이동 시 공통 예외 케이스 테스트 및 구현

f6c2eb4

feat: 비숍 이동 규칙 테스트 및 구현

22d29d1

feat: 룩 이동 규칙 테스트 및 구현

c877ccd

feat: 퀸 이동 규칙 테스트 및 구현

adc18b6

feat: 킹 이동 규칙 테스트 및 구현

f3c877a

feat: 나이트 이동 규칙 테스트 및 구현

12da4ca

feat: 폰 이동 규칙 테스트 및 구현

44ecf39

feat: 체스판에서 피스 이동 테스트 및 구현

4498862

refactor: 피스 색상별 체스판 분리

a40508d

feat: 비숍, 룩 공격 가능한 경로 반환하는 테스트 및 기능 구현

c30f242

feat: 폰 제외한 기물 패턴 테스트 및 기능 구현

483c149

feat: 기물 이동 시 공격 가능 위치를 갱신하는 테스트 및 기능 구현

c2f0dea

feat: 킹 이동제한조건 테스트 및 기능 구현, 예외 처리 및 게임 흐름 제어

c17c852

feat: 점수 계산 기능 테스트 및 구현

5d69a67

feat: 점수 및 게임 결과 출력 기능 구현

be1d67e

refactor: 패키지 구조 및 테스트 케이스 정리

aba4a3a

wbluke requested changes Aug 22, 2021

View reviewed changes

seongbeenkim added 7 commits August 31, 2021 23:51

refactor: LinkedHashMap -> HashMap 변경

1f8194b

refactor: Position 캐시 생성 코드 메소드 추출하여 가독성 개선

de1c9c7

refactor: 부정연산자 제거 및 변수 이름 변경

09575d2

refactor: 가독성을 위한 메소드 추출 및 '==' 제거

b1e17a0

refactor: ENUM 이름 규칙(대문자 적용) 및 테스트 코드 추가

527e5d9

refactor: 테스트 코드 추가

770f4cb

refactor: ENUM 클래스 map 자료구조 적용

2df66a6

seongbeenkim added 10 commits September 2, 2021 22:35

refactor: 변수 이름 변경 및 추상화 수준 한 단계 제거

4ef547b

refactor: MoveCoordinate -> Direction으로 변경

6f89fbf

refactor: MovePattern ENUM 클래스로 변경

f90cb51

refactor: 모든 기물 단위 방향만으로 이동할 수 있게 수정

93b8d34

refactor: AttackPositions -> AttackRange로 변경

8b1d2f5

refactor: 변수, 함수 이름 변경

123d3d3

refactor: 이동 경로 구하는 로직 Position -> Piece로 위임

296dd67

refactor: Set -> Collection 타입 반환 및 메소드 이름 변경

3347861

refactor: MoveParameters -> MoveOptions 변경

361883e

refactor: 명령어 enum으로 분리 및 옵션 포함된 명령어 객체 추가

d6edf85

seongbeenkim force-pushed the step1 branch from 1f16d1a to d6edf85 Compare September 3, 2021 19:18

seongbeenkim requested a review from wbluke September 3, 2021 19:22

refactor: 명령어 enum으로 분리 및 옵션 포함된 명령어 객체 추가

3ef6adb

seongbeenkim force-pushed the step1 branch from c85d73f to 3ef6adb Compare September 3, 2021 19:31

refactor: CommandOptions에서 MoveOptions 반환하게 수정

b21c36a

seongbeenkim force-pushed the step1 branch from be1e04e to b21c36a Compare September 13, 2021 11:38

refactor: 폰 공격 Direction 제거

af13054

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step1 : 체스 게임 1단계 리뷰 요청 드립니다! #11

Step1 : 체스 게임 1단계 리뷰 요청 드립니다! #11

seongbeenkim commented Aug 20, 2021

wbluke left a comment

wbluke Aug 22, 2021

seongbeenkim Aug 31, 2021

wbluke Aug 22, 2021

seongbeenkim Aug 31, 2021

wbluke Aug 22, 2021

seongbeenkim Sep 2, 2021

wbluke Aug 22, 2021

seongbeenkim Sep 2, 2021

wbluke Aug 22, 2021

seongbeenkim Sep 2, 2021

wbluke Aug 22, 2021

seongbeenkim Sep 3, 2021

wbluke Aug 22, 2021

seongbeenkim Sep 2, 2021

wbluke Aug 22, 2021

seongbeenkim Sep 2, 2021

wbluke Aug 22, 2021

seongbeenkim Sep 2, 2021

wbluke Aug 22, 2021

		Arrays.stream(File.values())
		.forEach(file -> Arrays.stream(Rank.values()).forEach(rank -> positions.put(createKey(file, rank), new Position(file, rank))));

		private final Collection<MoveCoordinate> infiniteMoveCoordinates;
		private final Collection<MoveCoordinate> finiteMoveCoordinates;

Step1 : 체스 게임 1단계 리뷰 요청 드립니다! #11

Are you sure you want to change the base?

Step1 : 체스 게임 1단계 리뷰 요청 드립니다! #11

Conversation

seongbeenkim commented Aug 20, 2021

wbluke 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