[Spring 체스 - 2단계] 써머(최혜원) 미션 제출합니다. #474

hyewoncc · 2022-04-30T06:54:27Z

안녕하세요, 핀!
저번에 남겨주신 리뷰 감사히 잘 읽었습니다 🙇

DAO와 Repository의 차이에 대해 남겨주신 코멘트에 질문이 있는데요,
2단계로 넘어오면서 GameRepository가 PieceDao와 BoardDao를 갖게 했습니다
그러니 GameService에서 piece, board 테이블에 각각 저장을 실행하지 않고,
gameRepository.saveGame(roomId, board); 하는 식으로 컬렉션 처리책임을 GameRepository로 분리할 수 있었습니다

그런데 이렇게 하니 board, piece 테이블에 개별로 접근해야 하는 메서드들은 책임이 분리되지 않고, 그냥 클래스 계층만 한 단계 깊어지게 되더라고요
그래서 GameService에서 각 dao 까지 갖게 하려니 계층 구조에 일관성이 없어서 좋지 않다고 생각했습니다
일단 GameService는 GameRepository를 사용하도록, RoomService는 개별 RoomDao와 BoardDao를 사용하도록 뒀는데, 여기에 대해 추가로 참고할 만한 키워드가 있을까요?
저수준의 CRUD는 DAO, 컬렉션 처리는 Repository 까지는 이해했는데, 제 코드에 적용하려니 어디까지가 컬렉션 처리인가? 저수준의 CRUD만 필요한 경우 dao를 바로 사용해도 되는가? 같은 고민 때문에 쉽지 않네요

그리고 컨트롤러 테스트의 경우 추가하고 있었는데, mock mvc를 사용하지 않고 하려니 통합 테스트가 되고, 메서드 순서가 보장되지 않아 데이터 삽입 시점을 알 수 없고, 데이터를 삭제해도 auto_increment 설정 된 키값이 초기화 되지 않는 등의 문제 때문에 의도대로 잘 되지 않았습니다 😭...
그래서 mock mvc를 공부 중인데 제출이 너무 늦어지는 것 같아 우선 pr을 올립니다, 일단은 주석처리 해뒀어요

hyewoncc · 2022-04-30T06:56:03Z

src/main/java/chess/service/GameService.java

-        int boardId = boardRepository.getBoardIdByRoom(roomId);
-        Board board = loadBoard(boardId);
-        board.loadTurn(boardRepository.getTurn(boardId));
+        int boardId = gameRepository.findBoardIdByRoom(roomId);


이런 경우에는 BoardDao만 이용하면 되는데, GameRepository를 통하면서 결국 계층만 깊어졌다 느꼈습니다

boardId를 단독으로 가져와서 다시 board를 조회할 필요가 있을까요?

아래와 같은 방식으로 할 수 있지 않을까요?

board = repository.findByRoomId(roomId);

hyewoncc · 2022-04-30T07:04:25Z

src/main/java/chess/web/RoomController.java

+    @GetMapping
+    public ResponseEntity<List<Map<String, String>>> findRooms() {
+        return ResponseEntity.ok(roomService.findRooms());
+    }


이 부분은 프론트에 방 목록을 보내주기 위해 필요한 부분인데요,
기존에 Room, RoomDto가 이미 있으나, 여기에는 모두 비밀번호를 나타나는 password 필드가 있습니다
그런데 api에 비밀번호를 담아 주고 싶지 않고, id와 name만 담는 dto 클래스를 만드려 하니,
그러면 반환 포맷에 변화가 생길 때 마다 dto를 추가해줘야 하나 싶어서 일단 Map 으로 만들었습니다...
dto를 추가해주는 게 맞을까요?

dto도 각 레이어별 (controller, service, repository 등) 존재할 수 있고, 원하는 형태의 응답을 위해 여러 많은 dto가 추가되는 건 자연스러운 일이에요.

Map을 사용하면 단순히 메서드 시그니처만 봤을 때는 어떤 응답을 만들어 주는지 알기 어렵고, 모든 값을 넣을 수 있다보니 실수로 넣지 않아야 하는 값을 넣는 문제도 있을 수 있을 것 같아요.

당장 Map을 사용하면 dto를 새로 만들지 않아도 되니까 편하겠지만 앞으로 이런 부분들이 계속해서 생겨나면 유지보수하는 작업이 어려워지지 않을까요?

lalize

써머 🏖️ , 안녕하세요!
2단계 잘 구현해주셨네요 👍
몇가지 코멘트 남겨두었으니 확인 부탁드릴게요!
궁금한 점 있으시면 편하게 DM 주세요~

lalize · 2022-05-02T01:49:37Z

src/main/java/chess/service/GameService.java

+        gameRepository.deleteOldBoard(roomId);
+        return gameRepository.saveGame(roomId, board);


save할 때 항상 이전 데이터의 삭제가 필요하면 saveGame 메서드에서 함께 처리하는 건 어떨까요?

lalize · 2022-05-02T01:56:41Z

src/main/java/chess/service/GameService.java

+        PieceDto pieceDto = gameRepository.findPiece(boardId, source).get();
        deleteMovedPieceFromSource(boardId, source);
        updateMovedPieceToTarget(boardId, target, pieceDto);
        updateGameState(board, boardId);


board, piece 테이블에 개별로 접근해야 하는 메서드들은 책임이 분리되지 않고, 그냥 클래스 계층만 한 단계 깊어지게 되더라고요

서비스에서 개별 테이블에 접근이 필요한 걸까요? #383 (comment)

move(...) { board = repository.find(...); board.move(...); repository.save(...); }

lalize · 2022-05-02T02:02:59Z

src/main/java/chess/repository/GameRepository.java

+            pieces.put(Position.of(pieceDto.getPosition()), PieceFactory.build(pieceDto));
+        }
+        Board board = new Board(() -> pieces);
+        board.loadTurn(boardDao.getTurn(boardId));


loadTurn대신 생성자에서 turn을 주입받으면 좋을 것 같아요.

lalize · 2022-05-02T02:04:35Z

src/main/java/chess/service/GameService.java

-        int boardId = boardRepository.getBoardIdByRoom(roomId);
-        Board board = loadBoard(boardId);
-        board.loadTurn(boardRepository.getTurn(boardId));
+        int boardId = gameRepository.findBoardIdByRoom(roomId);


boardId를 단독으로 가져와서 다시 board를 조회할 필요가 있을까요?

아래와 같은 방식으로 할 수 있지 않을까요?

board = repository.findByRoomId(roomId);

lalize · 2022-05-02T02:30:34Z

src/main/java/chess/web/RoomController.java

+    @GetMapping
+    public ResponseEntity<List<Map<String, String>>> findRooms() {
+        return ResponseEntity.ok(roomService.findRooms());
+    }


dto도 각 레이어별 (controller, service, repository 등) 존재할 수 있고, 원하는 형태의 응답을 위해 여러 많은 dto가 추가되는 건 자연스러운 일이에요.

Map을 사용하면 단순히 메서드 시그니처만 봤을 때는 어떤 응답을 만들어 주는지 알기 어렵고, 모든 값을 넣을 수 있다보니 실수로 넣지 않아야 하는 값을 넣는 문제도 있을 수 있을 것 같아요.

당장 Map을 사용하면 dto를 새로 만들지 않아도 되니까 편하겠지만 앞으로 이런 부분들이 계속해서 생겨나면 유지보수하는 작업이 어려워지지 않을까요?

lalize · 2022-05-02T02:32:07Z

src/main/java/chess/web/RoomController.java

-    @GetMapping(value = "/{roomId}", params = "command=load")
-    public ResponseEntity<BoardDto> loadGame(@PathVariable int roomId) {
-        return ResponseEntity.ok(gameService.loadGame(roomId));
+    @GetMapping(value = "/{roomId}/new")


POST가 적절해 보이네요!

lalize · 2022-05-02T02:35:38Z

src/main/java/chess/web/RoomController.java

+    @DeleteMapping("/{roomId}")
+    public ResponseEntity<Map<String, String>> deleteRoom(@PathVariable int roomId, @RequestParam String password) {
+        roomService.delete(roomId, password);
+        return ResponseEntity.ok(Map.of("url", "/"));


이 부분은 url을 내려주지 않아도 프론트가 어디로 이동해야 할지 알 수 있지 않을까요?

lalize · 2022-05-02T03:54:15Z

src/main/java/chess/service/RoomService.java

+        Optional<Integer> boardId = boardDao.findBoardIdByRoom(roomId);
+        Optional<RoomDto> room = roomDao.findById(roomId);
+        checkPassword(room, password);
+        if (boardId.isEmpty()) {
+            roomDao.delete(roomId);
+            return;
        }
-        return roomRepository.find(name).get();
+        checkGameEnd(boardId);
+        roomDao.delete(roomId);


RoomService는 개별 RoomDao와 BoardDao를 사용하도록 뒀는데, 여기에 대해 추가로 참고할 만한 키워드가 있을까요?
저수준의 CRUD는 DAO, 컬렉션 처리는 Repository 까지는 이해했는데, 제 코드에 적용하려니 어디까지가 컬렉션 처리인가? 저수준의 CRUD만 필요한 경우 dao를 바로 사용해도 되는가? 같은 고민 때문에 쉽지 않네요

Repository는 객체를 다루고, Dao는 테이블을 다룬다고 보면, 객체 구조가 너무나 단순하여 Repository가 단순 by-pass로 보여도 객체를 저장, 수정, 조회, 삭제를 하는 건 Repository를 만들어 사용하는 게 역할에 맞는 책임이라고 생각해요.

체스 코드에서는 Piece는 특정 Board에 속하고 모든 비즈니스는 Board를 중심으로 일어나기 때문에 객체 컬렉션을 처리하는 데 PieceRepository를 통하는 게 아닌 BoardRepository(GameRepository)를 통해야 합니다. 하나의 유기체처럼 동작하는 객체마다 일반적으로 Repository 한개가 만들어진다고 보면 됩니다.

lalize · 2022-05-02T04:04:08Z

src/main/java/chess/service/RoomService.java

-        Optional<RoomDto> roomDto = roomRepository.find(name);
-        if (roomDto.isEmpty()) {
-            roomRepository.save(name);
+    public void delete(int roomId, String password) {


dto와 getter로 비즈니스 로직을 수행하고 있는 것 같아요.
아래와 비슷한 구조로 변경해보면 어떨까요?

void delete(...) { room = repository.findById(roomId); if (!room.isCorrect(password) && ...) { throw ... } repository.delete(room); }

lalize · 2022-05-02T04:23:33Z

src/main/java/chess/repository/GameRepository.java

+import org.springframework.stereotype.Repository;
+
+@Repository
+public class GameRepository {


hyewoncc · 2022-05-03T04:52:02Z

안녕하세요, 핀! 남겨주신 피드백 모두 반영했습니다 🙇
처음 리뷰를 받았을 때는 DAO, Repository 차이에 대해 감이 잘 오지 않았는데 핀의 리뷰를 통해서 잘 이해할 수 있었어요
둘을 협력하는 객체가 아닌 DB에 접근하는 수단으로만 봐서 1단계에는 다분히 절차지향적인 코드를 짜버렸는데,
피드백을 반영하면서 둘을 객체지향적 관점으로 볼 수 있게 되었습니다, 감사해요 👍 덕분에 많이 배웠습니다
마지막으로 하나 여쭤보고 싶은 게 있는데, 지금 Room 객체를 저장한 후, DB에서 자동 생성된 id 값을 받아오기 위해 일시적으로 id에 null 값을 넣어 Room을 생성하고 있습니다
그런데 필드값에 null이 들어가는 게 논리적으로 맞지 않다고 여겨집니다, 다른 사람이 보면 null 인지 모르고 id 값에 접근할 수도 있고요
이런 경우에 entity를 사용하거나 해야할까요..? 이번 미션에서 다른 크루들과 얘기하다 엔티티 얘기를 처음 들어서 궁금하네요

lalize

써머 🏖️ , 안녕하세요!
리뷰 반영 잘 해주셨네요~ 👍
이번 미션은 여기서 마무리하도록 할게요!
궁금한 점 있으시면 언제든지 DM 주셔도 좋습니다~
고생하셨습니다~~

lalize · 2022-05-04T08:16:16Z

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

+public class Room {
+
+    public static final int NAME_LENGTH_LIMIT = 16;
+    public static final int PASSWORD_LENGTH_LIMIT = 16;
+
+    private long id;
+    private final String name;
+    private final String password;


#474 (comment)

도메인과 엔티티를 분리하는 것도 좋고, id 생성을 DB에 의존하지 않고 코드 레벨에서 uuid를 생성하는 방법도 있을 것 같아요 ㅎㅎ
역시나 정답은 없다고 생각해요 :)

hyewoncc and others added 30 commits April 19, 2022 16:34

feat: 기존 체스 코드 가져오기

1d6f834

refactor: 체스 코드 합치기

cc90334

feat: 체스방 입장

42f1646

fix: 테스트 수정

4259ef2

refactor: player 개념을 room으로 변경

4e6b15d

feat: 유효하지 않은 이름에 대해 400 에러 처리

fcea9ef

feat: 더블 레포지토리를 이용한 db 의존하지 않는 테스트 설정 작성

ec2b80e

feat: 게임 불러오기

8a0d883

feat: /move 요청 처리

71cc5b4

feat: /result 요청 처리

01dbf9a

fix: 이미지 이름 수정

719ba9d

feat: /end 요청 처리

ee08639

refactor: 응답 데이터를 dto 객체로 래핑

3191d0f

refactor: roomId를 query parameter가 아닌 path variable로 전달

f1f1ca1

feat: roomRepositoryImpl에 Spring jdbc 적용 구현

76a58a8

fix: 테스트 깨짐 수정

5fdbb93

refactor: 상수 처리

014f35b

feat: BoardRepositoryImpl에 Spring jdbc 적용 구현

e3b168e

refactor: PieceRepository를 jdbc 적용으로 교체

9c041c4

refactor: 패키지 변경, 스파크 애플리케이션과 dao 삭제

327cb50

refactor: 컨트롤러 분리

197fbd4

fix: url 변경으로 인한 테스트 깨짐 수정

4504416

feat: 프론트 개선

62711d1

fix: 테스트에서 value source를 사용하지 않던 문제

1a8d616

fix: merge 충돌 해결

a5f7460

docs: 2단계 요구사항 정리

cc78b06

feat: 이름과 비밀번호를 입력해 체스방 생성

88e1c8c

feat: 목록에서 체스방 보여주고 클릭 시 입장

6246c51

feat: 프론트 모달 창 닫기 추가

6bb0f2e

feat: 비밀번호 일치 시 방 삭제, 불일치시 거부

d6285d3

hyewoncc added 11 commits April 29, 2022 13:56

feat: 보드 테이블에 종료 여부를 판별하는 end 컬럼 추가

e6e1321

refactor: 콘솔 코드 삭제

2a7478a

refactor: 말 이동 요청을 POST -> PATCH 변경

3a56ed9

refactor: 어노테이션 변경 ControllerAdvice -> RestControllerAdvice

6d4b739

fix: 메서드 시그니쳐 변경으로 인해 깨진 테스트 정정

340b480

feat: 체스말 이동 시 점수 바로 갱신

48ba8bc

refactor: api url 디자인 개선

0938c76

refactor: board 찾는 api url 수정

914479b

refactor: 기존 repository -> dao 변경, dao를 통해 컬렉션 처리하는 repository 작성

ca1d935

feat: 프론트 css 수정

0d14217

refactor: interface 삭제

b3b75a9

hyewoncc commented Apr 30, 2022

View reviewed changes

lalize suggested changes May 2, 2022

View reviewed changes

hyewoncc added 13 commits May 2, 2022 14:27

refactor: 패키지 변경

b31b4a2

refactor: GameRepository 에서 바로 Board를 조회

aa83a5a

refactor: 컨트롤러에서 Map 대신 Dto 반환

07ccfb4

refactor: 새 게임 저장 시 기존 게임 삭제 책임을 service에서 repository로 이동

b32899d

refactor: 체스 말 이동 DB 제어 로직을 service -> repository 이동

5064437

refactor: Board 생성자에 turn 추가

71071e6

refactor: 새로운 게임 생성 매핑 메서드 변경 get -> post

578ef74

refactor: 방 비밀번호 검증을 get 없이 Room 도메인이 실행

d3d0d3a

refactor: 프론트에서 새로운 게임 생성 요청 method 변경 get -> post

c4561c4

refactor: room 관련 db 로직을 Room 객체 이용하도록 개선, 메서드 명 개선

5e79bf6

fix: 컨트롤러 테스트에서 dao를 사용해 예상값 처리

b5127af

refactor: 인스턴스 필드를 private 지정

5a0c5da

refactor: 메서드 순서 변경

204c760

lalize approved these changes May 4, 2022

View reviewed changes

lalize merged commit 45b72af into woowacourse:hyewoncc May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spring 체스 - 2단계] 써머(최혜원) 미션 제출합니다. #474

[Spring 체스 - 2단계] 써머(최혜원) 미션 제출합니다. #474

hyewoncc commented Apr 30, 2022

hyewoncc Apr 30, 2022

lalize May 2, 2022

hyewoncc Apr 30, 2022

lalize May 2, 2022

lalize left a comment

lalize May 2, 2022

lalize May 2, 2022

lalize May 2, 2022

lalize May 2, 2022

lalize May 2, 2022

lalize May 2, 2022

lalize May 2, 2022

lalize May 2, 2022

lalize May 2, 2022

lalize May 2, 2022

hyewoncc commented May 3, 2022

lalize left a comment

lalize May 4, 2022

		gameRepository.deleteOldBoard(roomId);
		return gameRepository.saveGame(roomId, board);

[Spring 체스 - 2단계] 써머(최혜원) 미션 제출합니다. #474

[Spring 체스 - 2단계] 써머(최혜원) 미션 제출합니다. #474

Conversation

hyewoncc commented Apr 30, 2022

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

lalize 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

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

hyewoncc commented May 3, 2022

lalize left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment