[1, 2, 3단계 - 체스] 이프(송인봉) 미션 제출합니다.#298
Conversation
hongbin-dev
left a comment
hongbin-dev
left a comment
There was a problem hiding this comment.
안녕하세요 이프, 리뷰어 앨런입니다 🙌
짧은기간인데 객체지향을 적용하려고 노력하신모습이 보이네요!
몇 가지 코멘트 남겼으니 확인해주세요~
| @Override | ||
| public PlayState run(String command) { | ||
| throw new UnsupportedOperationException("게임이 종료된 상태에서는 게임을 실행할 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
UnsupportedOperationException가 존재한다면 '잘못된 구현이 아닌가'라는 생각이 드네요
|
|
||
| @Override | ||
| public PlayState run(String command) { | ||
| if (!command.equals("start")) { |
| @Override | ||
| public PlayState run(String command) { | ||
| if (!command.equals("start")) { | ||
| throw new IllegalArgumentException("게임이 시작되지 않아 다른 명령을 실행할 수 없습니다."); |
There was a problem hiding this comment.
이 예외도 End에서 발생하는 비슷한 예외로 생각되는데요. 같은 예외로 가야하지않을까요?
| if (command.equals("status")) { | ||
| OutputView.printChessBoardStatus(chessBoard.calcualteScoreStatus()); | ||
| return this; | ||
| } | ||
| Matcher matcher = MOVE_COMMAND_PATTERN.matcher(command); | ||
| if (matcher.find()) { | ||
| movePieceByCommand(command); | ||
| OutputView.printChessBoard(chessBoard.getPieces()); | ||
| return moveNextCommand(); | ||
| } |
| return new End(); | ||
| } | ||
| if (command.equals("status")) { | ||
| OutputView.printChessBoardStatus(chessBoard.calcualteScoreStatus()); |
There was a problem hiding this comment.
체스보드의 책임이 많아서, 계산하는로직을 분리하는것도 좋을 것 같아요.
| QUEEN("Q", King::new), | ||
| ROOK("R", Rook::new), | ||
| BISHOP("B", Bishop::new), | ||
| KNIGHT("N", Knight::new), |
There was a problem hiding this comment.
| QUEEN("Q", King::new), | |
| ROOK("R", Rook::new), | |
| BISHOP("B", Bishop::new), | |
| KNIGHT("N", Knight::new), | |
| QUEEN("Q", Queen::new), | |
| ROOK("R", Rook::new), | |
| BISHOP("B", Bishop::new), | |
| KNIGHT("N", Knight::new), |
여기는 퀸이 들어가야하지 않을까요?
또 "Q", "R"... 같은 값은 객체 내에 존재하지않나요? 값이 응집되지 않는 것 같아요.
객체에 있는 값을 사용하면 좋을 것 같습니다~
|
|
||
| boolean isSameTeamPiece(Piece piece); | ||
|
|
||
| boolean isMovable(Position source, Position target, ChessBoard chessBoard); |
There was a problem hiding this comment.
chessBoard 없이 구현할 순 없을까요?
체스보드도 피스를알고, 피스도 체스보드를 알고 있네요
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public final class WhitePawn extends Pawn { |
There was a problem hiding this comment.
폰을 두개로 나눠주셨는데, 하나로 풀어낼 수는 없을까요?
두 벌로 관리하면 유지보수에 어려움이 생길 것 같아요
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public final class Knight extends SingleMovablePiece { |
| @Override | ||
| public boolean isPawn() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
마이너하지만 추상클래스에 false로 구현해놓고, Pawn에서만 true로 오버라이딩하는 방법은 어떨까요?
|
오랜 시간이 걸렸네요ㅜ 기존의 코드를 모두 제거하고 처음부터 다시 시작해봤어요.. 나름대로 객체지향적인 코드를 작성하기 위해 노력했는데 어떻게 보실지 모르겠네요 이미 이렇게나 많이 리뷰를 남겨주셨는데 새로운 코드를 들고 나타나서 죄송합니다. 기존에 피드백 주신 내용들이 전부 수정됐어요. 새롭게 피드백 주시면 하나씩 감사한 마음으로 답변드리겠습니다. |
hongbin-dev
left a comment
hongbin-dev
left a comment
There was a problem hiding this comment.
안녕하세요 이프.
1, 2, 3단계 미션 잘 진행해주셨네요.
이만 머지하겠습니다.
| public void execute(final ChessApplication chessApplication, | ||
| final ChessGame chessGame, | ||
| final List<String> commandOptions) { | ||
| chessApplicationExecution.execute(chessApplication, chessGame, commandOptions); | ||
| } |
There was a problem hiding this comment.
chessApplication에서 실행해도되보이는데, 이런형태로 풀어낸 이유가 있을까요?
There was a problem hiding this comment.
ChessApplication에서 각각의 명령(start, move, status, end)에 따른 로직을 따로 수행하기 위해서는
필연적으로 분기문을 사용해야 했습니다. 하지만 지금과 같이 CommandExecutor라는 Enum 클래스를 통해
각 명령어에 따른 실행 로직을 매핑시킴으로써 분기문을 제거할 수 있었습니다.
There was a problem hiding this comment.
분기를 지울 수 있었지만, 개인적으로 복잡도가 증가했다는 생각이 드네요
There was a problem hiding this comment.
맞습니다..ㅠ 하나의 어플리케이션 로직이 ChessApplication으로부터 CommandExecutor가 분리된 형태이기 때문에, 복잡도 뿐만 아니라 둘 간의 의존관계가 크게 형성되어 있다는 문제를 느끼고 있습니다. 이 부분에 대해 계속 고민중인데, 4~5단계를 진행하면서 해당 부분을 수정해보도록 하겠습니다!! 분기문 제거하는 과정이 다소 어렵네요😥
| package chess.domain; | ||
|
|
||
| public enum Color { |
There was a problem hiding this comment.
콘솔에 domain이라는 패키지가 조금 어색해보이는데요.
또 몇몇 클래스들은 콘솔/도메인에 2벌로 관리하신 이유가 있을까요?
There was a problem hiding this comment.
Model(chess.domain)과 View(chess.console.view)를 완전히 분리하고 싶었습니다.
따라서 Color의 대소문자 변환이나 PositionRange의 범위값 등 View에서도 필요한 비즈니스 로직을
몇몇은 Model(chess.domain)에 구현되어 있더라도, View(chess.console.domain)에 또다시 구현하여 사용했습니다.
물론 지금의 어플리케이션은 콘솔 상으로 로컬에서만 동작하는 어플리케이션입니다.
따라서 View(chess.console.view)에서 Model(chess.domain)을 가져다 사용해도 괜찮습니다.
Model에 View와 관련뢴 로직이 없앴기에, View의 변경에 Model이 영향을 받는 문제는 없습니다.
하지만 이 경우는 View가 Model에 의존적이게 됩니다.
이 부분에서 저는 View 또한 Model로부터 독립적이게 만들고 싶었습니다.
그래서 Model과 View간의 연결은 dto를 통한 데이터 전달로만 이뤄지고
Model과 View 각각에 도메인을 별도로 두어 서로 영향을 받지 않도록 독립시켰습니다.
| if (players.isOnlyOneKingLeft()) { | ||
| return new FinishedState(players); | ||
| } | ||
| if (players.isPlayerAbleToPromotePawn(color)) { | ||
| return new PromotionState(players, color); | ||
| } |
There was a problem hiding this comment.
MoveState입장에서는 왕이 죽었는지는 궁금하지 않을 것 같아요.
게임이 끝났는지 물어보는게 어떨까요? 프로모션도 마찬가지에요. 게임이끝났는지? 프로모션을 할 수 있는지?
가볍게 풀어내면 좋을 것 같네요
There was a problem hiding this comment.
메서드명을 바꿔보자는 말씀인가요??
만약 그렇다면, 지금의 메서드명에는 이유가 있습니다.
예전 코드와 같이 체스보드의 기물을 관리하는 객체가 ChessBoard였다면
board.isFinished / board.isPromotion 처럼 보드의 상태를 묻는 형식의 메서드명을 사용했겠지만,
지금은 ChessBoard 대신 Players를 통해 각 플레이어의 기물들을 관리하고 있습니다.
따라서 players.isFinished / players.isPromotion 는 어색함을 느꼈고,
players의 역할(Player를 관리)을 생각해서 players.isOnlyOneKingLeft / players.isPlayerAbleToPromotePawn으로
메서드명을 지었습니다.
players.isOnlyOneKingLeft의 경우는 players의 역할 특성상 다수의 플레이어가 존재할 수 있기 때문에(4인 체스와 같이 인원의 변경 가능성을 생각하여) 이렇게 이름을 지었습니다.
| private final char column; | ||
| private final char row; |
There was a problem hiding this comment.
column과 row를 enum으로 관리했을 때의 장점을 느끼지 못했습니다.
오히려 column을 enum으로 관리하면 'a'부터 'h'까지의 모든 값을 하드코딩해야 합니다.
이렇게 되면, 체스보드의 규격이 변경되었을 때 일일히 값을 추가하거나 삭제해야하는 변경사항이 생깁니다.
이런 상황을 고려했을 때, 저는 각각의 값을 Enum으로 관리할 필요없이
그 값의 범위를 Enum으로 관리하는 편이 좋다고 판단했습니다. 그 결과가 PositionRange입니다.
체스보드의 규격이 변경된다는 고민 자체가 오버프로그래밍일 수 있지만,
제가 생각하는 오버프로그래밍은 미래를 내다보고 미리 구현을 해놓는 것과
도메인 정책의 기본 규칙이 변경될지도 모른다니까 이런것들을 미리 대처해야한다!는 생각입니다.
여기서 도메인 정책의 기본 규칙이 변경될지도 모른다는 생각은
체스보드가 사라지는 등 체스규칙의 기본 틀이 깨져버리는 변경사항으로,
더이상 체스가 아니게 되어버리는 사항입니다.
그에 반해 지금 언급드린 체스보드의 규격이 변경된다은 체스라는 틀은 유지한 채,
추후의 발생가능한 서비스의 확장이라고 생각합니다.
실제로 체스라는 게임에서 파생되어 4인 체스와 그랜드 체스 등 여러 가지의 체스게임이 존재합니다.
이러한 관점에서 보았을 때, 현재의 어플리케이션이 미래에 확장될 수 있도록 지금의 구조를 설계하였습니다.
There was a problem hiding this comment.
체스보드의 규격이 변경되었을 때 일일히 값을 추가하거나 삭제해야하는 변경사항이 생깁니다.
그럼 지금 방식에서 체스보드 규격이 변경되면, 어떤식으로 변경이 일어날지도 궁금하네요🤔
There was a problem hiding this comment.
현재의 8x8 체스보드 규격이 그랜드 체스와 같이 10x10 체스보드 규격으로 변경된다면
PositionRange에서 범위를 수정함과 동시에, PlayerFactory에서 플레이어의 초기시작위치를 수정하면 됩니다.
만약 8x8 체스보드 규격의 기존 게임과 10x10 체스보드 규격의 변형 게임을 둘다 지원하겠다면
PositionRange와 PlayerFactor를 인터페이스화 하여 각각의 게임에 맞는 구현체를 생성하여 사용하면 됩니다.
| public Map<String, Double> getPlayerScores() { | ||
| return playerScores.entrySet() | ||
| .stream() | ||
| .collect(Collectors.toMap( | ||
| entry -> entry.getKey().getColorName(), | ||
| Entry::getValue | ||
| )); | ||
| } |
There was a problem hiding this comment.
이런 비즈니스가 필요하다면 dto가 아닌 별도의 객체에서 진행하면 좋을 것 같네요~
3 participants
앨런! 안녕하세요~ 이프입니다
체스 미션에서 고려할 부분이 많아서 다소 어려웠습니다ㅜ
기능 구현은 마무리되어 PR 요청을 드리려 하지만,
PR 요청에 앞서 미리 말씀드릴 부분이 있습니다.
지금의 코드는 아무래도 상당 부분 재설계될 것 같습니다...
이번 주말동안 여러 고민을 가지면서 객체지향에 대해 한번 생각해보았습니다.
인스턴스 필드를 왜 2개까지 허용했는가(요구사항)-> 객체가 지닌 정보가 적어진다.
-> 어떠한 행동을 수행하기 위해 필요한 정보가 있을 때, 자신에게 없기 때문에
-> 그 정보를 가지고 있는 객체와 소통을 해야만 한다.
-> 이러한 과정 속에서 객체 간의 협력이 이뤄진다.
-> 즉 객체 간의 협력을 장려하기 위함이다.
왜 도메인을 작게 유지하는가(요구사항)-> 하나의 객체가 수행하는 일을 줄여야 한다.
-> 수행하는 일이 많으면, 해당 객체를 변경(수정)해야하는 포인트가 늘어난다.
-> 이는 SRP 원칙을 위배한다.
그렇다면 왜 위와 같은 규칙을 지키면서까지 객체지향을 해야하는가(생각)-> 객체지향은 유연하다.
-> 유연하기 때문에 언제든 대처가 가능하다.
-> 우리의 사회는 매일매일 달라진다. 서비스의 요구사항은 매번 변경되고, 서비스는 언제든 확장될 수 있다.
-> 그렇기에 유지보수, 재사용성, 확장성을 고려하여 프로그래밍을 진행해야 한다.
-> 이를 제대로 충족시키는 것이 객체지향이다.
객체지향의 관점으로 봤을 때, 지금의 코드는 어떠한가(생각)-> 좋지 않다고 생각한다.
-> 첫째, MVC 관점에서 바라보면 view와 model 은 분리되어야 한다.
-> view와 model이 분리되어야 하는 이유는 view가 쉽게 변경될 수 있다는 특징도 있겠지만
-> 무엇보다 model을 재사용하기 위함이다.
-> view는 언제나 하나의 model에 하나의 view가 사용되지 않는다.
-> 지금 체스 미션과 같이 콘솔/웹이 동시에 지원될 수도 있다.
-> 만약 view와 model이 제대로 분리되어 있다면, view에 따른 controller만 영향을 받을 뿐
-> model은 어떠한 영향도 받지 않고 모든 view에 재사용될 수 있어야 한다.
-> 특히나 게임의 흐름을 관리하는
console/game/playstate만 봐도 View와의 결합이 강하게 얽혀있어-> 웹 서비스에서는 활용할 수 없는 지경에 이르렀다.
-> 둘째, 확장성이 닫혀있다.
-> 체스 게임은 언제든 새로운 규칙이 추가될 수 있다.
-> 2개의 새로운 기물이 추가된 10x10 보드판에서 이뤄지는
그랜드체스나-> 4방향에서 각자의 기물을 가지고 게임을 시작하는
4인 체스등-> 기존의 체스 게임을 변형/확장한 게임들이 여럿있다.
-> 이는 곧 지금의 코드가 실제 서비스되고 있다면 사업에 따라 언제든 변형/확장시킬 수 있음을 의미한다.
-> 그렇다면 과연 지금의 코드는 그러한 상황에 대처할 수 있을까
-> 절대 그렇지 않다.
-> 당장 ChessBoard만 봐도 너무많은 일을 수행하고 있으며,
-> 단순히 위아래 방향으로 진행되는 방향을 좌우 방향으로 바꾼다 생각해보면 변경해야 한 부분이 많다.
객체지향에 대한 생각을 레벨1이 끝나가는 지금에서야 가지게 되어 많이 늦었음을 알고 있지만
이번 미션에서는 코드를 새로 작성하는 한이 있더라도 꼭 객체지향적인 설계와 코드를 작성하고 싶습니다.
코드 리뷰에 있어 양해 부탁드립니다.ㅠ