Skip to content

Conversation

@junseoplee
Copy link

No description provided.

@poi1649 poi1649 changed the base branch from main to junseoplee July 26, 2024 09:53
Comment on lines 66 to 68
public void printWinningColor(Color currentTurn) {
System.out.println("Winning Color: " + currentTurn);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매개변수 이름이 currentTurn인 이유는 뭔가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런. 리팩토링 업데이트가 안됐네요

System.out.println("> 게임 시작 : start");
System.out.println("> 게임 종료 : end");
System.out.println("> 게임 이동 : move source위치 target위치 - 예. move b2 b3");
System.out.println("> 게임 이동 : move source 위치 target 위치 - 예. move b2 b3");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

디테일 👍

Comment on lines -58 to +66
private Path findPathForAttackMovement(final Position to, final Movement movement) {
private Path findPathForAttackMovement(final Position to, final Movement movement, final Map<Position, Piece> board) {
validateMovement(movement, AVAILABLE_ATTACK_MOVEMENTS.get(getColor()));

validateAttackMovement(to, board);
return new Path(List.of(to));
}
private void validateAttackMovement(final Position to, final Map<Position, Piece> board) {
Piece targetPiece = board.get(to);
if (targetPiece == null || targetPiece.getColor() == this.getColor()) {
throw new IllegalArgumentException(ErrorMessage.INVALID_DIRECTION.getMessage());
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 것 처럼 처음에 발견하지 못한 규칙들을 나중에 발견하게 되는 경우가 꽤 있는데, 이럴 때마다 테스트 코드를 작성하는 습관을 들여놓으면 좋습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

후후 좋습니다

}

private void checkAndHandleKingCapture(Piece capturedPiece, Color currentTurn) {
if (capturedPiece != null && capturedPiece.pieceType() == PieceInfo.KING) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 단계를 하면서 느꼈겠지만, null 값을 해당 칸에 피스가 없음 처럼 비즈니스 로직을 나타내는 의미로 사용하면 여러 문제가 발생합니다.
ex. 의도한 null 인지 의도하지 않은 null 인지 확인 불가, null 검사를 빼먹을 경우 어디서 에러가 발생하는지 찾기 힘듦...

이런 문제를 해결하기 위해서 여러 방법을 사용해볼 수 있는데, 대표적으로 null object pattern 이 있습니다.

한 번 공부해보시고, 저 패턴 외에 추가적으로 어떤 방법이 있을지, 또 이런 방법들은 어떤 장단점이 있을지 공부해보셔도 좋겠네요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null 객체 패턴이라니 재밌는(?) 내용이네요 힘내서 공부해보겠습니다

Comment on lines 24 to 29
public void start() {
if (state == State.RUNNING) {
throw new IllegalArgumentException(ErrorMessage.ALREADY_RUNNING.getMessage());
}
this.state = State.RUNNING;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

END 상태에서 START를 할 수는 없는 상태인가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

할 수 있습니다! 애플리케이션에서 end로 종료한 후에 continue 로 해당 게임을 불러오면 END 상태에 있고 start 커맨드를 입력하면 RUNNING 상태가 되어 다시 게임을 시작합니다

Comment on lines +47 to +51
final String type = (String) result.get(index++);
final String color = (String) result.get(index++);
final int file = (int) result.get(index++);
final int rank = (int) result.get(index++);
board.put(new Position(file, rank), PieceFactory.getPiece(type, color));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB에 있는 값이 전부 옳은 값이라고 믿어도 괜찮을까요?
혹시 같은 file, rank로 저장된 piece가 두 개 있다면 서버는 어떤 동작을 하는 게 적절할까요?

Copy link
Author

@junseoplee junseoplee Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 문제는 도메인이 관리해야하며 도메인의 책임이라 생각합니다.
하지만 문제가 있을 경우는 다음과 같이 처리하고 싶습니다.

  1. 유니크 키 등 제약 조건을 사용하여 데이터를 삽입하기 전 동일한 위치에 두 개의 피스가 저장되는 것을 방지할 수 있습니다.
  2. insert 메서드에서 데이터를 삽입하기 전에 동일 위치에 다른 피스가 존재하는지 확인할 수 있습니다.

만약 문제가 발생한다면 예외 처리하여 클라이언트에게 오류 메세지를 반환할 수 있습니다.

Comment on lines 24 to 29
public void insert(final long chessGameId, final Position position, final Piece piece) {
final String query = "INSERT INTO piece (chess_game_id, piece_file, piece_rank, color, type) " +
"VALUES (?, ?, ?, ?, ?)";
jdbcTemplate.executeUpdate(query, chessGameId, position.getFile(),
position.getRank(), piece.getColor().name(), piece.pieceType().name());
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 코드들에 대한 테스트코드는 어떻게 작성하는게 좋을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제 데이터베이스를 사용하지 않고 별도의 테스트용 데이터베이스를 사용하는 방법이 있습니다.
H2 같은 인메모리 데이터 베이스를 의존성으로 추가하여 사용할 수도 있습니다.

import java.util.Map;

public class InitialBoard {
public class BoardFactory {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 static 메서드로만 이루어진 Factory 클래스 같은 경우 실수로 객체를 생성하지 않도록, 기본 생성자를 private으로 막아주는것도 좋습니다.

}

public void checkmate() {
this.state = State.CHECKMATE;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkmate의 정의와 다르게 사용되고 있어서 오히려 혼란을 유발할 수도 있을 것 같네요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니다. 보다 명확하게 나타낼 수 있도록 상태를 kingIsCaptured로 변경했습니다

import java.util.Map;
import java.util.function.BiConsumer;

public class ChessController {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 클래스의 메서드는 어떤 순서로 배치되어 있는건가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔쌈하게 리팩토링했습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants