체스게임 1단계 미션 리뷰 요청드립니다. #12
Conversation
| - [x] 기물의 이동 경로를 반환한다. | ||
| - [x] 입력받은 위치에 기물이 있는지 확인한다. | ||
|
|
||
| ## 이동 규칙 |
There was a problem hiding this comment.
각 기물 뒤에 나온 숫자가 점수인 것으로 보이는데,
점수 계산에 대한 것도 기능 목록으로 빼면 어떨까요?
해당 코드만 보고 이해하는 사람에게는 어떤 숫자를 의미하는지 알기가 어려울 것 같아요.
There was a problem hiding this comment.
넵! Player 기능목록에 점수 계산도 추가했습니다
README.md
Outdated
| - [x] ERROR : 비숍 이동 패턴으로 이동할 수 없는 위치일 경우 | ||
|
|
||
| - [x] 퀸 (9) | ||
| - [x] 모든 방향 1칸 + α 이동 (모든 대각선 방향으로는 원하는 만큼 이동 가능) |
There was a problem hiding this comment.
퀸의 이동방향은 상하좌우대각선 모두 원하는만큼 이동인 것으로 알고 있는데,
상하좌우는 1칸씩밖에 이동하지 못하는건가요?
퀸의 이동 규칙을 한 번 다시 봐주시겠어요?
There was a problem hiding this comment.
넵 퀸 이동 규칙을 완전히 착각해버렸어요ㅠㅠ 수정했습니다!
| import java.util.List; | ||
|
|
||
| public class BoardDto { | ||
| private List<PositionDto> positionDtos = new ArrayList<>(); |
There was a problem hiding this comment.
positionsDto를 먼저 초기화 하는 �이유가 있을까요?
해당 클래스로 객체가 생성되는 상황이 아래 생성자 하나뿐인데,
실제 initial 될 때 할당하는 것은 어떨까요?
해당 필드는 출력할 때만 쓰이는 스냅샷이기 때문에 해당 디티오를 변하지 않도록 설계해주면
더 안전한 프로그램이 될 수 있을 것 같아요 :)
| public class BoardDto { | ||
| private List<PositionDto> positionDtos = new ArrayList<>(); | ||
|
|
||
| public BoardDto(final Board board) { |
There was a problem hiding this comment.
borad라는 도메인 객체와 출력을 위한 dto를 분리하는 좋은 방향을 잡으셨네요 👍
| private static final Map<String, Position> POSITIONS = createPositions(); | ||
|
|
||
| private static Map<String, Position> createPositions() { | ||
| Map<String, Position> positions = new LinkedHashMap<>(); |
There was a problem hiding this comment.
POSITIONS를 캐싱하지 않았을 때 element의 수정과 변경이 잦을 것으로 예상돼서 LinkedHashMap을 사용했었습니다!
근데 캐싱을 적용하면서 변경이 일어나지 않게 되었을 뿐 아니라,
지금 다시 알아보니 LinkedHashMap이랑 HashMap이랑 성능 차이가 크게 없으니 순서유지가 필요한 경우에만 LinkedHashMap을 사용하는게 좋다고 하네요..!
POSITIONS는 HashMap으로 구현하도록 수정했습니다!
| import java.util.*; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class Player { |
There was a problem hiding this comment.
전반적으로 Player가 너무 많은 책임을 부담하는 것 같아요.
책임을 조금 더 나누면 어떨까요?
player가 담당하고 있는 책임이 무엇이 있는지, 이걸 어떻게 나눌 수 있는지 고민해보면 좋을 것 같아요 :)
There was a problem hiding this comment.
옙.. 그런데 어떤 역할을 Player로부터 분리할 수 있을지 모르겠습니다ㅠㅠ
지금 리팩토링 마친 후 Player가 담당하는 역할은
- 본인 기물 이동 및 이동 중 경로 검색
- 본인 기물들이 공격할 수 있는 모든 경로 검색
- 본인 기물이 공격받은 경우 기물 제거
- 본인 기물 관련 정보 검색
- 본인 기물 점수 합계 계산
이렇게 총 5가지입니다. 이것들이 Player에 있는 이유는, 이 역할들은 Player의 기물들을 참조해야만 할 수 있는 일이기 때문인데요.. Piece를 밖으로 노출하지 않고, Player보다 추상화된 상위 객체가 이런 역할을 수행할 수 있는 방법이 있을까요??
There was a problem hiding this comment.
음 이거는 블랙잭 때도 살짝 언급드렸는데, (이게 무조건 옳은 방향이다라고 말씀드리는건 아닙니다!)
결국 한 객체가 많은 책임을 가져가는데는 결합도가 너무 높기 때문이라고 생각해요.
예를 들면 (각 번호는 위에서 언급하신 책임입니다)
- 기물 이동 및 이동 중 경로 검색은 보드가 있으면 가능한 기능입니다. 여기서 플레이어의 역할은 움직이려는 기물이 자신의 기물인지의 여부이죠.
- 기물들이 공격할 수 있는 경로도 보드와 피스만 있으면 가능합니다. (피스들이 어떤 팀인지에 대힌 정보를 가지고 있다면 말이죠)
- 기물을 제거하는 것도 보드에서 바로 제거가 가능합니다.
- 제가 잘 이해한 것인지 모르겠지만, 각 팀에 대한 기물 정보이지 플레이어에 대한 기물 정보는 아니지 않나요?
- 기물 점수 합계도 보드에 대한 정보만 있으면 되지 않을까요?
플레이어와 기물을 엮을 수 있는 중요한 연결고리가 팀이 될 수 있지 않을까요?(지우 선생님 코드에서는 Color로 표현되는 것이죠)
기존에 지우 선생님이 작성하신 코드에서 확장성 하나만 생각해봅시다.
만약 2:2 팀 배틀이 생긴다면 어떻게 분리를 하시게 될까요?
최대한 객체간의 관계를 끊어보시는 연습을 하시면 좋을 것 같아요
There was a problem hiding this comment.
넵...! 우선 말씀하신 내용을 바탕으로 생각을 해봤는데, 제가 클래스 이름을 잘못 지은 것 같아요!
1~5 책임에 대한 피드백에서 공통적으로 말씀하시는 부분이, 제가 기존에 Player의 책임이라고 해놓은 것들은 사실 Board에서 할 수 있는 일이라는 거잖아요? 그런데 제가 Board라고 이름 붙였던 클래스는 사실 진짜 보드의 역할을 하고 있지는 않고, Player라고 이름 붙였던 클래스가 오히려 그 역할을 수행하고 있더라고요.. 그래서 클래스 이름을 Player → Team, Board → ChessGame 이렇게 수정해봤어요.
그리고 저는 하나의 보드에서 모든 흑백 기물들을 다 관리하지 않고, 각 색상에 따라 각자의 기물들만 관리하도록 설계를 했거든요., 그래서 말씀하신 내용(보드만 있으면 플레이어 무관하게 다 할 수 있음)이 저는 각 Team에 있는게 맞다고 생각해요..!
또, 2:2 팀 배틀의 확장성에 대해서 생각해봤는데요, 체스 게임을 진행할 때는 플레이어 무관하게 Color에 기반해서 차례(turn)을 관리하는게 맞지 않나요?? 그래서 커맨드를 입력하는 플레이어가 같은 팀 안에 여럿 생긴다고 해도, 그건 ChessGame보다 앞단에서 처리해야 할 것 같아요....! 어떤 플레이어가 어떤 Color인지 ChessService에서 맵으로 관리하던지, 아니면 그냥 각각 리스트로 관리하던지 하면 되지 않을까요..?
There was a problem hiding this comment.
아 넵 기존에 Player라는 네이밍이 사용자를 의미하는 네이밍에 가깝다고 생각이 들어서요. 만약 확장이 되어서 WHITE와 BLACK에 대한 플레이어 이름을 입력하게 된다면 플레이어에 대한 정보와 piece에 대한 정보가 player안 쪽으로 가는 것이 네이밍상 자연스러운 흐름이 된다고 생각해서 말씀드렸던 부분입니다. Team이라는 이름으로 변경이 된다면 저도 지금 상태도 괜찮다고 생각합니다 :)
그리고 2:2 팀 배틀에 대해서는 각 턴 마다 이 사람이 WHITE인지만 확인하여 플레이를 한다면 한 사람이 계속 수를 둘 수 있는 경우가 생기지 않을까요? 턴에 대한 개념이 팀에 대한 개념과 실제로 어떤 사람이 움직일 수 있는지에 대한 개념까지 같이 가지고 가야하지 않을까 하는 생각이 있습니다.(물론 체스 게임이 2:2를 지원할 가능성은 매우 낮습니다😅)
There was a problem hiding this comment.
아하 넵! 그러니까 턴을 관리할때 Color 뿐 아니라 Player의 순서도 같이 관리하면 더 좋겠다는 말씀이시죠?? Turn에서 players를 관리하고 순서에 맞게 관리하도록 추가해봤습니다...!
| validateTarget(player, target); | ||
| validateKingMovable(player, enemy, source, target); | ||
|
|
||
| enemy.attacked(target); |
There was a problem hiding this comment.
이건 좀 더 언어적인 측면인데요.
enemy.attacked(target);은
enemy가 target을 attack했다와 같이 읽히는데,
enemy가 어택 당했다 target에 의해라는 것에 대해 좀 더 의미를 명확하게 전달할 수 있도록 메서드명을 변경하는 것은 어떨까요?
There was a problem hiding this comment.
넵! enemy.wasAttackBy(target) 으로 수정했는데 괜찮은가요!?
There was a problem hiding this comment.
아 여기서는 타겟에 의해 당한 것이 아니라 타겟 포지션을 어택 당한거군요!
By가 아니라 다른 전치사를 써주는게 좋을 것 같아요 :)
그런데 최대한 수동의 이미보다는 능동의 의미로 표현해주는 것이 조금 더 나은 방향이라고 생각하긴 합니다.
이건 개인적인 의견이라 본인이 생각하시는 좀 더 좋은 방향으로 표현해주시면 좋을 것 같아요 :)
There was a problem hiding this comment.
넵! 오랜만에 다시 보니까 그냥 removePiece()가 제일 나은 것 같아서 그렇게 수정했습니다
| import java.util.*; | ||
|
|
||
| public class Position { | ||
| private static final Map<String, Position> POSITIONS = createPositions(); |
| @ParameterizedTest | ||
| @CsvSource({"b2, b3, false", "a7, a6, true"}) | ||
| @DisplayName("자신의 기물이 아닌 기물을 선택할 경우 예외가 발생한다.") |
|
|
||
| import static chess.domain.player.MoveCoordinate.*; | ||
|
|
||
| public class MovePattern { |
There was a problem hiding this comment.
거의 대부분을 캐싱해서 사용하신 것 같은데 enum으로 관리해보면 어떨까요?
There was a problem hiding this comment.
아마 infinite pattern과 finite pattern에 대한 내용이 queen의 규칙을 잘못이해하신 것에서 비롯된 설계일 것 같아요.
이 부분은 기능을 수정하고 다시 설계해주세요 :)
There was a problem hiding this comment.
넵 상수들 enum으로 전환하고 설계 다시 하겠습니다!
수정, BoardDto와 OutputView 수정
jiwoo-kimm
force-pushed
the
jiwoo-kimm-step1
branch
from
8fb988d to
d97f271
Compare
안녕하세요 재주선생님!
체스게임 1단계 미션 제출드립니다. 리뷰 부탁드립니다!
이번에 체스 자체의 로직이 어려워서 구현하는 데에 많은 어려움이 있었지만
특히 MovePattern 추상화를 어떻게 할지, Piece마다 구현 분리를 해야 할지 고민을 많이 하느라 오래 걸렸습니다.
결론적으로는 Pawn 예외상황 추상화에 실패했는데.. 코드가 많이 어지러워진 것 같습니다..
지금 코드의 문제점, 그리고 어떤 방향으로 개선하면 좋을지 집중적으로 봐주시면 감사하겠습니다!