[체스 - 3, 4 단계] 재즈(함석명) 미션 제출합니다. #18

seokmyungham · 2024-04-01T08:14:34Z

원본 PR 입니당 다들 수고하셨어요 👍
woowacourse#789

* docs : 기능 구현 목록 작성 * feat : Poistion 객체 생성 Co-authored-by: seokmyungham <[email protected]> * feat : Unit 객체 생성 Co-authored-by: seokmyungham <[email protected]> * feat : 각 기물에 대한 객체 생성 및 보드 초기화 Co-authored-by: seokmyungham <[email protected]> * feat : 명령어를 입력 받고 초기화한 보드를 출력하는 기능 구현 Co-authored-by: seokmyungham <[email protected]> * refactor : 전략 인터페이스를 도입하여 체스판 초기화 리팩터링 Co-authored-by: seokmyungham <[email protected]> * fix : 행, 열 방향 수정 Co-authored-by: seokmyungham <[email protected]> * feat(RookMoveStrategy) : 룩 이동 전략에 따른 이동할 수 있는 위치를 계산하는 기능 구현 Co-authored-by: seokmyungham <[email protected]> * feat(BlackPawnMoveStrategy) : 블랙 폰 이동 전략에 따른 이동할 수 있는 위치를 계산하는 기능 구현 Co-authored-by: seokmyungham <[email protected]> * feat(WhitePawnMoveStrategy) : 화이트 폰 이동 전략에 따른 이동할 수 있는 위치를 계산하는 기능 구현 Co-authored-by: seokmyungham <[email protected]> * feat(BishopMoveStrategy) : 비숍 이동 전략에 따른 이동할 수 있는 위치를 계산하는 기능 구현 Co-authored-by: seokmyungham <[email protected]> * feat(QueenMoveStrategy) : 퀸 이동 전략에 따른 이동할 수 있는 위치를 계산하는 기능 구현 Co-authored-by: seokmyungham <[email protected]> * feat(KingMoveStrategy) : 킹 이동 전략에 따른 이동할 수 있는 위치를 계산하는 기능 구현 Co-authored-by: seokmyungham <[email protected]> * feat(KnightMoveStrategy) : 나이트 이동 전략에 따른 이동할 수 있는 위치를 계산하는 기능 구현 Co-authored-by: seokmyungham <[email protected]> * feat(Row) : 검증 추가 및 검증 테스트 Co-authored-by: seokmyungham <[email protected]> * feat(Column) : 검증 추가 및 검증 테스트 Co-authored-by: seokmyungham <[email protected]> * feat(ChessGameController) : 도메인과 뷰를 연결하는 컨트롤러 구현 Co-authored-by: seokmyungham <[email protected]> * refactor(SpecialPieceMoveStrategy) : 폰을 제외한 특별 기물의 중복 로직을 추상화 Co-authored-by: seokmyungham <[email protected]> * refactor(PawnMoveStrategy) : 폰 기물의 중복 로직을 추상화 Co-authored-by: seokmyungham <[email protected]> * refactor : 테스트코드가 추상 인터페이스에 의존하도록 리팩터링 Co-authored-by: seokmyungham <[email protected]> * fix(WhitePawnMoveStrategy) : 화이트 폰 이동 방향 수정 Co-authored-by: seokmyungham <[email protected]> * refactor(ChessGame) : generateMovablePositions 메서드 리팩터링 Co-authored-by: seokmyungham <[email protected]> * style : TODO 제거 Co-authored-by: seokmyungham <[email protected]> * docs : 구현 기능 목록 동기화 Co-authored-by: seokmyungham <[email protected]> * refactor(ChessGame): movePiece 메서드 리팩토링 * refactor(ChessGame): ChessGame 클래스 리팩토링 * fix: 추상 클래스 생성자 접근제한자 protected로 변경 * refactor(Board): initializePawn 메서드 내부 인덴트 1로 수정 * refactor(MoveStrategy): Deque 대신 Queue를 사용하도록 변경 * refactor(Position): 불필요한 toString 제거 * refactor(Row): 가독성 향상을 위해 Enum 이름 변경 * fix: 테스트 클래스 접근 제한자 public 제거 * refactor: 사용자 입력에 대한 관심사를 컨트롤러로 부터 분리 * refactor(OutputView): 체스 판을 의미하는 문자열 상수화 * fix(Command): 클래스 이름 오타 수정 * refactor(Board): 보드 초기화 로직을 별도의 factory 클래스로 분리 * fix(MoveRequestDto): 멤버 변수 private final 키워드 누락 수정 * refactor: 멤버 변수 forwardDirection을 사용하지 않도록 리팩토링 * refactor(ChessGame): 유효한 위치들을 모으는 로직의 중복을 제거 * refactor(PositionsFilter): 이동 가능한 로직만 모으는 로직을 PositionFilter 객체로 분리 * test(BoardTest): 보드 초기화 테스트 추가 * fix(chessGame): 필드 Board 접근 제한자 private으로 수정 * feat(chessGame): 체스 게임에 턴 개념 기능 추가 --------- Co-authored-by: jhon3242 <[email protected]>

Libienz

하이 재즈 😎
미션 진행한 코드 볼륨이 어마어마하네

확장되는 요구사항에 잘 버틸 수 있는 추상화를 많이 고민한 것 같아.
사실 나는 이만큼 깊게 고민해보지 못해서 코드리뷰 하면서 반성을 많이하게 되었네 ㅋㅋ

이번에는 가독성 측면의 리뷰가 특히 많고 주요한 로직에 대한 코멘트는 조금 적은 것 같아
내가 실력이 떨어져서리 부드럽게 이해해줘 🙇🏻‍♂️

얘기하고 싶은 추가적인 부분은 스터디 때 진행해보자구

미션 고생했으 👊

Libienz · 2024-04-02T12:56:39Z

src/main/java/chess/Application.java

+        RoomRepository roomRepository = new RoomRepositoryImpl();
+        BoardRepository boardRepository = new BoardRepositoryImpl();
+
+        GameService gameService = new GameService(roomRepository, boardRepository);
+        BoardService boardService = new BoardService(roomRepository, boardRepository);
+        ChessGameController chessGameController = new ChessGameController(gameService, boardService);
+        chessGameController.run();


추상화가 잘 되어 있고 의존관계가 복잡해지는 만큼 의존성 주입자 Config 클래스를 운용해보는 것도 나쁘지 않을 듯!
과할수도 있다고 생각할 수도 있으려나? 근데 나는 현재 코드 볼륨에 적용하기 괜찮을 것 같아 👍

오 나도 같은 생각을 하긴 했었는데, 리비도 그렇게 느끼는구나

아마 2단계 들어가면 이 주제로 대화해볼 기회가 있지 않을까 싶어👍

Libienz · 2024-04-02T13:11:49Z

src/main/java/chess/repository/BoardRepositoryImpl.java

+    private void processQuery(String query, QueryProcessor queryProcessor) {
+        try (final Connection connection = dbConnection.getConnection()) {
+            final PreparedStatement preparedStatement = connection.prepareStatement(query);
+            queryProcessor.process(preparedStatement);
+        } catch (SQLException e) {
+            throw new RuntimeException(e);
+        }
+    }
+}


중복되는 구문을 따로 빼서 추상화 시켜줬네
덕분에 다른 메서드들은 정말로 잘 보여야 하는 조회, 저장 등의 로직이 잘 보이게 된 것 같아 👍

나도 이번에 처음알았는데 JDBC를 이용하는 과정에서 이렇게 중복되는 과정들을 미리 정의한 라이브러리를 JdbcTemplate이라고 하더라.

망쵸가 JdbcTemplate과 비슷한 형태로 구현한 것 같은데 재즈도 한 번 염탐해보실? ㅋㅋ

오호 ㅋㅋㅋ 망쵸 코드 염탐하러 가야겠군😎

Libienz · 2024-04-02T13:15:10Z

src/main/java/chess/service/dto/ChessGameResult.java

+package chess.service.dto;
+


dto 패키지의 위치를 어디에 두는지 재즈만의 기준이 있을까

나는 dto 패키지를 다른 레이어들과 동등한 위치에 두었는데 재즈는 service안에 두었네

이와 관련해서 나는 왜 거기다 두었냐고 코멘트 받았는데 나만의 답을 잘 못찾겠더라고 재즈만의 팁 공유해주면 고마울 듯 👍

나는 일단 의존 흐름이 반대로 되지 않게끔 (위치를 잘못둬서 양방향 참조가 되지 않도록)하는게 일단 가장 중요한 것 같고, 해당 dto가 생성되는 패키지 위치에 있으면 괜찮다고 생각하고 있어

Libienz · 2024-04-02T13:18:50Z

src/main/java/chess/controller/ChessGameController.java

+    private Room createRoom() {
+        try {
+            String input = InputView.readRoomName();
+            return gameService.loadRoom(input);
+        } catch (RuntimeException error) {
+            OutputView.printError(error);
+            return createRoom();
+        }
+    }


사실 코드를 처음보는 입장에서 가장 먼저 하게 되는 일은 메서드 시그니처를 읽고 해당 메서드가 어떤 일을 하는지 예측하는 것인 것 같아. (적어도 나는 ㅋㅋ)

현재 createRoom()이라는 메서드 만 보았을 때는 Room을 하나 만들겠구나 예상 되는 데 안의 동작을 보면 서비스에서 게임을 로드하는 것으로 보여

좋은 시그니처가 가독성 좋은 코드로 이어진다고 믿는 사람으로써 재즈도 이러한 부분을 무게감 있게 생각해보면 좋을 것 같음! 👍

헉 맞아.. 내 pr 가서 읽어보면 아마 웃음이 나올 수도..

레디 리뷰에 시그니처 관련 리뷰를 잔뜩 달아놓고 정작 내가 더 문제가 심각한듯,, 반성중이야..

Libienz · 2024-04-02T13:21:59Z

src/main/java/chess/controller/ChessGameController.java

+    private State executeCommand(GameService gameService, BoardService boardService, State state, Long roomId) {
+        try {
+            Command command = CommandRouter.findCommendByInput(InputView.readCommend());
+            return command.execute(gameService, boardService, roomId);
+        } catch (RuntimeException error) {
+            OutputView.printError(error);
+            return state;
+        }
+    }


요청을 처리하기 위해 복수의 서비스 클래스가 필요하네.
하나의 요청을 처리하기 위해 복수의 서비스 클래스가 필요한 것은 분리가 필요함을 이야기하고 있는 것은 아닐까?

요청을 수행하는 것을 하나의 트랜잭션으로 보고 해당 트랜잭션 내부에서 여러 서비스 클래스가 필요하다면 응용 서비스로직이 많이 어려워질 수 있다는 생각이야.

그런데 내가 보기에도 쉽지 않은 개선인 것 같긴 하네 🫠

리뷰어께도 추상화가 적절히 이루어졌는지 의문이 든다는 피드백을 받았는데, 확실히 다시 고민해봐야할 부분인 것 같아. 정확하게 잘 짚어준듯👍

Libienz · 2024-04-02T13:22:58Z

src/main/java/chess/controller/command/Move.java

+    private static final Pattern POSITION_REGEX = Pattern.compile(""
+            + "move\\s+([a-h][1-8])\\s+([a-h][1-8])");


정규식... 고수인가?

정규식 나중에 알려주라 👊

조금 복잡한 정규식은 지쌤..(gpt) 도움을 많이 받긴해 😂

이걸 다 외우는건 효율적으로도 말이 안된다고 생각함 하하

Libienz · 2024-04-02T13:23:47Z

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

+        for (Column column : Column.values()) {
+            Position position = new Position(row, column);
+            if (color == Color.WHITE) {
+                map.put(position, new Piece(PieceType.WHITE_PAWN, color));
+            }
+            if (color == Color.BLACK) {
+                map.put(position, new Piece(PieceType.BLACK_PAWN, color));
+            }
+        }


뎁스가 깊어진다고 해도 이러한 형태로 풀어쓰는 것이 개인적으로 가독성 좋다고 생각 👍

Libienz · 2024-04-02T13:30:47Z

src/main/java/chess/domain/game/PositionsFilter.java

+    private boolean isEnemySpace(Direction direction, Piece piece, Position currentPosition, Long roomId) {
+        return currentPosition != null
+                && piece.isPawnAttackPossible(direction)
+                && boardRepository.existsPieceByPosition(currentPosition, roomId)
+                && piece.isEnemy(boardRepository.findPieceByPosition(currentPosition, roomId));
+    }


CurrentPosition이 널이 아닌지 검증하는 부분이 어떤 의도인지 잘 와닿지 않아
널이 안좋다고 말하는 게 아니라 어떤 경우를 조건식에 첨부하고 있는지 그 맥락이 잘 와닿지 않는다는 얘기 👀
나머지 조건식들은 메서드로 추상화 되어 있는 만큼
currentPosition != null도 추상화 준위를 맞추어 주면 가독성 좋은 코드가 나올 것 같아
null을 안쓸 수 있으면 더 좋으려나?

Libienz · 2024-04-02T13:32:16Z

src/main/java/chess/domain/game/Room.java

+public class Room {
+
+    private final Long id;
+    private final RoomName roomName;
+
+    public Room(Long id, String name) {
+        this.id = id;
+        this.roomName = new RoomName(name);
+    }
+
+    public String getName() {
+        return roomName.getValue();
+    }
+
+    public Long getId() {
+        return id;
+    }
+}


룸에 응집되고 있는 로직이 아직까진 보이지 않네 👀
재즈가 만약 플레이어 등의 개념을 추가해서 미션을 더 완성할 예정이라면 한번 알려줘 ㅋㅋ 구경하게 👊

Libienz · 2024-04-02T13:35:09Z

src/test/java/chess/repository/fake/FakeBoardRepository.java

+
+public class FakeBoardRepository implements BoardRepository {
+


Fake 👍
두 가지 방법 중에서 가짜 객체 방법을 사용했네

Fake 객체를 이용하는 방법은 인터페이스가 선언 되어 있어야 가능한 방법이더라
개인적으로는 하나의 구현체뿐인 인터페이스를 테스트를 위해서 생성해야 함이 불필요한 확장 같아서 Mock 방법을 나는 사용했어

두 방법 중 어떤 것이 더 나은지는 아직까지도 활발히 토론되고 있다고 하더라
개인적인 생각 공유해봤음 👍

seokmyungham and others added 18 commits March 26, 2024 01:10

refactor(Board): equals & hashcode 를 제거하고 보드 초기화 테스트 리팩토링

44befb2

feat(Board): 각 팀의 점수를 계산하는 기능 추가

67070ed

refactor(BoardFactory): 테스트를 위해 보드 초기화 로직이 map을 반환하도록 리팩토링

42cfa33

feat: 게임 점수를 계산하고 게임 결과를 출력하는 기능 추가

3cf2780

refactor(ChessGameController): 상태 Command를 사용하도록 리팩토링

2cb887d

refactor(Row): enum의 인덱스를 역순에서 순서대로 변경

5c47f78

feat(BoardRepository): 기존 Board를 데이터베이스에 접근하는 BoardRepository로 변경

972d9d8

feat(Room): 체스 게임 방 도메인 Room 생성

19d005c

feat: 체스 상황 저장 및 체스 게임방 생성 기능 추가

e495e9e

refactor: repository를 생성자로 주입하도록 변경

5062766

refactor: fake repository 패키지 구조 변경

0777b71

feat: fake repository 추가

b4ce577

test: 서비스 클래스 테스트 추가

ba433da

refactor: fake 객체를 주입하도록 리팩토링

d144ce7

docs: 구현 기능 목록 업데이트

3d97e2d

refactor: repository 리팩토링

c9d5677

refactor(OutputView): 코드 포맷팅

6ba1c2d

Libienz reviewed Apr 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[체스 - 3, 4 단계] 재즈(함석명) 미션 제출합니다. #18

[체스 - 3, 4 단계] 재즈(함석명) 미션 제출합니다. #18

seokmyungham commented Apr 1, 2024

Libienz left a comment

Libienz Apr 2, 2024

seokmyungham Apr 2, 2024

Libienz Apr 2, 2024

seokmyungham Apr 2, 2024

Libienz Apr 2, 2024

seokmyungham Apr 2, 2024 •

edited

Loading

Libienz Apr 2, 2024

seokmyungham Apr 2, 2024

Libienz Apr 2, 2024

seokmyungham Apr 2, 2024

Libienz Apr 2, 2024

seokmyungham Apr 2, 2024

Libienz Apr 2, 2024

Libienz Apr 2, 2024

Libienz Apr 2, 2024

Libienz Apr 2, 2024

		private static final Pattern POSITION_REGEX = Pattern.compile(""
		+ "move\\s+([a-h][1-8])\\s+([a-h][1-8])");


		public class FakeBoardRepository implements BoardRepository {

		package chess.service.dto;

[체스 - 3, 4 단계] 재즈(함석명) 미션 제출합니다. #18

Are you sure you want to change the base?

[체스 - 3, 4 단계] 재즈(함석명) 미션 제출합니다. #18

Conversation

seokmyungham commented Apr 1, 2024

Libienz 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

seokmyungham Apr 2, 2024 • edited Loading

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

seokmyungham Apr 2, 2024 •

edited

Loading