[3, 4단계 - 체스] 몰리(김지민) 미션 제출합니다. #791
Conversation
… 내부로 이동 - Policy의 isSatisfied메서드 인자를 적 유무 여부에서 타겟 Piece로 수정
…ghtObstacleFinder 구현
- obstaclefinder 패키지 명을 변경된 인터페이스 명에 맞게 변경 - 깃 충돌로 남아있던 attacker 삭제 및 내부 구현체 이동
There was a problem hiding this comment.
안녕하세요 현구막 🙌
3단계 리뷰 재요청 드립니다 ㅎㅎ
상태 패턴
지난 리뷰 때 현구막께서 상태패턴을 추천해주셔서 반영을 해보았습니다..!
상태 패턴을 도입하다보니 자연스래 게임 진행과 보드 관리에 대한 부분이 분리가 되어서 상태패턴의 장점을 느낄 수 있었던 것 같아요 😆
Attacker 관련
기존에 Attacker 인터페이스를 동일한 공격 정책(?)을 가진 기물들이 상속을 받도록 구현을 했는데요..!
현구막에게 설계에 대해서 질문을 드리고 다시 돌아보니… 제가 봐도 이해하기 어려운 코드 같더라구요…! 🥲
그래서 아래처럼 변경을 시도해보았습니다..!
Attacker라는 이름을 가진 인터페이스가 findObstacle이라는 책임을 수행하는 게 낯설어보일 수 있겠다는 생각이 들었어요.
또, 구현체들(DirectionAttackPiece,DiagonalAttackPiece)의 이름 또한 의미전달이 잘되지 않는 것 같다고 느꼈어요...!
- Attacker를 ObstacleFinder로 이름을 변경하고,
extends Piece implements Attacker를 구현했던 객체들 대신, 다음처럼 �사용하도록 했어요
- Pawn, Knight처럼 장애물 판단 로직이 다른 객체들에 대한 PawnObstacleFinder, KnightObstacleFinder를 구현
- 이동 방향으로 장애물을 찾는 나머지 객체들은 MovementDirectionObstacleFinder라는 객체를 사용하도록 함
기존 Piece의 이동 판단 로직에 대해서 생각을 해보았어요. 상위 클래스에서 전부 처리하는 것보다는 각 기물들이 자신이 가지고 있는 ObstacleFinder를 이용하여 장애물 목록을 받게하면 책임을 나눌 수 있지 않을까 하는 생각을 했어요..!
- Composition을 사용해 수정하여, 각 기물들이 생성 시에 자신에게 맞는 ObstacleFinder를 생성하도록 했어요.
- 이동 판단 시에는 장애물 목록을 각자의 규칙에 맞게 생성하여 반환하도록 하였습니다...!
설계쪽 수정이 잦아서 이해하기 힘드실 수 있을 것 같아요 😭
이번 리뷰도 잘 부탁드립니다... 🫡
| public class ChessGameController { | ||
|
|
||
| private final OutputView outputView; | ||
| private final InputView inputView; | ||
| private final ChessGameService gameService; |
There was a problem hiding this comment.
비즈니스 로직은 각각의 비즈니스 객체(domain 계층)가 수행할 수 있는 걸로 보여요.
그래야 비즈니스 변경 요구사항이 전달되었을 때 controller, service 패키지를 열어볼 필요 없이
domain 패키지 탐색을 시작으로 빠르게 수정할 대상을 찾아낼 수 있기 때문이에요.
(controller 가 view 와 domain 의 소통에만 집중해야하는 것과 같은 이유)
결국 service 계층은 2번처럼 비즈니스 로직과 영속 계층사이 소통만 담당하는 것이 이상적이겠네요!
| public interface TurnDao { | ||
|
|
||
| void save(final TurnDto turnDto); | ||
|
|
||
| List<TurnDto> findAll(); | ||
|
|
||
| void deleteAll(); |
There was a problem hiding this comment.
인터페이스가 DAO(DataAccessObject) 라는 명칭을 꼭 알 필요가 있을까요? 🤔
구현체가 어떤 형상을 가졌는지 노출되는 것 같아요.
There was a problem hiding this comment.
저는 일단 TurnDao 클래스만을 사용하다가, 해당 클래스의 테스트를 위해 FakeTurnDao를 추가하려고 하면서 인터페이스를 하게 되었는데요..!
즉, DAO를 추상화 하려는 의도로 인터페이스를 생성했기 때문에 인터페이스의 이름을 DAO로 작성을 했습니다...!
그런 DAO의 구현체이니 당연히 DAO라는 것을 알아도 괜찮지 않을까 라는 생각입니다...!
그런데, 사실 제가 현구막의 질문을 잘 이해하지 못한 것 같아요...🥲
제가 답변한 내용이 현구막의 의도한 부분이 맞을까요...??
src/main/java/chess/domain/piece/obstaclefinder/KnightObstacleFinder.java
Show resolved
Hide resolved
… 반환하도록 수정 - TurnDto 제거
…y에게 변경 - PieceDao에서 피스 조회 시 Map<Position, Piece>를 반환하도록 변경
- docker-compose.yml에도 관련 유저정보 추가
| public class ChessGameController { | ||
|
|
||
| private final OutputView outputView; | ||
| private final InputView inputView; | ||
| private final ChessGameService gameService; |
There was a problem hiding this comment.
넵넵 그동안 service 계층에 대한 오해가 조금 있었던 것 같네요 🤣
비즈니스 로직은 각각의 비즈니스 객체가 수행한다! 명심하겠습니다 ㅎㅎ
감사합니다 현구막!
|
|
||
| private static final String SERVER = "localhost:13306"; | ||
| private static final String DATABASE = "chess"; | ||
| private static final String OPTION = "?useSSL=false&allowPublicKeyRetrieval=true&serverTimezone=Asia/Seoul"; | ||
| private static final String USERNAME = "user"; | ||
| private static final String PASSWORD = "password"; | ||
|
|
There was a problem hiding this comment.
root로 접속하게 되면 모든 데이터베이스에 대한 권한을 가지고 있기 때문에 위험할 수 있다. (실수로 삭제를 한다던가 하는 등)
때문에 해당 DB를 관리하는 유저를 하나 생성하고 그 유저만 해당 DB에 접근하여 관리하도록 하는 것이 안전하다라는 이유를 들을 수 있었습니다!
👍
| public interface TurnDao { | ||
|
|
||
| void save(final String color); | ||
|
|
||
| Optional<Color> findAny(); | ||
|
|
||
| void deleteAll(); | ||
| } |
There was a problem hiding this comment.
DAO(DataAccessObject) 는 데이터베이스 혹은 데이터 영속을 관리하는 구현체를 표현하는 객체라는 뜻인데,
FakeTurnDao 의 경우는 실제 구현 내용이 영속화와 관련되어 있지 않고,
HashMap 을 이용해 영속행위를 묘사하고 있는 것 같아요.
때문에 DAO 대신 패키지 명처럼 저장소(Repository) 라는 명칭을 사용해보는 건 어떨까? 하는 의도의 코멘트였어요! 😄
There was a problem hiding this comment.
인터페이스를 설계하는 과정에 대해서는 짚고 넘어가고 싶은데요,
인터페이스를 설계하는 관점은 구현체가 아니라 사용처가 되어야 합니다.
- DAO 구현체를 추상화 하기 위해 인터페이스를 사용한다 -> X
- service 계층에서 영속화된 정보를 가져오기 위해 인터페이스를 설계하고, 그에 맞춰 DAO 로 인터페이스 설계 사항을 구현한다. -> O
인터페이스가 구현체의 관점에서 명세되면, 구현체가 변경될 때 인터페이스도 변경되고,
그 인터페이스를 의존하는 사용처도 함께 변경되어야 하기 떄문이에요.
그렇다면 인터페이스를 사용하는 의미가 희석되겠죠..!
There was a problem hiding this comment.
service 계층에서 영속화된 정보를 가져오기 위해 인터페이스를 설계하고, 그에 맞춰 DAO 로 인터페이스 설계 사항을 구현한다.
아하 넵넵 이해했습니다! ㅎㅎ
반영하겠습니다! 🙇
| public Map<Position, Piece> findAllPiece() { | ||
| return pieceDao.findAllPiece(); | ||
| } | ||
|
|
||
| public Optional<Color> findTurn() { | ||
| return turnDao.findAny(); | ||
| } | ||
|
|
||
| public void saveGame(final Game game) { | ||
| delete(); | ||
| saveAllPiece(game.getBoard()); | ||
| saveTurn(game.getTurn()); | ||
| } | ||
|
|
||
| private void saveAllPiece(final Map<Position, Piece> pieces) { | ||
| pieces.entrySet() | ||
| .forEach(entry -> pieceDao.save(entry.getValue(), entry.getKey())); | ||
| } | ||
|
|
||
| private void saveTurn(final Color color) { | ||
| turnDao.save(color.name()); | ||
| } | ||
|
|
||
| public void delete() { | ||
| pieceDao.deleteAll(); | ||
| turnDao.deleteAll(); | ||
| } |
There was a problem hiding this comment.
service 계층이 persistence 계층에 명령을 내리는 것만 깔끔하게 집중하네요 😄 👍
| try (final var connection = getConnection(); | ||
| final var preparedStatement = connection.prepareStatement(query)) { | ||
| preparedStatement.setString(1, piece.getColor().name()); | ||
| preparedStatement.setString(2, piece.getPieceType().name()); | ||
| preparedStatement.setInt(3, position.rank().getValue()); | ||
| preparedStatement.setInt(4, position.file().getValue()); | ||
| preparedStatement.executeUpdate(); | ||
| } catch (final SQLException e) { | ||
| throw new RuntimeException("[METHOD] save [TABLE] piece" + e.getMessage(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
때문에 3번을 선택하여 '처리가 필수이지 않은 RuntimeException(UncheckedException)을 사용하여 예외가 발생했음을 알리고 해당 예외가 SQLException인지를 �출력한다면 괜찮지 않을까' 라고 생각했습니다!
👍 👍 💯
SQL Exception 이 checked exception 을 날린다는 사실을 망각하고 잘못된 코멘트를 드렸어요 🥲 🙏
개떡같은 코멘트를 찰떡같이 해석해주셨네요.. 👍
영속 프레임워크 내부에서도 SQLException 을 감싸서 다시 RuntimeException 으로 던져주곤 하더라구요.
감싸주신 의도가 궁금했는데, 고민 과정까지 설명해주셔서 바로 이해되었어요 👍
There was a problem hiding this comment.
저도 한번 더 고민하고 정리해볼 수 있어서 좋았습니다 👍🙇
| CREATE TABLE Turn | ||
| ( | ||
| turn_id INT AUTO_INCREMENT PRIMARY KEY, | ||
| turn VARCHAR(255) NOT NULL | ||
| ); | ||
|
|
||
| CREATE TABLE Piece | ||
| ( | ||
| piece_id INT AUTO_INCREMENT PRIMARY KEY, | ||
| color VARCHAR(255) NOT NULL, | ||
| pieceType VARCHAR(255) NOT NULL, | ||
| `rank` INT NOT NULL, | ||
| file INT NOT NULL | ||
| ); |
There was a problem hiding this comment.
현재는 게임의 턴과 기물별 위치를 저장하고 있는데요,
그 외에도 게임 명령어가 어떻게 입력되었는지를 적재하는 방식도 있을 것 같아요.
몰리가 선택해주신 방법과 위 방법은 각각 어떤 장단점이 있고, 어떤 상황에 적용이 적합할까요?
혹은 두 가지 모두 저장하는 일도 있을까요?
There was a problem hiding this comment.
제가 생각하기에는 다음과 같습니다.
[게임의 턴과 기물별 위치만 저장하는 방식]
- 장점
- 현재 상태만 저장하기 때문에 관리해야 하는 데이터의 양이 적다.
- 바로 데이터를 불러와서 시작할 수 있어 초기 시작 시간이 짧다.
- 단점
- 어떤 차례를 통해 현재 상태가 되었는지 알 수 없다.
- 요구사항이 변경되어, 무르기 기능이나 퀸의 첫 이동 체크 등이 추가되었을 경우 히스토리가 없기 때문에 구현이 곤란할 것 같다.
- 어떤 상황에 적합할까
- 히스토리가 필요하지 않는 요구사항일 경우
- 게임의 진행 상태만 빠르게 복구하려는 경우
[게임 명령어가 어떻게 입력되었는지를 적재하는 방식]
- 장점
- 게임의 전체 내역을 관리할 수 있다.
- 오히려 모든 명령어를 저장하기 때문에 턴이나, 기타 다른 정보를 저장하지 않아도 될 수 있다.
- 단점
- 게임 시작 시 매번 전체 명령어를 실행을 시켜야 하기 때문에 리소스 낭비나 초기 시작 시간이 발생할 수 있다.
- 어떤 상황에 적합할까
- 실행 순서가 필요한 요구사항일 경우(ex. 앞서 말한 무르기, 퀸의 첫 이동 체크 등등)
[두 가지 모두 저장]
게임을 빠르게 진행하면서 게임의 진행 과정을 상세히 기록할 수 있는 장점을 모두 활용하려고 할 경우, 두 가지 모두를 저장할 수 있을 것 같습니다..!
또 실행 순서가 필요한 요구사항으로 변경되지 않는다는 보장이 없기 때문에 가능한 둘 다 저장하는 것이 좋을 것 같다는 생각이 듭니다..!
There was a problem hiding this comment.
"하나의 게임진행 데이터지만 필요에 의해 여러가지 방식으로 나누어 적재를 할 수도 있다." 라는 메세지를 전달드리고 싶었어요 👍
추후 프로젝트를 진행할 때도 이 점을 참고해서 시스템 복잡도를 낮추는 경험을 해보시면 좋겠네요 😄 👍
| CREATE TABLE Turn | ||
| ( | ||
| turn_id INT AUTO_INCREMENT PRIMARY KEY, | ||
| turn VARCHAR(255) NOT NULL | ||
| ); | ||
|
|
||
| CREATE TABLE Piece | ||
| ( | ||
| piece_id INT AUTO_INCREMENT PRIMARY KEY, | ||
| color VARCHAR(255) NOT NULL, | ||
| pieceType VARCHAR(255) NOT NULL, | ||
| `rank` INT NOT NULL, | ||
| file INT NOT NULL | ||
| ); |
There was a problem hiding this comment.
"하나의 게임진행 데이터지만 필요에 의해 여러가지 방식으로 나누어 적재를 할 수도 있다." 라는 메세지를 전달드리고 싶었어요 👍
추후 프로젝트를 진행할 때도 이 점을 참고해서 시스템 복잡도를 낮추는 경험을 해보시면 좋겠네요 😄 👍
안녕하세요 현구막!
3,4단계 코드리뷰도 잘 부탁드립니다..!
(3, 4단계 진행 분리를 개인적으로만 진행했네요… 말씀을 잘못 이해했어요 🥲)3단계 때 먼저 변경을 하고 4단계를 하다보니 생각보다 변경이 없긴 했지만 뭔가 계속 아쉬워서 수정을 하다보니 제출이 늦어진 것 같아요
저번 리뷰 때 객체지향 관련해서 조언을 부탁드렸었는데요..!
스스로 정답이 계속 없다고 생각해서 더 막막하게 느껴졌었는데, 현구막의 정답은 있다고 생각하신다는 말이 힘이 됐습니다 ㅎㅎ
설계 바뀐 부분
저번 코드리뷰를 통해 유지보수에 대해 고민을 해보았는데요!
유지보수를 고려할 때, 항상 저의 관점에서의 유지보수를 생각하고 재사용성을 높이는 것에만 초점을 맞추었던 것 같습니다.
그래서 이번 리뷰에는 어느정도 재사용성을 포기해보자 라는 생각을 했습니다.
각 Piece에 대한 객체
Piece(같은 역할을 기대)를 상속,Attack(특정 동작을 기대)라는 인터페이스를 구현한DirectionAttackPiece,DiagonalAttackPiece를 구현체를 생성하였습니다.Piece는PieceType과Color를 가지고,PieceType은 각 타입에 따른 이동 방향과 스코어 규칙을 가집니다.DirectionAttackPiece(StraightCaptureObstacleRule에서 이름 변경) 는 그대로 유지하되, 기물의 구현체들이DirectionAttackPiece를 상속받도록 하였습니다관련 질문
제시해주신 ROOK 의 전략이 변경되는 상황에서 제가 다른 개발자의 코드를 봤었더라면,
으로 진행을 할 것 같다는 생각으로 구현을 했는데요
사실 기존 전략에 대한 부분은 최대한 가져가면서 변경을 시도하려고 해서 확신이 잘 서지 않습니다 🥲
현구막이 보시기에는 상속으로 변경했다 하더라도 이전의 설계와 크게 달라보이시지 않는지… 궁금합니다..!
턴 도입
저번 리뷰 때 턴이 없어도 괜찮을 것 같다고 생각한다고 말씀 드렸는데요..!
이번 단계를 진행하면서, 체스는 도메인 규칙이 턴 패스를 허용하지 않는다는 것을 알게 되었습니다..!
따라서 턴을 도입하게 되었습니다… 도메인에 대한 이해가 부족했던 것 같아 아쉽습니다 😭
이번 단계도 잘 부탁드립니다!🙏😁