[3, 4단계 - 체스] 레디(최동근) 미션 제출합니다. #785
Conversation
src/main/java/db/Repository.java
Outdated
| if (unsaved(findTurn, findMoving)) { | ||
| restore(findTurn, findMoving, board); | ||
| } |
There was a problem hiding this comment.
현재는 기보와 보드+턴의 역할이 애매한 부분들이 보이네요.
보드+턴이 게임을 불러오는 친구라면, 이 친구들이 그런 일을 전임해야겠죠?
기보의 역할과 보드+턴의 역할을 명확히 구분해보는 것도 좋겠네요 😃
| @@ -0,0 +1,19 @@ | |||
| package db.connection; | |||
|
|
|||
| public enum ConnectionConst { | |||
There was a problem hiding this comment.
Enum이 왜 생겨났고 어떨 때 사용하는 것이 좋은지, 일반 상수랑 어떤 차이가 있는지 한 번 알아보시면 좋을 것 같네요 😉
| //then | ||
| assertThatThrownBy(() -> pieces.put(F4, piece)) | ||
| .isInstanceOf(UnsupportedOperationException.class); | ||
| } |
There was a problem hiding this comment.
TDD로 구현했다면, Board에 대한 단위 테스트가 어디에 존재해야할까요?
만약 ChessGameTest 내부 로직이 달라진다면, 해당 로직의 단위테스트는 존재한다고 확신할 수 있을까요?
| ### [기능 요구 사항 바로가기](https://github.com/reddevilmidzy/java-chess/blob/62e632aa09435db4f69502e663a0cc7ef78a31bb/docs/README.md) | ||
|
|
||
| <br> | ||
|
|
||
| ### [product db 스키마](https://github.com/reddevilmidzy/java-chess/blob/0f6324e83c0fef95c84bd95c5dba8df742a8b613/src/main/resources0) |
There was a problem hiding this comment.
기능 요구 사항 내의 스키마와 product db 스키마는 중복이라고 볼 수 있을까요?
레디는 어떻게 생각하시나요?
build.gradle
Outdated
| @@ -13,6 +13,7 @@ dependencies { | |||
| testImplementation platform('org.assertj:assertj-bom:3.25.1') | |||
| testImplementation('org.junit.jupiter:junit-jupiter') | |||
| testImplementation('org.assertj:assertj-core') | |||
| runtimeOnly("com.mysql:mysql-connector-j:8.3.0") | |||
There was a problem hiding this comment.
runtimeOnly란 무엇인가요? dependencies내의 해당 위치에 선언해준 이유가 있을까요?
src/main/java/db/BoardDao.java
Outdated
| result.put(positionDto, piece); | ||
| } | ||
| if (result.isEmpty()) { | ||
| return BoardDto.from(Board.create()); |
There was a problem hiding this comment.
dao에서 도메인을 알게하지 않으려고 dto를 보내고 사용하고 있는 것 같은데
여기서 갑작스레 도메인인 Board를 사용하는 것이 레디의 의도일까요?
There was a problem hiding this comment.
import 문을 확인해봐도 model의 Board가 갑툭튀군요...!!ㅎㅎㅎ 의도하지 않았던 부분입니다 변경하였습니다!!
src/main/java/db/MovingDao.java
Outdated
| public List<MovingDto> findAll() { | ||
| final String query = "SELECT * FROM moving"; | ||
| try (final Connection connection = DBConnectionUtil.getConnection(database); | ||
| final PreparedStatement preparedStatement = connection.prepareStatement(query)) { | ||
| final ResultSet resultSet = preparedStatement.executeQuery(); | ||
| return convert(resultSet); | ||
| } catch (SQLException exception) { | ||
| throw new DaoException(ErrorCode.FAIL_FIND); | ||
| } | ||
| } | ||
|
|
||
| private List<MovingDto> convert(final ResultSet resultSet) throws SQLException { | ||
| final List<MovingDto> moving = new ArrayList<>(); | ||
| while (resultSet.next()) { | ||
| String camp = resultSet.getString("camp"); | ||
| String current = resultSet.getString("start"); | ||
| String next = resultSet.getString("destination"); | ||
| moving.add(new MovingDto(camp, current, next)); | ||
| } | ||
| return moving; | ||
| } | ||
|
|
||
| public int countMoving() { |
There was a problem hiding this comment.
findAll[대상생략] / count[대상명시]
네이밍 기준이 좀 다르네요!
src/main/java/db/Repository.java
Outdated
| private boolean unsaved(final TurnDto findTurn, final List<MovingDto> findMoving) { | ||
| return findTurn.count() < findMoving.size(); | ||
| } |
There was a problem hiding this comment.
이런 부분은 변수로 빼도 될 것 같은데, 메서드로 뺀 이유가 있을까요?
src/main/java/db/Repository.java
Outdated
| private Camp findLastTurnCamp(final List<MovingDto> findMoving) { | ||
| if (findMoving.size() % 2 == 0) { | ||
| return Camp.WHITE; | ||
| } | ||
| return Camp.BLACK; | ||
| } |
src/main/java/model/piece/Piece.java
Outdated
| @@ -30,6 +30,14 @@ public boolean isSameCamp(final Camp target) { | |||
| return camp == target; | |||
| } | |||
|
|
|||
| public boolean isPawn() { | |||
| return Objects.equals(getClass(), WhitePawn.class) || Objects.equals(getClass(), BlackPawn.class); | |||
There was a problem hiding this comment.
결국 Entry<Position, Piece> entry로 부모 객체로 묶었지만, 자식 객체임을 알아야만 하는 상황은 동일하네요.
기존에 instanceof를 사용하여, Piece에서 자식객체를 알아야하는 부분을
변경하여 Piece로 사용하는 곳에서 각각 King과 Pawn을 알아야 하는 부분으로 살짝 방향은 틀었지만 마찬가지로 부모객체로 충분히 할 수 없는 자식객체들을 편의상 부모객체로 치환하여 사용했었다라고 해석할 수 있을 것 같아요.
수정을 하진 않더라도, 이런 부분을 레디는 어떻게 생각하는지 어떻게 해야 좋을지 고민해보는 것은 좋을 것 같아요 😉
| create table board | ||
| ( | ||
| position varchar(2) not null, | ||
| piece_type varchar(6) not null, | ||
| camp varchar(5) not null | ||
|
|
||
| ); | ||
|
|
||
| drop table if exists moving; | ||
|
|
||
| create table moving | ||
| ( | ||
| movement_id INT primary key auto_increment, | ||
| camp varchar(5) not null, | ||
| start varchar(2) not null, | ||
| destination varchar(2) not null | ||
| ); |
There was a problem hiding this comment.
INT가 혼자 톡 튀어 나와 있군요😅
일관성을 지키는게 중요하단걸 알고는 있지만 일관되게 지키는건 역시 어렵네요..
| create table turn | ||
| ( | ||
| camp varchar(5) not null, | ||
| count int not null | ||
| ); |
There was a problem hiding this comment.
오 좋은 질문인거 같아요
저장시점이 동일하여 함께 움직이긴 하지만 board는 체스판에 있는 기물 정보를 가지고 있는 테이블이고, turn은 체스 게임의 진행 횟수, 마지막 턴을 저장하고 있는 테이블입니다!
이 둘을 한번에 묶는 형태가 어색하다고 생각하여 두 테이블을 분리하였습니다
src/main/java/db/BoardDao.java
Outdated
| result.put(positionDto, piece); | ||
| } | ||
| if (result.isEmpty()) { | ||
| return BoardDto.from(Board.create()); |
There was a problem hiding this comment.
import 문을 확인해봐도 model의 Board가 갑툭튀군요...!!ㅎㅎㅎ 의도하지 않았던 부분입니다 변경하였습니다!!
src/main/java/db/MovingDao.java
Outdated
| public List<MovingDto> findAll() { | ||
| final String query = "SELECT * FROM moving"; | ||
| try (final Connection connection = DBConnectionUtil.getConnection(database); | ||
| final PreparedStatement preparedStatement = connection.prepareStatement(query)) { | ||
| final ResultSet resultSet = preparedStatement.executeQuery(); | ||
| return convert(resultSet); | ||
| } catch (SQLException exception) { | ||
| throw new DaoException(ErrorCode.FAIL_FIND); | ||
| } | ||
| } | ||
|
|
||
| private List<MovingDto> convert(final ResultSet resultSet) throws SQLException { | ||
| final List<MovingDto> moving = new ArrayList<>(); | ||
| while (resultSet.next()) { | ||
| String camp = resultSet.getString("camp"); | ||
| String current = resultSet.getString("start"); | ||
| String next = resultSet.getString("destination"); | ||
| moving.add(new MovingDto(camp, current, next)); | ||
| } | ||
| return moving; | ||
| } | ||
|
|
||
| public int countMoving() { |
| create table turn | ||
| ( | ||
| camp varchar(5) not null, | ||
| count int not null | ||
| ); |
There was a problem hiding this comment.
라고 생각했었는데 점심먹고 오니 둘이 매번 같이 다니고 있어 한번에 묶는게 합리적이라고 생각해 묶기로 하고 코드를 짜보았었습니다.
하지만!! 코드를 짜다보니 오히려 분리하는게 더 나은 거 같아 합치지 않았습니다..!
board는 남아있는 체스기물을 저장하고 있어 여러 row가 있지만 turn은 마지막 차례와 게임 진행 횟수를 저장하고 있어 하나의 row 밖에 없습니다
이렇게 봤을 때 이 두 테이블을 야무지게 합치는 방법이 떠오르지 않아 그냥 현 상태를 유지하기로 하였습니다..!
|
|
||
| public static Piece findValue(final Camp camp, final String type) { | ||
| return Arrays.stream(values()) | ||
| .filter(pieceType -> pieceType.pieceName.equals(type)) |
There was a problem hiding this comment.
클린코드의 디미터 법칙 부분을 읽은 뒤 반영해보았습니다!
findValue(final Piece piece) 메서드는 디미터 법칙을 어기고 있는 거 같아 Map을 만든 다음 값을 꺼내오는 방법으로 view/message/PieceType의 from 메서드와 동일한 방식을 사용하였고, findValue(final Camp camp, final String type) 메서드는 자신의 인스턴스 변수의 메서드를 한번 호출하는 거라고 보아 동일한 형태를 유지하였습니다!
step1 때에도 피드백 남겨주신 부분이었는데 이제야 반영해보네요 😅
| //then | ||
| assertThatThrownBy(() -> pieces.put(F4, piece)) | ||
| .isInstanceOf(UnsupportedOperationException.class); | ||
| } |
There was a problem hiding this comment.
우왓 이렇게 질문하시니 Borad 요 녀석도 validateKing을 해줘야 겠군요💪
| //then | ||
| assertThatThrownBy(() -> pieces.put(F4, piece)) | ||
| .isInstanceOf(UnsupportedOperationException.class); | ||
| } |
There was a problem hiding this comment.
테스트를 추가할 때는 다른 기물은 없는 상태에서 킹과 킹을 잡을 상대방 기물만 만들어 놓은 다음 테스트를 진행하였습니다
final Map<Position, Piece> pieces = Map.of(
E8, new King(Camp.BLACK),
E2, new Queen(Camp.WHITE));final Map<Position, Piece> pieces = new HashMap<>();
pieces.put(E8, new King(Camp.BLACK));
pieces.put(E2, new Queen(Camp.WHITE));이 두가지 중에 고민하였는데 Map.of 를 사용하여 불변 객체를 만드는 것이 좋을 거 같다고 사용했습니다. 그러면 이제 생성자로 넣을 때 객체의 연관을 끊어주기 위해 Board 생성자 안에서 new HashMap으로 한번 감싼 다음 생성을 하는 방향으로 변경했습니다!
| public Map<Position, Piece> getPieces() { | ||
| return pieces; | ||
| return Collections.unmodifiableMap(pieces); |
There was a problem hiding this comment.
불변 객체로 만들어 주는 메서드였습니다! 즉 조회만 가능한 컬렉션이 되고 수정이 일어날 시 UnsupportedOperationException 이 발생합니다!
.of() 를 사용하여도 불변 객체로 만들어 줍니다!
| import java.sql.DriverManager; | ||
| import java.sql.SQLException; | ||
|
|
||
| public class DBConnectionUtil { |
There was a problem hiding this comment.
Util적인 느낌이 있어 명시하기 위해 추가하였습니다 !
| private final InputView inputView; | ||
| private final OutputView outputView; | ||
| private final Repository repository = new Repository("chess"); |
There was a problem hiding this comment.
프로그래밍 요구 사항을 처음부터 지키려 해봤다면, 지금과 구조가 달라질 수도 있었겠죠?
그냥 지켜야겠다보단 왜 이런 요구사항들이 나왔을지 배경을 살펴보면 또 다르게 보일 수 있을 거에요 😃
| import java.sql.DriverManager; | ||
| import java.sql.SQLException; | ||
|
|
||
| public class DBConnectionUtil { |
There was a problem hiding this comment.
Util 적인 느낌이 무엇일까요 😉
Connection을 만들어주는 Factory 느낌이라고도 볼 수 있겠군요! 느낌은 조금 추상적인 말인 것 같아요!
| create table turn | ||
| ( | ||
| camp varchar(5) not null, | ||
| count int not null | ||
| ); |
There was a problem hiding this comment.
ㅎㅎ 맞습니다! 심심하시면 DB의 정규화와 역정규화 반정규화들도 알아보면 좋겠죠 😃
| final Board board = chessGame.getBoard(); | ||
| final Camp camp = chessGame.getCamp(); | ||
| final Turn turn = chessGame.getTurn(); | ||
| final BoardDto boardDto = BoardDto.from(board); | ||
| final TurnDto turnDto = TurnDto.from(camp, turn); | ||
| repository.save(boardDto, turnDto); |
There was a problem hiding this comment.
getter 사용을 매우 지양하고 있군요.
getter를 지양해야 하는 이유에 대해 조금 더 딥하게 알아볼까요?
| runtimeOnly("com.mysql:mysql-connector-j:8.3.0") | ||
| testImplementation platform('org.junit:junit-bom:5.9.1') |
There was a problem hiding this comment.
개행을 하는 것도 구분해줄 수 있는 방법이기도 합니다 😃
가독성있고 직관적인 의존성 관리도 좋죠!
| } | ||
|
|
||
| public static BoardDto create() { | ||
| return from(Board.create()); |
| public record PositionDto(String value) { | ||
|
|
||
| public static PositionDto from(final Position position) { | ||
| return new PositionDto(position.toString()); |
There was a problem hiding this comment.
다른 개발자가 디버깅을 한다고 toString을 맘대로 변경해버리면 어떻게 될까요?
용도에 맞는 메서드를 사용해줄까요!?
| private final float blackScore; | ||
| private final float whiteScore; |
There was a problem hiding this comment.
팀이 Black -> Yellow로 바뀌면 Score Dto도 바뀌어야하는군요!
| .toList(); | ||
| } | ||
|
|
||
| private Score duplicateFilePawns(final Map<File, Integer> count) { |
There was a problem hiding this comment.
duplicateFilePawns라는 메서드가 Score를 줄 것이라고 짐작할 수 있나요?
| @@ -5,6 +5,13 @@ public enum Camp { | |||
| BLACK, | |||
| WHITE; | |||
|
|
|||
| public static Camp calculateTurn(final int turn) { | |||
안녕하세요 제이미!!
db가 생기면서 구상해야 할게 한차원 더 생긴 느낌이라 조금 오래 걸렸네요..
고심하여 구현한 기능
현재 프로그램에서 킹이 잡히는 건 불가능하게 구현하였습니다.
다시 말해 킹이 보드에서 사라지지 않도록 구현되어 있고, 만약 킹이 잡히게 된다면 게임 종료 메시지와 함께 프로그램도 종료됩니다.
킹을 잡을 수 없게 설계한 이유는 체스 룰에 따랐습니다.
사용자가 move 명령어를 입력할 때마다 기보(Moving 테이블)를 저장하기 위해 insert 쿼리문이 나갑니다.
사용자가 end 명령어를 입력하면 그때 현재 보드(Board 테이블)와 턴(Turn 테이블)을 저장합니다.
그래서 만약 사용자가 도중에 프로그램을 종료하게 된다면 기보는 저장되어 있지만 보드와 턴은 저장되지 않습니다.
이후 다시 시작시에는 저장된 기보를 바탕으로 보드를 불러옵니다.
아쉬운 부분
현재 mySql DB를 사용하고 있는데 만약 DB를 변경하게 된다고 하면 싹다 수정되어야 할정도로 의존성이 강합니다. 요 부분을 해결하지 못한게 아쉽네요😣
리뷰 감사합니다!!