[1,2,3단계 - 체스] 조앤(서민정) 미션 제출합니다.

[seovalue]

안녕하세요, 데이브! 조앤입니다 🙂

이번 체스미션을 진행하면서 페어와 함께 생각보다 구현이 빨리 되어 다행이라고 여겼는데, 이후 코드를 다시 확인해보니 리팩토링해야 할 부분이 많이 보이더라구요 😂 우선 보기에 어렵거나, 메서드 흐름이 잘 안읽히는 부분을 최대한 분리해보려고 노력해보았는데, 가독성이 어떨 지 궁금하네요

우선 제가 이번 코드를 짜면서 질문이 생긴 부분들은 각 코드에 코멘트로 달아놓도록 하겠습니다 :)
다른 분들 PR을 보니 넘 대단하셔서 쭈구리가 될 것 같지만 ,, 뭐 괜찮습니다 ! 하핫 데이브께서 피드백 많이 남겨주시면 반영하여 더 멋진 코드가,, 될 것이라고,, 생각합니다 ㅎㅎㅎㅎ

바쁘신데 리뷰해주셔서 감사합니다 🙏🏻
학습 로그는 정리 후 작성해놓도록 하겠습니다!

[seovalue]

보드를 출력하기 위해 chessGame에서 보드와 차례를 get 해오는 이 부분에 대해서 어떻게 생각하시나요? DTO?와 같은 것으로 전달해야할지 고민도 해 보았지만 dto를 만드는 과정 자체도 getter가 필요할 것 같아서 우선은 이대로 두었습니다

[dave915]

DTO를 사용하여 View와 Domain의 관계를 끊어주면 좋지만 DTO를 만들지 않을 것이라면 값을 넘겨줄때 chessGame이 갖고있는 인스턴스를 그대로 주는것이 아니라 새로운 인스턴스로 반환하면 좋을 것 같아요.
지금처럼 그대로 준다면 View에서 도메인의 상태값을 바꿀수 있을 것 같네요 :)

[seovalue]

Command에 해당하는 명령어인지 보기 위해 원래는 Inputview에서 해당 splitCommand를 수행했지만, move 함수는 move 이외 뒤 명령어 (ex. move d1 d2에서 d1 d2)도 필요해서 split을 컨트롤러에서 하게 되었는데, 이 부분은 어떻게 생각하시나용 ?

[dave915]

Command의 apply 메소드에서 String command 를 사용하기 때문에 Command 내부에서 Split해도 되지 않을까 싶어요.
아니면, Command enum과 별개로 inputCmd를 이용해 아래같은 필드를 가진 객체를 만들어도 좋을 것 같아요.
코드를 타고 들어가보니, Positions에서도 split을 해주고 있는데, 객체를 만들게 되면 Positions의 split 로직은 필요없어 질 것 같아요!

private String command;
private List<String> options;

[seovalue]

List 에서 get(0), get(1)을 한 뒤 source, target을 받아와 chessGame에서 사용하였었는데요, 이 부분이 보기 싫어 Positions라는 객체를 만들어보았는데 뭔가 분리해서 작업했던 것에 비해 깔끔해졌다는 생각은 잘 들지 않더라구요.. 이 객체에 대해서 어떻게 생각하시나요? 기존의 List일 때의 커밋 링크도 남겨두었습니다.

[seovalue]

89dba13

[dave915]

private final List<Position> positions;

로 들고있기 보단 아래 처럼 들고 있는건 어떠세요?

private final Position source;
private final Position target;

객체명은 Positions 보다 움직임에 필요한 Position들이라는 의미로 수정하면 어떨까 싶어요 :)

[dave915]

조앤 안녕하세요! 리뷰를 맡게된 데이브입니다 :)
우선 질문주신 부분에 대한 코멘트와 몇가지 피드백 남겼습니다.
확인 후 고민해 보시고, 궁금한점이 있다면 언제든지 DM주세요!

[dave915]

Command는 도메인 객체인데 Command안에서 ChessController 메소드를 호출하는 부분이 부자연스럽다고 생각들어요.
ChessController의 각 메소드에서 도메인 로직을 Command로 옮기면 좋을 것 같아요 :)

[dave915]

Board로부터 Piece들을 받아서 이동시키지 말고 Board 내부에서 이동 시키도록 일을 시키면 어떨까요?

[dave915]

piece가 폰인지 아닌지 ChessGame에서 구분하지 않게 하려면 어떻게 해야할까요?
ChessGame은 Board에게 Board는 각 Piece에게 일을 시키도록 수정해보세요!
너무 어려우면 DM주세요 😁

[seovalue]

안녕하세요, 데이브. 조앤입니다 :)
뭔가 스스로는 리팩토링을 많이 했다고 생각하지만••• 잘 된 것이 맞는지 잘 모르겠고, 더 피드백 받고 싶은 부분이 많아 우선은 일단 제출합니다.
커맨드는 DM에서 여쭤봤던 대로 상태패턴 , 커맨드패턴을 적용해보려했지만 적용하기에 어려움이 있어 우선은 데이브께서 말씀주신 대로 enum 내부에서 처리하고, enum의 상태값을 통해 각 커맨드에 따른 view 로직은 컨트롤러에서 처리하도록 수정해보았어요.

그리고 chessGame -> Board -> Piece 각각에게 move의 책임을 분리하는 부분은 우선은 Piece의 공통 부분을 담고 있는 AbstractPiece에서 폰을 제외한 나머지 말들의 공통 움직임 로직을 담도록 하고, 폰만 해당 메서드를 오버라이드하여 자신의 로직에 알맞게 이동할 수 있도록 구현해보았는데요, 구현하다보니 순서가 중요한 부분이 많이 생겼는데, 그렇다보니 로직의 가독성이 떨어지는 것 같아 우선 주석은 삭제하지 않았습니다. 이 부분과 관련된 추가 질문은 해당 코드에 남겨놓도록 하겠습니다.

바쁘신데 리뷰해주셔서 항상 감사합니다.

[seovalue]

기존에는 apply를 실행하면 controller 내부의 메서드가 실행되도록 했었는데, 현재는 커맨드 내부의 메서드가 실행되도록 변경하였고 해당 커맨드의 상태에 따라 view 로직을 수행하는 것으로 변경해보았는데, 어떠신지 의견이 궁금합니다.

[seovalue]

이 부분은 지난 번에도 질문드렸었는데요, 말씀주신 대로 isEqualsState같은 메소드를 만들어서 인자로 State를 받아 현재 State와 같은지 비교해보면 어떨까요? 와 같이 진행해보려 했는데 그렇게 한다면 현재와 달리 isReady, isEnd, isRunning등의 메서드들이 어떻게 사라지게 되는지?? 이해가 잘 되지 않아 우선은 유지하였습니다 ㅠㅠㅠㅠ흑

[dave915]

아래처럼 해보시겠어요? 😁

// 사용
chessGame.isEqualsState(End.class);

// 정의
public boolean isEqualsState(Class<? extends State> state) {
    return this.getClass().equals(state);
}
``

[seovalue]

기존 chessGame에서 모든 move 로직을 수행하는 것에서, Board 내부 다음과 같은 move/ confirm/ target 메서드를 만들어 로직을 수행하도록 변경해보았는데 어떠신지 궁금합니다 !

[seovalue]

각 피스에서는 갈 수 있는 방향인지, 이동가능한 거리를 초과하지 않았는지 확인한 후 갈 수 있는 위치를 List 형태로 리턴하도록 구성해보았습니다.

[dave915]

한줄이더라도 중괄호를 사용하는 것을 추천드려요 :)

중괄호들이 if-else 문이나 for 문 같은 제어 구조의 일부분으로 사용되어질 때에는 중괄호들이 모든 문들(단 하나의 문일 경우에도)을 둘러싸는데 사용되어져야 한다(이렇게 사용하는 것이 중괄호를 닫는 것을 잊어버리는 것 때문에 발생하는 버그를 만들지 않고, 문을 추가하는 것에 큰 도움을 준다).

https://myeonguni.tistory.com/1596

[dave915]

아래처럼 해보시겠어요? 😁

// 사용
chessGame.isEqualsState(End.class);

// 정의
public boolean isEqualsState(Class<? extends State> state) {
    return this.getClass().equals(state);
}
``

[dave915]

조앤 안녕하세요!
리펙토링 리펙토링 진행 한 부분 확인했어요 👍
다음단계도 잘 부탁드립니다 :)
State체크하는 부분은 코멘트 남겨두었습니다!

[dave915]

Suggested change

	return IntStream.range(1, distance - 1).mapToObj(i -> path.move(direction, i)).collect(Collectors.toList());
	return IntStream.range(1, distance - 1)
	.mapToObj(i -> path.move(direction, i))
	.collect(Collectors.toList());

[dave915]

전체적으로 하드코딩은 제거해 주세요 :)

[seovalue]

조앤의 학습로그

DTO란 무엇일까?

dto란 계층 간 데이터 교환을 위한 객체이다. db에서 데이터를 얻어 service나 controller로 넘길 때에도 사용할 수 있고, view로 데이터를 보낼 때에도 사용할 수 있다. dto는 단순히 데이터 전달을 위한 객체이므로 비즈니스 로직을 갖는 메서드는 내부에 존재하지 않고 getter, (굳이 필요하다면) setter정도 존재한다. VO는 값 객체 자체를 의미하기 때문에 equals와 hashCode를 오버라이드해 주어야 하지만, dto의 경우에는 필요없다고 생각한다.

• 링크 - https://woowacourse.github.io/javable/post/2020-08-31-dto-vs-entity/

매개변수의 유효성 검사는 언제 해야할까?

이펙티브 자바 아이템 49. 매개변수가 유효한지 검사하라 파트에 이러한 내용이 있다.

메서드와 생성자 대부분은 입력 매개변수의 값이 특정 조건을 만족하길 바라며, 이러한 제약은 반드시 문서화해야하며 메서드 몸체가 시작되기 전에 검사해야한다. 생성자는 "나중에 쓰려고 저장하는 매개변수의 유효성을 검사하라"는 원칙의 특수한 사례로써 생성자 매개변수의 유효성 검사는 클래스 불변식을 어기는 객체가 만들어지지 않게 하는데 꼭 필요하다.

이렇듯 매개변수가 유효한지는 메서드 몸체가 시작되기 전에 검사해야한다고 한다, 그 이유는 다음과 같다.

1. 오류를 발생한 즉시 잡지 못하면 해당 오류를 감지하기 어려워지고, 감지하더라도 오류의 발생 시점을 찾기 어려워진다.
2. 잘못된 값이 넘어왔을 때 즉각적이고 깔끔한 방식으로 예외를 던질 수 있다.
3. 매개변수 검사를 제대로 하지 못한다면 어떠한 객체를 이상한 상태로 만들어 놓았기에 미래의 알 수 없는 시점에서 이 메서드와는 관련없는 오류를 발생할 수 있다. (실패 원자성을 어기는 결과를 낳을 수 있다.)

그리고 또, 이펙티브 자바에서는 이 책(엘옵)의 주장을 메서드 몸체 실행 전에 매개변수 유효성을 검사해야한다는 규칙의 예외로서 인정한다, 그 이유는 다음과 같다.

1. 유효성 검사 비용이 지나치게 높거나 실용적이지 않을 때
2. 계산 과정에서 암묵적으로 검사가 수행될 때 (ex. Collections.sort의 경우 객체 리스트를 정렬하는 메서드인데, 리스트 안의 객체들은 모두 상호 비교될 수 있어야하며 정렬 과정에서 이 비교가 이루어지므로 리스트 안의 모든 객체가 상호 비교할 수 있는지 검사해 볼 필요는 그닥 없다.)

아무튼 주절주절했지만 결론은 생성자 내부에서 매개변수에 대한 유효성 검사를 실행하되 그 비용이 너무 크다면 이후에 맡길 수 있다고 생각한다.