-
Notifications
You must be signed in to change notification settings - Fork 467
[1,2,3단계 - 체스] 포츈(정윤성) 미션 제출합니다. #197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
[JDK] Java8 Stream 학습 -2내용
링크
[OOP] 메소드와 클래스에도 final 을 붙인다. -3내용
링크[JDK] 컬렉션 선언 + 초기화, 람다 -2내용
Map<Integer, String> map = new HashMap<Integer, String>() {
{
put(1, "JYS");
put(2, "Fortune");
}
};
map.forEach((k, v) -> System.out.println(k + " " + v));
Map<Integer, String> map = new HashMap<Integer, String>() {
{
put(1, "JYS");
put(2, "Fortune");
}
};
map.values().forEach(System.out::println);
링크 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 포츈!
체스 미션을 함께할 재연링입니다 :)
어려운 미션이라 힘드셨을텐데 잘 구현해주셨네요 👍
질문의 답변과 피드백 남겨두었으니 확인 부탁드릴게요!!
System.out.println("> 체스 게임을 시작합니다."); | ||
System.out.println("> 게임 시작 : start"); | ||
System.out.println("> 게임 종료 : end"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 빼먹은 부분이네요!
src/main/java/chess/ChessGame.java
Outdated
return board; | ||
} | ||
|
||
public final void move(final Position startPoint, final Position endPoint, final Team team) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
게임 플레이 순서는 ChessGame에서 관리하는게 좋을 것 같은데 외부에서 관리하도록 하는 이유가 있으실까요!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 구현사항은 컨트롤러에서 턴을 주고 받는데, 체스게임에서 관리하는게 맞는거 같아요 ㅎㅎ
src/main/java/chess/ChessGame.java
Outdated
board = boardFactory.getBoard(); | ||
} | ||
|
||
public final void end() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 메서드에 final 키워드를 작성하셨네요!
클래스에 final 키워드를 붙이지 않고 메서드에 붙이신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메소드 오버라이딩을 원치 않아서, final을 붙여 놨던건데
체스게임의 경우에는 어차피 상속될일도 없으니 클래스 자체에 final 키워드를 붙이면 되겠군요!
src/main/java/chess/ChessGame.java
Outdated
private boolean isPlaying = true; | ||
private Team winner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
두 값을 상태로 가지는 요청이 왔을 때 확인하는 방법 두 가지는 장단점이 있는데 개인적으론 상태를 줄이는 것을 선호합니다
정답은 없기 때문에 어떠한 차이가 있는지 한번 고민해보면 좋을 것 같습니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 리팩토링 하면서 해당 부분도 한번 고민해볼게요 ㅎㅎ
src/main/java/chess/ChessGame.java
Outdated
private boolean isPlaying = true; | ||
private Team winner; | ||
|
||
public final void initSetting() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성 시 초기화하지 않고 초기화에 대한 로직을 분리하신 이유가 있을까요!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당시에 생성하는 작업을 따로 명명하기를 원해서 저렇게 해둔것으로 기억하는데, 재연링의 말대로 그냥 생성자에서 처리하면 될 부분인거 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다시 생각해봤는데, 저는 게임도중에 start를 또 입력받은 경우, 아예 게임을 새로 시작하기를 희망 했었어요.
때문에 initSetting 으로 초기화에 대한 로직을 분리했었는데, 재연링은 어떻게 생각하시나요?
@@ -0,0 +1,101 @@ | |||
## 기능 목록 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요구사항 작성 👍
src/main/java/docs/README.md
Outdated
|
||
## Refactoring TODO | ||
|
||
- [ ] 각 움직임에 관한 기능을 (대각선 이동은 퀸, 비숍) 인터페이스로 분리 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각 움직임에 관한 기능을 인터페이스로 분리할 경우 Board와 의존성을 제거할 수도 있습니다!
쉽지 않겠지만 한번 고민해보면 좋겠어요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이부분을 첫번째 PR 날리기전에 고민해보다가, 머리아파서 일단 리뷰 받으면서 수정하려고 PR을 보냈었어요.
짚어주신 부분들 리팩토링 하며 같이 진행해볼게요 ㅎㅎ
|
||
public class BoardFactoryTest { | ||
@Test | ||
void name() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 테스트는 미완성인걸까요..!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
으윽 저의 불찰입니다.
Position bishopPosition = bishop.getPosition(); | ||
|
||
assertThat(bishopPosition.getRow()).isEqualTo(0); | ||
assertThat(bishopPosition.getRow()).isNotEqualTo(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 코드는 없어도 괜찮을 것 같네요 :)
|
||
class KnightTest { | ||
Board board; | ||
Position whiteTeamPawnPosition = new Position(0, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공통적으로 초기화 할 작업을 선택하는 기준을 중복이 발생하는가?
의 기준이 아닌 동일한 테스트 초기화(준비) 단계가 필요한가?
라는 관점에서 본다면 좋을 것 같아요!
비슷한 맥락이긴 하지만 차이점이 분명이 존재한답니다!
위와 같은 맥락에서 봤을 때 해당 초기화는 공통된 초기화일까 고민해보아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어우.. 어제 패기있게 자기전까지 리뷰 요청 보낸다고 했는데, 이제서야 리뷰 요청 드리네요.
프로그램이 이전보다 복잡하고, 덩치가 커졌다보니까 살짝 리팩토링 하는데도 연관된게 많아서 고칠것이 연쇄적으로 우르르 쏟아지는 경험을 했어요.
다음부터는 응집도는 높고, 결합도는 낮은 구조인지를 계속 고민하며 구현 해야겠다는 생각이 팍팍 드는 리팩토링 이었습니다.
System.out.println("> 체스 게임을 시작합니다." ); | ||
System.out.println("> 게임 시작 : start" ); | ||
System.out.println("> 게임 종료 : end" ); | ||
System.out.println("> 게임 이동 : move source위치 target위치 - 예. move b2 b3" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요구사항에 맞춰서 메시지를 추가했어요!
end(); | ||
} | ||
currentTurnTeam = Team.getAnotherTeam(currentTurnTeam); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
원래 controller가 하고있던 턴 교체를 ChessGame이 수행하게 바꾸었어요
src/main/java/chess/ChessGame.java
Outdated
public final Team winner() { | ||
return winner; | ||
if (board.isEnemyKingDie(currentTurnTeam)) { | ||
return currentTurnTeam; | ||
} | ||
return Team.getAnotherTeam(currentTurnTeam); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
winner를 필드값으로 가지고 있지 않게 했어요!
public Board getBoard() { | ||
return new Board(board); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getBoard()시 일일히 초기화를 새로 하지않고, 초기화 상태로 만들어둔 board를 복사해서 던져주게 했어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로직 수정 후 get-이라는 네이밍보다는 create와 같은 네이밍이 자연스럽게 바꼈네요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create 로 메소드명을 바꾸어야겠어요!
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public enum ColumnConverter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Col이라고 축약해서 사용하고 있던것을 Column으로 바꿔주었고, 수행하고 있는 하는 역할에 맞춰서 네이밍을 변경시켜줬어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 Col이라는 이름으로 남아있는게 있는데 변수와 메스드들의 이름도 같이 변경해주면 좋을 것 같아요!
public static final int RANGE_MIN_PIVOT = 0; | ||
|
||
private final int row; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확실히 공백을 두니 가독성이 좋아지네요!
package chess.domain.pieces; | ||
|
||
import chess.domain.position.Position; | ||
|
||
public class PositionsForTest { | ||
public static final Position whiteTeamPawnPosition = new Position(0, 2); | ||
public static final Position crossBlackTeamPawnPosition = new Position(0, 0); | ||
public static final Position straightBlackTeamPawnPosition = new Position(1, 0); | ||
public static final Position crossBlankPosition = new Position(2, 0); | ||
public static final Position straightBlankPosition = new Position(2, 1); | ||
|
||
public static final Position straightCrossBlackTeamPawnPosition = new Position(3, 0); | ||
public static final Position straightCrossBlankPosition = new Position(0, 3); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드에 공통적으로 사용하는 객체들을 모아서 하나의 클래스로 만들어 주었어요.
public interface MultiMove { | ||
default boolean isMoveAbleDir(final List<Position> movablePositions, final Board board, final int nextRow, final int nextCol, final Team team) { | ||
if (!board.validateRange(nextRow, nextCol)) { | ||
return false; | ||
} | ||
if (board.piecesByTeam(team).containByPosition(new Position(nextRow, nextCol))) { | ||
return false; | ||
} | ||
if (board.piecesByTeam(Team.getAnotherTeam(team)).containByPosition(new Position(nextRow, nextCol))) { | ||
movablePositions.add(new Position(nextRow, nextCol)); | ||
return false; | ||
} | ||
return true; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공통되는 기능은 인터페이스의 디폴트 메소드를 이용하도록 했어요.
protected List<Position> getMovablePositionsByDir(final Board board, final int[] rowDirection, final int[] colDirection) { | ||
List<Position> movablePositions = new ArrayList<>(); | ||
for (int dir = 0; dir < colDirection.length; ++dir) { | ||
addMovablePositions(movablePositions, board, rowDirection[dir], colDirection[dir]); | ||
} | ||
return movablePositions; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모든 Piece가 공통적으로 사용하는 메소드는, 부모 추상 클래스까지 끌어올려주었어요.
private void pawnInitSetting(final List<Piece> black, final List<Piece> white) { | ||
List<Integer> cols = ColumnConverter.getPawnInitCols(); | ||
cols.forEach((col) -> { | ||
black.add(Pawn.of(Team.BLACK, col)); | ||
white.add(Pawn.of(Team.WHITE, col)); | ||
}); | ||
black.addAll(Pawn.getInitPawns(Team.BLACK)); | ||
white.addAll(Pawn.getInitPawns(Team.WHITE)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초기화를 할때 알고 있어야할 모든 값을 다 조합해 초기화를 하지않고,
각 Piece에게 Team타입을 던져주면 Piece가 팀에 맞춰 초기화 시키게 했어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백을 잘 반영해주셨습니다 👍
소소한 피드백 남겨두었으니 확인해주시고 다음 단계에 반영해주세요 :)
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public enum ColumnConverter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 Col이라는 이름으로 남아있는게 있는데 변수와 메스드들의 이름도 같이 변경해주면 좋을 것 같아요!
import java.util.List; | ||
|
||
public interface MultiMove { | ||
default boolean isMoveAbleDir(final List<Position> movablePositions, final Board board, final int nextRow, final int nextCol, final Team team) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default boolean isMoveAbleDir(final List<Position> movablePositions, final Board board, final int nextRow, final int nextCol, final Team team) { | |
default boolean isMovableDirection(final List<Position> movablePositions, final Board board, final int nextRow, final int nextCol, final Team team) { |
와 같이 작성하는 것이 의도가 맞았을까요!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dir이 다르게 읽힐수도 있겠네요!
return false; | ||
} | ||
if (board.piecesByTeam(Team.getAnotherTeam(team)).containByPosition(new Position(nextRow, nextCol))) { | ||
movablePositions.add(new Position(nextRow, nextCol)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 메서드의 네이밍으로 봤을 때 상태를 조회하는(Query)의 역할을 의도하신 것 같은데요!
movablePositions에 추가(Command)하는 로직과 같이 존재한다면 어떠한 문제가 있을지 고민해보아요
충분히 고민 후 명령 쿼리 분리 원칙에 대해 알아보면 도움이 될 것 같습니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
명령 쿼리 분리 원칙
처음들어보는 원칙 이네요
재연링 덕분에 새롭게 공부할것을 찾아갑니다 👍
안녕하세요 재연링, 이번에 재밌게 같이 코드리뷰 진행해봐요!
이번 체스미션은 이전의 미션들에 비해서 굉장히 난이도가 높아진 느낌이 있었어요.
재연링이 리뷰를 하기전에 미리 알아두면 좋을만한 구현 로직을 설명 해드릴게요,
저희의 경우 움직이려는
말(Piece)
이 갈수 있는 위치를 다 얻어오도록 했어요.즉 a1에 있는 말을 b2로 움직이기를 원한다면, a1기준에서 말이 움직일수 있는 좌표 (ex.. a2, a3 ,a4....) 를 다 얻어오고
그중에 b2가 포함되어 있는지 확인하게 했네요!
구현하면서 고민했던점
지금 현재 코드는 중복되는 부분이 많아요.
Rook 과는 방향값만 다르고 동일하게 동작해요
이부분을 한단계 더 추상화 해서 리팩토링을 하려고 해보았는데,
getMovablePositions()
의 경우addMovablePositions()
에 의존적이여서 인터페이스로 뺴기에도 애매하고, 추상메소드의 위로 올리기에도 애매한 구조가 되어버리더라구요.Command의 형태가 좀 기형적이에요
Enum(객체)을 따로두고 처리하게 하니 move 같은 경우 추가적인 입력을 또 받아야 하고,
status
역시 도메인 상태를 출력 해야해서 뷰와 직접적인 통신이 필요해지더라구요.이러한 부분들 때문에 enum에서 view에 접근해야 하는 이상한 구조가 되어버렸어요.
enum에서 contoller의 메소드를 호출해 뷰와 통신하는것도 이상하고, 그렇다고 enum에서 바로 view로 접근하는것은 mvc를 어기는것이라.. 고민을 많이 했네요.
일단 move 와 status 같은 경우는, Enum에서 서비스단의 메소드를 호출 하지않고, 컨트롤러에서 메소드를 호출하게 해놨어요.
근데 이렇게 보니 Command의 구현이 이상하게 되어버렸네요 ㅠ
좀 더 질문사항이 있는데! 머릿속에 정리가되면 추가하거나 DM으로 여쭈어 볼게요!