켈리 3 & 4 제출 합니다. by kelly6bf · Pull Request #25 · woowacourse-6th-code-review-study/java-chess

kelly6bf · 2024-04-03T02:06:50Z

No description provided.

* docs: 기능 명세서 초안 작성 * feat(position): Position 인스턴스 생성 로직 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(inputView): 사용자로부터 명령을 입력받는 기능 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * fix(position): 위치 객체에서 체스 도메인 용어를 사용하도록 변경 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(board): 체스판의 말을 시작 위치로 배치하는 기능 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(outputView): 체스판 출력 기능 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * docs(readme): 2단계 기능 요구사항 추가 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor(/position): 위치정보와 관련된 클래스들을 한 패키지로 모음 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor(position): Position 생성자 파라미터 순서 변경 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(unitVector): 8방위를 나타내는 단위 벡터 객체 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(position): 현재 위치에서 백터를 누적한 새로운 위치를 생성하는 로직 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(orthogonalMoveStrategy): 직선 방향으로 이동 가능 여부를 확인하는 전략 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(continuousMoveStrategy): 이동 가능한 방향벡터와 이동 횟수 제한을 가지는 연속 이동 전략 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor(boardInitializer):체스판 초기화 책임 분리 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor(continuousMoveStrategy): 스트림 내의 Predicate 메서드 분리 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(board): 체스 말을 이동시키는 기능 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(knightMoveStrategy): 나이트 이동 전략 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(pawnMoveStrategy): 폰의 이동 전략 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * test(boardTest): 기물을 이동시키는 로직의 테스트 추가 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor(board): 이동 불가능한 경우별 예외에 메시지 구분 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(inputView): 사용자의 명령을 입력받는 기능 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(chessController): 팀을 번갈아 가며 게임을 진행하는 기능 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * test(knightMoveStrategyTest): 나이트 움직임에 대한 테스트 코드 추가 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * test(pawnMoveStrategyTest): 폰 움직임에 대한 테스트 코드 추가 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * style(positionTest): 테스트 설명 수정 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat(pieceFactory): PieceType을 전달받아 올바른 Piece를 생성하는 객체 추가 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * test(boardTest): Board 출발지 -> 목적지 기물 배치 성공 & 실패 테스트 추가 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor(continuousMoveStrategy): 메서드 이름 변경 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * style(position): 미사용 TODO 삭제 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor(chessController): 사용자 입력에 따라 게임을 진행하는 기능 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor(requestDto): 명령 인자에서 Optional 제거 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * fix(pawnMoveStrategy): 폰이 직선 이동에서 상대 기물을 공격하는 버그 수정 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor: position 도메인 객체 수정 - 기존 클래스명을 더 명확하게 변경함. - 부족한 테스트 케이스를 추가함. - 메서드 파라미터를 더 명확한 타입으로 개선함. Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * refactor: direction 도메인 추상화 - direction enum들을 MovementDirection으로 추상화 함. - 테스트를 수정함. Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat: Bishop 구현 Co-authored-by: Hyunguk Ryu <hw0603@naver.com> * feat: Rook 구현 Co-authored-현by: Hyunguk Ryu <hw0603@naver.com> * feat: King 구현 Co-authore현d-by: Hyunguk Ryu <hw0603@naver.com> * feat: Queen 구현 Co-authore현d-by: Hyunguk Ryu <hw0603@naver.com> * feat: Knight 구현 * feat: Pawn 구현 Co-authored-현by: Hyunguk Ryu <hw0603@naver.com> * refactor: board 패키지 생성 * refactor: Piece 도메인 클래스에 Type 추가 Co-authored-by: Hyu성nguk Ryu <hw0603@naver.com> * remove: 기존 코드 제거 Co-authored-by: Hyunguk Ryu <가hw0603@naver.com> * refactor: piece 패키지 정리 * refactor: board 도메인 개선 * refactor: view & dto 개선 * docs: 기능 명세서 개선 * refactor: 상태 페턴 적용 * other: 상태에 Controller를 넘기는 케이스 코드 추가 * refactor: import 와일드 카드 제거 * refactor: 기물 이동 메서드명 변경 및 불필요한 메서드 분리 제거 * refactor: Piece 이동 시 Board 클래스를 입력받도록 리펙토링 * refactor: Bishop & Rook의 방향 유효성 검증을 CommonMovementDirection에 위임 * refactor: BoardInitializer 개선 --------- Co-authored-by: Hyunguk Ryu <hw0603@naver.com> Co-authored-by: Hyunguk Ryu <가hw0603@naver.com>

seokmyungham

대짱켈리~ 체스 미션 고생 많았어요 😤
방학 기간동안 스프링 뿌셔서 레벨 2도 달려봅시다 👍👍

seokmyungham · 2024-04-03T02:10:44Z

src/main/java/controller/GameController.java

+    public void movePiece(final Position source, final Position destination) {
+        chessGame.movePiece(source, destination);
+    }


이건 비즈니스 로직이라고 봐도 될 것 같은데
요 로직이 왜 컨트롤러에 있을까용? 사용처가 안보입니다 😄

seokmyungham · 2024-04-03T02:12:23Z

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

+        if (values.length != POSITIONS_VALUES_SIZE) {
+            throw new IllegalArgumentException("잘못된 위치값입니다.");


리뷰어께 3,4 단계 리뷰를 받으면서,
에러메시지는 개발자가 아닌 사람도 알아볼 수 있게 작성하는 것이 좋다. 라는 피드백을 받았는데
켈리도 에러메시지에 신경을 더 써보면 좋을 것 같아 리뷰 남겨용 😄

seokmyungham · 2024-04-03T02:12:41Z

src/main/java/domain/game/GameCommandType.java

+        if (values.length != COMMAND_WITH_POSITIONS_SIZE) {
+            throw new IllegalArgumentException("잘못된 출발지/도착지입니다.");


"move b2 b4" 의 split 사이즈가 3이 아닌 것과,
"잘못된 출발지/도착지입니다." 라는 에러메시지가 매칭이 안되는 것 같아요. 🤔

seokmyungham · 2024-04-03T02:13:25Z

src/main/java/domain/piece/CommonMovementDirection.java

+    public void checkBishopMovableMovement() {
+        List<CommonMovementDirection> bishopMovableMovement = List.of(UP_RIGHT, UP_LEFT, DOWN_RIGHT, DOWN_LEFT);
+
+        if (!bishopMovableMovement.contains(this)) {
+            throw new IllegalArgumentException("비숍이 이동할 수 있는 방향이 아닙니다.");
+        }
+    }


나는 각 기물이 이동할 수 있는 Direction을 가지고 스스로 판단하도록 구현했었는데

켈리는 각 기물이 Direction에 물어봐서 이동할 수 있는 방향인지 설계했네
이렇게 구현했을 때 어떤 점이 좋았고, 어느 부분에서 안좋게 느꼈는지 공유해주면 좋을듯 👍

seokmyungham · 2024-04-03T02:14:22Z

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

+    public Piece(final PieceColor color) {
+        this.color = color;
+    }


Piece 생성자가 자식 클래스에만 사용된다면 접근제한자를 protected로 변경해주는 것도 좋을듯 👍

seokmyungham · 2024-04-03T02:20:35Z

src/main/java/domain/piece/CommonMovementDirection.java

+        if (rowDistance == 0 && columnDistance == 0
+                || (!(Math.abs(rowDistance) == Math.abs(columnDistance))
+                && !(rowDistance == 0 || columnDistance == 0))) {
+            throw new IllegalArgumentException(("상/하/좌/우 혹은 대각선으로 이동할 수 없는 칸입니다."));


저는 개인적으로 중첩 조건문에서 메서드 분리 없이 부정연산자까지 쓰면 가독성이 엄청 안좋아진다 생각합니다 😎

seokmyungham · 2024-04-03T02:24:24Z

src/main/java/domain/game/New.java

+    @Override
+    public void execute(final GameController gameController) {
+        if (gameController.gameStatus() != GameStatus.WAITING) {
+            throw new IllegalArgumentException("지금 실행할 수 있는 명령어가 아닙니다.");
+        }


Command의 매개변수로 컨트롤러가 들어오네요
저는 Service를 매개변수로 넘겨서 서비스 메서드 호출로 기능을 진행하고,
execute가 게임 상태를 반환하여 상태에 따른 판단을 컨트롤러에서 진행했는데
켈리는 저랑 정반대인 것 같아 의도가 궁금합니다 🤔

저는 컨트롤러가 매개변수로 들어오는게 뭔가 조금 어색한 것 같아요

seokmyungham · 2024-04-03T02:27:32Z

src/main/java/database/JdbcConnectionPool.java

+    public static JdbcConnectionPool getInstance() {
+        return INSTANCE;
+    }
+
+    private void initializeConnectionPool() {
+        pool = new ArrayBlockingQueue<>(INITIAL_POOL_SIZE);


오우.. 커넥션 풀까지 구현하다니 👍👍
전 계획만 세우다가 막상 하려니까 막막해서 포기했는데 잘 구현하셨네요

seokmyungham · 2024-04-03T02:32:39Z

src/main/java/database/init.sql

+create table current_player_color
+(
+    id bigint auto_increment primary key,
+    player_color varchar(10) not null


네이밍 때문에 오히려 가독성이 안좋아진 케이스라고 생각합니다.
그냥 간단하게 테이블 명명을 current_turn, turn 으로 해줘도 좋을 것 같아요.
Turn을 의미하는건가...? 라는 인지적 비용이 한 번 더 발생하네용 😄

seokmyungham · 2024-04-03T02:40:43Z

src/main/java/domain/game/Score.java

+    public static Score of(final Board board, final PieceColor pieceColor) {
+        Map<Position, Piece> piecePositions = board.piecePositions();
+        double existTargetColorPiecesScoreSum = sumExistTargetPiecesScore(piecePositions, pieceColor);
+        double scoreValue = existTargetColorPiecesScoreSum - calculateDecreaseScoreForExistSameFilePawns(piecePositions, pieceColor);


저도 점수 계산 로직을 Board에서 분리하고 싶었는데
뭔가 매개변수로 Board를 넘겨줘서 하는게 자연스러운건가...? 확신이 없어서 이렇게 못했었던 것 같아요

켈리가 이렇게 구현한 이유를 공유해주면, 뭔가 확신을 얻을 수 있을 것 같아요
아니면 따로 이렇게 구현했을 때 리뷰어로부터 리뷰를 받은 게 있었는지 궁금합니다! 🤗

reddevilmidzy

레벨 1 수고많았어 켈리!!
방학 푹 쉬고 레벨 2도 화이팅

reddevilmidzy · 2024-04-03T02:13:21Z

.gitmessage.txt

+
+
+Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
+


진짜 사소한 의견이긴 한데 공백도 컨벤션이니 위의 두줄은 없어도 되지 않을까..?

reddevilmidzy · 2024-04-03T02:13:43Z

src/main/java/domain/game/GameCommandType.java

+                .filter(it -> it.value.equalsIgnoreCase(values[COMMAND_INDEX]))
+                .map(it -> it.gameCommand.apply(values))
+                .findAny()
+                .orElseThrow(() -> new IllegalArgumentException("유효하지 않은 명령어입니다!"));


유효하지 않은 명령어입니다! 메시지만 !로 문장을 끝낸 이유가 궁금!

reddevilmidzy · 2024-04-03T02:22:52Z

src/main/java/database/JdbcConnectionPool.java

+    private static final String FAILED_INITIALIZE = "커넥션 풀 초기화에 실패했습니다.";
+    private static final String FAILED_TO_GET_CONNECTION = "커넥션 획득에 실패했습니다.";
+    private static final String FAILED_RELEASE = "커넥션 해제에 실패했습니다.";
+    private static final String FAILED_CLOSE = "커넥션 종료에 실패했습니다.";


다른 도메인에서 발생할 수 있는 예외에선 예외 메시지를 리터럴로 두었지만 커넥션과 관련된 예외 메시지에선 상수로 관리한 이유가 궁금!

reddevilmidzy · 2024-04-03T02:24:20Z

src/main/java/domain/board/BoardInitializer.java

+package domain.board;
+
+
+import domain.piece.Bishop;


진짜 사소하지만 공백 하나 없어도 될듯!

reddevilmidzy · 2024-04-03T02:25:51Z

src/main/java/domain/board/File.java

+
+    public static File of(final String value) {
+        return Arrays.stream(values())
+                .filter(file -> file.name().equals(value.toUpperCase()))


enum의 name 메서드는 어떨때 사용하는 게 좋을까?

reddevilmidzy · 2024-04-03T03:49:43Z

src/main/java/dao/ProductionPieceDao.java

+
+    @Override
+    public List<PieceEntity> findAll() {
+        final String query = "SELECT * FROM piece";


final 이 붙은건 네오의 코드 복붙해오는 과정에서 사용했다고 생각하는데 final을 붙일거면 전부다 필사적으로 붙여야 한다고 생각!

reddevilmidzy · 2024-04-03T03:52:24Z

src/main/java/dao/ProductionPieceDao.java

+            ResultSet resultSet = preparedStatement.executeQuery();
+            return createPieceEntities(resultSet);
+        } catch (SQLException e) {
+            throw new RuntimeException(e);


SQLException을 잡고 RuntimeExcpetion으로 넘기기 보다 IllegalStatementException을 던지는건 어때?

reddevilmidzy · 2024-04-03T03:56:13Z

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

+    private static PieceEntity convertPieceEntity(final Piece piece, final Position position) {
+        return new PieceEntity(piece.pieceType(), piece.pieceColor(), position.file(), position.rank());
+    }


이 메서드가 Board 안에 있는게 맞을지 아니면 PieceEntity 안에 정적 팩터리 메서드를 넣는게 나을지 켈리는 어떻게 생각해?

reddevilmidzy · 2024-04-03T04:04:08Z

src/main/java/domain/piece/CommonMovementDirection.java

+    }
+
+    public void checkBishopMovableMovement() {
+        List<CommonMovementDirection> bishopMovableMovement = List.of(UP_RIGHT, UP_LEFT, DOWN_RIGHT, DOWN_LEFT);


이 List는 상수로 분리해도 되지 않을까?

reddevilmidzy · 2024-04-03T04:06:38Z

src/main/java/domain/piece/Knight.java

+    @Override
+    public PieceType pieceType() {
+        return PIECE_TYPE;
+    }
+
+    @Override
+    public double score() {
+        return pieceType().score();
+    }
+


사실상 getter인데 메서드에 get을 사용하지 않는 이유가 궁금!

kelly6bf and others added 18 commits March 26, 2024 15:37

refactor: ChessGame 상태를 별도 enum으로 분리

7367e53

remove: 불필요한 클래스 제거

30a24f4

refactor: ChessGame 잘못된 로직 정정 및 메서드 수정

227468e

refactor: 상태 Context를 Controller로 변경

1587af3

feat: 체스게임 중간 점수 조회 및 승패 판단 기능 구현

a83fbb9

feat: 현재 차례의 색상을 출력하는 기능 추가

829536b

feat: DB 연결 및 게임 불러오기 구현

4ccb6b0

refactor: 불필요한 메서드 및 메시지 제거

1e00559

docs: DDL 추가

0cc9b84

refactor: import 와일드카드 제거

2a0e6d1

refactor: 메서드명 변경

71b1478

refactor: MysqlCurrentPlayerColorDao 클래스 & 메서드명 변경

5fa072e

refactor: MysqlPieceDao 클래스 * 메서드명 변경 및 매직 리터럴 제거

bd27350

refactor: DB 계정정보 은닉

215a75a

feat: 커넥션풀 종료 로직 구현

2c6e406

refactor: DDL문 업데이트

f408e3e

refactor: 각 진영별 점수 계산 책임을 Score에 위임

7a47024

seokmyungham reviewed Apr 3, 2024

View reviewed changes

reddevilmidzy reviewed Apr 3, 2024

View reviewed changes

		if (values.length != POSITIONS_VALUES_SIZE) {
		throw new IllegalArgumentException("잘못된 위치값입니다.");

		if (values.length != COMMAND_WITH_POSITIONS_SIZE) {
		throw new IllegalArgumentException("잘못된 출발지/도착지입니다.");

Conversation

kelly6bf commented Apr 3, 2024

Uh oh!

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

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants