[1, 2단계 - 체스] 구름(김민수) 미션 제출합니다.

[alstn113]

안녕하세요 스티치! 구름 ⛅️ 입니다.

미션 이외에 다른 이슈들이 많아서 시간적으로 촉박했습니다.
최대한 기능 요구 사항을 만족시키는 걸로 초점을 맞췄습니다.

리뷰 잘 부탁드립니다!

[lxxjn0]

안녕하세요 구름, 이번에 체스 미션을 리뷰하게 된 스티치입니다.

코드를 전체적으로 정말 잘 짜주신 것 같습니다. 너무 과하거나 부족하지 않은 군더더기 없는 사이즈의 코드 같습니다. 제가 일차적으로 보이는 부분에 대해 피드백 남겨드렸으니 한번 확인 후 반영 부탁드릴게요 👍🏼

[lxxjn0]

종료를 제외하면 이 두 명령은 동일하게 체스보드를 출력하는 것으로 보여집니다. 그렇다면 커멘드에 대한 행위에서는 출력을 제외하는 것은 어떨까요? 그렇다면 현재 클래스에서 도메인 로직만 한번 더 뽑아내서 따로 관리해 볼 수도 있을 것 같습니다.

[alstn113]

커맨드의 행동에 분리가 정확히 무엇을 의미하는지 모르겠어요.
예를 들어 설명해주실 수 있나요?

그리고 대신에 ChessGame을 Controller처럼 썼다가 ChessController를 만들고 ChessGame을 도메인으로 분리했어요.
이를 통해 게임 상태와 체스판에 대한 정보를 Controller에서 분리했어요.

public class ChessGame {
    private final Board board;
    private GameState gameState;

    public ChessGame(Board board) {
        this.board = board;
        this.gameState = new ReadyState();
    }

    public void start() {
        this.gameState = gameState.start();
    }

    public void movePiece(String source, String target) {
        Position sourcePosition = Position.convert(source);
        Position targetPosition = Position.convert(target);

        this.gameState = gameState.move(board, sourcePosition, targetPosition);
    }

    public void end() {
        this.gameState = gameState.end();
    }

    public boolean isPlaying() {
        return gameState.isPlaying();
    }

    public Board getBoard() {
        return board;
    }
}

[lxxjn0]

제가 설명이 좀 부족했네요 😢 그런데 저의 의도대로 잘 바꿔주신 것 같습니다 👍🏼

[lxxjn0]

구름, 피드백 반영하시느라 수고하셨습니다.

사실 코드가 이미 전체적으로 깔끔하게 잘 구조가 잡혀있어서 추가로 피드백 드릴만한 부분은 더 보이질 않네요 👍🏼 이번 단계는 여기서 마무리하고 다음 단계에서 조금 더 리뷰 진행해보도록 할게요 :)
남겨둔 피드백은 다음 단계에 반영해주세요

[lxxjn0]

제가 설명이 좀 부족했네요 😢 그런데 저의 의도대로 잘 바꿔주신 것 같습니다 👍🏼

[lxxjn0]

동일한 객체 내의 메서드를 파라미터로 넘길 이유가 있을까요? 내부에서 직접 executeCommand를 호출하면 될 것 같습니다.

[alstn113]

이해가 잘 안됩니다.
혹시 아래와 같이 수정하라는 말씀이실까요??
예외가 발생했을 때 재입력 받는 다른 메서드가 있을 경우 retryOnException을 재사용할려고 의도했었습니다.

private void retryOnException() {
    try {
        executeCommand();
    } catch (RuntimeException e) {
        OutputView.printErrorMessage(e.getMessage());
        executeCommand();
    }
}

[lxxjn0]

현재는 해당 라인 말고는 사용하는 곳이 없지 않나요? 다른 곳에서 사용할 수도 있다는 가정으로 개발을 진행한다면 어디서도 위와 같은 맥락으로 코드를 작성할 수도 있습니다. 확장성을 고려하는 의미는 어떠한 상황에서도 대응이 되는 코드가 아닌, 확장이 발생할 수 있는 가능성이 있거나 계획이 되어있는 부분에 대한 대응이라고 생각합니다. 해당 부분은 그러한 맥락에 해당되지 않기에 구름이 코멘트로 작성해주신 방법대로 코드를 쓰는 것이 올바른 방향일 것 같습니다 :)

[alstn113]

좋은 것 같습니다!
이름도 구체적으로 "repeatUntilValidCommand" 라고 변경했습니다.

[lxxjn0]

제가 생각했던 방향은 CommandCodition의 필드로 GameCommand와 Args를 가지는 형태였습니다. 명령어의 입력은 start, end, move arg1 arg2 의 형태만 가능합니다. 그렇다면 CommandCodition을 생성할 때 첫번째 문자열의 값을 GameCommand로 생성하고, 이 때 GameCommand가 가지는 argsCount를 통해 args의 유효성을 검증한 후 생성할 수 있지 않을까요? GameCommand에 CommandCodition 전체를 넘겨주는 것이 약간 어색하게 느껴졌습니다. 명령어(start,end,move)만을 관리하는 객체에 명령어 묶음을 관리하는 객체 전체를 넘기기보다는 앞서 제가 말씀드린 방법은 어떨까 싶습니다 :) 한번 참고 부탁드릴게요.

[alstn113]

제가 말씀해주신 것을 제대로 이해했나 모르겠네요.
혹시 몰라서 수정에 대한 링크를 남기겠습니다.
https://github.dev/alstn113/java-chess/tree/step2
ChessController, GameCommand, CommandCondition을 변경했습니다.

package chess.domain.command;

import java.util.List;

public record CommandCondition(GameCommand gameCommand, List<String> args) {
    private static final int COMMAND_INDEX = 0;

    public static CommandCondition of(List<String> inputCommand) {
        GameCommand gameCommand = GameCommand.from(inputCommand.get(COMMAND_INDEX));
        List<String> args = inputCommand.subList(COMMAND_INDEX + 1, inputCommand.size());

        validateIsSameArgsCount(gameCommand, args);

        return new CommandCondition(gameCommand, args);
    }

    private static void validateIsSameArgsCount(GameCommand gameCommand, List<String> args) {
        if (!gameCommand.isSameArgsCount(args.size())) {
            throw new IllegalArgumentException("명령어의 인자 개수가 올바르지 않습니다.");
        }
    }

    public String getArg(int index) {
        if (args.size() < index) {
            throw new IllegalArgumentException("인자가 부족합니다.");
        }

        return args.get(index);
    }
}

getSource와 getTarget이라는 메서드를 사용했었습니다.
하지만 move 이외에는 사용하지 않기도 하고, 사용할 경우 index out of bound 에러가 발생합니다.
그래서 getArgs로 변경하고 검증 로직을 추가했습니다.

[lxxjn0]

GameCommand에서 전달받는 argument의 개수를 관리하고 있지 않나요? 그렇다면 아래와 같은 형식의 코드는 어떠신가요?

public record CommandCondition(GameCommand gameCommand, List<String> arguments) {

    private static final int COMMAND_INDEX = 0;

    public CommandCondition(GameCommand gameCommand) {
        this(gameCommand, List.of());
    }

    public static CommandCondition of(List<String> commandInput) {
        GameCommand gameCommand = GameCommand.from(commandInput.get(COMMAND_INDEX));
        
        if (gameCommand.hasArguments()) {
            validateArgumentsSize(commandInput, gameCommand.getArgumentCount());
            List<String> arguments = findArgumentsByCount(commandInput, gameCommand.getArgumentCount());

            return new CommandCondition(gameCommand, arguments);
        }

        return new CommandCondition(gameCommand);
    }

    private static List<String> findArgumentsByCount(List<String> commandInput, int argumentCount) {
        int argumentStartIndex = COMMAND_INDEX + 1;

        return IntStream.range(argumentStartIndex, argumentStartIndex + argumentCount)
                .mapToObj(index -> commandInput.get(index))
                .toList();
    }

    private static void validateArgumentsSize(List<String> commandInput, int argumentCount) {
        // ...
    }

    public String getSourcePosition() {
        if (gameCommand != MOVE) {
            throw new IllegalStateException("...");
        }

        return arguments.get(0); // 0도 상수로 처리
    }

    public String getTargetPosition() {
        // 위와 마찬가지
    }
}

이런 식이 될 수 있을 것 같아요. 또는 List<String> arguments가 아닌 sourcePosition, targetPosition으로 직접 저장해서 관리하는 것도 방법이 될 수 있을 것 같습니다!!

[lxxjn0]

상수 네이밍 컨벤션 및 이름도 상수에 맞게 지어주세요. ofKnight와 같은 이름은 행위를 나타내는 메서드의 이름으로 보여집니다. KNIGHT_DIRECTION 등과 같은 명사형으로 작성해주세요.

[alstn113]

앗 그대로 옮기다보니 이름을 고치지 않았네요.
죄송합니다. 미션 제출부터 피드백 이후까지 계속 자잘한 실수를 하는 것 같네요.
반성하겠습니다. 😥

[lxxjn0]

Fixture를 사용하는 것에는 이견이 없지만 너무 많은 값들을 생성해서 관리하다보니 놓치는 분이 있을 수도 있을 것 같아요. 아니면 Position에 정적 팩토리 메서드를 하나 제공해보는 것도 방법이 될 수 있을 것 같아요. 예를 들어 Position.of("A2")와 같이요.

[alstn113]

Position.convert("a1")가 이미 구현되어 있기는 합니다.
근데 rank, file이 변할 경우 문제가 될까봐 fixture로 했습니다.
어떻게 생각하시나요?

[lxxjn0]

사실 체스에서의 기물 위치 표기법은 이미 전통적으로 사용되는 방식이고 변경될 여지가 없을 것 같아서 상관이 없을 것 같습니다. 변경이 일어날 가능성이 높은 케이스에 대해서 대응하는 개발이 되고 그 외로는 현재 주어진 요구사항에 맞는 개발을 진행하셔도 충분할 것 같아요.

확장성을 고려한 코드는 트레이드 오프로 복잡성을 야기하게 됩니다. 이 둘은 함께 가져가기 어려운 속성이니 적절한 범위 내에서 결정을 해야합니다. 꼭 필요한 확장이 아닌 이상은 주어진 요구조건에 맞는 개발을 하는 것이 좋다고 생각합니다 :)