🚀 사이클1 - 미션 (보드 초기화 + 기물 이동) 밍구 미션 제출합니다.#240
🚀 사이클1 - 미션 (보드 초기화 + 기물 이동) 밍구 미션 제출합니다.#240koomingu wants to merge 66 commits intowoowacourse:koomingufrom
Conversation
younghoondoodoom
left a comment
younghoondoodoom
left a comment
There was a problem hiding this comment.
안녕하세요 밍구! 리뷰어 두둠입니다. 이번 미션도 잘 부탁드립니다 🙌
이번 미션도 잘 구현해주셨는데, 요구사항을 충족하지 못하는 동작과 프로그래밍 요구사항을 지키는걸 놓치신거 같아요. 그 부분 코드 리뷰로 남겼으니 수정 부탁드립니다! 특히 프로그래밍 요구사항은 객체지향 프로그래밍을 훈련하기 위함이니 꼭 지켜서 수정해주세요 💪
다음은 질문에 대한 답변입니다.
Piece라는 클래스를 인터페이스화해서 기물이 구현하는 방식 vs 이동 방식을 추상화하여 구현체에서 구현하는 방식
이 부분은 코드 리뷰 코멘트를 통해 이야기 나눠보시죵
객체 간 정보 전달에 DTO를 사용하는 것
DTO는 Data Transfer Object의 약자로 데이터를 전달하는 객체입니다. 컨트롤러와 뷰 사이의 객체 값 전달만 DTO로 하는건 아닙니다. 객체 간에 값을 전달할때 사용되는 범용적인 개념입니다.
하지만 모든 객체 간에 값을 전달할때 DTO를 사용하는건 아닙니다. 보통 객체 사이에 값을 전달할때 변경에 따른 영향범위를 줄이기 위해 사이에 DTO를 두고 통신하는 경우가 많습니다. 그래서 레이어 간 통신할때 DTO를 보통 사용하곤합니다~
이동 전략에서 포지션 객체를 생성해도(& 가져도) 되는가?
제 생각에는 Position은 단순히 값 객체이기 때문에 이동 전략(MoveStorage)에서 사용되어야할것 같습니다. 왜 사용하는게 이상하다고 생각하신걸까요?
빈 기물을 표현하는 방식
지금처럼 아예 정의하지 않는 방식보다 명시적으로 빈 기물이라고 표시하는 객체를 만드는게 가독성이 좋다고 생각합니다~!
| for (int i = 0; i < 10; i++) { | ||
| while (true) { | ||
| try { | ||
| List<Integer> positions = inputView.playTurn(currentTeam.getTeam()); | ||
| Position from = Position.of(Row.of(positions.get(0)), Column.of(positions.get(1))); | ||
| Position to = Position.of(Row.of(positions.get(2)), Column.of(positions.get(3))); | ||
|
|
||
| board.move(from, to); | ||
| outputView.printBoard(BoardDto.from(board)); | ||
|
|
||
| currentTeam = currentTeam.switchTeam(); | ||
| break; | ||
|
|
||
| } catch (BusinessException e) { | ||
| outputView.printErrorMessage(e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
3 depth네요. 1depth를 유지하도록 수정해보시죠!
| if (fromX == toX) { | ||
| int start = Math.min(fromY, toY) + 1; | ||
| int end = Math.max(fromY, toY); | ||
|
|
||
| for (int i = start; i < end; i++) { | ||
| Position position = Position.of(Row.of(fromX), Column.of(i)); | ||
| if (boardState.hasPieceAt(position)) { | ||
| return false; | ||
| } | ||
| } | ||
| } |
|
|
||
| import java.util.List; | ||
|
|
||
| public class GungAndSaMoveStorage implements MoveStorage{ |
There was a problem hiding this comment.
궁과 사를 하나의 클래스로 만드셨네요. 따로 분리하지않고 하나로 만든게 유지보수 관점에서 어떤점이 유리하다고 생각하셨나요?
There was a problem hiding this comment.
궁과 사는 다른 객체이지만 둘은 궁성 내부에서만 이동 가능하며 이동 규칙이 똑같습니다. 따라서 구현 코드가 같은데, 둘을 따로 만들게 되었을 때 이동 규칙이 바뀐다면 중복된 두 클래스를 모두 수정해야 합니다. 따라서 한 클래스에서 관리하도록 하였습니다. 궁과 사의 이동 규칙이라는 점을 직접적으로 전달하고 싶어 클래스 명을 GungAndSaMoveStorage로 지었습니다만, 두 객체를 한 클래스에서 관리한다는 느낌을 줄이기 위해 PalaceMoveStorage로 클래스명을 바꾸는 건 어떻게 생각하시나요?
| board.put(Position.of(Row.of(1), Column.of(2)), | ||
| new Piece(new SangMoveStorage(), Team.HAN, 7, "包")); | ||
| board.put(Position.of(Row.of(7), Column.of(2)), | ||
| new Piece(new SangMoveStorage(), Team.HAN, 7, "包")); |
There was a problem hiding this comment.
포가 전부 상의 움직임 규칙으로 초기화 되고있습니다.
| private static void SetHanPieces(Map<Position, Piece> board) { | ||
| // 차 | ||
| board.put(Position.of(Row.of(0), Column.of(0)), | ||
| new Piece(new ChaMoveStorage(), Team.HAN, 13, "車")); |
There was a problem hiding this comment.
위치, 점수, 이름 데이터가 factory에서 관리되고있으니 헷갈릴만한 여지가 많은거 같습니다. 어떻게 하면 성격이 같은 데이터끼리 모아서 관리할 수 있을지 고민해보세요!
| Position from = Position.of(Row.of(positions.get(0)), Column.of(positions.get(1))); | ||
| Position to = Position.of(Row.of(positions.get(2)), Column.of(positions.get(3))); |
There was a problem hiding this comment.
controller에서 input의 형식을 알고있습니다. input에서 몇번째가 누구의 입력인지를 파악해서 추출하는게 controller의 책임일까요?
| Position from = Position.of(Row.of(positions.get(0)), Column.of(positions.get(1))); | ||
| Position to = Position.of(Row.of(positions.get(2)), Column.of(positions.get(3))); | ||
|
|
||
| board.move(from, to); |
There was a problem hiding this comment.
팀에 대한 정보는 보드를 옮길때 넘겨주지 않습니다. 선택한 기물이 어떤 팀인지에 판단은 어떤 객체에서 하고있나요?
| break; | ||
|
|
||
| } catch (BusinessException e) { | ||
| outputView.printErrorMessage(e.getMessage()); |
There was a problem hiding this comment.
사용자가 실수로 잘못된 입력을 한 경우 어플리케이션이 그냥 끝나겠군요. 사용자 입장에서 한참 게임하고있는데 게임이 끝나버리는 경우가 생길거 같습니다.
| import janggi.domain.Position; | ||
|
|
||
| public interface MoveStorage { | ||
| boolean canMove(Position from, Position to, BoardState boardState); |
There was a problem hiding this comment.
Piece라는 클래스를 인터페이스화해서 기물이 구현하는 방식 vs 이동 방식을 추상화하여 구현체에서 구현하는 방식
이동 방식을 추상화해야한다는건 동의합니다. 다만, 추상화하는 방법은 여러개가 있습니다. Piece를 인터페이스로 만드는 방법도 있고, 추상클래스로 만드는 방법 등이 있을 거 같아요. 그 행위를 하는 메소드 하나만 추상화하면 되는데 인터페이스로 분리한거라면 명확한 이유가 있어야한다고 생각합니다. 인터페이스로 해당 동작을 분리한 이유가 뭔가요?
| if (fromX == toX) { | ||
| int start = Math.min(fromY, toY) + 1; | ||
| int end = Math.max(fromY, toY); | ||
|
|
||
| for (int i = start; i < end; i++) { | ||
| Position position = Position.of(Row.of(fromX), Column.of(i)); | ||
| if (boardState.hasPieceAt(position)) { | ||
| Piece jumpPiece = boardState.getPieceAt(position); | ||
| if (jumpPiece.getName().equals("包")) { | ||
| return false; | ||
| } | ||
| jumpCount++; | ||
| } | ||
| } | ||
| } |
3 participants
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
Piece라는 클래스를 인터페이스화해서 기물이 구현하는 방식 vs 이동 방식을 추상화하여 구현체에서 구현하는 방식
상황: 초반에는 이동 방식을 추상화시키는 게 좋을 것 같다는 예감에 이동 방식을 인터페이스화하는 것을 선택하여 구현하였습니다.
질문: 기물을 뜻하는 Piece 클래스와 이동 방식을 나타내는 클래스 중 하나를 인터페이스화하여 구현해야 하는데, 두둠은 설계 단계에서 이러한 상황일 때 어떻게 선택하는지 기준이 있는지 궁금합니다.
상황: 이동 전략을 추상화시켰기 때문에 궁과 사의 이동 전략이 같아 한 클래스로 구현할 수 있었습니다.
질문: 궁과 사는 다른 객체이지만 이동 전략이 같기 때문에 한 클래스로 구현해도 되는지 궁금합니다.
객체 간 정보 전달에 DTO를 사용하는 것
이동 전략에서 포지션 객체를 생성해도(& 가져도) 되는가?
빈 기물을 표현하는 방식