[터틀] 체스 웹/DB 미션 제출합니다. #157

begaonnuri · 2020-04-08T17:09:22Z

감사합니다 :)

- WebUIChessApplication의 로직을 ChessWebController로 이동 - chess.html 추가

- ChessBoardDto를 통해 뷰에 체스판을 전달하도록 변경

- chess.html 변경 - chess.css 추가 - 기물 이미지 추가

- StatusDto 추가

- 클래스명 변경

- Connectable 인터페이스를 Dao가 구현 - RoomDao가 Dao를 상속받도록 구현 - CRUD 기능 추가 - 테스트 작성

- SELECT 결과가 Optional<String>을 반환하도록 변경 - INSERT 결과가 새로 추가한 room_id를 반환하도록 변경 - 테스트 변경

- List<Piece>를 List<List<String>>으로 변경하는 기능을 static 메소드로 생성

- 테스트 추가

- 모든 방 목록을 db에서 가져오는 기능 추가 - RoomsDto 클래스 추가

- String을 Board로 변환하는 기능 추가 - String을 Side로 변환하는 기능 추가

- String으로 Piece를 만들어 내는 기능 추가

- Controller의 로직을 Service와 DTO로 이동

Hue9010

구현을 깔끔히 잘 해주셨네요! 👍
피드백 남겼는데 확인해 주세요!!

Hue9010 · 2020-04-09T22:37:34Z

src/main/java/chess/contoller/WebController.java

+			} catch (RuntimeException e) {
+				model.put("error", e.getMessage());
+			}
+			transfer(chessGame, model);


transfer가 안보이네요. 😢

chessGame에 error가 뜨지 않았나요? chessGame이 선언된건 try문 안이라서 scope 문제가 있습니다.

웹상에서 방 생성, 입장까지 밖엔 못해봤는데.. 😢
이동도 마저 push 해주시면 체스 플레이를 마저 진행해볼게요 😄

악... 변경하는 과정에서 실수했네요 😥
커밋 전에 빌드와 테스트 모두 돌려보고 올리겠습니다 죄송합니다...

Hue9010 · 2020-04-09T22:38:06Z

src/main/java/chess/contoller/WebController.java

+			return render(model, "index.html");
+			putGameInfoToModel(chessGame, model);
+			return render(model, "chess.html");


악... 변경하는 과정에서 실수했네요 😥
커밋 전에 빌드와 테스트 모두 돌려보고 올리겠습니다 죄송합니다...

Hue9010 · 2020-04-09T22:52:42Z

src/test/java/chess/domain/dao/RoomDaoTest.java

+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class RoomDaoTest {


테스트에 순서가 존재하네요.
add -> find 순으로 테스트하면 통과하지만
add -> update or delete -> find 순으로 테스트하면 실패합니다. 테스트는 순서에 상관없이 성공해야 합니다! 그래서 전체 테스트를 돌려보는게 좋아요.

보드의 초기화를 setUp 단계에서 진행해주시거나 이번 단계에서는 crud 테스트를 한 메서드에서 다 테스트할 수 있을 것 같아요.

test 페키지 우 클릭 후 전체 테스트를 돌려서 확인해보세요. 😉

테스트는 순서에 상관없이 성공해야 합니다!

setUp 메소드를 생성해 순서에 상관없이 돌아가도록 변경하겠습니다! 감사합니다 :)

Hue9010 · 2020-04-09T22:58:51Z

src/main/java/chess/contoller/WebController.java

+	private void joinRoom() {
+		post("/join", (req, res) -> {
+			Map<String, Object> model = new HashMap<>();
+			ChessGame chessGame = chessGameService.load(req.queryParams("room-name"));
+			putGameInfoToModel(chessGame, model);
+			return render(model, "chess.html");
+		});
+	}


존재하지 않는 방에 입장하려고하면 기본 에러페이지가 뜨는데 직접 만든 에러 페이지 혹은 index.html에서 방이 존재하지 않아 안된다는 문구로 사용자한테 안내해주면 좋을 것 같아요.

감사합니다 :) 예외 처리 로직을 모두 빼먹은 것 같네요!

[입장] 존재하지 않는 방에 입장하는 경우

[생성] 중복된 이름으로 방을 생성하는 경우

두 가지 모두 예외처리 추가하도록 하겠습니다 😄

Hue9010 · 2020-04-09T23:02:09Z

src/main/java/chess/contoller/WebController.java

+	private void mainRendering() {
+		get("/", (req, res) -> {
+			Map<String, Object> model = new HashMap<>();
+			model.put("room", new RoomsDto(chessGameService.findAllRooms()));


RoomsDto가 아니라 List로 제공해주면 될 것 같아요.

List를 제공하도록 변경했습니다!

Hue9010 · 2020-04-09T23:09:10Z

src/main/java/chess/domain/service/ChessGameService.java

+	public ChessGame create(String roomName) throws SQLException {
+		ChessGame chessGame = ChessGame.start();
+
+		String board = BoardConverter.convertToString(chessGame.getBoard(), BLANK_MARK);


convertToString의 사용처를 보면 BLANK_MARK만 사용합니다. BLANK_MARK 외 사용처가 있을까요?

굳이 ChessGameService에서 주입해줄 필요가 없다는 생각이 드네요.
BLANK_MARK를 BoardConverter로 이동하겠습니다!

Hue9010 · 2020-04-09T23:14:00Z

src/main/java/chess/domain/service/ChessGameService.java

+		String finishFlag = roomDao.findByRoomName(roomName, "finish_flag")
+				.orElseThrow(NoSuchElementException::new);
+		validateFinish(finishFlag);
+
+		String board = roomDao.findByRoomName(roomName, "board")
+				.orElseThrow(NoSuchElementException::new);
+		String turn = roomDao.findByRoomName(roomName, "turn")
+				.orElseThrow(NoSuchElementException::new);


controller와 outputView간 데이터를 주고 받을 dto를 잘 만들어 주셨는데 dao와 service 계층간에 사용할 dto를 만들어 보는건 어떨까요?

service에서 dao로 전송할때 ChessGameDto를 만들어 전달하도록 변경했습니다!

dao에서 service로 전송할때도 dto를 만들어서 전송해도 되는건가요? dao에서 dto까지 만들어서 service로 전달하게 되면 책임이 커진다고 생각이 들어서요.😢

책임이 커진다보다는 dao의 책임인가를 봐야해요. 결국 dto를 쓴다는건 계층간 데이터의 전송은 dto를 이용한다는 것이기 때문에 dto로 반환해줘야 맞습니다. dto를 반환하지 않는다면 다른 대안이 map일텐데 map도 결국 put을 직접 해줘야하죠.
지금의 경우는 connection부터 직접 맺고 모든걸 다 처리해줘서 그런데 차후에 마이바티스, JPA를 사용하신다면 터틀이 직접적으로 보지 못하는 무엇인가가 변환을 대신 해준답니다. 😉

Hue9010 · 2020-04-09T23:16:14Z

src/main/java/chess/domain/ChessGame.java

+	public void restart() {
+		board = ChessBoardFactory.create();
+		turn = Side.WHITE;
+	}
+
+	public void load(ChessBoard chessBoard, Side side) {
+		board = chessBoard;
+		turn = side;
 	}


사용이 안되고 있는 걸로 보이는데 필요가 없어졌다면 제거해주세요!

사용되지 않는 메소드 제거했습니다! 감사합니다 :)

- ChessGameService에서 종료를 확인하는 부분을 FinishFlag 클래스를 사용하도록 변경

- ChessGameService에서 생성해서 반환해주도록 변경

- ChessGame의 load, restart 메소드 제거

- ChessGameService에서 ChessGameDto를 통해 RoomDao로 전달하도록 변경

begaonnuri · 2020-04-13T08:59:43Z

dto를 만들면서 궁금한 점이 생겼는데요,

service->dao로 보내는 dto에서, ChessGameDto엔 roomName필드가 있지만 도메인인 ChessGame엔 roomName필드가 없습니다. 저는 이 부분이 어색하다고 생각합니다. 도메인엔 roomName 관련 로직이 없어서 dto에만 있는 상황이 생긴것 같은데 어떻게 생각하시나요?
dto를 생성하는 곳은 꼭 서비스가 되어야 하는건가요? 컨트롤러에서 뷰로 아래와 같이 전달하는데 서비스에서 메소드를 만들어 리턴해줘야 하는지 궁금합니다.
```
model.put("board", ChessBoardDto.of(chessGame));
model.put("status", StatusDto.of(chessGame));
```

Hue9010

구현을 잘 해 주셨네요. 👍 두개의 방을 만들어 플레이 해봤는데 둘 다 잘 되네요 😉
이번 단계는 머지하도록 할게요. 고생 많으셨습니다!

Hue9010 · 2020-04-14T00:05:18Z

src/main/java/chess/contoller/WebController.java

+			} catch (RuntimeException e) {
+				model.put("room", chessGameService.findAllRooms());
+				model.put("error", e.getMessage());
+				return render(model, "index.html");
+			}


SQLException을 제외하면 IllegalArgumentException만 발생하지 않나요?
catch 절에서는 발생이 예상되는 Exception들을 잡아주는게 좋답니다. Exception e 보다는 낫지만 어떤 RuntimeException이 나던 통제하지 못하고 일괄적으로 처리하는 걸로 보입니다. 더군다나 다른 사람이 봤을 때는 여기서 어떤 예외를 잡고자 하는건지 알기가 힘들답니다. 그래서 가능하다면 catch 절에서는 잡고자 하는 예외를 정확히 사용해주시는게 좋아요.

참고로 SQLException은 RuntimeException이 아니라서 여기서 잡아주지 않는 답니다.

Hue9010 · 2020-04-14T00:23:10Z

src/main/java/chess/contoller/WebController.java

+				ChessGame chessGame = chessGameService.create(roomName);
+				putGameInfoToModel(roomName, chessGame, model);
+			} catch (RuntimeException e) {
+				model.put("room", chessGameService.findAllRooms());


chessGameService.findAllRooms()만 post() 밖으로 빼보면 SQLException에 대한 처리를 강요한답니다.(spark 내부에서 처리를 해주는 걸로 보이네요.)
Service에선 이에 대한 처리로 throws를 강요받으셨을 텐데 이 시점에 checked, unchecked Exception에 대해서 다시 한번 보면 기존보다 보이는게 더 많을 수 있을 것 같아요.
SQLException에 대한 처리를 본격적으로 하기엔 쉽지 않으니 별도의 처리를 하실 필욘 없어보여요. 😉

예외 처리가 쉬운듯 어려우면서 나름 피곤하답니다. 대신 예외 처리들을 잘 해두면 문제가 발생했을 때 파악도 용이하고, 지금처럼 사용자한테 무엇을 잘 못 했는지 알려주는 방법으로는 사용자 경험을 향상 시켜 줄 수도 있답니다.
예외부분에 대해서는 학습하는데 있어 다른 것보다 우선순위를 높게 보진 않지만 한번쯤은 다양한 방법으로 예외 처리하는 부분을 접하고자 몇가지 요청을 드렸네요.

참고로 checked/unchecked Exception의 차이에 대해서는 면접에서 간단하게 물어 볼 수 있는 질문이랍니다. 뭘 더 알 필요는 없고 테크코스의 강의 자료를 본 결과 그 정도만 알고 있으면 됩니다. 😉

Hue9010 · 2020-04-14T00:41:23Z

src/main/java/chess/contoller/WebController.java

+	private void mainRendering() {
+		get("/", (req, res) -> {
+			Map<String, Object> model = new HashMap<>();
+			model.put("room", chessGameService.findAllRooms());


복수개가 있는 리스트를 보내주기 때문에 rooms가 맞을 것 같아요.

Hue9010 · 2020-04-14T00:56:32Z

src/main/java/chess/domain/dto/ChessGameDto.java

+import chess.domain.ChessGame;
+import chess.domain.FinishFlag;
+
+public class ChessGameDto {


chessGameDto라고 하셨는데 이건 받아 사용하는 주체인 dao의 해당 테이블이 room이라서 서로간의 괴리가 발생한 것 같아요. table 이름이 chessGmae이였다면 좀더 이해가 쉬울 수 있는데 실제 디스크에 쓰인 물리적 데이터와 논리적 개념이 도메인 모델과 서로 상이해서 발생한거라 그에 따른 물리적 데이터를 위한 데이터로서 DataTransferObject가 필요해진 상황이에요. 저는 주로 물리적/논리적으로 차이가 발생할 때 dto를 사용하는 편입니다.(이전에 말한 request/response가 그런 경우들이 많아 우선적으로 쓰는 편이고요. 이 부분도 자신이 다루는 서비스의 특징에 따라서는 다를 수 있습니다.)

더 생각해볼게 거꾸로 String name만 가지고 있는 RoomDto를 Dto를 만들지 않고 List으로 model에 넣어줄 수도 있답니다. 굳이 변환 과정을 거쳐야 하죠. 그래서 db의 데이터와 도메인 모델, 요청/응답 모델이 모두 같은 값을 가져도 되는 상황이라면 각 레이어 별로 dto를 만들 필요가 없을 수도 있답니다.(이 경우도 레이어간의 이동이기 때문에 꼭 만들어야 한다는 얘기도 있답니다.)

설명을 쉽게 해드리지 못했는데.. dto에 대해서는 주로 다루는 글들이 단순히 계층간 이동엔 dto를 쓴다는 개념적인 얘기만 하거나 경험적인 부분을 내포하고 쓰는 얘기들이 많아서 지금 단계에서 추천드릴 만한 글은 찾기가 힘드네요. 저 같은경우도 경험적인 부분을 내포하고서 설명을 드리다보니 쉽지가 않네요. 😢 그래도 의문이 드는 점은 다시 여쭤봐 주세요!

begaonnuri added 25 commits April 5, 2020 15:40

refactor: ChessWebController 추가

f9defe5

- WebUIChessApplication의 로직을 ChessWebController로 이동 - chess.html 추가

feat: ChessBoardDto 추가

b4784b7

- ChessBoardDto를 통해 뷰에 체스판을 전달하도록 변경

feat: 최초의 체스판을 보여주는 기능 구현

c001c52

- chess.html 변경 - chess.css 추가 - 기물 이미지 추가

feat: 웹에서 기물을 이동시키는 기능 구현

46cc6bc

feat: 각 팀의 점수를 보여주는 기능 구현

73e2a45

- StatusDto 추가

refactor: 메소드 분리

9cbfe49

- 클래스명 변경

refactor: ChessGame의 move, end 메서드가 상태를 변경하도록 수정

4a09086

feat: 게임을 재시작, 종료하는 기능 추가

b09e372

feat: 데이터베이스와 연결하는 기능 추가

6f90667

- Connectable 인터페이스를 Dao가 구현 - RoomDao가 Dao를 상속받도록 구현 - CRUD 기능 추가 - 테스트 작성

refactor: RoomDao 변경

ab75172

- SELECT 결과가 Optional<String>을 반환하도록 변경 - INSERT 결과가 새로 추가한 room_id를 반환하도록 변경 - 테스트 변경

refactor: BoardConverter 클래스 추가

705dd4d

- List<Piece>를 List<List<String>>으로 변경하는 기능을 static 메소드로 생성

feat: 방을 생성하는 기능 구현

e9fdc83

refactor: 게임 종료를 방에서 나가기로 변경

0a5a326

feat: 기물의 움직임을 DB에 반영하는 기능 추가

216df3d

feat: RoomName 클래스 추가

2584d3d

- 테스트 추가

feat: 중복된 방 이름을 검증하는 기능 추가

9261171

style: BoardService 클래스의 이름을 ChessGameService로 변경

ee3e465

feat: 방 목록을 보여주는 기능 구현

5923c98

- 모든 방 목록을 db에서 가져오는 기능 추가 - RoomsDto 클래스 추가

refactor: move를 db에 반영하는 메소드 수정

8b539b2

feat: BoardConverter 변경

2a17a6c

- String을 Board로 변환하는 기능 추가 - String을 Side로 변환하는 기능 추가

feat: PieceFactory 추가

9c9e4d9

- String으로 Piece를 만들어 내는 기능 추가

feat: 방에 들어가는 기능 추가

2d33811

refactor: 접근제어자 및 메소드명 변견

0c2b683

refactor: Controller의 로직 제거

61642a6

- Controller의 로직을 Service와 DTO로 이동

feat: 현재 턴을 보여주는 기능 추가

f7785b6

Hue9010 suggested changes Apr 9, 2020

View reviewed changes

begaonnuri added 4 commits April 12, 2020 17:27

test: RoomDaoTest를 호출 순서에 상관없이 테스트할 수 있도록 변경

48689ff

refactor: BoardConverter 변경

fe62e75

- ChessGameService에서 종료를 확인하는 부분을 FinishFlag 클래스를 사용하도록 변경

feat: 방을 생성, 입장할 때 예외 메시지를 보여주도록 변경

4d0df33

refactor: RoomsDto를 List<RoomDto>로 변경

e675486

- ChessGameService에서 생성해서 반환해주도록 변경

begaonnuri added 4 commits April 12, 2020 18:22

refactor: BLANK_MARK를 BoardConverter로 이동

73906b8

- ChessGame의 load, restart 메소드 제거

refactor: move할때도 방 이름을 인자로 받도록 변경

5b1bcd8

refactor: ChessGameDto 클래스 추가

1e0d157

- ChessGameService에서 ChessGameDto를 통해 RoomDao로 전달하도록 변경

test: 테스트 추가 및 실패하는 테스트 수정

152f438

Hue9010 approved these changes Apr 14, 2020

View reviewed changes

Hue9010 merged commit a096c18 into woowacourse:begaonnuri Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[터틀] 체스 웹/DB 미션 제출합니다. #157

[터틀] 체스 웹/DB 미션 제출합니다. #157

begaonnuri commented Apr 8, 2020 •

edited

Loading

Hue9010 left a comment

Hue9010 Apr 9, 2020

Hue9010 Apr 9, 2020

begaonnuri Apr 12, 2020

Hue9010 Apr 9, 2020

begaonnuri Apr 12, 2020

Hue9010 Apr 9, 2020

Hue9010 Apr 9, 2020

begaonnuri Apr 12, 2020

Hue9010 Apr 9, 2020

begaonnuri Apr 12, 2020 •

edited

Loading

Hue9010 Apr 9, 2020

begaonnuri Apr 12, 2020

Hue9010 Apr 9, 2020

begaonnuri Apr 12, 2020

Hue9010 Apr 9, 2020

begaonnuri Apr 13, 2020

begaonnuri Apr 13, 2020 •

edited

Loading

Hue9010 Apr 14, 2020

Hue9010 Apr 9, 2020

begaonnuri Apr 12, 2020

begaonnuri commented Apr 13, 2020

Hue9010 left a comment

Hue9010 Apr 14, 2020

Hue9010 Apr 14, 2020

Hue9010 Apr 14, 2020

Hue9010 Apr 14, 2020

Hue9010 Apr 14, 2020

[터틀] 체스 웹/DB 미션 제출합니다. #157

[터틀] 체스 웹/DB 미션 제출합니다. #157

Conversation

begaonnuri commented Apr 8, 2020 • edited Loading

Hue9010 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

begaonnuri Apr 12, 2020 • 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

begaonnuri Apr 13, 2020 • 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

begaonnuri commented Apr 13, 2020

Hue9010 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

begaonnuri commented Apr 8, 2020 •

edited

Loading

begaonnuri Apr 12, 2020 •

edited

Loading

begaonnuri Apr 13, 2020 •

edited

Loading