1,2,3 단계 - 체스

체스 미션 1,2,3 단계 저장소 링크

PR 링크

리뷰어: 또링

고민한 내용

상태 패턴

흑팀과 백팀의 턴을 번갈아가면서 진행해야한다. 라는 점에서 상태패턴을 떠올렸고, 적용해보았습니다. 글너데 isEnd()와 같은 메소드로 현재상태가 어떤지 확인해보아야한다는 점에서 제대로 적용한 것이 맞나 하는 생각이 들어요. (외부에서 현재 상태가 어떤지를 물어서 확인해야 하나 하는 고민이 들었습니다.)

public interface State {
    ...
    boolean isEnd();
}

또 상태패턴을 사용하면 if-else로 분기문을 줄일 수 있다고 학습하였는데, 제가 작성한 코드에서는 결국 if문으로 현재 상태를 물어야했습니다. 이 부분에 대한 리뷰어님의 의견을 듣고싶습니다.

public class ChessGame {

    private final ChessBoard chessBoard;
    private State state;

    ....

    public void progress(Command command) {
        if (command.isStart()) {
            start();
            return;
        }

        if (command.isEnd()) {
            stop();
            return;
        }

        state = state.changeTurn(command, chessBoard);
    }
}

isXXX메소드들

ChessGame 뿐 아니라 Position, Command에서 다음과 같이 isXXX()과 같은 형태의 메소드들이 존재하는데, 이런 메소드를 제거하려고 여러 고민을 해보았지만 답을 찾지 못하였다. 이와 관련한 리뷰어님의 생각을 듣고 싶습니다.

public class ChessGame {
	public boolean isExistKing() {
        return chessBoard.isExistKing();
    }
    ...
    public boolean isEnd() {
        return state.isEnd();
    }
    ...
}

public class Position {
    public boolean isSameRank(Position position) {
        Rank rank = position.getRank();

        return this.rank == rank;
    }

    public boolean isSameFile(Position position) {
        File file = position.getFile();

        return this.file == file;
    }

    public boolean isReductionRank(Position position) {
        return rank.calculateRank(position.getRank()) > 0;
    }

    public boolean isIncreaseRank(Position position) {
        return rank.calculateRank(position.getRank()) < 0;
    }

    public boolean isDiagonal(Position position) {
        Rank rank = position.getRank();
        File file = position.getFile();

        int rankDifference = this.rank.calculateAbsoluteValue(rank);
        int fileDifference = this.file.calculateAbsoluteValue(file);

        return rankDifference == fileDifference;
    }
}

public class Command {
	public boolean isEnd() {
        return command.equals("end");
    }

    public boolean isMoveCommand() {
        return command.startsWith("move");
    }

    public boolean isStart() {
        return command.equals("start");
    }

    public boolean isStatus() {
        return command.equals("status");
    }
    ...
}

해당 고민 내용에 대해서 지금와서 다시 생각해보면 이런 고민을 했던 이유가 무엇일까? 하는 생각이 든다.
즉, 근보적으로 이런 메소드들을 제거하려고 한 이유가 무엇일까?
isXXX() 메소드는 좋지 않은 메소드인가?
아마 당시에 isXXX() 형태의 메소드는 어떤 기능을 수행하는 것이 아니라 '~~이니?'라고 묻는 메소드이기 때문에 제거해볼 수 있지 않을까 하는 생각에서 시작된 고민이었다고 생각된다.
또 Command의 isXXX() 메소드를 사용하는 progress() 메소드에서 isStart() 인지를 물어보고 start() 메소드를 호출해야하나? 하는 생각에서 고민했던 것 같다.

피드백 내용

Q. 상태패턴을 적용했음에도 상태에 따라 다른 동작을 하기 위해 분기가 존재하고 있다. 이 부분도 상태 안으로 넣어볼 수 있지 않을까요?

	public void progress(Command command) {
        if (command.isStart()) {
            start();
            return;
        }

        if (command.isEnd()) {
            stop();
            return;
        }

        state = state.changeTurn(command, chessBoard);
    }

A. 해당 부분을 상태 안으로 넣음으로써 상태를 사용하는 곳, 즉 외부에서는 현재 어떤 상태인지를 알 필요없이 사용할 수 있게 된다. 개선된 코드는 다음과 같다.

public interface State {

    boolean isEnd();

    State execute(Command command, ChessBoard chessBoard);
}

public class ChessGame {
	...
    public void progress(Command command) {
        state = state.execute(command, chessBoard);
    }
    ...
}

Q. 계산하는 로직이 상태객체에 있는 이유가 무엇인가요?

public class End implements State {
	@Override
    public Map<Team, Double> status(ChessBoard chessBoard) {
        Double whiteScore = calculateScore(chessBoard, WHITE);
        Double blackScore = calculateScore(chessBoard, BLACK);

        Map<Team, Double> result = new HashMap<>();
        result.put(WHITE, whiteScore);
        result.put(BLACK, blackScore);

        return result;
    }

    private double calculateScore(ChessBoard chessBoard, Team team) {
        return chessBoard.calculateByTeam(team);
    }
}

A. Status 요구사항을 제대로 반영하려고 수정하면서 "게산 로직"을 ChessGame 으로 뺐습니다!
당시에는 별 생각없이 status라는 상태를 둬야지..근데 점수를 반환해줘야하네..하는 생각에서 상태 객체 안에 두었는데요(심지어 End 상태일 때 status() 메소드 호출이 가능하다고 요구사항을 잘못이해해서...)
또링의 말씀을 고민해보니 계산하는 로직을 상태객체에 둘 필요가 없네요...감사합니다😃

개선된 코드는 다음과 같습니다.
(status(chessBoard) 메소드는 ChessGame 으로 로직 그대로 calculateResult 라는 이름으로 이동시켰습니다.)

public class End implements State {

    @Override
    public boolean isEnd() {
        return true;
    }

    @Override
    public State execute(Command command, ChessBoard chessBoard) {
        if (!command.isEnd()) {
            throw new IllegalArgumentException("이미 게임이 종료되었습니다.");
        }
        return this;
    }
}

Q. symbol은 domain에서 관리하는 값이 맞을까요? 만약 피스를 콘솔이 아닌 이미지로 보여줘야 한다면 어떻게 될까요?

		pieces.put(Position.of(A, rank), new Rook(team, symbols.get("rook")));
        pieces.put(Position.of(B, rank), new Knight(team, symbols.get("knight")));
        pieces.put(Position.of(C, rank), new Bishop(team, symbols.get("bishop")));
        pieces.put(Position.of(D, rank), new Queen(team, symbols.get("queen")));
        pieces.put(Position.of(E, rank), new King(team, symbols.get("king")));
        pieces.put(Position.of(F, rank), new Bishop(team, symbols.get("bishop")));
        pieces.put(Position.of(G, rank), new Knight(team, symbols.get("knight")));
        pieces.put(Position.of(H, rank), new Rook(team, symbols.get("rook")));

A. 현재 여기서 나타나는 symbol은 "뷰"와 관련이 있는 값이다. 즉 도메인에서 뷰에 의존하고 있다고 볼 수 있다. 따라서 뷰가 콘솔이 아닌 이미지를 통해서 보여주겠다 라고 바뀌게 된다면 해당 코드가 변경되어야 한다.
따라서 Symbol 이라고하는 열거형을 다음과 같이 두고, 이를 뷰에 전달하여 뷰에서 적절하게 조작하여 사용자에게 보여줄 수 있도록 할 수 있다.

	public enum Symbol {
    KING("k"),
    QUEEN("q"),
    BISHOP("b"),
    KNIGHT("n"),
    ROOK("r"),
    PAWN("p");

    private final String symbol;

    Symbol(String symbol) {
        this.symbol = symbol;
    }

    public String getSymbol(Team team) {
        if (team.equals(BLACK)) {
            return symbol.toUpperCase();
        }
        return symbol;
    }
}

위 코드에서 BLACK 인 경우 대문자를 반환해주는 이유는 각 팀을 구별해주기 위해서이다.
(이것이 마음에 안든다면 뷰로 데이터를 전달할 때 Symbol과 Team을 함께 전달해주는 방법을 고려할 수 있다.)

비슷하게

	public String findWinTeam(Map<Team, Double> teamScores) {
        Double whiteScore = teamScores.get(WHITE);
        Double blackScore = teamScores.get(BLACK);

        if (isExistKing() && whiteScore.equals(blackScore)) {
            return "무승부";
        }

        Piece winKing = getWinKing();

        return winKing.getTeam().toString();
    }

위 코드도 도메인에서 view에 의존하고 있는데, Result 열거형을 추가해줌으로써 view와의 의존성을 해결해줄 수 있다.

	public Result findWinTeam(Map<Team, Double> teamScores) {
        Double whiteScore = teamScores.get(WHITE);
        Double blackScore = teamScores.get(BLACK);

        if (isExistKing() && whiteScore.equals(blackScore)) {
            return Result.DRAW;
        }

        Team winTeam = getWinTeam();
        return Result.of(winTeam);
    }

Q. chessGame의 메소드들을 하나씩 수행하기보다는 객체 스스로 일을 할 수 있도록 해볼까요?

		Map<Team, Double> teamScores = chessGame.calculateResult();

        String winTeamName = chessGame.getWinTeam(teamScores)

A. 'status' 요구사항을 반영하면서 게임을 종료시에는 점수와 함께 승자팀 출력, status 입력시에는 현재 점수만을 출력하도록 함으로써 각 팀의 점수를 출력하는 부분과 승자를 출력하는 부분을 분리할 필요가 있게 되었습니다. 따라서 이 둘을 분리하는 방향으로 리팩토링하였습니다.

Q. positions 의 키 값을 알아야 사용할 수 있겠네요. 값 객체로 감싸서 사용하는건 어떨가요? ex. moveCommand.getSource()

		Map<String, Position> positions = command.makePositions();

        Position source = positions.get("source");

A. 단순 문자열로써 positions로부터 값을 가져온다는 것은 좋지 않아보입니다. source 와 target Position을 Map으로 가지는 일급컬렉션 Positions를 도입해볼 수 있을 것 같습니다.

Q. IntelliJ가 주는 경고를 잘 살펴볼까요? Hint. 해당 메소드는 !isPosition() 형태로만 사용되고 있다.

	private static boolean isPosition(String token) {
        char first = token.charAt(FILE_INDEX);
        char second = token.charAt(RANK_INDEX);

        return File.isFile(first) && Rank.isRank(Character.getNumericValue(second));
    }

A. 이를 변경하는 것은 쉬웠지만, 메소드 명을 어떻게 바꿔야할지에 대해서는 고민이 들었다.
왜냐하면 아마 클릭코드 책에서 부정적인 메소드명을 부여하는 것은 읽는 사람으로 하여금 가독성을 해친다는 내용을 보았고, 이에 동의하였기 때문이다. (머릿속으로 한 번 더 생각하게끔 한다.)
따라서 우선은 validatePosition()이라는 메소드명을 부여했지만, 앞으로 이러한 case를 더 많이 마주할 것이라고 생각되기 때문에 더 좋은 메소드명을 어떻게 지을지 고민해보아야겠다.

	private static void validatePosition(String token) {
    	char first = token.charAt(FILE_INDEX);
        char second = token.charAt(RANK_INDEX);
        
        if (!File.isFile(first) || !Rank.isRank(Character.getNumericValue(second))) {
        	throw new IllegalArgumentException("형식이 잘못되었거나 범위를 벗어났습니다.");
        }
   	}

---

4, 5 단계 - 체스(웹)

체스 미션 4,5 단계 저장소 링크

PR 링크

리뷰어: 또링

고민한 내용

1,2,3 단계 피드백 내용이었던 "컨트롤러가 하는 일이 너무 많은 것 같다. 컨트롤러는 딱 컨트롤러의 역할만 하도록 만들어 보자." 를 보고, 컨트롤러의 역할은 무엇인가? 고민하였다.

내가 생각하는 컨트롤러는 도메인과 뷰에 대해서 알고 있고, 이 둘 사이의 데이터 전달 등 직접적인 의존을 끊고 중간다리 역할을 하기 위해서 존재한다고 생각한다.

이러한 관점에서 내가 만든 "ChessController"는 주어진 역할을 충분히 수행한다고 생각하였었다.

하지만 여러가지 고민을 하고 보니, 뷰 출력을 위해서 "캐싱"해둔 Position 리스트 그리고 이에 대한 정렬을 수행해주고 있었다.

그런데 이것이 진짜 Controller의 역할인가?? ChessGame이라는 체스게임 진행과 관련된 책임을 가지는 도메인의 역할은 아닐까?
여기서 symbol등를 만들어주고, 이를 반환해주면 되지 않을까? 라는 생각이 들었고, 수정하게 되었다.

피드백 내용

• 파일 끝부분에 개행을 하지 않으면 git에서 merge하는 과정에서 파일의 끝부분이 꼬여서(EOF) 이상하게 동작할 수 있다.

Q. 불필요한 공백이 많아 보인다. 가독성을 위해 필요한 부분만 공백을 추가해볼까요?

		get("/start", (request,response) -> {
            Map<String, Object> model = new HashMap<>();

            String gameName = request.queryParams("game_name");

            List<String> chessBoard = chessService.findByName(gameName);

            model.put("chessboard", chessBoard);

            return render(model, "chess.html");
        });

A. 이 부분이 항상 어려운 것 같아요..클린코드 책을 읽을 때, 연관있는 '개념'을 단위로 줄바꿈을 해주라는 내용을 보았던 것으로 기억하는데요..!
저는 이 부분을 서로 연관있는 개념끼리는 공백을 주지 않고, 각 개념이 서로 연관성이 떨어진다면 공백을 추가하라는 의미로 이해했어요!

예를 들어 status 부분에서 model.put()들은 서로 연관이 있는 내용이기 때문에 공백으로 구분하지 않았습니다.

	get("/status", (request, response) -> {
            Map<String, Object> model = new HashMap<>();

            Map<Team, Double> score = chessService.getScore();

            List<String> chessBoard = chessService.getCurrentChessBoard();

            model.put("blackScore", score.get(BLACK));
            model.put("whiteScore", score.get(WHITE));
            model.put("chessboard", chessBoard);

            return render(model, "chess.html");
        });

말씀해주신 start 부분에서는 음....38번 라인과 40번 라인 정도가 연관이 있어 보여요..!
혹시 리뷰어라면 어떻게 수정 하실지...궁금합니다!!

아하 그런 기준이었군요! 저도 디우와 마찬가지로 연관성이 떨어진 부분은 개행을 하고 있어요. 다만 개인마다 생각이 다르기 때문에 차이가 좀 있는 것 같네요. ㅎㅎ 이 부분은 디우의 생각대로 가져가도 좋을 것 같아요.
만약 저라면 게임찾기/데이터 넘겨주기 정도로 나눌 것 같아요 :)

		get("/start", (request,response) -> {
            String gameName = request.queryParams("game_name");
            List<String> chessBoard = chessService.findByName(gameName);
            
            Map<String, Object> model = new HashMap<>();
            model.put("chessboard", chessBoard);
            return render(model, "chess.html");
        });

결론: 리뷰어님의 고백 기준을 참고하고 보니, 너무 작은 단위 즉 본인의 경우처럼 코드 한줄한줄의 의미를 기준으로 공백을 추가하는 것도 좋지만, 큰 개념 위주로 분리하는 것도 좋다는 생각을 하였다. 예를 들어 리뷰어님처럼 "게임찾기/데이터 넘겨주기" 와 같이 분리하는 것도 좋은 방법이라고 생각한다. -> 메소드 흐름을 한줄한줄이 아니라 개념별로 이해할 수 있도록 돕는 느낌이 들었다.

Q.DTO는 왜 사용할까? 또 DTO에서 도메인을 필드로 가지게 된다면 어떤 문제가 있을까?

A. DTO(Data Transfer Object)는 말 그대로 비즈니스 로직과 뷰, 컨트롤러 등 계층 사이의 데이터 전송을 위한 객체라고 이해하고 있다. 따라서 별도의 비즈니스 로직을 갖지 않고, Getter(와 Setter) 메소드만 가진 객체라고 생각한다.

이러한 DTO를 사용하는 이유는 도메인의 모든 정보를 view에 전달할 이유가 없기 때문이다. 만약 DB의 PK를 도메인에서 필드로 함께 가진다고 하면 이를 프런트에 보여줄 이유가 없다. 예를 들어 User(userId, password, address, email ...)와 같은 도메인이 있다고 하면 우리가 view에 전달하고 싶은 데이터는 userId, address, email인데 User자체를 뷰에 넘기면 password에 대한 접근 가능성이 존재하게 된다. 따라서 UserDto(userId, password, address, email, ...)과 같은 DTO를 만들어 사용한다고 생각한다.
또한 현재 코드의 경우 사용자의 입력(form)이 하나의 문자열(String)이기 때문에 별도의 DTO를 사용하고 있지는 않지만, 만약 회원가입과 같이 여러 데이터를 한 번에 form으로 전송하는 경우 DTO로 만들어 사용자가 form에 넣은 데이터를 DTO로 전달하기 위해서 사용한다고 생각한다.(request DTO)

정리하면 다음과 같다.

• 도메인의 모든 벙보를 view에 전달할 필요가 없다.
• 사용자 요청을 전달하기 위해서 DTO를 사용한다.

즉 위의 두 내용을 요약해보면 계층간 데이터 전송을 위해서 DTO를 사용한다고 볼 수 있다.

이러한 DTO에서 도메인을 필드로 가진다면 이러한 DTO 사용 이유가 사라진다고 생각하는데, 도메인의 모든 정보를 전달할 필요가 없으므로 view에 전달하지 않을 불필요한 데이터들을 보호하기 위해서(도메인을 보호하기 위해서) DTO를 사용하는 것인데, 이를 필드로 가진다면 도메인에 접근할 수 있다는 이야기이고, 이는 DTO 사용 이류를 무색하게 만든다고 생각한다.

view에 필요한 정보만 내려주기 위해 사용하기도 하지만 다른 이점들도 있다. 다음 글을 참고해보자.
요청과 응답으로 엔티티 대신 DTO를 사용하자

Q. try-with-resources에 대해 학습하셨군요. 추가로 JDBC의 Connection, Statement, ResultSet close 잘하기를 참고해볼까요?

A. 알려주신 글과 함께 개발자의 실수를 줄여주는 java.sql.Connection 만들기 를 읽고 개선해보았습니다.
이전에 Connection 을 close 하는 이유는 알겠지만 Statement와 ResultSet을 close하는 이유를 모르겠다고 말씀드렸었는데요,

DB 와의 연결을 관리하는 Connection 인스턴스의 경우 사용한 뒤 반납하지 않으면 계속해서 연결을 유지하고 어느 시점에는 DB 연결이 부족한 상황이 발생할 수 있어 사용하고 나서는 resource를 해제해주어야한다고 이해하였습니다! (동일한 이유로 커넥션 풀을 사용하기도 하는 것으로 알고 있습니다!)

위 글(개발자의 실수를 줄여주는 java.sql.Connection 만들기)을 읽고, Statement와 ResultSet을 close해줘야 하는 이유를 알 수 있었습니다.

• Statement를 닫지 않을 경우, 생성된 Statement의 개수가 증가하여 더 이상 Statement를 생성할 수 없게 된다.
• close() 하지 않으면 불필요한 자원 (네트워크 및 메모리)을 낭비하게 된다.

그리고 추가적으로 JDBC API 를 직접 읽어보진 않았지만 블로그 글 내용을 참고하여 다음의 내용을 알 수 있었습니다.

• Statement를 close() 하면 Statement의 현재(즉, 가장 최근에 생성한) ResultSet 도 close() 된다.
• ResultSet은 그 ResultSet을 생성한 Statement가 닫히거나, 또는 executeQuery() 메소드를 실행하는 경우 close 된다.

즉, Statement만 잘 닫아주면 그와 관련된 ResultSet은 자동으로 닫힌다는 것을 알 수 있었습니다.