[4, 5단계 - 체스] 후디(조동현) 미션 제출합니다. by devHudi · Pull Request #419 · woowacourse/java-chess

devHudi · 2022-04-07T08:04:13Z

안녕하세요, 에단!
난이도가 높아서 코드 퀄리티가 좋지 않은 상태로 리뷰 요청을 드리게되어 아쉽습니다.
구현만 하기에도 시간이 빠듯할 것 같아 TDD 규칙을 제대로 지키지 못한 점도 아쉬워요.

그래도 예상보다는 도메인 코드의 변경은 별로 없어서 '나쁘지 않은 설계를 했구나' 라는 생각이 들었어요.
이번 미션에는 처음으로 DAO 와 Service 레이어를 사용해봤는데, Controller, Service 에 각각 적절한 책임을 부여하는 것이 어려웠어요.

추가적인 질문은 코멘트로 남길게요 감사합니다 😄

- ChessController 를 ConsoleController 로 변경 - InputView와 OutputView를 console 패키지로 이동

- 기물 이동을 위한 POST 라우트 추가 - 기물 이동 명령을 위한 form 추가

- ScoreResultDto 구현 - OutputView 에서 ScoreResultDto 를 사용하도록 변경

- DAO 에 대한 테스트코드 추가 - Board 가 Map 을 직접 전달받아 생성될 수 있도록 정적 팩토리 메소드 구현 - Position 의 from(XAxis, YAxis) 메소드의 이름을 of 로 변경 - XAxis 와 YAxis 가 문자열 value 를 전달받아 해당하는 열거 객체를 반환하는 메소드 getByValue 메소드 오버로딩 - Board, Piece 에 toString 재정의

- 테스트 코드 작성 - init.sql 수정 - Position 에 XAxis, YAxis 게터 추가 - 기물 생성, 기물 제거, 기물 위치 변경에 대한 DTO 추가

- 변경된 DTO 에 따라 View 로직 변경 - DTO를 각각 성격에 따라 request, response 패키지로 분리

- GameDto 의 도메인 변환로직을 Service 로 옮김

- 킹이 잡혀도 GameState 가 ReadyToStart 로 변경되지 않음 - 킹이 잡힌 상태로 기물을 움직일 수 없게 되었음 - PieceColor를 전달해 해당 색상의 킹이 보드에 존재하는지 확인할 수 있음 - 웹뷰에 승자를 표시함

- GSON 라이브러리 의존성 추가 - html, css, js 작성 - TurnDto 를 PieceColorDto 로 이름 변경

- BodyParser 분리 - JsonMapper 분리

Laterality

안녕하세요 후디.

어려운 요구사항이었을텐데, 잘 구현해주셨어요 👍

몇가지 피드백을 남겨드렸어요. 확인 부탁드려요 :)

devHudi · 2022-04-07T13:24:58Z

+    private final PieceType pieceType;
+    private final PieceColor pieceColor;


DTO 는 UI 단으로 도메인이 노출되어 실수로 변경되는 것으로부터 보호하기 위해 사용하는 것 으로 알고있습니다. 반대로 Enum 과 불변 도메인은 변경될일이 없으므로 DTO의 필드로 사용해도 된다고 판단하였습니다. 괜찮은 판단일까요?

뷰에서 도메인을 직접 참조하면 도메인이 변경됐을 때 뷰에도 영향이 있겠네요.

만약 뷰와 도메인 사이에 DTO가 있다면 어떨까요?

Enum 혹은 불변 클래스일 경우 도메인이 변경될일이 없다고 생각해서 뷰와 도메인 사이에 DTO를 만들고 (Enum 및 불변 클래스를) 필드로 사용하였습니다!

앗, 제 답변이 중의적이었네요 😅

위에서 "도메인이 변경"된다는 의미는 도메인의 코드가 변경된다는 의미였어요!

도메인의 필드가 사라지거나 다른 타입으로 바뀌는 등 여러가지 변경이 생길 수 있는데요. 이러한 변경이 발생했을 때 DTO의 유무에 따라 어떤 차이가 생길까요?

- 예외를 JSON으로 매핑하여 보여주도록 개선

Laterality

안녕하세요 후디.

잘 수정해주셨어요 👍

간단한 리팩터링과 테스트에 관해 의견을 남겨드렸어요. 확인 부탁드려요!

Laterality · 2022-04-10T11:25:28Z

+    private final PieceType pieceType;
+    private final PieceColor pieceColor;


뷰에서 도메인을 직접 참조하면 도메인이 변경됐을 때 뷰에도 영향이 있겠네요.

만약 뷰와 도메인 사이에 DTO가 있다면 어떨까요?

- ChessService 테스트코드 작성

Laterality

안녕하세요 후디,

잘 수정해주셨네요!

이번 미션은 이만 머지하도록 할게요.

추가로 리팩터링해 볼 만한 곳과 질문 주셨던 부분에 코멘트를 남겨드렸어요.

확인 부탁드리고, 궁금하신 점 있으시면 언제든 DM 주세요.

레벨 1 고생 많으셨습니다. 다음 레벨도 화이팅하세요 :)

Laterality · 2022-04-13T13:56:40Z

+    private final PieceType pieceType;
+    private final PieceColor pieceColor;


앗, 제 답변이 중의적이었네요 😅

위에서 "도메인이 변경"된다는 의미는 도메인의 코드가 변경된다는 의미였어요!

도메인의 필드가 사라지거나 다른 타입으로 바뀌는 등 여러가지 변경이 생길 수 있는데요. 이러한 변경이 발생했을 때 DTO의 유무에 따라 어떤 차이가 생길까요?

Laterality · 2022-04-13T14:00:51Z

+import java.util.HashMap;
+import java.util.Map;
+
+public class BoardDaoImpl extends Dao implements BoardDao {


이 부분은 다른 크루에게 남겼던 코멘트를 참고해주세요!

Laterality · 2022-04-13T14:01:29Z

+        String query = String.format("SELECT x_axis, y_axis, piece_type, piece_color FROM %s WHERE game_id = ?",
+                TABLE_NAME);
+
+        try (Connection connection = getConnection()) {
+            PreparedStatement preparedStatement = connection.prepareStatement(query);
+            preparedStatement.setString(1, gameId);
+            ResultSet resultSet = preparedStatement.executeQuery();
+
+            Map<Position, Piece> boardValue = new HashMap<>();
+            while (resultSet.next()) {
+                XAxis xAxis = XAxis.getByValue(resultSet.getString("x_axis"));
+                YAxis yAxis = YAxis.getByValue(resultSet.getString("y_axis"));
+                PieceType pieceType = PieceType.valueOf(resultSet.getString("piece_type"));
+                PieceColor pieceColor = PieceColor.valueOf(resultSet.getString("piece_color"));
+
+                boardValue.put(Position.of(xAxis, yAxis), new Piece(pieceType, pieceColor));
+            }
+
+            return BoardDto.from(boardValue);
+        } catch (SQLException e) {
+            throw new DatabaseException(e.getMessage());
+        }


어떻게 하면 메서드의 길이를 줄일 수 있을까요?

Laterality · 2022-04-13T14:02:44Z

+    private void updateTurn(String gameId, String turn) {
+        String query = String.format("UPDATE %s SET turn = ? WHERE id = ?", TABLE_NAME);
+
+        try (Connection connection = getConnection()) {


Connection을 얻는 블럭이 중복되네요. 이 중복을 없앨 방법이 있을까요?

Laterality · 2022-04-13T14:05:16Z

+                new Piece(PIECE_TYPE, PIECE_COLOR));
+
+        // when & then
+        boardDao.createPiece(createPieceDto);


createPiece가 정상적으로 동작하는지 테스트하는 것으로 보여요. 대상 메서드를 호출한 뒤 결과에 대한 검증은 어떻게 할 수 있을까요?

devHudi force-pushed the step2 branch from 29b1631 to 7280be0 Compare April 7, 2022 08:13

devHudi added 29 commits April 7, 2022 17:37

docs(README): 4단계 기능 구현 목록 작성

d10983e

refactor(chess): 컨트롤러와 뷰의 이름과 경로를 변경

ae8fe41

- ChessController 를 ConsoleController 로 변경 - InputView와 OutputView를 console 패키지로 이동

feat(view): 웹 환경에서 체스판을 출력

8c8ac05

feat(WebController): 기물 이동 구현

6b2b36e

- 기물 이동을 위한 POST 라우트 추가 - 기물 이동 명령을 위한 form 추가

feat(chess): 웹뷰에 백팀, 흑팀의 점수 현황 출력

562f9ec

- ScoreResultDto 구현 - OutputView 에서 ScoreResultDto 를 사용하도록 변경

feat(view): 현재 차례를 유저에게 출력하는 기능 구현

732b54c

chore: MySQL을 위한 도커 셋업

519492a

chore: mysql-connector 의존성 추가

539613e

chore: 컨테이너 최초 실행 시 실행될 SQL 추가

02e1b83

feat(dao): 기존 PieceDao를 제거하고, 새로운 BoardDao 및 GameDao를 구현

a5910d9

- 테스트 코드 작성 - init.sql 수정 - Position 에 XAxis, YAxis 게터 추가 - 기물 생성, 기물 제거, 기물 위치 변경에 대한 DTO 추가

feat(service): 웹 서버가 다시 실행되어도 체스게임을 이어서할 수 있도록 개선

4fe2007

refactor(BoardDto): 자료구조를 중첩 ArrayList 에서 HashMap 으로 변경

de44417

- 변경된 DTO 에 따라 View 로직 변경 - DTO를 각각 성격에 따라 request, response 패키지로 분리

refactor(WebController): 컨트롤러 코드 메소드 추출

c0dcf13

- GameDto 의 도메인 변환로직을 Service 로 옮김

feat(controller): 게임 재시작 기능 구현

a834c33

feat(view): 기물 이동을 form 에서 fetch api 로 변경

2ab345b

refactor(dto): GameDto 를 ChessGameDto 로 이름 변경

b430481

refactor(PieceDto): 출력을 위한 정보를 DTO에서 뷰로 별개의 enum 클래스로 분리

1309f71

feat(controller): 템플릿엔진을 사용하지 않고, JSON으로 통신하도록 변경

49d18fc

- GSON 라이브러리 의존성 추가 - html, css, js 작성 - TurnDto 를 PieceColorDto 로 이름 변경

chore: 필요없는 출력 코드 제거

5a9180d

refactor(BoardDto): getPieceImages 메소드 제거

00a3b43

chore: view.console 패키지를 consoleview 패키지로 이름 변경

f515a81

refactor(controller): form data 대신 json 을 사용하여 기물 움직임 구현

d8a6692

- BodyParser 분리 - JsonMapper 분리

fix(database): pk에 game_id 추가

56b79db

test: 도메인의 변경으로 인해 실패하는 테스트 변경

c310ac5

refactor(PieceColorDto): 출력을 위한 로직을 뷰로 분리

f174a5f

fix(BoardDao): 조건에 game_id 컬럼 추가

1c94c37

refactor(service): getBoardDto 를 getBoard 로 변경

f2c2dd8

docs(README): 기능구현목록 갱신

5debaed

devHudi force-pushed the step2 branch from 7280be0 to 5debaed Compare April 7, 2022 08:38

devHudi closed this Apr 7, 2022

devHudi deleted the step2 branch April 7, 2022 08:39

devHudi restored the step2 branch April 7, 2022 08:39

devHudi reopened this Apr 7, 2022

Laterality requested changes Apr 7, 2022

View reviewed changes

devHudi commented Apr 7, 2022

View reviewed changes

Comment thread src/main/java/chess/dto/response/BoardDto.java Resolved

devHudi added 2 commits April 7, 2022 23:10

refactor(dao): try-with-resources 를 사용하여 Connection 자원 해제

b9f2113

refactor(BoardDto): getValue 에서 컬렉션 반환 전 방어적 복사

f862feb

devHudi force-pushed the step2 branch from c181777 to f862feb Compare April 7, 2022 14:16

devHudi added 4 commits April 9, 2022 23:11

refactor(BoardDto): 필드명 value를 board로 변경

5ddf3e1

refactor: 승자가 존재하지 않을 때 승자의 색상을 요청하는 경우 예외가 발생

6944778

- 예외를 JSON으로 매핑하여 보여주도록 개선

refactor: 게임 ID가 존재하지 않을 경우 예외 발생

eee3b3e

refactor: DatabaseException에 예외 메세지를 넘겨주도록 개선

9dc46e7

Laterality requested changes Apr 10, 2022

View reviewed changes

devHudi added 3 commits April 11, 2022 14:26

refactor(Dao): 메소드 및 상수 추출

6db4e58

refactor(ChessService): ChessService 테스트를 위한 Dao 인터페이스 및 가짜 객체 구현

6221e6c

- ChessService 테스트코드 작성

refactor(BoardDto): Indent 를 1로 개선

c5fe30a

Laterality approved these changes Apr 13, 2022

View reviewed changes

Laterality merged commit f7931e5 into woowacourse:devhudi Apr 13, 2022

		private final PieceType pieceType;
		private final PieceColor pieceColor;

Conversation

devHudi commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Laterality left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Laterality left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

devHudi commented Apr 7, 2022 •

edited

Loading