Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3, 4단계 - 체스] 제이(이재윤) 미션 제출합니다. #530

Merged
merged 45 commits into from Mar 23, 2023

Conversation

sosow0212
Copy link

@sosow0212 sosow0212 commented Mar 21, 2023

카프카, 안녕하세요! 주말은 잘 보내셨나요?

이번 3, 4단계 미션은 어려웠지만, 정말 배운 것도 많았고 재밌게 진행 했습니다.
특히 "JPA가 정말 편한 기술이었구나"를 많이 느꼈습니다..😭

미션의 단계가 높아질 수록, 컨트롤러가 많이 복잡해졌습니다.
가독성과 확장성을 고려했을 때 컨트롤러 부분에 커맨드 패턴을 적용하고 싶었으나, 잘 모르고 쓰는 느낌이 강해서 일단은 기존 방식대로 진행했습니다.

이번 미션에서 DAO를 사용할 때 컨트롤러와 연결되는 부분이 많아지면서, 코드가 복잡해지고 스파게티 코드가 된 것 같기도 합니다 ㅠㅠ
컨트롤러의 제 역할을 제대로 수행하지 못하는 느낌이었습니다.

어떻게 할까 많은 고민을 하다가 "최대한 컨트롤러의 가독성을 살려보자"라는 생각으로 최대한 깔끔하게 코드를 다듬고 제출합니다 !

그리고 현재 컨트롤러 안에 상수로 있는 BOARD_ID는 추후에 추가 미션을 구현할 때 바꿀 예정입니다.
현재는 게임을 불러오고 저장하는 역할만 하면 되기 때문에, BOARD_ID로 저장 ID값을 고정시켜놨습니다.

바쁘실텐데 리뷰 해주셔서 정말 감사합니다 :)

--> Pawn에서 오버라이딩을 통해 Pawn에서만 점수를 바꿀 수 있도록 변경
Copy link

@include42 include42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 제이, 리뷰어 카프카입니다.

코드 잘 보았습니다. 이번에 dao 적용과 점수 계산까지 어려움이 컸을텐데, 빠르게 작업해주셨네요 💯
다만 현재 DB 테이블의 구조의 개선이 필요하고, 이외에 몇몇 검토해야 할 내용이 있어 해당 내용들 우선으로 리뷰 진행하였습니다. 참고해서 작업 진행 후 다시 리뷰 요청 부탁드립니다.
진행하면서 궁금하거나 막히는 부분이 있다면 편하게 코멘트나 dm 남겨주세요.

작업하시느라 고생 많으셨습니다 👍

README.md Outdated
- DB 계획도
- Room (room_id, game_id, user1, user2) --> 추가 미션 예정
- Game (game_id, board_id) --> 추가 미션 예정
- board (board_id, position(String), piece(String), isLowerTeamTurn) --> isLowerTeamTurn은 추가 미션 때 Game 테이블로 옮길 예정입니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재는 BOARD_ID를 통해 단일 저장만 지원하도록 로직 작성해주셨고,
이후 추가 미션으로 나머지 테이블에 대해 구현을 수행하는 것으로 확인했습니다. 👍

다만 테이블의 구조 변경이 필요해 보이므로, 이를 수행한 뒤 이 부분도 변경해주시길 바랍니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@include42

카프카, 안녕하세요? 추가로 질문이 있습니다 !

개인적으로 DB로 데이터를 넣고, 받아오는 과정은 매우 중요하다고 생각합니다.
(만약 협업을 가정한다고 했을 때, 동료 개발자가 실수로 도메인 메서드를 DAO와 관련된 부분에서 실행하면 값이 바뀔 수도 있기 때문입니다!)

따라서 Controller <-> Service <-> DAO 에서 DTO를 적용해보고 싶은데,
이런 경우 DTO에서 BoardUtil에서 하는 역할을 불러서 수행을 시키고 보내는게 맞을까요?

카프카의 생각은 어떤가요?? DTO의 책임은 어느정도까지인지 감이 잡히질 않아서 질문 남깁니다 :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 제이!
말씀해주신 이유와 같은 이유로, 저도 DTO가 각 레이어간 통신에 필요하다고 생각합니다.

  • 우선 View <-> Controller <-> Service 간에 적용해보면 좋을 것으로 보입니다.

그리고 BoardUtil에서 하는 역할을 불러서 수행시킨다는 것은, Board를 가공하는 과정을 DTO가 책임져도 된다는 의미의 질문일까요?

저는 DTO의 책임은 값을 저장하고, 저장된 값을 전달한다 에 있다고 생각합니다. 그렇기 때문에 이상적인 DTO라면 생성자와 getter만 있어도 충분하겠지요.
다만 위 원칙대로라면 별도의 Converter 클래스가 Domain <-> DTO를 수행해야 하는데, 이러면서 클래스가 너무 많아져 관리가 어려울 경우 DTO에 정적 팩토리 메소드로 도메인을 인자로 받아 생성하는 정도는 허용하기도 합니다.
그 외의 연산 로직은 DTO에 필요할 것 같지는 않네요. 예를 들어 BoardUtil의 메소드들이 DTO에 있는 것은, 책임을 넘어서는 구조라고 생각이 됩니다. (정적 팩토리 메소드에서 호출해주는 정도는 가능하겠지만, 일단 DTO가 너무 복잡해지는 것은 좋지 않다고 생각합니다)

현재의 상황에서 제일 단순한 해결법은, 서비스 내에서 BoardUtil을 통해 필요한 값을 만들어주고 해당 값을 DTO에 넣어서 컨트롤러에 전달해주는 방법일 것 같습니다. 그러면 Controller는 해당 DTO를 다시 View에 넘기고요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@include42
그렇군요! 감사합니다 :)
말씀해주신 내용을 바탕으로 추가로 커밋하였습니다!


import java.util.Map;

public interface BoardDao {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 BoardDao와 BoardDaoImpl을 나눈 이유가 있을까요?
실제로 개발 현장에서 많이 쓰는 패턴이기는 하나, 사용에 명확한 이유가 없다면 하나로 합쳐도 괜찮아 보입니다.

import java.util.Map;
import java.util.Map.Entry;

public class BoardDaoImpl implements BoardDao {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로컬에 DB를 올려서 테스트를 수행해 보았는데, 현재의 테이블 구조에 문제가 있어 변경이 필요해 보입니다.
현재 테이블에 pk(primary key) 설정은 수행하지 않으신거죠?
확인해보니 저장할때 아래와 같이 저장을 진행하더군요.

  • id, 포지션, 기물, 턴 순으로 저장
  • 이를 전체 기물 수만큼 저장하게 됨

image

이러한 구조는 적절하지 않습니다. 제가 생각하는 그 이유는 다음과 같습니다.

  • id는 고유값으로 설정하는 것이 좋습니다. db에 요청을 보낼 때, 아이디를 통해 각각의 값을 구분하기 때문입니다. 현재 구조는 비유하자면, 대한민국 국민을 나이별로 묶어 같은 주민번호를 사용하도록 한 것과 같습니다. 이 경우 유사한 데이터간의 구분이 어려워집니다.
  • board라는 테이블의 구조를 생각할 때, 하나의 데이터는 하나의 보드의 정보를 저장해야 합니다. 현재 구조는 하나의 보드 정보 저장을 위해 board 64개를 db에 삽입하게 됩니다.
  • 도중에 에러가 나 연산이 중단된다면 문제가 발생할 수 있습니다. (원자성에 대한 문제인데, 일단 그럴 수 있다라고만 알아두셔도 지금은 충분합니다)

아직 database에 대해 학습하신 내용이 적어서, 충분히 할 수 있는 실수입니다.
그러나 해당 설계로 프로젝트 진행할 경우 이후의 추가 미션 진행에도 어려움이 있을 것으로 보입니다.
이번에는 테이블의 수정만 진행해 주시고, 추후 레벨2 진행하는 과정에서 db 학습을 이어나가셔도 충분할 것으로 생각됩니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 아래와 같은 개선이 필요하다고 생각해요.

  • 보드당 id는 1개만 사용(하나의 보드 정보 저장 시 하나의 값으로 저장)
  • position과 piece 대신, 문자열 1개로 압축(길이가 64인 문자열이 되겠지요)한 필드 정보를 저장
  • Board <-> String을 수행할 util 클래스를 구현하고, 변환 책임은 그쪽으로 옮겨둠

물론 더 복잡한 구조로 구현할수도 있겠으나, 이 경우 오버엔지니어링이 될 위험이 있습니다.
또한 이번 미션은 DB를 프로젝트에 적용해보는 것이 목표이므로, 최대한 간단하게 테이블 정책을 잡는 것이 학습 목표에 맞다고 생각하여 조언 드렸습니다.
만약 위의 방향보다 더 좋으면서 id의 고유성을 보장하는 방법이 있다면, 해당 방법을 바탕으로 리팩토링 진행하셔도 괜찮습니다.

해당 내용 수행 후, 테이블 생성시 사용된 DDL을 코멘트로 남겨주시면 참고해서 리뷰 진행하겠습니다.

Copy link
Author

@sosow0212 sosow0212 Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

문자열 1개로 압출하는 방법이 있었군요..! 왜 이 생각을 못했을까요 ㅠㅠ

저는 기존에는 64개(Position, Piece)를 모두 저장해야한다는 생각 때문에, 기본키를 누가 가져야하는지에 대해서 고민이 많았습니다.

board_id가 가진다면 기본키는 중복이 안되기에 position, piece와 함께 저장이 모두 안되기 때문에 기본키를 없애거나, board_id와 position을 기본키로 설정하는 방식으로 진행했는데, 좋은 방법 하나 배워갑니다!

감사합니다 :)

테이블 생성 DDL은 다음과 같습니다.

CREATE TABLE `chess`.`board` (
  `board_id` INT NOT NULL,
  `position` VARCHAR(300) NOT NULL,
  `piece` VARCHAR(300) NOT NULL,
  `isLowerTeamTurn` TINYINT NOT NULL,
  PRIMARY KEY (`board_id`));

@@ -0,0 +1,18 @@
package chess.exception;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동일 패키지의 다른 클래스들은 xxxMessage 라는 이름을 쓰던데, CommandException 클래스만 네이밍이 다르네요.
의도된 부분이 아니라면 수정이 필요해 보입니다.

Copy link

@include42 include42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 제이, 리뷰어 카프카입니다.

저번에 드린 피드백을 빠르게 반영해 주셨네요! 피드백드린 내용 이상으로 좋은 결과물을 이끌어주고 계셔서, 저도 리뷰하며 즐거움이 크네요. 💯
전반적인 구조는 충분하지만, DTO 관련된 부분에서 수정이 필요해 보입니다. 이 부분의 수정이 잘 마무리되면, 충분히 approve할 수 있겠다는 생각이 듭니다.

레벨1의 막바지라 많이 바쁘실텐데, 고생 많으셨습니다! 👍



### DB 테이블 DDL (추가 미션 전)
Board

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DDL 여기에도 추가해 주셨네요! 좋습니다.
이번에 PK 설정 추가한 것 외에는, 현재 미션에서 더 학습해야 할 내용은 없습니다.
작업하시느라 고생 많으셨습니다 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 만약 저장된 게임이 없거나 (추후 room을 추가할 경우) id를 잘못 입력한 경우에 대응이 되는지 확인해 보았는데요,
BoardService에서 이에 대해 대응해주고 있네요. 아주 훌륭합니다. 😄

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다만 고민이 되는 지점이 있는데요, 가만히 생각해보니 position과 piece를 따로 저장할 필요 없이 piece를 순서대로 저장해주면 util이 충분히 읽어올 수 있지 않을까요?
현재의 따로 저장하는 구조가 가지는 이점과 순서대로 하나의 값으로 저장하는 구조의 이점을 저울질해서 결정해봐야 겠네요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 지적 감사합니다 :)

테이블(board_id, "Position + Piece")로 관리하라는 뜻으로 이해했습니다!

한참을 고민 했는데, 좌표와 기물을 따로 저장하는 구조를 선택했습니다.

사실 아직 데이터베이스 지식이 많지 않아서 제 생각이 맞는지는 잘 모르겠지만, Position, Piece를 따로 관리 할 때 추가 요구사항이 왔을 때 유연하게 대처할 수 있다고 생각했습니다. (예를 들면 좌표와 기물을 이용한 기능 추가 요청이 들어왔을 때..?)

이런 추가 요구 사항이 온다고 가정 했을 땐, 좌표와 기물을 나누고 좌표 혹은 기물을 파싱해주는 것보다는 각각 필요한 것만 불러오고 파싱해주는게 훨 유연한 것 같습니다!

이에 대해 카프카는 어떻게 생각하시나요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제이의 의견에 동의합니다! 물론 컬럼이 하나 더 생기긴 하지만, 위에서 제시한 방법보다 확장에 더 유연한 구조가 맞지요.

  • 같이 저장할 경우 보드의 크기와 순서 등 현재 명세에 데이터가 지나치게 의존하게 됨
  • 추후 명세 변경시, 기존 데이터를 마이그레이션하는(새로운 명세에 맞게 수정해주는) 작업이 필요해짐
  • 그에 비해 좌표/기물 따로 저장시, 대응에 좀 더 유연할 수 있음.

아무래도 이번 프로젝트가 DB쪽을 초보적으로 다뤄보는 프로젝트라서, 저도 최대한 간단한 방향을 가이드하게 되는데
제이가 보다 고민해서 좋은 방향으로 의견 내줘서 좋습니다. 💯

ChessGame chessGame = boardService.findById(BOARD_ID).getChessGame();
outputView.printBoard(BoardResultDto.toDto(chessGame));
return chessGame;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

나중에 room 을 구현하게 되면 여기에서는 room에 포함되는 boardId로만 대체해줘도 로딩에 문제가 없겠네요.
로드하는 부분의 구조를 잘 짜주셨습니다.

Comment on lines 83 to 88
private boolean isGameEnd(final ChessGame chessGame, final Command command) {
if (isCommandStatus(chessGame, command) || command.isGameStop()) {
return true;
}

return false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분은 별도의 로직 처리가 없다면 (isCommandStatus(chessGame, command) || command.isGameStop())을 바로 반환해줘도 괜찮겠네요!
물론 if문으로 쓸 경우 확장성이 더 좋고 의미가 명확해지므로, 제이가 생각하는 더 좋은 방향을 결정해주세요 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 전 리뷰어분께서도 말씀해주셨는데, 습관이 돼서 자꾸 인지를 못하네요 ㅠㅠ

굳이 if문을 쓰지 않아도 이해하는데 문제가 없기 때문에 저도 카프카와 동의합니다!

꼼꼼하게 봐주셔서 감사합니다🙇🏻‍♂️


public boolean isUpperTeamWin() {
Piece kingOfWinner = board.values().stream()
.filter(piece -> piece.isKing())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter(piece -> piece.isKing())filter(Piece::isKing) 으로 대체할 수 있습니다.
IntelliJ에서 이러한 방법을 적극적으로 권하는데, 혹시 코드에 노란 줄이 가면 그에 대해 확인해보면 좋습니다 😄


public class BoardResultDto {

private final Map<Position, Piece> board;
Copy link

@include42 include42 Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map<Position, Piece>를 넣어줘도 되겠지만, BoardUtil에서 제공하는 compressBoard를 수행한 값을 넣어서 보내도 되지 않을까요?
물론 View 레이어에서 어느정도 가공의 책임을 맡는다는 것은 좋으나, 해당 데이터는 서버에서 가공해서 보내더라도 이상하지 않은 데이터라는 생각이 들어서요. 또한 Position과 Piece라는 클래스가 View 레이어에 노출되는 것이 좋은 구조일지에 대한 고민도 있고요.

그리고 저의 경우, DTO 클래스를 작성할 때에는 다른 DTO를 포장하는 컬렉션과 String, Long, Boolean...과 같은 클래스만 사용하는 편입니다. 도메인 클래스를 직접 보내면서 생기는 문제를 방지하기 위해서 DTO를 사용하는 것이니까요.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

즉 설령 현재 구조를 유지하더라도, Map<Position, Piece>를 보내기보다는, 아래처럼 클래스를 만드는 것이 더 좋다는 의미입니다.
다만 아래처럼 구현하면 구조가 너무 복잡해지므로, 적절한 선에서 타협한 것이 String으로 compress해서 보내자는 의견이었고요.

public class BoardResultDto {

    private final List<PieceDto> pieces;

    private BoardResultDto(final List<PieceDto> pieces) {
        this.pieces = pieces;
    }

    public static BoardResultDto toDto(final ChessGame chessGame) {
        (...)
    }
    (getter)
}

public class PieceDto {
    private final int row;
    private final int col;
    private final String symbol;
    
    (getter)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다만 제가 지적한 부분에 대해 아쉬워하실 필요는 없습니다.
View 레이어가 프론트엔드로 붙는 이후 미션에서는, 어차피 객체를 보낼 수 없기 때문에 자연스럽게 위에서 말한 것처럼 DTO를 구성하게 될 것입니다. 단지 레벨1의 미션이 콘솔 기반이라는 독특한 형태이기 때문에 할 수 있는 실수라고 생각합니다.
이번에 학습한 내용에 더 참고가 되도록, tecoble에 올라온 DTO글을 첨부합니다.

그리고 이건 민망하지만...제가 취준시절에 블로그에 올린, 지금의 고민 과정에서 참고가 될만한 글입니다.
워낙 미흡한 글이라, 혼자 보시고 소문내지는 말아주세요 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

새벽에 글 재밌게 잘 읽었습니다 :)

카프카가 말씀해주신 것처럼 Dto는 값을 변경하지 못하게 사용해야 하는데 제가 만든 구조는 단지 한번 포장만 한 느낌이었군요!

이렇게 포장만 했을 때 put 혹은 add() 등등 같은 걸로 실제 값이 변경 될 수 있다고 이해했는데 이게 맞을까요?!
따라서 이런 방법을 방지하기 위해서 Integer, Double, Boolean 같은 타입 혹은 Dto를 감싼 값들을 Dto에서 쓰는 것이고요.

카프카가 말씀해주신 내용을 바탕으로 Dto에는 단지 값을 감추는 용도가 아닌 정말 외부에서 알더라도 원본은 수정이 불가능하게 사용해야한다는 것을 배웠습니다.
(혹시 틀리면 말씀해주세요..!)

늦은 시간에 좋은 리뷰 남겨주셔서 정말 감사합니다🙇🏻‍♂️

Copy link

@include42 include42 Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 맞습니다! 물론 상황에 따라 DTO도 setter 등을 사용하기도 하지만, 최소한 그 조작으로 인해 다른 클래스에 영향이 가지 않도록 해야 하는 것이죠.

그리고 더 큰 이유도 하나 있는데, DTO는 레이어 간 통신을 할 때 어떤 값을 줄지에 대한 규약이기도 합니다.
만약 도메인 클래스를 통신에 사용하면, 도메인 클래스 수정 시 레이어 간 주고받는 값이 갑자기 변경되겠지요?
반대로 뷰에서 특정한 값을 요구하는 상황에서, 그 값을 도메인 클래스에 추가해줘야 한다면 그것도 부담이 크고요.
하지만 DTO를 사용한다면, 이러한 변화 시에도 Domain<->DTO를 수행하는 로직만 변경해주면 되므로 간편함이 큽니다.

예: 쇼핑몰 사이트에서 상품 정보를 보여준다.

  • 도메인 클래스: Product, Discount 두개가 있다고 가정
  • 컨트롤러는 뷰가 요청할 경우 상품 정보와 할인가 정보를 보내준다.
  • 뷰에서 보내주는 상품 정보에 할인가를 묶어서 하나로 보내달라고 요청한다. Discount를 따로 받아서 계산하려니 불편하다고 한다.
    • Product 클래스를 바로 보내주고 있던 경우: 뷰 레이어의 변화때문에 Product 내에 갑자기 할인가 필드를 추가해야 됨.
    • DTO를 보내줄 경우: Product -> DTO를 수행하는 과정에서 Discount 정책을 참고해서 할인가를 DTO에 넣어 보내주면 됨
  • Product 클래스에 새로운 판매자 필드가 생겼다.
    • Product 클래스를 바로 보내주고 있던 경우: 갑자기 뷰에 판매자 정보가 노출된다.
    • DTO를 보내줄 경우: 영향이 없다.


public class ChessGameResponseDto {

private final ChessGame chessGame;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분도 마찬가지입니다. DTO 안에 ChessGame 클래스를 담아 보낸다면, View 레이어에서 해당 클래스를 get해서 조작할 수 있지 않을까요? 설령 생성 과정에서 새로 만든 ChessGame이더라도, 불필요하게 다른 레이어에 해당 클래스를 노출할 필요는 없어보입니다. DTO 클래스는 값을 저장해서 보내는 것이 목적인데, 값 저장을 위해 체스게임을 통째로 만들 필요는 없으니까요.
ChessGameResponseDto는 앞서 만든 BoardResultDto와 boolean isLowerTeamTurn 두 개의 필드를 가지면 되겠다는 생각이 듭니다. 그렇게 구성하면 구현에도 어려움이 없겠다는 생각이 듭니다. 👍


import chess.domain.game.ChessGame;

public class GameScoreResultDto {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GameScoreResultDto 의 형태가 제가 위 리뷰에서 말씀드린 순수한 DTO에 가깝다고 생각합니다.
이부분은 잘 작성해주셔서, 따로 코멘트할 부분이 없네요. 😄

import java.util.List;
import java.util.Map;

public class BoardService {
Copy link

@include42 include42 Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저번 리뷰때 DB쪽을 다루느라 코멘트를 잘 못했는데, BoardService를 만들어주신 부분 매우 좋습니다.
Service 레이어는 DB 등과의 통신을 수행하면서 연산 순서를 보장하고, 도메인을 통한 비즈니스 로직의 처리를 수행하면서 Controller가 외부 요청에 대한 중계만 수행할 수 있도록 해줍니다. 본 클래스 역시, 그 역할에 충실하다고 생각합니다.

Copy link
Author

@sosow0212 sosow0212 Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

카프카 덕분에 마지막 미션에서 많은 것을 배웠고, 고민도 하고 적용도 하면서 정말 재밌게 마지막 미션을 진행할 수 있었습니다!

다시 한 번 감사 인사 드립니다 :)

@@ -15,10 +16,10 @@ public void printStartMessage() {
+ "> 게임 이동 : move source위치 target위치 - 예. move b2 b3");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아, 그리고 여기에 status 명령어에 대한 설명도 추가해야겠네요 😄

Copy link

@include42 include42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 제이, 리뷰어 카프카입니다.
이번에도 빠르게 피드백 반영해 주셨군요! DTO 관련으로 코멘트 드린 부분 잘 수정해 주셨습니다. 👍
수정이 필요하거나 고민해볼 부분에 코멘트 남겼으나, 미션에서 요구되는 완성도에 이제 충분히 도달했다고 생각됩니다.
이번 미션은 여기서 approve 할게요!

그동안 미션 진행하시면서 고생 많으셨습니다.
2주동안 논의하면서 제이가 많이 성장하고 고민하는 모습을 볼 수 있었습니다.
레벨 2에서도 이러한 자세를 이어나가시도록 응원하겠습니다. 😄

@@ -59,7 +63,7 @@ private void playChess(ChessGame chessGame) {
chessGame = checkCreateNewGame(chessGame, command);

checkMovePiece(chessGame, command);
outputView.printBoard(BoardResultDto.toDto(chessGame));
outputView.printBoard(BoardResultDto.toDto(new Board(chessGame.getBoard())).getPieces());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성하는 부분에서 불필요하게 객체를 새로 생성할 필요는 없어보입니다.
이부분을 개선할 수 있는 방법을 고민해보면 좋겠네요.


public class BoardResultDto {

private final Map<Position, Piece> board;
private final List<PieceDto> pieces;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO 작업하신 코드 봤는데, 구조를 잘 짜주셨네요!
DTO들이 toDto 메소드를 통해 생성되도록 구조를 짰는데, 이 부분도 적절하다고 생각됩니다.💯

@@ -30,6 +34,21 @@ public static Board createBoard() {
return new Board(chessBoard);
}

public static Board createFromDto(final BoardResultDto boardResultDto) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DTO -> Board 처리를 어디서 할까 궁금했는데, BoardFactory에서 수행해주고 있네요! 이부분은 책임을 적절히 지정해주었다고 생각됩니다.

public void printBoard(final BoardResultDto boardResultDto) {
System.out.println();
public void printBoard(final List<PieceDto> pieces) {
Map<Position, Piece> board = new HashMap<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Position은 위치에 대한 정보만 가지니까 여기서 키로 써도 괜찮을 것 같고, 다만 value로는 Piece 대신 PieceDto의 piece값(String)을 넣어줘도 되지 않을까요?

import chess.domain.pieces.Queen;
import chess.domain.pieces.Rook;

public class PieceParser {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PieceParser에 대한 테스트도 간단하게 만들어두면 좋을 것으로 보이네요. 😄

@include42 include42 merged commit 43f7beb into woowacourse:sosow0212 Mar 23, 2023
include42

This comment was marked as resolved.

sosow0212 added a commit to sosow0212/java-chess that referenced this pull request Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants