[서브웨이] 체스 미션 제출입니다. #64
Conversation
| if (command.isMove()) { | ||
| chessBoard.move(command.sourcePosition(), command.targetPosition()); | ||
| ChessOutputView.printChessBoard(chessBoard); | ||
| } | ||
| if (command.isStatus()) { | ||
| ChessOutputView.scoreOf(chessBoard.getPlayerColor(), chessBoard); |
There was a problem hiding this comment.
Command가 추가될 떄마다 매번 메서드가 추가되겠네요. :)
enum을 통해 관리하고 switch 문을 통해 처리할 수 있지 않을까요?
There was a problem hiding this comment.
리뷰어님 객체지향 생활체조에서 if/else 에서 else 문을 허용하지 않는다 라는 규칙이 있는걸로 아는데이 말이 switch문 또한 허용하지 않는것으로 알고 있었는데 switch문을 통해 구현을 해도 괜찮은 건가요? 결국엔 스위치 문도 커맨드가 추가될때마다 메서드가 추가되는 구현방법 아닌가요?
리뷰어님 생각을 듣고싶습니다!
There was a problem hiding this comment.
말씀드린 메서드는 isStatus, isMove등의 매서드를 말씀드린거였어요. :)
굳이 그런식으로 확인하지 않아도
switch(command) {
MOVE : ~
STATUS: ~
}
로 처리가 가능하니까요.
그리고 말씀하신대로 else 구문을 허용하지 않는다는 말 그대로 else만을 쓰지 말라는 것 보다는 한 메서드 내에 브랜치는 되도록 적게 만들라는 의미가 더 큰데요. 그런 의미에서는 현재 메서드는 switch랑 크게 다를게 없어요.
해당 규칙을 지키고 싶으시다면 command별로 요청을 처리하는 객체를 돌려주는 팩토리를 만들면 될 것 같습니다.
이 객체는 view를 보여줄 수 있는 객체를 돌려줘야할텐데 보여줘야할 정보의 인터페이스가 맞지 않아서 어려울거에요. 또 각 객체별로 동일한 ChessBoard를 사용하도록 하기 위해서 ChessBoard를 싱글톤으로 관리하는 등 구현상 복잡해지는 부분도 있고요. (chessBoard를 넘겨주고 안쪽에서 출력까지 처리하기에는 뷰와 도메인, 컨트롤러의 경계가 무너지구요.)
혹은 이 방법에 대해 아이디어가 있다면 도전해보시는것도 좋을 것 같아요. :)
| } catch (RuntimeException e) { | ||
| ChessOutputView.printError(e.getMessage()); | ||
| command = Command.of(ChessInputView.inputCommand()); | ||
| checkStartCommand(chessBoard, command); |
|
|
||
| public class RenderChessBoardView { | ||
| private static final String EMPTY_CHESS_PIECE = "."; | ||
| private List<List<String>> chessBoardView; |
There was a problem hiding this comment.
하위 제네릭 List도 별도 일급 컬렉션으로 묶어줬어도 좋을 것 같네요. :)
안에서는 해당 부분에 대한 View 생성을 담당하고요.
|
|
||
| public static void scoreOf(PieceColor pieceColor, ChessBoard chessBoard) { | ||
| System.out.println(String.format("%s점수: %.1f", pieceColor, | ||
| ChessCalculator.calculateScoreOf(pieceColor, chessBoard))); |
There was a problem hiding this comment.
ChessCalculator는 뷰보다는 도메인 모델에 가까운 것 같아요. :)
|
|
||
| import chess.domain.position.Position; | ||
|
|
||
| public interface RuleStrategy { |
There was a problem hiding this comment.
첫번째 질문이랑 유사한 내용인 것 같아 여기에 답변을 추가할게요.
1.리뷰어님 질문이 있습니다. 이번에 객체의 책임 분리를 많이 고려를 해서 만들다 보니 클래스객체가 굉장히 많이 만들어져서 오히려 코드를 이해하는데 있어서 복잡성이 올라가 코드 이해를 방해하는 요소가 된거 같아서"지금 객체 분리를 너무 불필요하게 많이 한것인가 아닌가?"에 있어서 페어와 의문이 계속 들었습니다.
이렇게 객체가 분리가 되었지만 코드를 이해하는데 있어서 오히려 마이너스 요소가 된다면객체분리를 과도하게 분리해서 객체의 분리를 잘못한것인지 아니면 이런식으로 하는 방법이 맞는 방법인지 질문드립니다.!
'과도한 추상화'라는 말이 있습니다. 혹은 오버 엔지니어링이라고도 하고요.
추상화도 사실 어느정도 추상화 할지는 개발자의 선택 혹은 역량입니다만,
만약 말씀하신대로 오히려 코드 이해도가 떨어진다면 의심해 볼 필요는 있어요.
실제로 저 또한 개발할 때도 이 부분에 대해서 많은 고민을 하고 있고요.
이 객체를 지금 굳이 확장성을 고려해서 분리할 필요가 있는지 일단 고민해보시면 좋을 것 같아요.
예를들어 현재 각 말의 행위에 대해 전략 패턴으로 분리했지만 이는 동일한 말이지만 다른 행동을 할 수 있다는 전제 하에 개발을 한것으로 보일 수 있어요. (예를 들어 나이트인데 킹의 행동을 하는 등, 현재 구조상은 말이라는 객체 자체는 표시를 위한 데이터 말고는 아무 의미가 없으니까요.)
만약 그런 의도를 가지고 추상화한게 아니라면 적어도 RuleStrategy를 생성자를 통해 주입을 못하도록 (구현체별로 특정 Strategy를 사용하도록) 강제를 하는 쪽이 맞고, 결국에는 Piece에 따라서 행동할 전략은 하나인데 인터페이스로 관리할 필요가 있을까? 라는 의문으로 결론이 나올수도 있어요. (그냥 상속이나 구현으로 구현하는 방법도 있겠죠. :))
동일한 말(Piece)이지만 다른 행동을 할 수 있다는 전제를 염두해 두셨다면 어떠한 케이스가 있을지도 고민해보시면 좋을 것 같네요. :)
추상화 라는게 어느정도는 개발하는 사람이 어떤식으로 확장을 할 수 있는지에 대해 의도를 보여준다고도 볼 수 있기 때문에 무조건적으로 유연한 구조는 독으로 돌아올수도 있어요. :)
만약 현재 요구사항에서 굳이 분리할 필요가 없다면 그냥 하나의 코드로 들고있고 새로운 요구사항이 들어올 때 리팩토링하는 방법도 좋은 방법이에요. :)
지금 구조에서 다 바꿀 필요는 없지만 적어도 Piece에 대한 전략을 주입해주는 방향보다는 강제하도록 해주는게 좋을 것 같아요.
물론 이 경우는 객체지향 원칙에서 OCP, DIP에 어긋납니다. :)
| while (!(command.isEnd())) { | ||
| command = Command.of(ChessInputView.inputCommand(chessBoard)); | ||
| if (command.isMove()) { | ||
| chessBoard.move(command.sourcePosition(), command.targetPosition()); | ||
| ChessOutputView.printChessBoard(chessBoard); | ||
| } | ||
| if (command.isStatus()) { | ||
| ChessOutputView.scoreOf(chessBoard.getPlayerColor(), chessBoard); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
3.mvc에 대해 구글링 하던 도중에 controller를 여러개를 갖을수 있고 컨트롤러는 서로에 대해 모르면서 라우터(?)에서
각 컨트롤러를 다룬다고 나와있는 설명을 보았는데 해당 글을 보고 command와 chessboard controller를 따로 나누고
커맨드에 따라 main에서 게임이 흘러가는 식으로 다뤄 주려고 해봤는데 해보면서 뭔가 잘못접근한건가 하는 생각이 들어라구요..
그래서 혹시 리뷰어님께서 간단한 예시를 들어주실수 있나 해서 마지막 질문도 달아봅니다!!
비슷한 개념이에요. web에서는 url이라는 어떤 컨트롤러에 접근할지에 대한 주소를 넘기고, 프레임워크(디스패쳐 서블릿)에서는 해당 컨트롤러를 찾아 요청을 전달합니다.
현재는 컨트롤러를 추가로 나눌 필요는 없고 지금 하신대로 도메인 객체(체스 게임)을 목적에 맡게 호출하셔도 충분해보이네요. :)
| import chess.domain.chessPiece.ChessPiece; | ||
| import chess.domain.chessPiece.pieceType.PieceColor; | ||
|
|
||
| public abstract class PlayerTurnState { |
There was a problem hiding this comment.
색을 반전하는 것 때문에 나누신 것 같은데 사실 이는 PieceColor에서 책임져도 충분하지 않을까 싶네요. :)
playerColor.getOppositeColor() 등으로..
| public abstract double getScore(); | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
이펙티브 자바 아이템 12. toString을 항상 재정의하라 내용 참고해주세요.
toString은 해당 객체의 정보를 모두 표현해주는게 좋아요. :)
| } | ||
|
|
||
| public boolean isStart() { | ||
| return this.command[COMMAND_INDEX].equals("start"); |
| } | ||
|
|
||
| private static String chessBoardContainPieceAt(ChessBoard chessBoard, ChessFile file, ChessRank rank) { | ||
| if (chessBoard.contains(file.toString() + rank.toString())) { |
There was a problem hiding this comment.
이펙티브 자바 아이템 12. toString을 항상 재정의하라를 참고해보시고요.
toString을 view 혹은 다른 용도로 클라이언트가 사용하는것은 최대한 방지하는게 좋을 것 같아요. :)
만약 허용한다면 toString의 변경에 대해 엄격하게 관리가 필요하구요.
There was a problem hiding this comment.
이펙티브 자바 아이템 12. toString을 항상 재정의하라 이 부분을 참고해봤는데
클라이언트가 view혹은 다른 용도 직접 사용하는 것을 방지한다는 내용에 대해 나와있는게 없는것 같아서요!
리뷰어님이 해주신 말씀을 toString을 직접 호출하지 말아라 라고 이해했는데
toString을 직접 호출하지 않아도 자동으로 사용되는데 클라이언트가 직접사용하는것고 차이가 있나해서 질문드립니다!
toString을 클라이언트가 직접 사용하는 것을 방지하는 이유가 있나요???
There was a problem hiding this comment.
toString 자체가 언제든지 스펙이 변경이 될 수 있는 부분이기 때문에 로깅 외의 용도로는 사용되지 않는게 좋아요.
직접 정의한 메서드와 달리 toString은 객체의 정보를 돌려준는 역할을 담당하고있지 어떤 값을 돌려준다는 명시되어있지 않으니까요.
다른 용도로 사용한다면 위에 말한대로 toString 부분은 절대 변경되지 않음과, '특정 값만'을 돌려줘야한다는 것을 주석을 통해 명시하고 구현에 따라 문제가 생길수도 있음도 인지해야하구요.
물론 현재 ChessFile, ChessRank같은 경우는 상속도 불가능하며 돌려줘야 할 값 자체도 심플하지만,
toString보다는 사용해야할 값의 getter를 제공하는게 더 안전합니다.
혹은 해당 케이스는 굳이 toString을 쓸 필요도 없이 Position of(ChessFile chessFile, ChessRank chessRank)를 사용하는게 더 나을 것 같아요. 이 로직을 도메인으로 분리한 다음에요. :)
commandType을 관리하는 enum 적용 view와 domain 분리
| public abstract class ChessPiece implements Movable, Catchable { | ||
|
|
||
| protected final PieceColor pieceColor; | ||
| protected State state; |
There was a problem hiding this comment.
State라는 클래스의 작성 의도를 알 수 있을까요?
PieceState가 현재 필요한지, 이정도 추상가 필요 없다면 그냥 Piece가 직접 RuleStrategy를 가지고 있거나
Strategy를 작성하지 않고 구현체에서 구현을 해도 될 것 같아서요.
State를 사용함으로써 의미를 가지는 인터페이스는 movedState라는 메서드인데
InitialState, MovedState에 따라서 처리하는 부분도 없어보여서 더욱 의미가 없어 보이고 코드 복잡도는 늘어나는 것 같아요.
There was a problem hiding this comment.
Pawn이 초기상태와 움직인 상태의 규칙이 달라지기떄문에 추가했고 원래는 페어와 특수규칙에 대해 구현해볼 생각으로 state를 넣었는데 특수규칙을 구현하지 못해서 state가 의미를 갖지 않는것 처럼 보이네요!
킹과 룩의 초기상태에서 적용 될 수 있는 케슬링이라는 특수규칙까지 구현할 생각으로 state를 만드려고 했는데 거기까지 구현을 하지 못해서 리뷰어님이 말한대로 그냥 코드 복잡도만 들어나는 것 같네요!
해당부분 리팩해보겠습니다!
| } | ||
|
|
||
| private static void addOtherPiecesBy(Map<Position, ChessPiece> chessBoard, PieceColor pieceColor, ChessRank rank) { | ||
| chessBoard.put(Position.of(ChessFile.from('a'), rank), new Rook(pieceColor)); |
There was a problem hiding this comment.
ChessFile을 이렇게 하드 코딩 방식으로 사용할 경우 비즈니스에 변경이 있을 시 컴파일 타임에서 확인이 어려울 것 같아요.
어차피 갯수도 정해져있으니 enum을 통해 코드로 관리해보면 어떨까요?
|
|
||
| public void checkCaughtPieceIsKing() { | ||
| if (this instanceof King) { | ||
| throw new CatchKingException("왕이 잡혔습니다."); |
There was a problem hiding this comment.
'이펙티브 자바 아이템 69. 예외는 진짜 예외 상황에만 사용하라'
왕이 잡힌 상황이 예외 상황일까요? :)
| } | ||
|
|
||
| public void checkCaughtPieceIsKing() { | ||
| if (this instanceof King) { |
There was a problem hiding this comment.
추상체가 특정 구현체에 대해 의존하도록 하기보다는, 이에 대한 체크는 외부에서 해주는게 좋을 것 같네요.
There was a problem hiding this comment.
추상체가 구현체에 대해 알고있으면 안되는 건가요??
혹시 알고있으면 안되는 이유가 구현체의 변경에대해 추상체가 영향을 받기 때문에 의존성을 제거해주는 것인지
궁금해서 질문드립니다!
There was a problem hiding this comment.
네. 추상체에서 제공하는 기능은 되도록이면 구현체에 의존을 하지 않는게 좋습니다.
| public double commandStatusRun(ChessBoard chessBoard) { | ||
| return ChessCalculator.calculateScoreOf(chessBoard); | ||
| } |
There was a problem hiding this comment.
Status를 계산 하는 로직 자체는 도메인에 더 가깝지 않을까요?
ChessCalculator자체가 chessBoard와 결합도도 높다보니 command에 있을 필요는 없을 것 같아요.
이런 역할을 하는 메서드를 ChessBoard에서 제공해줘도 좋을 것 같네요. :)
| import java.util.Objects; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class ChessCalculator { |
There was a problem hiding this comment.
ChessBoard와의 결합도가 상당히 높네요.
ChessBoard랑 같은 패키지로 관리해보면 어떨까요?
| private static final int REDUCING_PAWN_SCORE_LOWER_BOUND = 1; | ||
| private static final int REDUCING_RATE = 2; | ||
|
|
||
| public static double calculateScoreOf(ChessBoard chessBoard) { |
There was a problem hiding this comment.
ChessBoard 내부 구현으로 해당 기능을 제공한다면 chessBoard 내부에서 chessBoard가 가지고있는 map 자체를 직접 전달해주는 형태로도 풀 수 있을 것 같네요. :)
There was a problem hiding this comment.
리뷰어님 저도 calculateScoreOf 를 chessBoard 내부에서 구현 함으로서 map 자체를 던져주어서 getter를 부르지 않도록 하는 방법이 있을수 있겠다고 생각했었는데
해당 클래스에서 점수를 구하는 과정에서 chessBoard의 현재 플레이어 턴과 같은 색의 piece를 필터링하기위해 chessBoard.getPlayerColor() 를 불러주는것을 확인했습니다.
이로인해서 chessBoard에서 인스턴스 변수들인 Map과 플레이어의 색을 직접 전달해주는 방법과 chessBoard 자신인 this를 전달해주는 방법 2가지의 경우가 생겼는데
어떤경우가 좀 더 나은 방법인지 알고싶어 질문을 달았습니다! 현재는 this를 전달해주어서 getter로 받아오는식으로 구현한상탠데 어떤 방법이 더 좋은 방법일까요?
There was a problem hiding this comment.
chessBoard.getChessBoard().entrySet().stream()
.filter(entry -> entry.getKey().isSameFilePosition(chessFile))
.filter(entry -> entry.getValue().isSamePieceColorWith(chessBoard.getPlayerColor()))
.map(Map.Entry::getValue)
ChessFile.values().stream().flatMap(chessFile -> getChessPiecesOn(chessFile, chessBoard))
해당 기능으로 만들어진 자료구조까지 chessBoard가 제공해서 전달할 수 있지 않을까요?
전략의 변경 가능성을 열어두어 만들었던 인터페이스 전략의 강제주입으로 변경하여 이름을 수정함
public static void printCaughtKing(ChessBoard chessBoard) {
System.out.println(String.format("왕이 잡혔습니다 %s승", chessBoard.getPlayerColor()));
}chessBoard.getPlayerColor() 는 PieceColor 객체를 부름 수정후 public static void printCaughtKing(ChessBoard chessBoard) {
PieceColor playerTurn = chessBoard.getPlayerColor();
System.out.println(String.format("왕이 잡혔습니다 %s승", playerTurn.getColor()));
}playerTurn.getColor() 는 객체의 인스턴스를 부름 리뷰어님 질문이 있습니다. toString을 view에서 안쓰는 방향으로 구현을하라고 말씀하셔서 |
추가한 webFont 에서 인식하지 못하는 url 이 생겨 ignore setting 해줘야 오류가 안생김
| return pawnRuleStrategy.get(); | ||
| } | ||
|
|
||
| public String getColor() { |
There was a problem hiding this comment.
PieceColor.name값 (WHITE, BLACK)이 아닌 화면에 표현해야되는 값 white, black이 필요하다면 이렇게 getter로 제공하기도 합니다. :)
체스 미션 제출합니다.
1.리뷰어님 질문이 있습니다. 이번에 객체의 책임 분리를 많이 고려를 해서 만들다 보니 클래스객체가 굉장히 많이 만들어져서 오히려 코드를 이해하는데 있어서 복잡성이 올라가 코드 이해를 방해하는 요소가 된거 같아서
"지금 객체 분리를 너무 불필요하게 많이 한것인가 아닌가?"에 있어서 페어와 의문이 계속 들었습니다.
이렇게 객체가 분리가 되었지만 코드를 이해하는데 있어서 오히려 마이너스 요소가 된다면
객체분리를 과도하게 분리해서 객체의 분리를 잘못한것인지 아니면 이런식으로 하는 방법이 맞는 방법인지 질문드립니다.!
2.view의 도메인 의존성에 대해 질문입니다.
view가 도메인의 로직을 알고있으면 안된다는 말을 들었는데 이 말에 대해 의문이 생겨서 질문드립니다!
라는 outputView를 만들었는데 view에 도메인과 의존성이 없어야 한다는 이 말이 제가 짠 코드처럼
scoreOf 메서드 안에서 파라미터로 받은 도메인의 객체를 저장하지 않고 메서드를 불러서 출력해야 한다는 말인 것인가요?
즉, 파라미터로 받은 도메인 객체를 메서드에서 저장해서 직접 데이터를 조작하는 행위를 하지 말라고 도메인 객체의 매서드를 불러서 활용해라 라는 말인지
View에서 파라미터조차 domain 인스턴스를 받으면 안되는 것인지
즉,controller에서 getter로 상태를 넘겨주는식으로 짜야하는 것인지 궁금합니다!!
3.mvc에 대해 구글링 하던 도중에 controller를 여러개를 갖을수 있고 컨트롤러는 서로에 대해 모르면서 라우터(?)에서 각 컨트롤러를 다룬다고 나와있는 설명을 보았는데 해당 글을 보고 command와 chessboard controller를 따로 나누고 커맨드에 따라 main에서 게임이 흘러가는 식으로 다뤄 주려고 해봤는데 해보면서 뭔가 잘못접근한건가 하는 생각이 들어라구요..
그래서 혹시 리뷰어님께서 간단한 예시를 들어주실수 있나 해서 마지막 질문도 달아봅니다!!