[3단계 - 체스] 미아(이종미) 미션 제출합니다. #736

jongmee · 2024-03-28T04:13:13Z

안녕하세요 오리! 안내 주신 대로 3단계 먼저 PR 보냅니다 ☺️

3단계에서 추가된 기능

게임 순서에 따라 사용자가 입력하도록 한다.
기물마다 점수가 존재해, 사용자가 status를 입력하면 각 진영의 총 점수를 출력한다.
한 쪽 진영의 King이 잡히면 게임이 종료된다.

변경된 부분

controller 패키지에서 상태 패턴 사용

ChessGame의 역할이 커지는 것 같아 게임의 상태를 클래스로 분리해서 체스게임 어플리케이션을 실행하도록 했습니다. 패키지 이름을 고민했는데, 'game'으로 하기에는 'domain' 패키지와 혼동될 것 같아(ex. game은 도메인 클래스가 아닌가?) 'controller'를 유지했습니다. 게임의 상태들 역시 도메인(Model) 클래스들과 UI(View) 클래스들 사이에서 어플레이케이션을 제어하기 때문에, MVC 패턴에서 Controller의 역할을 수행한다고 생각했기 때문입니다.

`PieceValue` 인터페이스 생성

Pawn과 다른 기물들 모두 기물 가치(점수)를 가지고 있습니다. 이를 PieceValue로 추상화하고 Pawn과 다른 기물들에 대한 기물 가치를 구현하였습니다.

`ChessBoardGenerator` 인터페이스 생성

기존에 존재하던 ChessBoardInitializer의 인터페이스입니다. 체스판 생성을 유연하게 변경할 수 있도록 인터페이스를 생성하였습니다. 실제로 ChessBoard를 테스트할 때 TestChessBoardGenerator를 구현하여 쉽게 테스트할 수 있었습니다.

3단계도 잘 부탁드립니다!

jinyoungchoi95

안녕하세요 미아 😄

3단계 잘 구현해주셨네요. 몇가지 코멘트 남겨두었으니 확인부탁드려요.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jinyoungchoi95 · 2024-03-28T09:59:50Z

src/main/java/chess/model/board/ChessBoardGenerator.java

+
+import java.util.Map;
+
+public interface ChessBoardGenerator {


기존에 존재하던 ChessBoardInitializer의 인터페이스입니다. 체스판 생성을 유연하게 변경할 수 있도록 인터페이스를 생성하였습니다. 실제로 ChessBoard를 테스트할 때 TestChessBoardGenerator를 구현하여 쉽게 테스트할 수 있었습니다.

ChessBoard의 생성자

public ChessBoard(Map<Position, Piece> board) { this.board = new HashMap<>(board); }

에서 테스트에 필요한 원하는 position이 담긴 ChessBoard를 생성해도 될텐데 Generator를 추상화한 목적이 불분명하지 않을까요?

처음에는 Map을 직접 생성하고 아래와 같이 ChessBoard를 생성하여 테스트하였습니다.

private ChessBoard createCustomChessBoard(Knight knight, Position knightPosition) { Map<Position, Piece> chessBoard = Arrays.stream(File.values()) .flatMap(this::createPositionStream) .collect(toMap(identity(), position -> Blank.INSTANCE)); chessBoard.put(knightPosition, knight); chessBoard.put(knightPosition.calculateNextPosition(0, 1), Pawn.from(Side.WHITE)); return new ChessBoard(chessBoard); } private Stream<Position> createPositionStream(File file) { return Arrays.stream(Rank.values()) .map(rank -> Position.of(file, rank)); }

빈 보드에 기물 2개만 생성하고 있는데, 코드 길이도 길고 가독성도 떨어진다고 생각했습니다. 아래와 같이 특정 체스 보드 상황으로 초기화하고 싶은 경우는 차라리 Fixture를 List로 생성해놓고 사용하는게 이해가 더 쉬울 거 같다고 생각했습니다.

/* ChessBoardFixture.java 바보 체크메이트(Fool's Mate) 상황 */ public static final List<Piece> WHITE_FOOLS_MATE_LOSE = List.of( Rook.from(BLACK), Knight.from(BLACK), Bishop.from(BLACK), Blank.INSTANCE, King.from(BLACK), Bishop.from(BLACK), Knight.from(BLACK), Rook.from(BLACK), Pawn.from(BLACK), Pawn.from(BLACK), Pawn.from(BLACK), Pawn.from(BLACK), Pawn.from(BLACK), Pawn.from(BLACK), Pawn.from(BLACK), Pawn.from(BLACK), Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Pawn.from(BLACK), Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Pawn.from(WHITE), Blank.INSTANCE, Pawn.from(WHITE), Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Pawn.from(WHITE), Blank.INSTANCE, Blank.INSTANCE, Blank.INSTANCE, Pawn.from(WHITE), Pawn.from(WHITE), Pawn.from(WHITE), Pawn.from(WHITE), Blank.INSTANCE, Blank.INSTANCE, Pawn.from(WHITE), Rook.from(WHITE), Knight.from(WHITE), Bishop.from(WHITE), Queen.from(WHITE), Queen.from(BLACK), Bishop.from(WHITE), Knight.from(WHITE), Rook.from(WHITE) );

따라서 List를 Map<ChessPosition, Piece>로 변환해주는 TestChessBoardGenerator를 테스트에 사용했습니다. 프로덕션 코드에서도 초기 체스판 구성이 바뀔 경우 새로운 ChessBoardGenerator 구현체를 생성하면 될 것입니다.

Generator를 추상화한 저의 의도는 위와 같은데, ChessBoard의 생성자에서 Map<ChessPosition, Piece>을 받을 수 있기 때문에 추상화 목적으로는 충분하지 않다고 보시나요 ?? 오리의 생각이 궁금합니다 !

ChessBoard가 만약 Map<ChessPosition, Piece>를 들고 있지 않고 Generator구현체만을 생성자에서 들고 있을 때 그걸 테스트하기 위해서 추상화한다면 미아가 이야기한 상황이 맞겠죠. 다만 말씀하신대로 ChessBoard가 Map<ChessPosition, Piece>를 받을 수 있는데 Generator를 추상화하는 건 서로 목적이 다른 것 같아요.

테스트를 위해서 추상화한다는 목적은 이해하였지만 클래스의 단위테스트를 진행할 때는 최대한 다른 클래스와의 의존성은 필요한만큼만 사용할 수 있도록 하는 것이 좋아요. 그래야 변경의 여파가 서로에게 영향이 안가는 테스트를 작성할 수 있습니다.

다음 step에서 더 고민해보고 질문 남기겠습니다!

jinyoungchoi95 · 2024-03-28T11:27:22Z

src/main/java/chess/model/piece/Queen.java

+
+    @Override
+    public PieceValue value() {
+        return new CommonValue(9);


점수 9와 같은 값들은 의미있는 상수로 변경할 수 있겠네요.

각 기물 클래스 모두 기물 가치 값을 상수화 하였습니다. PieceValue는 값이 다양해서 캐싱하려면 그 값들을 모두 알아야하기 때문에 캐싱하지 않았는데요! 각 Piece에서 상수로 PieceValue 객체 자체를 가지고 있는 것에 대해서 어떻게 생각하시나요??

PieceValue에서 값이 다양해서 미리 캐싱을 해두지 못한다면 Cache를 두고 getOrPut과 같은 방법으로 이미 생성 요청이 들어왔던 PieceValue에 대해서는 재사용하는 방법도 있을 것 같아요.

각 Piece에서 상수로 PieceValue 객체 자체를 가지고 있는 것에 대해서 어떻게 생각하시나요??

좋은 것 같아요 :)

jinyoungchoi95 · 2024-03-28T11:28:45Z

src/main/java/chess/model/piece/Blank.java

+
+    @Override
+    public PieceValue value() {
+        throw new UnsupportedOperationException("빈칸은 기물 가치(Piece Value)가 없습니다.");


예외의 경우에는 가능하면 테스트가 작성되었으면 합니다 :)

Blank에 대해서도 테스트를 추가하였습니다!

jinyoungchoi95 · 2024-03-28T11:29:31Z

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

-    public abstract Path findPath(ChessPosition source, ChessPosition target, Piece targetPiece);
+    public abstract Path findPath(Position source, Position target, Piece targetPiece);
+
+    public abstract PieceValue value();


PieceValue와 value보다는 point와 같은 점수임을 나타내는게 어떨까요?

point나 score도 좋지만 체스에서 '기물 가치(PieceValue)'라고 부르는 것 같아 해당 용어를 사용해보았습니다..!

앞서 언급한 것처럼 체스의 각 기물들은 서로 다른 가치를 가지고 있습니다. 기물 가치는 기물들의 강함의 정도를 나타냅니다. 폰은 1점, 나이트와 비숍은 3점, 룩은 5점, 퀸은 9점입니다.

https://www.chess.com/ko/terms/chess-piece-value-ko

jinyoungchoi95 · 2024-03-28T11:30:51Z

src/main/java/chess/model/evaluation/PawnValue.java

+import static java.util.stream.Collectors.counting;
+import static java.util.stream.Collectors.groupingBy;
+
+public class PawnValue implements PieceValue {


이미 Pawn에 한정되어있는 점수객체라면 point와 unfavorablePoint를 받아서 생성하기보다는 상수로 들고 있어도 되지 않을까요?

현재 PieceValue 의 역할은 각 기물의 가치 계산을 하는 것인데요..! Pawn이 두 점수를 상수로 들고 있다면 Pawn 내부에서 가치 계산을 해주는게 좋을 것 같은데, 1) Piece의 역할이 커지는 것 같고 2) 기물 가치 계산을 evaluation 패키지가 담당하도록 분리하였습니다.

오리는 Piece에서 기물 가치 계산을 하는 게 더 역할이 잘 분배된 구조라고 보시나요 ??

Pawn이 들고있다라기보다는 PawnValue라는 구현체가 이미 pawn의 점수를 가지고 있는 네이밍과 책임을 들고 있기 때문에 PawnValue가 두 값을 상수로 들고 있는걸 말씀드린 것이었어요.

역할 분배자체는 PieceValue로 충분히 잘되어있다고 생각합니다.

jinyoungchoi95 · 2024-03-28T11:32:59Z

src/main/java/chess/model/board/Turn.java

+        return Turn.from(side.getOppositeSide());
+    }
+
+    public boolean isNotCorrect(Piece piece) {


"옳지 않다"라는 이름이 이 메소드를 잘 표현해주고 있지 않은 것 같아요. 더 명확한 네이밍은 없을까요?

Piece가 차례에 맞지 않는 것이므로 doesNotMatch로 변경해봤습니다! 저는 괜찮은 것 같은데 명확하다고 생각하시나요 ?

네 괜찮은 것 같아요 :)

jinyoungchoi95 · 2024-03-28T11:37:04Z

src/main/java/chess/model/board/ChessBoard.java

            throw new IllegalArgumentException("소스 위치에 기물이 존재하지 않습니다.");
        }
+        if (turn.isNotCorrect(sourcePiece)) {
+            throw new IllegalArgumentException("올바른 게임 차례가 아닙니다.");


무엇이 올바른지 명확하게 명시하는게 어떨까요? 예외 메시지는 명확하면 명확할수록 예외가 발생했을 때 어떤 문제가 발생했는지 알려줄 수 있어요.

"소스 위치에 있는 기물이 현재 게임 차례에 맞지 않습니다."라고 변경해보았습니다

jinyoungchoi95 · 2024-03-28T11:38:27Z

src/main/java/chess/controller/GameState.java

+import chess.view.input.InputView;
+import chess.view.output.OutputView;
+
+public interface GameState {


jinyoungchoi95 · 2024-03-28T11:40:53Z

src/main/java/chess/controller/Prepare.java

+        ChessBoardGenerator chessBoardInitializer = new ChessBoardInitializer();
+        ChessBoard chessBoard = new ChessBoard(chessBoardInitializer.create());
+        outputView.printChessBoard(chessBoard);
+        return new Run(chessBoard, Turn.from(Side.WHITE));


(반영하지 않으셔도 됩니다.) new Run(chessBoard, Turn.from(Side.WHITE))는 Run의 첫턴이기 때문에 Run에서 첫턴에 해당하는 네이밍을 가진 생성 메소드를 정적팩토리메소드에서 제공할수도 있겠네요.

아래와 같이 정적팩토리메소드를 생성했습니다!

public static Run initializeWithFirstTurn(ChessBoard chessBoard) { return new Run(chessBoard, Turn.from(Side.WHITE)); }

첫번째 차례를 더 분명하게 보여줄 수 있는 것 같습니다 👍

jinyoungchoi95 · 2024-03-28T11:42:39Z

README.md

GameState에 대한 정리도 있었으면 좋겠네요 :)

GameState에 따라 비즈니스 기능이 구분되도록 수정하였습니다!

jinyoungchoi95

안녕하세요 미아 😄

피드백 잘 반영해주셨네요. 다음단계 진행을 위해 머지하겠습니다. 남겨둔 코멘트에 궁금하신점이 있다면 다음 pr에 함께 올려주세요.

궁금하거나 고민이 되는 부분이 있으시다면 언제든 pr 코멘트 또는 dm으로 요청 부탁드립니다.
감사합니다 🙇‍♂️

jongmee added 27 commits March 26, 2024 18:42

feat(Side): 반대편 진영을 구한다

8d67fa3

feat(Turn): 게임 순서 도메인 클래스 생성, 캐싱한다

5fa8f80

feat: 게임 차례에 맞지 않는 Source 기물의 위치를 입력하면 예외가 발생한다

a40067e

docs: step3 구현할 기능 목록 추가

e5c090f

refactor: rule 패키지명 game으로 변경

8df9252

fix: 잘못된 import 수정

98d0776

feat(PieceValue): 각 기물에 대한 점수를 구한다

42e8dd7

feat(Piece): 각 기물은 기물 가치를 가진다

47e4ca2

feat(PositionEvaluation): 체스판에 있는 기물에 대한 점수를 구한다

b2019a1

fix: NONE Side인 기물은 캐싱하지 않기

761ba5f

feat(InputView, GameCommand): 점수 출력 명령어를 입력한다

c8447c4

feat(OustputView, ChessGame): 현재 차례의 남아 있는 기물들의 점수 합을 출력한다

648d021

refactor: controller에서 game 진행을 state로 관리하기

5f61f69

feat(ChessBoard): 체스보드에 각 진영의 King이 모두 존재하면 게임을 이어갈 수 있다

767e519

refactor(ChessBoard): getter 사용을 PositionEvaluation을 생성하는 메소드로 대체

d3470a6

refactor(ChessBoard): 메소드 이름 변경

8de32d4

refactor(PositionEvaluation): Stream 변수명 변경

926ef6b

refactor(Position): ChessPosition 클래스 이름 변경

b2a0b6c

refactor: game 패키지 이름을 evaluation으로 변경

c831f55

test(ChessBoardInitializer): 안 쓰는 테스트 삭제

0a85b84

refactor(ChessBoardGenerator): 체스 보드 생성기 추상화 및 테스트 적용

4a6dc07

fix(ChessGame): 예외 처리 버그 해결

e7cf52f

test(ChessBoardTest): 한 쪽이 이겨 한 King이 존재하지 않을 경우 게임이 종료된다

13d3406

refactor: 테스트 유틸 패키지 위치 이동

67aecbb

refactor(PawnValue): 필드명 변경

850086f

docs: 기능 구현 목록 정리

ba071da

fix(ChessGame): 불필요한 예외처리 제거

a00a9d8

jinyoungchoi95 requested changes Mar 28, 2024

View reviewed changes

jongmee added 2 commits March 28, 2024 23:26

style: Stream 개행 추가

f03fd6a

refactor(PieceValue): 메서드명 통일

2b75966

jongmee added 6 commits March 29, 2024 00:00

test(BlankTest): Blank Piece 테스트 추가

655e91c

refactor(Turn): 메소드명 변경

48f5c74

chore(ChessBoard): 예외 메세지 변경

bdb5c56

refactor(Piece): 기물 가치 점수 상수화

0a45b12

refactor(Run): 첫 번째 체스 게임 실행 관련 팩토리 메소드 생성

1c50ce4

docs: GameState에 따라 비즈니스 기능 분리

5bbf3ca

jinyoungchoi95 approved these changes Mar 29, 2024

View reviewed changes

jinyoungchoi95 merged commit ec2b369 into woowacourse:jongmee Mar 29, 2024

jongmee mentioned this pull request Mar 31, 2024

[4단계 - 체스] 미아(이종미) 미션 제출합니다. #778

Merged

[3단계 - 체스] 미아(이종미) 미션 제출합니다. #736

[3단계 - 체스] 미아(이종미) 미션 제출합니다. #736

Uh oh!

Conversation

jongmee commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

3단계에서 추가된 기능

변경된 부분

controller 패키지에서 상태 패턴 사용

PieceValue 인터페이스 생성

ChessBoardGenerator 인터페이스 생성

Uh oh!

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

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!

jinyoungchoi95 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jongmee commented Mar 28, 2024 •

edited

Loading

`PieceValue` 인터페이스 생성

`ChessBoardGenerator` 인터페이스 생성