[체스] 베디 미션 제출합니다.

[dpudpu]

summer님 안녕하세요. 이번에 리뷰를 받게 된 베디입니다.
먼저 귀한 시간 내서 리뷰 해주셔서 감사합니다.

제 프로젝트에 대해서 간단하게 설명해 드리자면, 콘솔 UI는 구현하지 않고 웹 UI만 구현했습니다.
DB는 h2 메모리로 구현해서 따로 설정해주지 않으셔도 main을 실행하면 자동으로 테이블 생성 및 샘플코드가 추가됩니다..

실행하시면 main 페이지에 있는 1번, 2번은 아직 끝나지 않은 기존 게임으로 이어 하기입니다.
2번은 왕을 잡기 직전인 상태로 (E7 -> E8) 입력하시면 게임이 종료됩니다.

저는 이번 미션에서 설계하면서 신경 썼던 부분이 Rule이 이동, 공격, 점수 외의 상태 및 행동만 하는 것이었습니다.
이렇게 구현 했더니 장점은 유연해져서 추가사항이 있을 때 변경이 수월한 데요.
단점은 상위 모듈에서 계속 하위 모듈까지 여러 번 들어가서 복잡해지는 단점이 있더라고요.
이렇게 여러 단계로 나누는 방법이 괜찮은 방법일까요?
처음부터 Rule이 컬러, 포지션, 이동규칙 다 가졌다면 간단하긴 했을 거 같은데 너무 여러 행동을 하는 건 아닐까 생각도 들더라고요.
이 부분에서 의견 주시면 감사하겠습니다.

이번 미션에서는 규칙이 많다보니 코드가 복잡한데, 이해가 잘 되실지 걱정되네요.
혹시라도 DM 주시면 최대한 설명이나 혹은 빠르게 수정하겠습니다.

마지막으로 '이거까지 피드백해도 될까?' 싶은 부분도 피드백 해주시면 감사하겠습니다!
감사합니다.

[dpudpu]

안녕하세요. 써머님. 피드백 주신 부분 다 반영했습니다.

피드백 중에서 Square 없애는 구조 변경에 가장 신경 썼는데요.
처음에는 Square를 없애고 Rule과 Piece를 합치려고 시도했습니다. 그러나 어느새 캡틴 포비의 코드와 비슷해지더라고요. 그래서 기존의 방식을 어느 정도 유지하면서 수정해보려고 Square만 없앴는데 확실히 더 깔끔해진 거 같습니다.
그 전에는 계층을 과하게 나눈 게 느껴지더라고요. 좋은 피드백 감사합니다.

그리고 한 가지 질문이 있습니다.
Piece가 Color, Type, Position 상태를 가지고 이동과 공격 모든 부분을 담당한다면, 너무 많은 역할을 가지는 게 아닐까요?

DM까지 주시고 질문에 친절하게 답변해주셔서 감사합니다.
덕분에 재미있게 피드백 반영할 수 있었습니다

[jihan805]

안녕하세요 베디님, 피드백 잘 반영해주셨네요 💯
간단한 피드백 남겼으니 확인 후 머지 진행할게요 ~!
감사합니다 :)

[jihan805]

해당 쿼리는 게임이 종료되면 status를 TRUE로 설정하고 있어요.
새로운 게임이 생성되면 PLAYING 끝나면 ENDED 등과 같이 상태를 확인할 수 있도록 구현하면 더 좋을 것 같아요.

[dpudpu]

bolean 보다는 문자열로 하라는 말씀이신가요?
그렇게 하는 이유는 종료, 진행 중 외에도 추가 될 수도 있어서인가요?

[jihan805]

해당 테이블을 처음 보는 사람은 room과 round가 헷갈릴 수 있을 것 같아요.
조금 더 명확하게 어떤 필드인지 나타낼 수 있는 네이밍 혹은 설명을 추가해도 좋을 것 같아요.

[dpudpu]

네 확실히 그렇게 느껴질거 같아요 round -> turn으로 수정했습니다 피드백 감사합니다..

[jihan805]

file에 대한 path도 상수로 추출하면 좋을 것 같아요.

[dpudpu]

네 추출했습니다 피드백 감사합니다

[jihan805]

이 쿼리의 의미를 이해하는데 어려웠어요!
아래 남긴 피드백을 참고하여 변경해보면 어떨까요?

[dpudpu]

변경했습니다 감사합니다!

[jihan805]

현재 작성된 코드 기준으로 Piece는 Color, Type, Rule(Pawn, King, Bishop 등) 모두를 가지고 있어요.
특히 getSecondPawnIfFirst 과 같이 Pawn의 움직임에 해당하는 로직으로 Pawn이 구현하는 것이 맞지 않을까요?

저는 Rule 클래스와 Piece 클래스를 합칠 수 있을 것 같아요.
Piece와 Rule을 분리된 클래스로 생각하기 보다 지금처럼 Color, Type, Postion등을 가진 추상 클래스로 변경하고
모든 말(King, Pawn 등)들을 움직임에 대한 Rule이 아닌 주체(움직이는 말)가 되어 자신의 움직임을 구현하면 어떨까요?

포비님의 코드와 비슷해졌다고 하더라도, 이미 베디님이 많은 고민을 하였기 때문에 조금 더 괜찮은 구조라는 생각이 든다면 기존의 방식을 유지하는 것도 좋지만 적용해보는 것도 좋은 연습이 될 것 같아요.

[jihan805]

해당 숫자가 가지는 의미를 상수로 표현할 수 있을 것 같아요!

[dpudpu]

네 상수로 추출했습니다. 감사합니다!

[dpudpu]

써머님 조언해주신 대로 Rule과 Piece를 합쳐봤습니다.
확실히 간결해졌는데, 기존 방식을 지키면서 변경하려고 해서 그런지 코드가 깔끔하지 못해서 아쉬운 마음이 드네요.
완전히 새로 해볼까 생각도 들었지만, 시간이 모자라서 어쩔 수 없이 타협을 보게 되었네요 ㅜㅜ 방학 때 새로 다시 짜보려고요.

한가지 궁금한 게 있는데 Piece가 이동하면 Position 변경을 두 가지의 해결책을 찾았는데 어떤 방식이 더 좋을까요?

1번. move해주면 setter로 값 변경

public class Board{
     
  	...
       
    private void chanePiece(final Piece origin, final Piece target) {
      map.put(origin.getPosition(), Empty.create(origin.getPosition()));
      origin.move(target.getPosition());
      map.put(target.getPosition(), origin);
  }
}

public abstract class Piece {
    private final Type type;
    private final Color color;
    private Position position;
  
    void move(final Position position) {
        this.position = position;
    }
}

이렇게 하면 불변이 보장이 안되서 별로인거 같아서 2번 방법을 시도해봤는데요.

2번. copy(Position position) 메소드로 새로운 인스턴스 생성해서 리턴

public class Board{
     
  	...
       
    private void chanePiece(final Piece origin, final Piece target) {
       map.put(origin.getPosition(), Empty.create(origin.getPosition()));
       map.put(target.getPosition(), origin.copy(target.getPosition()));
  }
}

public class Piece {
  ...
    
  protected abstract Piece copy(final Position position);
	
  ...
}

public class Queen extends Piece {
    ...
    
    @Override
    protected Piece copy(final Position position) {
        return new Queen(this.getColor(), position);
    }
}

이렇게 copy 메소드를 만들어주는 방식인데요

이렇게 하면 단점은 Piece의 하위클래스에서 copy를 구현해줘야 하는 단점이 있는데요.

어떤 방식이 더 좋을까요?

[jihan805]

안녕하세요 베디님.
피드백 잘 반영해주셔서 감사합니다~ 마지막 미션까지 끝내셨네요 ㅎㅎ
꼭 방학기간동안 체스 다시 구현해보세요!
그동안 고생 많으셨어요, 머지 진행할게요 :)

[jihan805]

chane->change로 바뀌어야 할 것 같아요!

추가로 1번 방법으로 사용해도 괜찮을 것 같아요.
값을 직접 setting 해준다는 의미로 해석되어 setter의 역할이 아닐까에 대해 고민하고 있는데요,
우선 이 메서드는 움직이라는 메시지를 보낸다라고 해석하는게 더 좋을 것 같아요(setter로 사용하려고 만든 메서드가 아니니까)
이해를 돕기 위해 첫번째 과제였던 자동차 경주에서도 자동차를 move를 구현한 메서드를 떠올려보면 어떨까요?
자동차의 경우 move에 조건이 있었기 때문에 setter와 같이 느껴지지 않았을지 모르겠지만 결국 이 역시 position의 위치를 변경하는 메서드니까요.