[3, 4단계 - 체스] 조조(조은별) 미션 제출합니다.#784
Conversation
- Type 클래스 삭제 - 기물 별 타입 체크 메서드 삭제
- PieceDao, PieceDto 삭제
pci2676
left a comment
There was a problem hiding this comment.
전체적으로 깔끔하게 작성해주신것 같습니다.
질문에 대한 답변과 고민해보실만한 부분에 리뷰를 남겨두었으니 참고해주세요.
| public ChessGame(InputView inputView, OutputView outputView) { | ||
| this.inputView = inputView; | ||
| this.outputView = outputView; | ||
| DataBaseConnector dataBaseConnector = new ProductionConnector(); |
There was a problem hiding this comment.
view는 외부에서 생성하는데 db 의존성은 내부에서 생성하고 있습니다.
이 또한 외부로 생성의 책임을 분리해보는건 어떨까요
There was a problem hiding this comment.
상위 레이어인, Application에서 Service와 하위 도메인을 생성하여 주입하도록 수정했습니다!
| this.inputView = inputView; | ||
| this.outputView = outputView; | ||
| DataBaseConnector dataBaseConnector = new ProductionConnector(); | ||
| chessService = new ChessService(new ChessGameDao(dataBaseConnector)); |
There was a problem hiding this comment.
controller와 service의 역할과 책임은 어디까지라고 생각하시나요?
각 계층에서 어디까지 구현 세부사항이 노출되어야 하는지 고민해보시면 좋을것 같습니다.
| CREATE TABLE IF NOT EXISTS `chessgame` | ||
| ( | ||
| `id` BIGINT NOT NULL AUTO_INCREMENT, | ||
| `turn` VARCHAR(8) NOT NULL, | ||
| `pieces` VARCHAR(100) NOT NULL, | ||
| PRIMARY KEY (`id`) | ||
| ); | ||
|
|
||
| USE `chess_test`; | ||
|
|
||
| CREATE TABLE IF NOT EXISTS `chessgame` | ||
| ( | ||
| `id` BIGINT NOT NULL AUTO_INCREMENT, | ||
| `turn` VARCHAR(8) NOT NULL, | ||
| `pieces` VARCHAR(100) NOT NULL, | ||
| PRIMARY KEY (`id`) | ||
| ); |
There was a problem hiding this comment.
아실수도 있지만
보통 테스트용도로 테이블을 별도로 구성하지 않고
DB 자체를 환경별(local, test, dev, prod 등...)로 구성하는 편입니다.
지금 단계에서는 굳이 반영하실 필요는 없으니 참고만 해주시면 됩니다.
| CREATE TABLE IF NOT EXISTS `chessgame` | ||
| ( | ||
| `id` BIGINT NOT NULL AUTO_INCREMENT, | ||
| `turn` VARCHAR(8) NOT NULL, | ||
| `pieces` VARCHAR(100) NOT NULL, | ||
| PRIMARY KEY (`id`) | ||
| ); |
There was a problem hiding this comment.
처음에는 piece 테이블을 따로 빼서, 속성을 구분하여 저장했었습니다.
그러나, 수정 포인트가 많아지고 복잡해져서
CustomBoardFactory를 활용하여 board의 스냅샷을 String으로 저장하는 것이 가장 단순하다고 생각했습니다.그러나 이런 설계 방식이 실무에서 납득할 수 있는 방향일지에 대한 의문은 있습니다.
pieces에 대한 재가공이 어렵기 때문입니다.
설계에 대해 의견이 궁금합니다🤔
애플리케이션이 동작하고 사용하는 사람에게 문제가 없으면 되기 때문에 어떠한 애플리케이션을 설계하는데 있어 방법은 무궁무진 할 수 있을것 같습니다.
데이터를 어떻게 저장할것이냐에 대해서도 동일하다고 봅니다.
그리고 이 영역 또한 경험이 크게 작용하는 부분으로 보입니다.
제가 한가지 요구사항을 더 드려볼테니 이를 대응할 수 있는 데이터 설계를 시도해보시면 좋을것 같습니다.
(애플리케이션의 요구사항이 추가되는 것은 아니니 데이터 저장에 대한 부분만 고려를 해주시면 됩니다.)
요구사항
- 게임이 끝나고 리플레이를 시청 할 수 있어야 한다.
There was a problem hiding this comment.
움직임을 저장할 수 있도록, pieces을 movement 테이블로 분리했습니다.
CREATE TABLE IF NOT EXISTS `chessgame`
(
`id` BIGINT NOT NULL AUTO_INCREMENT,
`turn` VARCHAR(8) NOT NULL,
PRIMARY KEY (`id`)
);
CREATE TABLE IF NOT EXISTS `movement`
(
`id` BIGINT NOT NULL AUTO_INCREMENT,
`pieces` VARCHAR(100) NOT NULL,
`gameId` BIGINT NOT NULL,
PRIMARY KEY (`id`)
);리플레이를 구현하기 위해
move, end(게임이 끝나지 않았다고 봄) 명령어 시행 시, movement 테이블에 정보가 저장됩니다.
예시 :
| id | pieces | gameId |
|---|---|---|
| 1 | RNBQKBNR/PPPPPPPP/......../......../......../......../pppppppp/rnbqkbnr | 1 |
| 2 | RNBQKBNR/PPPPPPPP/......../......../p......./......../.ppppppp/rnbqkbnr | 1 |
| 3 | RNBQKBNR/PPPPPPPP/......../......../p......./......../.ppppppp/rnbqkbnr | 1 |
| 4 | RNBQKBNR/.PPPPPPP/......../P......./p......./......../.ppppppp/rnbqkbnr | 1 |
| 5 | RNBQKBNR/.PPPPPPP/......../P......./pp....../......../..pppppp/rnbqkbnr | 1 |
king이 잡히는 순간, 게임이 종료되며 게임의 정보는 삭제됩니다.
| public final class Winner { | ||
|
|
||
| private Winner() { | ||
| } | ||
|
|
||
| public static WinnerDto of(ColorScoreDto firstColor, ColorScoreDto secondColor) { | ||
| if (firstColor.score() > secondColor.score()) { | ||
| return new WinnerDto(firstColor.color()); | ||
| } | ||
| if (firstColor.score() < secondColor.score()) { | ||
| return new WinnerDto(secondColor.color()); | ||
| } | ||
| return WinnerDto.from(Color.NONE); | ||
| } | ||
| } |
There was a problem hiding this comment.
static한 기능만 제공하는 클래스는 제거해주시고 객체간 협력으로 표현해주시면 좋을것 같습니다.
There was a problem hiding this comment.
Step4에서 사용하지 않는 로직이지만, 객체화 하는 방식으로 리팩터링 했습니다.
킹이 잡히지 않고 끝내는 경우로도 기능을 확장시킬 수 있기 때문에 삭제하지 않았습니다!
Q. 객체 간 협력을 제가 잘 이해한 것이 맞을까요?
Bad : 직접 값 비교 firstColor.score() > secondColor.score()
Better : 객체에게 질문 firstScore.isHigherThan(secondScore)
Q. static 클래스를 지양해야 하는 이유는 무엇인가요?
기존 로직에서는 Winner를 판단 해주는 Util 클래스로 구현했었습니다.
인스턴스 필드가 불필요한 경우엔 Util로 만들면 먼저 떠오르는데, 어떤 문제점이 있나요?
객체로 바라보는 것이 여전히 어려운 것 같습니다..!
| private static final List<Movement> MOVEMENTS = List.of( | ||
| UP_LEFT, DOWN_LEFT, UP_RIGHT, DOWN_RIGHT | ||
| ); | ||
| private static final double POINT = 3; |
There was a problem hiding this comment.
꼭 반영하실 필요는 없는 리뷰입니다.
객체를 설계하다보면 여러가지 요구사항을 충족시킬 수 있는 프로퍼티들이 객체 내에 위치하기 마련입니다.
그러다 보면 객체가 많은 책임을 가지게 됩니다.
모두 객체가 알고 있는 정보로 보이고 객체가 책임져야하는게 당연해 보일 수 있지만 조금 더 멀리서 바라본다면 서로 다른 책임이 하나의 객체에 위치하는 것 처럼 볼 수도 있습니다.
가령 A라는 객체에 a라는 프로퍼티와 이를 사용하는 A 메서드 그리고 b라는 프로퍼티와 이를 사용하는 B메서드가 있다고 가정해보겠습니다.
a와 A는 응집도가 높습니다. 그리고 b와 B 또한 응집도가 높다고 볼 수 있습니다.
하지만 a와 b는 응집도가 높다고 볼 수 있을까요?
추상적으로 바라보았을때 서로 연관이 없는 책임이 하나의 객체에 모두 위치하게 되는 경우가 발생할 수 있으니 한번 쯤 의심해보시는 것을 추천해드립니다.
There was a problem hiding this comment.
서로 연관이 없는 책임이 하나의 객체에 모두 위치하게 되는 경우가 발생할 수 있으니 한번 쯤 의심해보시는 것을 추천
객체를 분리하는 것 또한 항상 고민이 되는 부분이었는데, 좋은 팁이 될 것 같아요👍
a는 movements, b는 point를 표현해주신거군요!
그 둘은 모두 Piece라는 범주로 묶이니 하나의 객체로 표현되어야 할까 싶지만,
movements와 point는 연관이 없으니, 다른 객체로 분리하는 것이 좋은 것 같습니다.
point를 분리하면 기물 타입을 체크해주는 로직이 들어가서, 단순하게 구현하기 위해 Pieces 내에서 처리했습니다.
기물 타입을 체크하는 데 불필요한 코드를 줄이면서 객체를 분리할 수 있는 방법이 있을까요?
적용한다면 PieceMapper enum(==으로 같은 객체인지 확인)과 비슷하게 구현하게 될 것 같습니다.
There was a problem hiding this comment.
point를 분리하면 기물 타입을 체크해주는 로직이 들어가서, 단순하게 구현하기 위해 Pieces 내에서 처리했습니다.
기물 타입을 체크하는 데 불필요한 코드를 줄이면서 객체를 분리할 수 있는 방법이 있을까요?
적용한다면 PieceMapper enum(==으로 같은 객체인지 확인)과 비슷하게 구현하게 될 것 같습니다.
구현방식이야 언제나 그렇듯 여러가지가 있을것 같습니다.
말씀하신대로 enum을 활용하는것도 방법이 될 수 있고
Map을 이용한 일급컬렉션으로 기물과 점수에 대한 관리를 하는 방향도 있을것 같습니다.
| ChessGameDto chessGameDto = new ChessGameDto( | ||
| 0L, | ||
| Color.WHITE.name(), | ||
| "......../K...P.../K...P.../K...P.../K...P.../K...P.../K...P.../K...P..." |
| import java.util.Map; | ||
| import java.util.Map.Entry; | ||
|
|
||
| public final class ScoreCalculator { |
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| public class ChessGameDao { |
There was a problem hiding this comment.
service의 입장에서 dao를 이용해 외부에 데이터를 어떻게 저장할지는 관심사가 아닙니다.
저장하는 방식은 언젠가 바뀔 수 있으니 이를 감안하여 설계해보시는걸 추천드립니다.
There was a problem hiding this comment.
Dao를 인터페이스로 분리하여 Service는 인터페이스만 알도록 했습니다.
같은 맥락으로, Service도 인터페이스로 분리했습니다!
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| public class ChessGameDao { |
There was a problem hiding this comment.
데이터베이스 connection을 이용하는 행위는 다음과 같이 추상화 해볼수 있습니다.
- 쿼리작성
- 쿼리파라미터주입
- 쿼리실행
- 결과추출
이를 추상화 하여 하나의 템플릿으로 제공해보는 것을 도전해보시면 좋을것 같습니다.
There was a problem hiding this comment.
추상 클래스로 DaoTemplate 템플릿을 만들었습니다.
반환값에 따라 메서드를 분리했습니다.
void executeUpdate: 저장, 수정, 삭제<T> Optional<T> executeQueryForSingleData: 단일 조회<T> List<T> executeQueryForMultiData: 복수 조회
최대한 단순하게 만들어보려고 했는데, 최적화할 수 있는 부분이 있다면 말씀해주세요!
구현하면서 해결하지 못한 부분이 있습니다.
MovementDaoImpl 코드 입니다.
getLastId 메서드 내 try-catch문을 빼서 가독성을 높이고 싶습니다.
Q. getLastId에서 ResultSet을 다루는 로직을 DaoTemplate 내부에서 처리할 수 있는 방법이 있을까요?
private Long findLastId() {
String query = "SELECT MAX(id) lastId FROM " + TABLE_NAME;
String errorMessage = "움직임 마지막 id 조회 실패";
return executeQueryForSingleData(query, errorMessage, this::getLastId)
.orElse(AUTO_INCREMENT_DEFAULT);
}
private Long getLastId(ResultSet resultSet) {
try {
return resultSet.getLong("lastId");
} catch (SQLException e) {
throw new RuntimeException("움직임 마지막 id 조회 실패");
}
}There was a problem hiding this comment.
추상 클래스로 DaoTemplate 템플릿을 만들었습니다.
반환값에 따라 메서드를 분리했습니다.
void executeUpdate : 저장, 수정, 삭제
Optional executeQueryForSingleData : 단일 조회
List executeQueryForMultiData : 복수 조회
최대한 단순하게 만들어보려고 했는데, 최적화할 수 있는 부분이 있다면 말씀해주세요!
👍
There was a problem hiding this comment.
MovementDaoImpl 코드 입니다.
getLastId 메서드 내 try-catch문을 빼서 가독성을 높이고 싶습니다.Q. getLastId에서 ResultSet을 다루는 로직을 DaoTemplate 내부에서 처리할 수 있는 방법이 있을까요?
아래와 같은 방법이 있을것 같은데 질문주신 방향과 맞을지 모르겠네요
public interface ResultSetMapper<T>{
T map(ResultSet resultSet) throws SQLException;
} public <T> Optional<T> executeQueryForSingleData(
String query,
String errorMessage,
ResultSetMapper<T> extractData,
Object... parameters
) {
try (Connection connection = getConnection();
PreparedStatement preparedStatement = connection.prepareStatement(query)) {
setParameters(preparedStatement, parameters);
ResultSet resultSet = preparedStatement.executeQuery();
return extractSingle(resultSet, extractData);
} catch (SQLException e) {
System.err.println(e.getMessage());
throw new RuntimeException(errorMessage);
}
}
private <T> Optional<T> extractSingle(ResultSet resultSet, ResultSetMapper<T> resultSetMapper)
throws SQLException {
if (resultSet.next()) {
return Optional.of(resultSetMapper.map(resultSet));
}
return Optional.empty();
} private Long findLastId() {
String query = "SELECT MAX(id) lastId FROM " + TABLE_NAME;
String errorMessage = "움직임 마지막 id 조회 실패";
return executeQueryForSingleData(
query,
errorMessage,
(resultSet) -> resultSet.getLong("lastId")
)
.orElse(AUTO_INCREMENT_DEFAULT);
}There was a problem hiding this comment.
함수형 인터페이스를 만들어서 throw 하면 해결되는 문제였군요!!
반영해보았습니다.
감사합니다👍
|
비밥, 안녕하세요! 지난 리뷰들을 전부 반영하지는 못했지만, 수정 사항과 질문을 답글로 달아두었습니다.
🤔 주요 수정 사항1. 리플레이를 고려한 DB 스키마 변경
2. DaoTemplate 템플릿 정의반환값에 따라 메서드를 분리했습니다.
|
| CREATE TABLE IF NOT EXISTS `chessgame` | ||
| ( | ||
| `id` BIGINT NOT NULL AUTO_INCREMENT, | ||
| `turn` VARCHAR(8) NOT NULL, | ||
| PRIMARY KEY (`id`) | ||
| ); | ||
|
|
||
| CREATE TABLE IF NOT EXISTS `movement` | ||
| ( | ||
| `id` BIGINT NOT NULL AUTO_INCREMENT, | ||
| `pieces` VARCHAR(100) NOT NULL, | ||
| `gameId` BIGINT NOT NULL, | ||
| PRIMARY KEY (`id`) | ||
| ); |
There was a problem hiding this comment.
데이터를 저장하는 방법은 무궁무진하다고 표현드린것 처럼 다음과 같이 표현하는 방식도 있습니다. 참고만해주세요.
지금 데이터 구조에서 조금 아쉬운 점은 데이터를 저장할 때 view의 구체적인 내용이 chessGame에 노출된다는 것입니다.
만약 체스가 아닌 다른 게임이라고 가정하고, 그 게임에서는 position의 개수가 무궁무진해질 수 있다면 지금 구조로도 잘 대응할 수 있을까요?
모든 정보를 저장하는 것이 오히려 독이 될 수도 있습니다.
따라서 체스 게임이 시작되고 수행된 모든 명령어 이력을 저장하는 것도 하나의 방법이 될 수 있습니다.
DB에는 명령어1, 명령어2, 명령어3 .... 명령어n 이 순차적으로 저장된다면
게임 1의 명령어만 전부 다시 불러와서 명령어를 순서대로 chessGame에게 재수행 시켜주면 원상태로 복구가 가능해지고 리플레이도 가능해질 수 있습니다.
| import chess.dto.mapper.ColorMapper; | ||
| import chess.model.material.Color; | ||
|
|
||
| public record WinnerDto(String winner) { |
There was a problem hiding this comment.
Q. 객체 간 협력을 제가 잘 이해한 것이 맞을까요?
Bad : 직접 값 비교 firstColor.score() > secondColor.score()
Better : 객체에게 질문 firstScore.isHigherThan(secondScore)
네 직접 값을 get해와서 비교하는 것보다 값을 알고있는 객체들간 협력을 통해 필요한 정보만을 얻는 것이 좋아보입니다.
Q. static 클래스를 지양해야 하는 이유는 무엇인가요?
기존 로직에서는 Winner를 판단 해주는 Util 클래스로 구현했었습니다.
인스턴스 필드가 불필요한 경우엔 Util로 만들면 먼저 떠오르는데, 어떤 문제점이 있나요?
객체로 바라보는 것이 여전히 어려운 것 같습니다..!
보통 이러한 경우 무분별한 getter가 생성되고 사용됩니다. 이는 곧 객체로 프로퍼티를 추상화한 이유가 없게 됩니다.
Winner를 구분하는게 Util성 기능이라 보이지는 않고 체스 애플리케이션에서 중요한 부분을 차지하는 도메인 로직으로 보입니다.
There was a problem hiding this comment.
무분별한 getter가 생성되고 사용됩니다.
그렇네요..!
객체로 바라보는 연습을 더 많이 해야 할 것 같습니다.
답변 주셔서 감사합니다😊
| private static final List<Movement> MOVEMENTS = List.of( | ||
| UP_LEFT, DOWN_LEFT, UP_RIGHT, DOWN_RIGHT | ||
| ); | ||
| private static final double POINT = 3; |
There was a problem hiding this comment.
point를 분리하면 기물 타입을 체크해주는 로직이 들어가서, 단순하게 구현하기 위해 Pieces 내에서 처리했습니다.
기물 타입을 체크하는 데 불필요한 코드를 줄이면서 객체를 분리할 수 있는 방법이 있을까요?
적용한다면 PieceMapper enum(==으로 같은 객체인지 확인)과 비슷하게 구현하게 될 것 같습니다.
구현방식이야 언제나 그렇듯 여러가지가 있을것 같습니다.
말씀하신대로 enum을 활용하는것도 방법이 될 수 있고
Map을 이용한 일급컬렉션으로 기물과 점수에 대한 관리를 하는 방향도 있을것 같습니다.
pci2676
left a comment
There was a problem hiding this comment.
체스를 포함해 레벨 1 수행하느라 고생 많으셨습니다.
머지하겠습니다. 🚀
즐거운 방학 보내세요.

비밥, 안녕하세요😃
조조입니다 !
3, 4단계를 순차적으로 구현 후 한번에 리뷰 요청드립니다.
커밋에 시행착오가 조금 많아서 깔끔하지 못한 점 양해 부탁드립니다..!
더 빨리 리뷰 요청을 드리고 싶었는데,
기존 구조를 가져가면서 확장시키는 것이 어려워서 시간이 좀 걸렸던 것 같아요.
DB 실행하는 방법은 README에 적어두었습니다.
따로 sql문을 실행 할 필요 없이, 도커만 실행하면 생성됩니다. 참고 부탁드려요!
지난번에 주신 리뷰는 많이 반영하지 못 했어요. 추후 혹은 미션이 끝나더라도 꼭 적용해보겠습니다!
+) 몇 가지 요구사항을 만족하지 못한 부분들이 있습니다. 우선은 TODO로 남겨두고 수정해보겠습니다.
🤔 코드를 작성할 때 신경 쓴 점
1. 승패 및 점수 로직
다음과 같은 기준으로 로직을 정의했습니다.
King이 잡히지 않고status명령어 입력 시 ->점수만 출력move명령어 실행 후King이 잡히는 경우 -> 게임 종료 ->점수와승패출력King이 잡히지 않고end명령어 입력 시 -> 게임 종료 ->점수만 출력2. 도메인 변경을 최소화 한 DB 스키마
스키마:
데이터 예시:
[ { "id": 1, "turn": "BLACK", "pieces": "RNBQKBNR/PPPPPPPP/......../......../p......./......../.ppppppp/rnbqkbnr" } ]처음에는 piece 테이블을 따로 빼서, 속성을 구분하여 저장했었습니다.
그러나, 수정 포인트가 많아지고 복잡해져서
CustomBoardFactory를 활용하여board의 스냅샷을 String으로 저장하는 것이 가장 단순하다고 생각했습니다.그러나 이런 설계 방식이 실무에서 납득할 수 있는 방향일지에 대한 의문은 있습니다.
pieces에 대한 재가공이 어렵기 때문입니다.
설계에 대해 의견이 궁금합니다🤔
3. 프로덕션 DB, 테스트 DB 분리
테스트 코드를 여러 번 실행했을 때 실패하는 문제가 생겨서
TRUNCATE키워드로 데이터를 삭제하는 방식으로 테스트 코드를 작성했습니다.그러다보니, 실제 DB 데이터도 삭제될 위험이 있어 DB를 분리했습니다.