Skip to content

[1,2단계 - 체스] 켈리(양성욱) 미션 제출합니다.#643

Merged
her0807 merged 54 commits intowoowacourse:kelly6bffrom
kelly6bf:step1
Mar 26, 2024
Merged

[1,2단계 - 체스] 켈리(양성욱) 미션 제출합니다.#643
her0807 merged 54 commits intowoowacourse:kelly6bffrom
kelly6bf:step1

Conversation

@kelly6bf
Copy link
Copy Markdown

안녕하세요 수달! 체스 미션 리뷰를 부탁드릴 6기 켈리입니다! 잘 부탁 드려요!!

이번 체스 미션은 어떻게든 제출만 해보자는 목표로 구현하다보니 코드도 굉장히 더럽고 프로그래밍 요구사항도 많이 준수하지 못해서 너무 마음이 아프네요... 레벨1 최종 보스라 그런지 많이 강하네요.. 😵‍💫

다음 단계 미션 진행하면서 클린하지 못한 부분들을 잘 다듬어 보겠습니다! 다시한번 잘 부탁 드려요!!

kelly6bf and others added 30 commits March 19, 2024 17:21
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Copy link
Copy Markdown

@her0807 her0807 left a comment

Choose a reason for hiding this comment

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

안녕하세요 켈리 :)

체스 미션 동작은 잘되는 것을 확인했는데요!
각 기물별 테스트가 부족한 것 같아요.

테스트가 어려운 이유는 객체간 책임이 분산되어 있거나 너무 복잡해서 그럴수도 있기 때문에
게임의 핵심 객체인 "말"에 대해서 조금 더 고민해보고 리팩터링이 필요할 것 같아서
빠르게 리뷰 요청보냅니다!

어려운 미션인데 구현하느라 고생많으셨어요 😊

고민해보고 질문남겨주세요~

docs/README.md Outdated
Comment on lines +8 to +11
- [x] 체스판을 출력한다.
- [ ] 체스 기물을 이동시킨다.
- [ ] 이동이 가능 여부를 확인한다.
- [ ] 기물 위치를 업데이트한다.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ㅎㅎ 완료여부를 표시하다가 멈추셨군요!

@@ -0,0 +1,61 @@
package controller;

//import domain.*;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

불필요한 주석은 제거해도 좋을 것 같아요!

Comment on lines +14 to +17
private static final String START_COMMAND = "start";
private static final String MOVE_COMMAND = "move";
private static final String END_COMMAND = "end";
private static final Map<String, GameCommand> gameCommands = Map.of(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

켈리~

현재 게임은 한번 시작하고, 또 시작을 하면 게임이 예상치 못하게 종료되어요.
이유는 현재 게임의 상태를 보관하고, 상태에 맞는 예외처리가 각각 이뤄져야하는데 그러지 못한 것 같아요.

상태에 맞는 행위를 하도록 구현해볼까요?

  • start
  • move b2, b3
  • start

hit Menu 라는 인터페이스 play 라는 행위만 있고, 이것에 구현체로 move, start, end, .. 등등이 있으면 어떨까요?


import java.util.Set;

public class Piece {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

말이라는 객체가 가지는 속성이 어떻게 되나요?

  • 말이름
  • 흰색인지, 검은색인지
  • 말이 갈 수 있는 위치

이렇게 말자체에서 드러나야하는 값들이 안보이는 것 같아요!

말 객체에 대한 고민을 조금 해보고 다시 구현해볼까요?

추가로, 말은 어떤 색인지, 움직일 수 있는지 스스로 검증할 수 있어야한다고 생각하는데요.
여기서는 구체적으로 어떤 말이 어떻게 움직이는지 알 수 없네요.

바로 직전에 상속과 조합을 배우셨습니다!

말이라는 큰 개념이 있고, 그 말은 아래와 같은 공통 행위가 있으며,

각각 말마다 고유한 움직임이 있으니 상속 형태의 객체로 구현해보는 것은 어떨까요?

  • 움직인다
  • 색을 가지고 있다.

docs/README.md Outdated
- 폰
- [ ] 한 칸만 앞으로 전진만 할 수 있다.
- [ ] 다른 말을 공격하는 경우 대각선 방향으로 한 칸만 이동할 수 있다.
- [ ] 최초로 이동할 경우 한 칸 또는 두 칸 이동할 수 있다.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

폰이 첫 수일 경우 예외 사항

  • 스타트 지점에서 1칸 또는 2칸 이동이 가능하다.
  • [예외] 이동 경로에 다른 말이 있다면 예외를 던진다.

docs/README.md Outdated
- [ ] 동서남북 방향으로 한 칸 전진 후, 전진한 방향에서 대각선으로 한 칸 이동할 수 있다.
- [ ] 다른 말을 뛰어넘을 수 있다.
- 폰
- [ ] 한 칸만 앞으로 전진만 할 수 있다.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

한칸만 전진할 경우

  • [예외] 도착 위치에 말이 있다면 예외를 던진다.

docs/README.md Outdated
- [ ] 다른 말을 뛰어넘을 수 있다.
- 폰
- [ ] 한 칸만 앞으로 전진만 할 수 있다.
- [ ] 다른 말을 공격하는 경우 대각선 방향으로 한 칸만 이동할 수 있다.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • [예외] 도착 위치에 같은 팀의 말이 있다면 예외를 던진다.

docs/README.md Outdated
- 나이트
- [ ] 동서남북 방향으로 한 칸 전진 후, 전진한 방향에서 대각선으로 한 칸 이동할 수 있다.
- [ ] 다른 말을 뛰어넘을 수 있다.
- 폰
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • 폰이 움직일 수 없는 명령이 들어오면 예외를 발생시킨다.

docs/README.md Outdated
- 비숍
- [ ] 대각선 방향으로 직진할 수 있다.
- 퀸
- [ ] 동서남북, 혹은 대각선 방향으로 직진할 수 있다.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

        - [x] [예외] 말의 규칙과 경로가 일치하는지 확인한다.
        - [x] [예외] 이동 경로에 다른 말이 있다면 예외를 던진다.
        - [x] [예외] 도착 위치에 같은 팀의 말이 있다면 예외를 던진다.

docs/README.md Outdated
- 록
- [ ] 동서남북 방향으로 직진할 수 있다.
- 비숍
- [ ] 대각선 방향으로 직진할 수 있다.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

        - [x] [예외] 말의 규칙과 경로가 일치하는지 확인한다.
        - [x] [예외] 이동 경로에 다른 말이 있다면 예외를 던진다.
        - [x] [예외] 도착 위치에 같은 팀의 말이 있다면 예외를 던진다.

@her0807
Copy link
Copy Markdown

her0807 commented Mar 21, 2024

각 기물별 예외 요구사항을 정의해보고, 그 예외를 기물 내에서 처리할 수 있도록 변경해볼까요? 🙂

kelly6bf and others added 15 commits March 22, 2024 17:11
- 기존 클래스명을 더 명확하게 변경함.
- 부족한 테스트 케이스를 추가함.
- 메서드 파라미터를 더 명확한 타입으로 개선함.

Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
- direction enum들을 MovementDirection으로 추상화 함.
- 테스트를 수정함.

Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-현by: Hyunguk Ryu <hw0603@naver.com>
Co-authore현d-by: Hyunguk Ryu <hw0603@naver.com>
Co-authore현d-by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-현by: Hyunguk Ryu <hw0603@naver.com>
Co-authored-by: Hyu성nguk Ryu <hw0603@naver.com>
Co-authored-by: Hyunguk Ryu <가hw0603@naver.com>
@kelly6bf
Copy link
Copy Markdown
Author

안녕하세요 수달!! 리뷰 엄청 빨리 해주셨는데 반영 요청을 너무 늦게드렸네용.. 늦은만큼 주셨던 피드백을 모두 반영하기 위해 몸부림 쳐봤습니다...!!

원래는 리뷰어님이 주신 코드백 하나하나에 제 개인적인 반론(?)이나 공감을 적는편인데 이번에 주셨던 피드백은 모두 수용해보고 싶어서 아예 반영하고 나서 코멘트 따로 드립니다!! 이번에도 잘 부탁 드립니다..!!

반영한 내용은 다음과 같습니다.

  • Piece 클래스를 추상클래스로 추상화 및 각 기물별 클래스 생성
  • 예상되는 예외에 대한 처리 구현 및 테스트 코드 작성
  • Controller & DTO & Game 클래스 개선

Copy link
Copy Markdown

@her0807 her0807 left a comment

Choose a reason for hiding this comment

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

안녕하세요 켈리~

제가 드린 코멘트를 전부 수용해주셨다고 했는데요!
수용하면서 좋았던 점이나 아쉬웠던 점, 궁금한 점들을 코멘트로
남겨주시면 더 즐거운 리뷰가 될 것 같아요.

추가로 명령어 처리 부분이 반영이 안되어 있는 것 같습니다 😅

요구사항이 복잡해지면서 구현이 어려웠죠?

이번 리뷰 반영에는 객체 간 메세지를 보내는 형태를 만드는 것에 집중하여
객체 응집도를 올려보는 연습을 해보시면 좋을 것 같아요.

레벨 1 미션 조금만 더 힘내보아요.
다시 한 번 파이팅입니다 :)

if (moveRequest.isEndCommand()) {
return false;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

전체적으로 개행정리를 한번 해볼까요?

Comment on lines +64 to +80
public MoveRequestDto inputMoveRequest() {
String input = sc.nextLine();
validateInputStringEmpty(input);

String[] inputStrings = input.split(" ");
validateInputCommand(inputStrings);

GameCommand gameCommand = gameCommands.get(inputStrings[0]);
if (!gameCommand.isMoveCommand()) {
return MoveRequestDto.of(gameCommand);
}

PositionDto source = convertInputToPosition(inputStrings[1]);
PositionDto destination = convertInputToPosition(inputStrings[2]);

return new MoveRequestDto(gameCommand, source, destination);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Command 가 무엇인지, 반환하는 책임을 GameCommand 로 옮겨볼까요? 🙂

}
}

public MoveRequestDto inputMoveRequest() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Start, Move, End 모두 핸들링 되는 것 같은데요!
위 내용을 포함하는,, MoveRequestDto 보다 더 적절한 이름이면 좋을 것 같아요🤩

Comment on lines +82 to +94
private void validateInputCommand(final String[] input) {
if (input.length != 1 && input.length != 3) {
throw new IllegalArgumentException("잘못된 입력입니다. 올바른 형식으로 입력해주세요. ex) move b2 b3");
}

if (!gameCommands.containsKey(input[0])) {
throw new IllegalArgumentException("존재하지 않는 명령입니다.");
}

if (input[0].equals(START_COMMAND)) {
throw new IllegalArgumentException("MOVE 혹은 END 명령만 입력할 수 있습니다.");
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Command 로 해당 책임을 넘기면 훨씬 깔끔해지겠네요.

Comment on lines +19 to +36
private static final Map<String, Integer> fileIndexes = Map.ofEntries(
Map.entry("a", 0),
Map.entry("b", 1),
Map.entry("c", 2),
Map.entry("d", 3),
Map.entry("e", 4),
Map.entry("f", 5),
Map.entry("g", 6),
Map.entry("h", 7),
Map.entry("8", 0),
Map.entry("7", 1),
Map.entry("6", 2),
Map.entry("5", 3),
Map.entry("4", 4),
Map.entry("3", 5),
Map.entry("2", 6),
Map.entry("1", 7)
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

켈리에 멋진 inputView 에서는 이른 검증이 잘 되고 있는 것 같아요.
검증할 수 있는건 최대한 빨리 검증하는것이 불필요한 비용을 타지 않고, 빠르게 원하는 행위를 유도하기 좋다고 생각해요.

여기서 조금 더 고민해볼 부분은 뷰에서 어디까지 검증 할 수 있는가? 인데요!

체스 판의 크기와 위치 자체는 저희가 게임을 만들 때 지정한 요구사항이고, 이 요구사항이 저는 도메인을 구성한다고 생각해요.
그런데 뷰에서 이렇게 각 위치가 노출되어 있다보니 도메인과 강한 결합이 되는 것 같아요.

지금은 콘솔 뷰로 받고 있지만, 뷰가 바뀔 때마다 검증을 추가해줘야하는 비용도 있고요. 도메인을 생성할 때도 한번 더 검증이 되니 저는
도메인 내에서하는 것만으로도 충분하다고 생각합니다!

public enum File {
    A(0),
    B(1),
    C(2),
    D(3),
    E(4),
    F(5),
    G(6),
    H(7);

    private final int index;

    File(final int index) {
        this.index = index;
    }

    public static File of(final int index) {
        return Arrays.stream(values())
                .filter(file -> file.getIndex() == index)
                .findAny()
                .orElseThrow(() -> new IllegalArgumentException("유효하지 않은 인덱스입니다."));
    }

켈리가 생각하는 뷰의 검증 영역은 어디까지인가요? 🙂

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

수달 의견에 동의합니다! 제가 생각하는 View의 검증 영역 역시 입력된 문자열 혹은 정수의 유효성 정도이고 입력된 리터럴이 특정 도메인의 규칙을 준수하는가?에 대한 유효성은 View의 영역이 아니라고 생각해요!

그런 관점에서 지금의 코드는 View가 도메인의 규칙까지 검사하고 있기 때문에 (체스판의 인덱스 규칙을 알고 있음) 개선이 필요하다고 생각이 들어요! 코멘트 반영해보겠습니다!

import static domain.piece.CommonMovementDirection.*;

public class Bishop extends Piece {
private static final List<CommonMovementDirection> MOVABLE_DIRECTIONS = List.of(UP_RIGHT, UP_LEFT, DOWN_RIGHT, DOWN_LEFT);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

비숍이 움직일 수 있는 위치, 킹이 움직일 수 있는 위치 이런 것들은 CommonMovementDirection 여기서 메서드로 관리해보면 어떨까요? 각 말들의 위치를 한눈에 파악하기 좋을 것 같아요!

Comment on lines +36 to +41
private Position move(
final Position source,
final Position destination,
final CommonMovementDirection movementDirection,
final Map<Position, Piece> piecePositions
) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

매당 메서드 시그니처가 너무 많은 것 같아요. 객체간 메세지를 보내는 형태로 변경해볼까요?

}

@Override
public void checkMovable(final Position source, final Position destination, final Map<Position, Piece> piecePositions) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

해당 메서드의 이름은 검증하는 것처럼 보여지는데 move 도 진행하는군요?

이 메서드명이 move 이면 실행을 위한 검증이있구나! 하고 흐름이 자연스럽게 느껴질 것 같아요 🤔

validatePosition(pieceColor, source, destination);

Piece targetPiece = piecePositions.get(source);
targetPiece.checkMovable(source, destination, parsePiecePositionsIgnoreTargetPiece(source));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

움직이려는 이동 경로나 위치에 말이 있는지는 보드에게 메세지를 던지는게 더 적합할 것 같아요.

Piece 에게 보드 자체를 넘겨주는 것이 서로에게 의존성이 생겨서 한곳에 변화가 생겼을 때 둘다 영향을 받는
순환참조 구조가 발생할 것 같습니다. 😇

import static domain.piece.PieceColor.WHITE;

public class BoardInitializer {
private static final Map<Position, Piece> initialPiecePositions = Map.ofEntries(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

File, Rank 를 사용해서 반복문으로 구현하면 어떨까요?

@kelly6bf
Copy link
Copy Markdown
Author

안녕하세요 수달!! 피드백 반영해서 다시 요청드려요!! 상태 패턴을 처음 도입해봤는데 생각보다 재밌기도 하고 InputView코드가 많이 줄어서 신기했습니다 🥰

다만 수달이 정성스럽게 보내주신 예제 코드를 읽으면서 궁금한 포인트가 생겨 질문 하나 남깁니다! 이번에도 잘 부탁 드려요!!

상태에 Controller를 넘겨도 괜찮을까?

상태 패턴은 말 그대로 특정 Context의 상태를 객체로 만들어서 각 상태별 행위를 수행할 수 있게 하는 패턴으로 이해했어요! 물론 Controller역시 우리의 프로그램에서는 하나의 객체이기 때문에 상태 패턴의 Context 역할을 수행할 수 있지만 지금 상태로 만든 Command친구들은 Controller보다는 ChessGame의 상태로 간주하는게 더 적절하지 않을까? 하는 생각이 들었어요!!

우선 궁금증에 ControllerChessGame을 Context로 가지는 코드를 각각 작성해보았아요! 수달의 의견이 궁금해요 🥰

  • othercase 디렉터리에 들어있는 코드 -> Context로 Controller를 넘김
  • domain디렉터리에 존재하는 코드 -> ChessGame을 넘김

@her0807
Copy link
Copy Markdown

her0807 commented Mar 26, 2024

상태 패턴은 말 그대로 특정 Context의 상태를 객체로 만들어서 각 상태별 행위를 수행할 수 있게 하는 패턴으로 이해했어요! 물론 Controller역시 우리의 프로그램에서는 하나의 객체이기 때문에 상태 패턴의 Context 역할을 수행할 수 있지만 지금 상태로 만든 Command친구들은 Controller보다는 ChessGame의 상태로 간주하는게 더 적절하지 않을까? 하는 생각이 들었어요!!

좋은 의견인 것 같습니다. 뷰와 의존성 때문에 Controller 를 넘겼던 것 같은데 행위의 주체를 ChessGame 로 해도 무방할 것 같습니다.

Copy link
Copy Markdown

@her0807 her0807 left a comment

Choose a reason for hiding this comment

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

안녕하세요 켈리~

리뷰 반영된 내용 확인했습니다 :)
무언가 명확하지 않을 때는 직접해보는게 제일 좋은 것 같아요!
고생 많으셨어요.

다음 단계 진행하면서 더 많은 의견 나눠요~ 🖐🏻

@her0807 her0807 merged commit f96ec49 into woowacourse:kelly6bf Mar 26, 2024
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