다즐의 체스 게임 #8
Open
다즐의 체스 게임 #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
Co-authored-by: swonny <[email protected]>
greeng00se
reviewed
Mar 19, 2023
| end | ||
| ``` | ||
|
|
||
| ## 구현 기능 목록 |
| this.board = board; | ||
| } | ||
|
|
||
| private Color color = Color.WHITE; |
There was a problem hiding this comment.
어떤 용도로 사용하는지 변수명으로 나타내보는건 어떨까?
Comment on lines
30
to
37
| if (!piece.isSameColor(color)) { | ||
| throw new IllegalArgumentException(format("%s의 차례입니다.", color)); | ||
| } | ||
| } | ||
|
|
||
| private void moveToDestination(final Piece piece, final Square source, final Square destination) { | ||
| final List<Square> route = piece.findRoute(source, destination); | ||
| if (!isMovable(piece, source, route)) { |
There was a problem hiding this comment.
부정문은 한 번 더 생각을 해야하니까 긍정문으로 조건을 표시하는건 어때?
There was a problem hiding this comment.
!isMovable, isNotMovable은 가독성에서 큰 차이가 없다고 판단하였습니다.
위와 같은 수정이 맞나용?
Comment on lines
+30
to
+63
| private static final Map<Square, Piece> initPiece = Map.ofEntries( | ||
| Map.entry(new Square(A, ONE), new Rook(WHITE)), | ||
| Map.entry(new Square(B, ONE), new Knight(WHITE)), | ||
| Map.entry(new Square(C, ONE), new Bishop(WHITE)), | ||
| Map.entry(new Square(D, ONE), new Queen(WHITE)), | ||
| Map.entry(new Square(E, ONE), new King(WHITE)), | ||
| Map.entry(new Square(F, ONE), new Bishop(WHITE)), | ||
| Map.entry(new Square(G, ONE), new Knight(WHITE)), | ||
| Map.entry(new Square(H, ONE), new Rook(WHITE)), | ||
| Map.entry(new Square(A, TWO), new Pawn(WHITE)), | ||
| Map.entry(new Square(B, TWO), new Pawn(WHITE)), | ||
| Map.entry(new Square(C, TWO), new Pawn(WHITE)), | ||
| Map.entry(new Square(D, TWO), new Pawn(WHITE)), | ||
| Map.entry(new Square(E, TWO), new Pawn(WHITE)), | ||
| Map.entry(new Square(F, TWO), new Pawn(WHITE)), | ||
| Map.entry(new Square(G, TWO), new Pawn(WHITE)), | ||
| Map.entry(new Square(H, TWO), new Pawn(WHITE)), | ||
| Map.entry(new Square(A, SEVEN), new Pawn(BLACK)), | ||
| Map.entry(new Square(B, SEVEN), new Pawn(BLACK)), | ||
| Map.entry(new Square(C, SEVEN), new Pawn(BLACK)), | ||
| Map.entry(new Square(D, SEVEN), new Pawn(BLACK)), | ||
| Map.entry(new Square(E, SEVEN), new Pawn(BLACK)), | ||
| Map.entry(new Square(F, SEVEN), new Pawn(BLACK)), | ||
| Map.entry(new Square(G, SEVEN), new Pawn(BLACK)), | ||
| Map.entry(new Square(H, SEVEN), new Pawn(BLACK)), | ||
| Map.entry(new Square(A, EIGHT), new Rook(BLACK)), | ||
| Map.entry(new Square(B, EIGHT), new Knight(BLACK)), | ||
| Map.entry(new Square(C, EIGHT), new Bishop(BLACK)), | ||
| Map.entry(new Square(D, EIGHT), new Queen(BLACK)), | ||
| Map.entry(new Square(E, EIGHT), new King(BLACK)), | ||
| Map.entry(new Square(F, EIGHT), new Bishop(BLACK)), | ||
| Map.entry(new Square(G, EIGHT), new Knight(BLACK)), | ||
| Map.entry(new Square(H, EIGHT), new Rook(BLACK)) | ||
| ); |
There was a problem hiding this comment.
난 이것도 좋다고 생각하는데, 중복을 제거하는거에 대해선 어떻게 생각해?
There was a problem hiding this comment.
좋은 방법이라고 생각합니다 👍
수정 전에는 중복을 제거하는 방법을 사용했는데, 직관적으로도 표현해보고 싶어서 수정해보았습니다. 기물이 같은 행, 같은 열에 존재하지 않도록 변하면 해당 메서드 유지보수 비용이 더 클꺼라는 관점도 추가되었던 코드입니다.
Comment on lines
+16
to
+21
| public static Square from(final String value) { | ||
| validate(value); | ||
| final File file = File.from(value.charAt(0)); | ||
| final Rank rank = Rank.from(value.charAt(1)); | ||
| return new Square(file, rank); | ||
| } |
There was a problem hiding this comment.
Rank와 File의 index 필드를 이용해 캐싱해서 charAt을 제거해보는건 어떨까?
There was a problem hiding this comment.
이번 요구사항에 getter 사용을 지양한다라는 요구사항이 존재해서 사용하지 않았고, Map<File, Map<Rank, Square>>과 같은 구조를 만들기 싫었습니다 ..
Comment on lines
9
to
21
| NORTH(0, 1, (file, rank) -> file == 0 && rank > 0), | ||
| SOUTH(0, -1, (file, rank) -> file == 0 && rank < 0), | ||
| WEST(-1, 0, (file, rank) -> file < 0 && rank == 0), | ||
| EAST(1, 0, (file, rank) -> file > 0 && rank == 0), | ||
| SOUTHEAST(1, -1, (file, rank) -> file > 0 && rank < 0 && Math.abs(file) == Math.abs(rank)), | ||
| SOUTHWEST(-1, -1, (file, rank) -> file < 0 && rank < 0 && Math.abs(file) == Math.abs(rank)), | ||
| NORTHEAST(1, 1, (file, rank) -> file > 0 && rank > 0 && Math.abs(file) == Math.abs(rank)), | ||
| NORTHWEST(-1, 1, (file, rank) -> file < 0 && rank > 0 && Math.abs(file) == Math.abs(rank)); | ||
|
|
||
| private final int unitFile; | ||
| private final int unitRank; | ||
| private final BiFunction<Integer, Integer, Boolean> way; | ||
|
|
| private static final Scanner scanner = new Scanner(System.in); | ||
|
|
||
| public List<String> readExecuteCommands() { | ||
| return Arrays.stream(scanner.nextLine().split(" ", -1)) |
There was a problem hiding this comment.
split에서는 매직넘버를 사용하지 않아도 명시적으로 의미를 알 수 있다고 판단하였습니다!
| @Test | ||
| void 이동경로로_이동할_수_있으면_true_반환한다() { | ||
| final Board board = BoardFactory.create(); | ||
| //Knight |
There was a problem hiding this comment.
나이트의 경우라고 Nested 클래스명으로 적혀있는데 Knight의 위치라고 주석으로 명시해줄 필요가 있을까?
추가로 when절에 나이트의 위치가 있는데 given 절에서 변수명으로 충분히 표현할 수 있을 것 같아! 😄
| class ColorTest { | ||
|
|
||
| @Test | ||
| void 색은_개수가_2개이다() { |
There was a problem hiding this comment.
색은_검은색_흰색으로_구분한다 는 어떨까?
색은 개수가 2개이다 라고 되어있는데 검증부와 다른 느낌이 들어서!
drunkenhw
reviewed
Mar 20, 2023
Comment on lines
+19
to
+20
| private static final InputView inputView = new InputView(); | ||
| private static final OutputView outputView = new OutputView(); |
There was a problem hiding this comment.
클래스 필드가 늘어나는 것도 막을 수 있고, 뷰 자체로 상태를 가지지 않기에 static으로 둬도 괜찮다고 판단하였습니다!
Comment on lines
+35
to
+37
| outputView.printStartMessage(); | ||
| while (executeState != END) { | ||
| playChessGame(); |
|
|
||
| private ExecuteState readExecuteState(final List<String> commands) { | ||
| validateCommands(commands); | ||
| return ExecuteState.from(commands.get(0)); |
Comment on lines
8
to
10
| START, | ||
|
|
||
| MOVE, |
Comment on lines
13
to
19
| private final Board board; | ||
|
|
||
| public ChessGame(final Board board) { | ||
| this.board = board; | ||
| } | ||
|
|
||
| private Color color = Color.WHITE; |
| ); | ||
|
|
||
| public static Board create() { | ||
| return new Board(new HashMap<>(initPiece)); |
| } | ||
|
|
||
| public File prev() { | ||
| return File.from(((char) (index - 1))); |
Comment on lines
+25
to
+31
| public boolean hasSameColor(final Piece piece) { | ||
| return isSameColor(piece.color); | ||
| } | ||
|
|
||
| public boolean isSameColor(final Color color) { | ||
| return this.color == color; | ||
| } |
There was a problem hiding this comment.
위는 상대편인지 확인하는 메서드이고, 아래는 내 턴인지 확인하는 메서드입니다!
Comment on lines
+25
to
+28
| void 움직일_수_있다면_기물을_움직인다() { | ||
| final ChessGame chessGame = new ChessGame(BoardFactory.create()); | ||
|
|
||
| assertThatNoException().isThrownBy(() -> chessGame.move(A_TWO, A_THREE)); |
There was a problem hiding this comment.
exception을 발생하지 않는다는 것이 기물을 움직인다는 테스트가 될 수 있나요? 기물의 위치를 찾아봐야할 것 같은ㄷ데요
|
|
||
| @Test | ||
| void 검은색인지_확인한다() { | ||
| final Piece piece = new TestPiece(BLACK); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
리뷰 잘 부탁드립니다 :)