[4, 5단계 - 체스] 릭(나일성) 미션 제출합니다. #353

nailseong · 2022-04-04T08:42:49Z

안녕하세요 철시!!!! 릭입니다 😊
이번 리뷰도 잘 부탁드려요!! 👍 🔨

pkch93

안녕하세요 릭~ 👋
2단계 빠르게 구현해주셨네요~ 몇가지 피드백 드렸는데 확인 부탁드려요~!

README.md

pkch93 · 2022-04-05T12:07:03Z

src/main/java/chess/controller/ChessWebController.java

+                Map<String, Object> model = new HashMap<>();
+                model.put("roomExist", false);
+                final Gson gson = new Gson();
+                halt(401, gson.toJson(model));


before이 잘못된 경로에 대한 처리로 보여지네요. 401 응답을 주신 이유가 있을까요?

src/main/java/chess/controller/ChessWebController.java

pkch93 · 2022-04-05T12:39:15Z

src/main/java/chess/controller/ChessWebController.java

+            }
+            model.put("roomExist", false);
+            return render(model, "board.html");
+        });


Path 메서드 내에서 룸이 있는지 판단하고 그값을 세팅해서 내려주네요. Service 영역을 정의하셨는데 Service에 위 로직이 들어가면 되지 않을까싶네요.
/room/:roomName 뿐만 아니라 다른 path 메서드도 비슷한데요. controller 내의 로직을 Service 영역에서 처리하면 어떨까요?

src/main/java/chess/service/ChessService.java

pkch93 · 2022-04-05T12:53:01Z

src/main/java/chess/service/ChessService.java

+            updateRoomStatusTo(roomName, GameStatus.PLAYING);
+        } catch (IllegalArgumentException e) {
+            if (chessGame.canPlay()) {
+                result = findAllPiece(roomName);


에러가 발생하더라도 findAllPiece를 해주네요..? 에러 메세지만 담아서 내려줘도 되지 않을까 싶은데 의도가 있을까요?

pkch93 · 2022-04-05T12:59:48Z

src/main/java/chess/dao/ChessPieceDao.java

+        return 0;
+    }
+
+    public int saveAll(final String roomName, final Map<Position, ChessPiece> pieceByPosition) {


일반적으로 bulk insert는 하나의 트랜잭션으로 묶어서 처리하는데 위와 같이 사용해도 하나의 트랜잭션으로 처리가 되니 괜찮은 방법으로 보여요. 다만, 좀 더 가독성을 높이면 좋을거 같네요.

pkch93 · 2022-04-05T13:01:32Z

src/main/java/chess/dao/ChessPieceDao.java

+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+public class ChessPieceDao {


Dao의 메서드들이 모두 10줄이 넘어가네요. 분리할 수 있는 부분이 있어보이는데 분리해서 가독성을 높이면 좋을거 같아요.

src/main/java/chess/service/ChessService.java

pkch93 · 2022-04-05T13:10:12Z

src/main/java/chess/view/JsonGenerator.java

+import java.util.Map.Entry;
+import java.util.Objects;
+
+public class JsonGenerator {


JsonGenerator에 질문이 있었군요! JsonGenerator를 정의하신 이유가 Controller에서 JsonGenerator를 통해서 응답을 만들어주려고 하신걸까요? Service 영역에서 응답을 만들어서 내려주는건 고려해보신걸까요?

nailseong

안녕하세요 철시!! 릭입니다! 😊
우선 좋은 피드백 감사해요!! 구현하기 급해서 개선할 부분을 많이 놓쳤는데 철시 덕분에 훨씬 구조가 좋아진 것 같아요.

피드백 반영하면서 아래와 같은 부분을 집중적으로 했어요.

Service에서 응답을 만들어서 내려주도록 변경
Controller를 최대한 깔끔하게 변경해서 역할에 맞게 코드를 분리
url과 http method를 잘 활용해서 api를 심플하고 직관적이게 변경
README를 좀더 상세하게 변경
Dao에 중복 코드를 메서드로 분리해서 가독성 향상

피드백을 반영하고 리팩토링하면서 Dao를 좀더 깔끔하게 개선하고, 가독성을 올리는 방법을 찾기가 어려웠는데요. 이 부분에 대해서 철시의 조언을 듣고싶어요 🤔

pkch93

릭~ 피드백 잘 반영해주신거 같아요.
피드백 좀 더 드린게 있는데 이부분만 확인해보면 좋을거 같아서 남깁니다.
레벨 1 완주가 멀지 않은거 같은데 조금만 힘내요 💪

pkch93 · 2022-04-07T14:24:35Z

src/main/java/chess/dao/ChessPieceDao.java

+            setParameter(statement, roomName, position.getValue());
+            return statement.executeUpdate();
+        } catch (SQLException e) {
+            e.printStackTrace();


전반적으로 예외상황에서 e.printStackTrace를 하고계시네요. 에러에 대해 또다른 예외를 던지거나 예외에 대한 값을 리턴하는게 필요하지 않을까 싶은데요.

delete 같은 경우는 아래 0을 반환하긴하지만 executeUpdate의 결과로도 0이 나올수 있잖아요. 0이 정상과 예외 두 상황을 담고 있어서 예외 상황에 대해서는 예외를 표현할 수 있도록 하는게 낫지 않을까 싶은데 어떻게 생각하시나요

pkch93 · 2022-04-07T14:32:49Z

src/main/java/chess/dao/ChessPieceDao.java

+        return 0;
+    }
+
+    public int saveAll(final String roomName, final Map<Position, ChessPiece> pieceByPosition) {


위 부분이 잘 이해가 안되신걸까요?
SQL문을 만들어 주는부분이 한눈에 들어오지는 않았던거 같아요. sql 만들어주는 부분은 따로 빼서 처리해봐도 좋을거 같아요. 그러면서 SQL 만들어주는 부분도 테스트가 될 수 있지 않을까요?

pkch93 · 2022-04-07T14:37:36Z

src/main/java/chess/service/ChessService.java

+        this.gson = new Gson();
+    }
+
+    public String findAllPiece(final Request req, final Response res) {


Service에 Request, Response 객체가 들어왔네요. 덕분에 컨트롤러가 깔끔해지긴 했지만 Request, Response를 Service 영역에서 알아도 무방할까요?

pkch93 · 2022-04-07T14:41:41Z

src/main/java/chess/service/ChessService.java

+            res.status(404);
+            final Map<String, String> map = new HashMap<>();
+            map.put("error", e.getMessage());
+            return gson.toJson(map);


에러 로직을 처리하는 부분들이 전부 비슷해보이네요. 로직을 공통화할 수 있을거 같네요.

nailseong

안녕하세요 철시~ 피드백 감사합니다!! 피드백 반영 진행하면서 질문 한 가지 남겼는데 함께 확인 부탁드려요!!

nailseong · 2022-04-08T07:35:52Z

src/main/java/chess/dao/ChessPieceDao.java

+        } catch (SQLException e) {
+            e.printStackTrace();
+            throw new SQLQueryException("position에 해당하는 기물 삭제에 실패했습니다.", e);


e.printStackTrace를 하고 다른 RuntimException을 던지도록 해봤어요.
그리고 기존의 예외를 보관하기 위해서 SQLException을 생성자에 넣도록 해봤어요.

(질문)
e.printStackTrace를 쓰거나 logger를 활용해서 서버에 로그를 남기는 게 좋다고 생각하는데요. 로그는 어디에서 남겨야 할까?라는 생각이 들었어요.

Dao는 service만 의존하고 있으니까 service에서 예외 처리를 하면서 log를 남기면 log 처리 관련 로직을 한곳으로 모을 수 있다!

한 곳에서 모든 예외에 대해 log 처리를 하면 그 메서드에게 책임이 많아진다. 따라서 log는 예외가 발생한 곳에서 처리하는 게 좋다.

위와 같은 고민이 생겼는데 철시는 어떻게 생각하시는지 의견이 듣고 싶어요!!

좋은 고민하셨네요 ㅎㅎ 어떤 점이 나은가요? 저는 log는 예외가 발생한 곳에서 남기는게 이슈 트래킹을 할때에도 유리하다고 생각이 들어요. 발생한 예외를 넘겨주는 것도 좋네요 👍
저번에 남긴 리뷰는 예외 처리를 로그만 찍고 기본값과 구분이 되지 않는 값을 반환하고 있어서 리뷰를 남겼었던거라 예외에 대한 처리와 함께 로그를 같이 찍어주는게 낫다고 생각이 들어요 😃

nailseong · 2022-04-08T07:45:39Z

src/main/java/chess/controller/ChessWebController.java

+            delete("/:name", (req, res) -> {
+                final String json = roomService.deleteRoom(extractRoomName(req));
+                handleError(json, res);
+                return json;
+            });


Service가 Request, Response 객체를 의존하지 않도록 변경해 봤어요.
service 입장에서 사용자의 입력을 어떤 식으로 받아오는지 (ex. body로 받아오기, path variable인지 등등)는 알 필요가 없다고 생각해요. 그리고 service에서 응답을 어떻게 내려줄지 (ex. body에 담아서, html을 그려서 등등) 역시 알 필요가 없다고 생각해요.

리팩토링 하다보니 Service에서 실패 응답을 내려줄 때 http 상태 코드를 함께 내려줘야 한다고 생각해서 StatusCode와 ErrorResponseDto를 만들게 되었어요.

pkch93

릭 피드백 반영 확인했어요~
이번 체스 미션은 이만 머지할께요.
우테코 레벨 1 고생많으셨어요~ 다음 과정도 응원할께요 💪🦾

nailseong added 30 commits April 1, 2022 14:02

feat(templates): 체스판 html

9f82991

feat(templates): 시작 버튼

a50a255

feat(WepApplication): post 요청 추가

f353f45

feat(WebApplication): 이동 기능 추가

b6639ad

feat(WebApplication): 점수 표시 기능 추가

783d20c

feat(WebApplication): 예외 처리 로직 추가

69565bc

feat(WebApplication): 이동 명령 예외 처리

0a27f47

feat(WebApplication): 게임 종료 기능 추가

42985e9

refactor(WebApplication): start post 메서드 이동

76db3e1

refactor(WebApplication): move post 메서드 수정

c7d01b2

feat(resources): 기물 이미지 파일, 자바스크립트 코드 추가

a07911c

refactor(PieceName): 도메인 모델에 View관련 코드 분리

05d8d6e

feat(ChessService): ChessWebController와 매핑되는 service 레이어 추가

e639573

fix(Pawn): A, H 파일의 폰이 대각선 기물을 잡지 못하는 문제 해결

a2228d5

refactor(ChessService): 점수 쿼리 메서드 리팩토링

5a02bf9

docs(README): api 명세 정리

9e19a11

feat(ChessWebController): 점수 반환 api 추가

8e3a7c3

feat(ChessController): 현재 턴 색깔 반환 api

67971fc

feat(templates): 현재 턴 표기 기능 추가

9dde7fc

feat(ChessController): 게임 승패 결과 api 추가

e49a4cb

feat(templates): 게임 결과 출력 기능 추가

eeaed4d

docs(docker): DB추가를 위한 도커 파일

cc9d4df

feat(init.sql): DB 테이블 생성 DDL 추가

aa21f51

feat(Color): 문자열에 해당하는 색깔을 찾는 기능

574f569

feat(ChessPieceDto): 쿼리 결과를 담을 Dto

5835eb7

feat(ChessPieceDao): 포지션으로 기물을 찾는 기능

a4ade8d

feat(ChessPieceDao): 모든 기물을 조회하는 기능

6927c8d

feat(ChessPieceDao): 기물 저장, 삭제 기능 추가

d9df779

feat(ChessPieceDao): 모든 기물을 저장하는 기능

e8a7c44

feat(ChessPieceDao): 모든 기물을 삭제하는 기능

6fbad83

pkch93 suggested changes Apr 5, 2022

View reviewed changes

nailseong added 18 commits April 6, 2022 11:31

refactor(ChessService): 예외 발생 시 예외 메시지만 내려주도록 변경

b1374b1

refactor(service): 메서드 내부에서 Dao를 생성하지 않도록 변경

30c55e7

refactor(ChessWebController): Room 체크 로직을 service 계층에 할당

998a61b

feat(MoveRequestDto): Move 요청과 매핑되는 Dto

f7a30ff

refactor(service): Service에서 내려줄 응답을 만들도록 변경

e1848c2

refactor(ChessWebController): api의 uri를 직관적으로 변경

55ee761

refactor(templates): api 변경에 대응해서 변경

a6ec41f

docs(README): 프로그램 실행 방법 작성 및 api url 정보 갱신

7b80b61

refactor(ChessWebController): Room 존재 여부 확인 로직 이동

25cb4c7

docks(README): 포맷 수정

65189d0

fix(board.html): 이동 명령 수행 후 화면에 그려지지 않는 문제 해결

4c6559e

fix(index.html): 화면을 그리기 전에 서버의 요청이 끝나지 않는 문제 해결

08a54ae

refactor(service): 예외 상황에서 적절한 Http status code를 내려주도록 변경

212f93a

refactor: 불필요한 메서드, 필드 삭제

a1da5e3

refactor: 메서드를 분리해서 가독성 높임

1dd6ee6

refactor(dao): Dto생성자를 변경해서 dao 메서드 가독성 향상

3e5a7e0

refactor(dao): 중복 로직 메서드 추출

43ee40d

refactor(ChessPieceDao): 메서드 추출

e2f15ff

nailseong commented Apr 6, 2022

View reviewed changes

fix(StatementUtil): 파라미터 인덱스를 잘못 지정하는 버그 해결

2e688ae

pkch93 suggested changes Apr 7, 2022

View reviewed changes

nailseong added 5 commits April 8, 2022 00:18

refactor(ResponseUtil): 예외 로직 처리 중복 제거

436cab0

refactor(ChessWebController): service에서 Request, Response 객체를 분리

9d7b026

refactor(ChessPieceDao): Bulk insert sql 생성 부분 추출

0710445

feat(dao): 커스텀 예외를 던지는 기능 추가

51c84a8

feat(service): 커스텀 예외 처리 기능 추가

2a24aed

nailseong commented Apr 8, 2022

View reviewed changes

pkch93 approved these changes Apr 8, 2022

View reviewed changes

pkch93 merged commit 1c5999f into woowacourse:nailseong Apr 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4, 5단계 - 체스] 릭(나일성) 미션 제출합니다. #353

[4, 5단계 - 체스] 릭(나일성) 미션 제출합니다. #353

nailseong commented Apr 4, 2022

pkch93 left a comment

pkch93 Apr 5, 2022

pkch93 Apr 5, 2022

pkch93 Apr 5, 2022

pkch93 Apr 5, 2022

pkch93 Apr 5, 2022

pkch93 Apr 5, 2022

nailseong left a comment •

edited

Loading

pkch93 left a comment

pkch93 Apr 7, 2022

pkch93 Apr 7, 2022

pkch93 Apr 7, 2022

pkch93 Apr 7, 2022

nailseong left a comment •

edited

Loading

nailseong Apr 8, 2022

pkch93 Apr 8, 2022

nailseong Apr 8, 2022

pkch93 left a comment

[4, 5단계 - 체스] 릭(나일성) 미션 제출합니다. #353

[4, 5단계 - 체스] 릭(나일성) 미션 제출합니다. #353

Conversation

nailseong commented Apr 4, 2022

pkch93 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

Choose a reason for hiding this comment

nailseong left a comment • edited Loading

Choose a reason for hiding this comment

pkch93 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

nailseong left a comment • 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

pkch93 left a comment

Choose a reason for hiding this comment

nailseong left a comment •

edited

Loading

nailseong left a comment •

edited

Loading