[라빈] 체스 웹/DB 미션 제출합니다 #133

giantim · 2020-04-06T16:56:55Z

화투 안녕하세요! 라빈입니다.
체스 미션 1단계 머지 해주시면서 남겨주셨던 피드백 먼저 반영하고 웹과 DB 구현했습니다.

이번에 처음으로 DAO 라는 개념에 대해 알게 되었고 적용해서 프로그램을 구현해 봤습니다. 잘못 사용한 부분이 있다면 조언 부탁드리겠습니다!

감사합니다!

refactor: final 키워드 추가

- 게임을 진행하는 ChessRunner 클래스 구현

…/ 각 체스 말을 초기화 할때의 정보를 enum 으로 구현

Home

refactor: 디자인 수정

phs1116

안녕하세요, 라빈!
구현 잘해주셨어요. :)
몇가지 피드백 추가했는데 참고 부탁드릴게요.
궁금하신 점 있으시면 DM 주세요!

phs1116 · 2020-04-08T16:54:13Z

src/main/java/chess/WebUIChessApplication.java

+            Map<String, Object> model = new HashMap<>();
+
+            webChessController.start();
+            List<TileDto> tileDtos = webChessController.getTiles();
+            TeamDto teamDto = webChessController.getCurrentTeam();
+
+            model.put("tiles", tileDtos);
+            model.put("currentTeam", teamDto);
+
+            return render(model, "game.html");


Controller는 model에 대한 매핑과 어떤 뷰를 노출할지에 대한 책임까지 가지고있습니다.
이러한 로직들도 컨트롤러에 넘기면 어떨까요? :)

컨트롤러는 뷰를 선택하는 역할을 하도록 하고 내부 로직은 서비스 레이어를 만들어서 처리하도록 수정했습니다

phs1116 · 2020-04-08T16:55:52Z

src/main/java/chess/controller/WebChessController.java

+        ChessBoardDAO chessBoardDAO = new ChessBoardDAO();
+        CurrentTeamDAO currentTeamDAO = new CurrentTeamDAO();
+        PieceDAO pieceDAO = new PieceDAO();


DAO는 매번 재생성 될 필요가 없는 객체인데요.
생성해서 재사용하도록 수정해보면 어떨까요?

서비스 레이어에서 한번만 DAO 를 생성해서 재사용 하도록 수정했습니다.

phs1116 · 2020-04-08T16:56:28Z

src/main/java/chess/controller/WebChessController.java

+    private ChessRunner chessRunner;
+    private ChessBoard chessBoard;
+    private CurrentTeam currentTeam;
+    private PieceOnBoards originalPieces;


웹 컨트롤러 상에서는 상태를 가지는 객체를 들고있지 않도록 하는게 좋아요.
여러 쓰레드에서 접근하기 때문에 동기화 문제가 발생할 여지가 있습니다. :)
DAO를 통해 데이터를 꺼내가며 생성하도록 구현해보는게 좋을 것 같아요.

컨트롤러가 상태를 갖는 객체를 갖지 않도록 서비스 레이어로 이동했습니다.

phs1116 · 2020-04-08T16:58:53Z

src/main/java/chess/controller/dao/ChessBoardDAO.java

+        PreparedStatement pstmt = con.prepareStatement(query);
+        ResultSet rs = pstmt.executeQuery();


두 객체에 대해서도 꼭 close 해주세요. :)
try-with-resources를 참고해봐도 좋을 것 같아요.

알려주신 내용을 참고해서 트랜잭션 처리 로직을 변경했습니다. 감사합니다 😀

phs1116 · 2020-04-08T17:01:04Z

src/main/java/chess/controller/dao/PieceDAO.java

+import java.util.List;
+
+public class PieceDAO {
+    public void addPiece(int chessBoardId, List<TileDto> tileDtos) throws SQLException {


DAO에 Dto를 사용하는건 프레젠테이션 레이어까지 의존이 생길 여지가 있네요.
가능하면 여기서 사용하는 모델을 통해 저장하도록 해주는게 좋을 것 같아요. :)

DAO 에서는 데이터베이스에 저장된 데이터와 매핑되는 모델만을 이용하도록 변경했습니다.

phs1116 · 2020-04-08T17:03:23Z

src/main/java/chess/controller/WebChessController.java

+            pieceDAO.deletePiece(deletedPiece);
+        }
+        Optional<PieceOnBoard> sourcePiece = this.originalPieces.find(source);
+        pieceDAO.updatePiece(sourcePiece.get(), target);


Optional.get은 왠만하면 지양하는 편이 좋아요. :)
get보다는 그외 제공하는 메서드를 통해 null에 대해서도 처리해주세요.

Optional 에서 get() 을 호출하기 전 isPresent() 를 먼저 호출해서 null 처리를 해주었습니다.

phs1116 · 2020-04-08T17:04:44Z

src/main/java/chess/domain/strategy/initialize/BishopInitializer.java

@@ -39,4 +39,26 @@
    public Map<Position, Piece> initialize() {
        return InitialBishop.initializeBishops();
    }
+
+    @Override
+    public Map<Position, Piece> webInitialize(Map<String, String> pieceOnBoards) {


webInitialize등 도메인에서 web에 대한 의존을 굳이 표현할 필요는 없을 것 같아요.

phs1116 · 2020-04-08T17:06:24Z

src/main/java/chess/domain/strategy/initialize/BishopInitializer.java

+    @Override
+    public Map<Position, Piece> webInitialize(Map<String, String> pieceOnBoards) {
+        Map<String, String> bishops = pieceOnBoards.entrySet().stream()
+                .filter(entry -> entry.getValue().substring(0, 1).toLowerCase().equals("b"))


"b"등 하드코딩은 지양하는게 좋을 것 같네요. :)

phs1116 · 2020-04-08T17:12:50Z

src/main/java/chess/domain/strategy/initialize/KnightInitializer.java

+        Map<String, String> knights = pieceOnBoards.entrySet().stream()
+                .filter(entry -> entry.getValue().substring(0, 1).toLowerCase().equals("n"))
+                .collect(Collectors.toMap(entry -> entry.getKey(), entry -> entry.getValue()));


pieceOnBoard에 대해서 pieceUrl을 파싱하는게 아닌,
피스에 대한 정보 없이 이미지 이름 가지고 도메인을 풀어가는것보다는
pieceOnBoard자체에 어떤 피스인지에 대한 정보를 가지고있는게 맞지 않을까요?
도메인을 조금 더 드러나게 모델링해보아요.

데이터베이스에 저장되는 piece -> pieceOnBoard 로 변경하고 이미지가 아닌 pieceType, team 정보를 저장하도록 변경하였습니다.

phs1116 · 2020-04-08T17:13:53Z

src/main/java/chess/domain/ChessRunner.java

+                    .filter(td -> td.getPosition().equals(tile.position()))
+                    .findFirst()
+                    .orElseThrow(IllegalArgumentException::new);
+            tileDto.setPieceImageUrl(tile.pieceImageUrl());


setPieceImageUrl
세터는 되도록 지양하도록 구현해보아요. :)
다른 피드백에 드렸다시피 Tile 자체가 어떤 말인지에 대한 정보를 가지고있다면 이러한 주소는 굳이 가지고 있지 않아도 될 것 같아요.

도메인이 드러나도록 모델링을 변경하니 Tile 클래스가 사라지게 되었습니다. 감사합니다!

Web chess

giantim · 2020-04-13T08:41:52Z

화투 안녕하세요!
수정하고 추가한 내용이 많아서 피드백 반영이 조금 늦었습니다. 죄송합니다!
제가 생성한 데이터베이스 코드 첨부합니다. 감사합니다!

CREATE DATABASE webChess DEFAULT CHARACTER SET utf8 COLLATE utf8_general_ci;

USE webChess;

CREATE TABLE chessBoard (
		chessBoardId int NOT NULL AUTO_INCREMENT,
		PRIMARY KEY(chessBoardId));

CREATE TABLE currentTeam (
		currentTeamId int NOT NULL AUTO_INCREMENT,
		team VARCHAR(5) NOT NULL,
		chessBoardId int NOT NULL,
		PRIMARY KEY(currentTeamId),
		FOREIGN KEY(chessBoardId) REFERENCES chessBoard(chessBoardId));

CREATE TABLE pieceOnBoard (
		pieceId int NOT NULL AUTO_INCREMENT,
		position VARCHAR(2) NOT NULL,
		pieceType VARCHAR(10) NOT NULL,
                team VARCHAR(5) NOT NULL,
		chessBoardId int NOT NULL,
		PRIMARY KEY(pieceId),
		FOREIGN KEY(chessBoardId) REFERENCES chessBoard(chessBoardId));

CREATE TABLE player (
		playerId int NOT NULL AUTO_INCREMENT,
		whitePlayer VARCHAR(10) NOT NULL,
		blackPlayer VARCHAR(10) NOT NULL,
		chessBoardId int NOT NULL,
		PRIMARY KEY(playerId),
		FOREIGN KEY(chessBoardId) REFERENCES chessBoard(chessBoardId));

phs1116

안녕하세요! 라빈.
개발 잘해주셨어요.
몇가지 피드백 더 추가했는데 참고 부탁드릴게요. :)

phs1116 · 2020-04-15T13:13:26Z

src/main/java/chess/dao/ChessBoardDAO.java

+             PreparedStatement pstmt = con.prepareStatement(query)) {
+            pstmt.executeUpdate();
+        } catch (SQLException e) {
+            e.printStackTrace();


이펙티브 자바 아이템 77. 예외를 무시하지 말라
해당 케이스는 복구 불가능한 예외로 보이는데요, 예외를 무시하지 말고 커스텀 예외를 더 던져주는게 좋아보여요 :)

답변 주신 내용을 바탕으로 CustomSQLException 을 만들어서 예외 출력 페이지로 이동시켰습니다!

phs1116 · 2020-04-15T13:14:10Z

src/main/java/chess/dao/ChessBoardDAO.java

+            return chessBoard;
+        } catch (SQLException e) {
+            e.printStackTrace();
+            return null;


데이터가 없는 경우에 대해서는 null 혹은 Optional을 사용할 수 있지만
해당 케이스에서는 예외를 새로 던져보아요. :)

phs1116 · 2020-04-15T13:14:43Z

src/main/java/chess/dao/ConnectionManager.java

+            System.out.println("정상적으로 연결되었습니다.");
+        } catch (SQLException e) {
+            System.err.println("연결 오류:" + e.getMessage());
+            e.printStackTrace();


연결 실패시에도 null을 돌려주어 null 위험성을 만드는 것 보다는 예외를 추가로 던져주는게 좋을 것 같아요. :)

phs1116 · 2020-04-15T13:16:39Z

src/main/java/chess/dao/PieceOnBoard.java

+    private String pieceType;
+    private String team;


pieceType, team에 대해서도 enum으로 관리해도 될 것 같네요.
position도 하나의 문자열로 처리하기보다는 기존 enum을 사용해서 자료구조를 명확하게 드러내주는게 좋을 것 같아요.

답변 주신 내용을 바탕으로 도메인을 드러내도록 표현하게 변경하였습니다 :)

phs1116 · 2020-04-15T13:16:57Z

src/main/java/chess/dao/PieceOnBoardDAO.java

+            for (PieceOnBoard pieceOnBoard : pieceOnBoards) {
+                String query = "INSERT INTO pieceOnBoard (position, pieceType, team, chessBoardId) "
+                        + "VALUES(?, ?, ?, ?)";
+                PreparedStatement pstmt = con.prepareStatement(query);


PreparedStatement에 대해서도 꼭 close 해주세요!

try-with-resource 구문의 안쪽에 선언된 PreparedStatement, ResultSet 를 닫는 메서드를 추가하였습니다 :)

phs1116 · 2020-04-15T13:17:15Z

src/main/java/chess/dao/PieceOnBoardDAO.java

+        try (Connection con = ConnectionManager.getConnection();
+             PreparedStatement pstmt = con.prepareStatement(query);) {
+            pstmt.setInt(1, chessBoard.getChessBoardId());
+            ResultSet rs = pstmt.executeQuery();


result set도 close 해주세요. :)

phs1116 · 2020-04-15T13:18:05Z

src/main/java/chess/dao/PieceOnBoards.java

+    public Optional<PieceOnBoard> find(String position) {
+        Optional<PieceOnBoard> piece = Optional.ofNullable(this.pieceOnBoards.stream()
+                .filter(p -> p.getPosition().equals(position))
+                .findFirst()


Optional을 굳이 추가로 래핑하지 않아도 findFirst 자체가 옵셔널을 리턴해요. :)

phs1116 · 2020-04-15T13:19:06Z

src/main/java/chess/domain/ChessRunner.java

+                    .filter(td -> td.getPosition().equals(entry.getKey().toString()))
+                    .findFirst()
+                    .orElseThrow(IllegalArgumentException::new)
+                    .setPieceImageUrl(entry.getValue().toSymbol() + entry.getValue().teamName().toLowerCase());


set을 사용하지 않을수 있지 않을까요?

DTO 에서 사용하는 모든 setter 메서드를 제거하였습니다.

phs1116 · 2020-04-15T13:20:11Z

src/main/java/chess/domain/ChessRunner.java

+        BoardScoreDTO currentTeam = new BoardScoreDTO(calculateScore());
+        currentTeam.setTeam(this.currentTeam.name());


굳이 setter를 사용하지 않아도 되지 않을까요? 그냥 생성자를 통해 넘겨줘도 될 것 같네요. :)

phs1116 · 2020-04-15T13:22:20Z

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

+    private ChessBoard chessBoard;
+    private CurrentTeam currentTeam;
+    private PieceOnBoards originalPieces;


상태를 가지고 있는 해당 객체들은 싱글 인스턴스 내 필드로 관리하지 말고 메서드 단위로 생명주기를 짧게 가져가도록 해보면 어떨까요?
해당 서비스는 웹 요청 시 여러 쓰레드를 통해 요청이 들어오기 때문에 동기화 문제가 있을 수 있어요.

chessBoardId 를 이용하여 해당 객체들의 생명 주기를 메서드 단위로 변경하였습니다.

giantim · 2020-04-16T12:46:40Z

화투 안녕하세요! 피드백 주신 내용 반영했습니다. 잘못 이해해서 반영한 부분이 있다면 지적 부탁드립니다!
항상 구체적이고 꼼꼼한 리뷰 감사합니다. 정말 많은 도움이 되고있습니다 :)

phs1116

안녕하세요, 라빈.
피드백 반영 잘해주셨어요!
이번 단계 머지할게요!
간단한 피드백 추가했는데 참고만 한번 부탁드려요!

phs1116 · 2020-04-19T10:33:29Z

src/main/java/chess/controller/WebChessController.java

+            } catch (CustomSQLException e) {
+                model.put("errMessage", e.getMessage());


그냥 모든 예외를 잡아 처리하도록 하여
http://sparkjava.com/documentation#error-handling
해당 기능을 활용했어도 좋았을 것 같네요. :)

phs1116 · 2020-04-19T10:50:51Z

src/main/java/chess/controller/ChessController.java

+        do {
+            command = Command.of(inputView.askGameCommand());
+        } while (isEmptyCommand(command));
+        return command.get();


Optional.get은 직접적으로 사용되지 않는게 좋아요.

phs1116 · 2020-04-19T10:52:46Z

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

+    }
+
+    private void updateChessBoard(int chessBoardId, String source, String target) {
+        PieceOnBoard piece = null;


자료 선언구와 초기화 구는 최대한 같이 해주시는고 참조 변경은 최대한 없애는게 좋을 것 같아요. :)
두개를 선언하게 되더라도요. :)

giantim and others added 30 commits March 24, 2020 14:06

doc: 체스 1단계 기능 요구 사항 정리

67ceea2

feat: 위치 구현

eee6977

feat: 가로 축 구현

a49d949

feat: 보드판 초기화

16267b5

feat: 체스 게임을 시작하는 명령어 입력

9d82c72

refactor: final 키워드 추가

feat: 이동 전략 구현

3dcf3f9

feat: 폰의 이동 전략과 팀별 이동 전략

b0392f3

feat: 말 이동 시 장애물 탐색

08ab69b

- 게임을 진행하는 ChessRunner 클래스 구현

feat: 방향 전략 구현

1d69dbf

feat: 2단계 구현

c6fd4f6

feat: 왕이 죽었을 때 승리한 팀 출력

b757656

feat: 왕이 죽었을 때 게임 종료

202d590

feat: status 명령어 입력 시 점수 계산

622ee0a

feat: 3단계 기능 구현

6a94761

doc: README 업데이트

fdc9cfa

refactor: 세부 네이밍 수정

fb4d0af

refactor: 방향 전략 생성 분기문 삭제

236e066

refactor: 테스트 코드 디렉토리 추가

bf61f63

refactor: 테스트 코드 추가

9d29cfd

refactor: 변경되면 안되는 매개 변수, 클래스에 final 키워드 추가

db9009f

refactor: 컨트롤러의 실행 메서드를 명령어 별로 분리

fd4d688

refactor: DirectionStrategy 의 구현 클래스들의 테스트 코드 추가

6f15464

refactor: 컨트롤러 추상화

31e70ae

refactor: 컨트롤러 추상화(2)

a1f1431

refactor: 1차 수정

7bd04ff

refactor: Dto 추가 / ChessRunner 에서 Dto 에 적합한 값을 넘겨주도록 메서드 수정

8888b08

refactor: 게임 종료 경우에 따라 점수 출력 로직 구현 / 객체 null 반환을 Optional 을 이용하도록 변경 …

08cbd73

…/ 각 체스 말을 초기화 할때의 정보를 enum 으로 구현

Merge branch 'mission1' into home

12de745

Merge pull request #1 from giantim/home

a6b5b67

Home

refactor: 수정한 클래스에 맞게 테스트 코드 추가

bdf1a0a

giantim added 7 commits April 7, 2020 14:50

feat: 플레이어 입력, 게임 방을 위한 뷰 구현

ca89964

feat: 플레이어 정보 저장, 검색, 삭제 기능 구현

352729a

feat: 기존의 게임 정보 불러오기 구현

f6ae741

feat: 게임 목록에서 불러오기 기능 구현(1)

f46bc67

feat: 게임 목록에서 불러오기 기능 구현(2) - 데이터 불러오기 기능 구현

3fef558

feat: 종료 기능 구현

2e747b9

refactor: 디자인 수정

refactor: DAO 의 코드 수정 / 연결하는 DB 변경

6fe417b

phs1116 suggested changes Apr 8, 2020

View reviewed changes

giantim and others added 10 commits April 9, 2020 20:18

refactor: Service Layer 추가 / DAO, DTO 패키지 이동

23b2f34

refactor: DAO 에 try-with-resources 적용

44888f8

refactor: service layer 추가

c21faed

refactor: controller 에 dao 를 이용하는 역할 제거

65e1171

refactor: piece -> pieceOnBoard 모델로 변경

a358f99

refactor: 신규 게임 시작 기능 수정

d1bbe30

refactor: controller 의 기능을 Service layer 로 분리

9530093

refactor: Optional 객체의 null 검사 로직 추가

a3629ac

refactor: 메서드 위치 수정

7cdbc88

Merge pull request #5 from giantim/web-chess

4360c88

Web chess

phs1116 suggested changes Apr 15, 2020

View reviewed changes

giantim added 6 commits April 16, 2020 14:35

refactor: ResultSet, PreparedStatement 닫는 구문 추가

e145ae0

refactor: 체스 게임 화면에 현재 진행중인 게임 id 출력하게 변경

3a7ba78

refactor: 서비스 내의 상태를 갖는 객체의 생명주기를 메서드 단위로 수정

5d5d8c5

refactor: DAO 가 도메인 자료 구조를 사용하도록 변경

df0e3ed

refactor: TileDTO, BoardScoreDTO 의 setter 메서드 삭제

f25d61b

refactor: SQLException 에 대한 Custom RuntimeException 을 만들어서 예외 페이지 출력

5170429

refactor: findFirst 를 Optional 로 래핑한 부분 수정

e5b625f

phs1116 approved these changes Apr 19, 2020

View reviewed changes

phs1116 merged commit 86c76d6 into woowacourse:giantim Apr 19, 2020

		PreparedStatement pstmt = con.prepareStatement(query);
		ResultSet rs = pstmt.executeQuery();

		BoardScoreDTO currentTeam = new BoardScoreDTO(calculateScore());
		currentTeam.setTeam(this.currentTeam.name());

		} catch (CustomSQLException e) {
		model.put("errMessage", e.getMessage());

[라빈] 체스 웹/DB 미션 제출합니다 #133

[라빈] 체스 웹/DB 미션 제출합니다 #133

Conversation

giantim commented Apr 6, 2020

phs1116 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

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

giantim commented Apr 13, 2020

phs1116 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

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

giantim commented Apr 16, 2020

phs1116 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