New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1,2,3단계 - 체스] 인비(김태희) 미션 제출합니다. #176
[1,2,3단계 - 체스] 인비(김태희) 미션 제출합니다. #176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 인비! 제이입니다.
미션 구현하시느라 고생많으셨습니다 🙇♂️
몇 가지 피드백 남겼는데 확인부탁드려요 🙏
|
||
private void setPiece(PieceWithColorType pieceWithColorType, Position position) { | ||
Piece piece = null; | ||
if (pieceWithColorType != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
colorType만 null검사를 해주신 이유가 있으신가요~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PieceWithColorType은 색깔 타입이 아니고, 색깔이 있는 기물 타입입니다!
저 검사는 주입받은 보드 세팅의 특정 위치 칸이 비어있는지 아닌지 검사하는 로직입니다. 😃
return PiecesCache.find(pieceType, teamColor); | ||
} | ||
|
||
public boolean isMovableTo(MoveRoute moveRoute, Board board) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Board가 Cell을 가지고있고 Cell이 Piece를 가지고 있는 상태에서 Piece가 Board를 인자로 받고있어요.
순환이 생기는거같은데, 이렇게 구현하신 이유가 있으실까요?
또한 기물이 보드를 알고있는 구조에 대해서 어떻게 생각하시나요?
현재 이구조를 변경하려면 공수가 꽤 많이 들것으로 보여요. 타당한 이유가 있다면 의견 남겨주세요 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 코드를 다시보았는데, 말이 보드를 아는것은 말들이 주체적으로 움직일 수 있다는 관점에서는 맞아보이기도하네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드의 설계 의도는 다음과 같아요.
보드 : 기물아, 너 이 경로(MoveRoute)로 이동하래. 내 정보(Board) 줄테니까, 너가 직접 움직여보고, 갈 수 있는지 알려줘.
기물 : 내가 직접 가 봤는데, 갈 수 있어.
보드 : 그래? 그럼 내가 보드에서 말 이동시킬게.
이런 방식으로 생각했어요!!
기물이 보드를 매개변수를 통해 알게되는 점을 페어와 함께 많이 고민했어요.
말씀하신 것처럼 기물이 보드를 모르는 경우, 기물이 이동 패턴만 반환하게 돼요.
그러면 보드가 기물이 반환한 이동 패턴에 요청받은 이동 경로가 해당되는지 검사해요.
그리고, 기물이 이동 경로로 갈 수 있는지도 보드가 직접 확인하게 됩니다.
이렇게 했을 때 보드가 너무 많은 책임을 가진다고 생각했어요.
King, Queen, Bishop, Rook은 이동 방식이 동일하지만(이동 경로 중간에 기물이 존재하면 안 되고, 도착지에 자신의 말이 존재하면 안 된다.),
Pawn과 Knight는 이동 방식이 각각 개별적으로 너무 달라요.
그래서 보드가 이 모든 것을 하려면,
이동하고자 하는 기물이 어떤 타입의 기물인지 일일이 확인하고,
Pawn일 때, Knight일 때, King, Queen, Bishop, Rook일 때 각각 구분해서 로직을 적용해야 합니다.
그러면 함수가 너무 많아지고, 많은 책임을 갖게 된다고 생각해 현재처럼 구현했습니다.
제이의 생각은 어떠신가요? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인비의 말도 타당하다고 생각해요. 그래서 그럴수있을거같다는 생각이 들어 두 번째 코멘트를 남겼구요 :)
제가 보드에서 하면 좋을 거같다고 생각되는 이유는 2가지입니다.
첫 번째, 유지보수하기 좋다.
Board에 다 때려박으면 클래스가 뚱뚱해질텐데 무슨 유지보수가 좋냐 넌 srp도 모르냐 라고 반문할수있어요.
반대로 생각하면, Player가 추가될때 (현재는 없죠. 물론 요구사항에도 없구요) 말을 움직이는건 Player가 될테니 Board의 로직만을 각 Player에게 옮겨주면 됩니다. 각 기물이 해당 역할을 가지고있을 때는 모든 기물 클래스를 찾아가서 전부 변경해주어야 합니다. 요구사항이 변화될 때 코드의 영향도도 최소화되겠죠.
이 부분은 장단이 있기 때문에 확실하게 주장을 하기엔 조금은 부족한건 인정합니다.
두 번째, 잠재적 위협을 줄일 수 있다.
두 번째 이유가 사실은 더 큰데요. 보드가 말을알고 말이 보드를 알면 서로가 서로를 호출할 수 있습니다. 현재 코드는 더 복잡 합니다. (Board가 Cell을 가지고있고 Cell이 Piece를 가지고 있는 상태에서 Piece가 Board....) . 내부적으로 타고타고 가야 서로를 참조하는것을 알텐데 도메인을 잘 모르는 새로운 사람이와서 인비가 만든 체스프로그램을 받아서 작업할 수도있고, 시간이 흘러 내가 짠게 내가짠게 아닌 망각의 상태에 도달할 수 있습니다. 그 상황에서 저렇게 얽혀있는 코드를 단박에 알아챌 수 있을까요?
"낼까지 빨리좀 작업해주세요~~" "아네~" 그러다가 자칫 서로의 엔티티를 불러 무한루프에 빠져 앱이 죽게된다면 큰 불상사가 생길 수 있겠죠. 그럴바에 차라리 한쪽 의존을 과감히 끊어버리는쪽이 낫다고 생각합니다. (비록 보드의 역할이 많아졌지만) 반대로 얘기하면 보드만 봐도 파악이 가능하겠죠. 저는 위협이 될 수 있는 부분은 애초에 싹을 잘라버리는것을 좋아합니다.
인비의 의견도 맞는말이에요. 지금 구조로 가셔도 되어요. 다만 저는 이렇게 생각하기 때문에 참고만 해주세요 🙏
안녕하세요, 제이!! 😃피드백 정말 감사합니다. 🙏 질문 남겨주셨던 부분들에 제 생각을 코멘트로 달았어요!! 감사합니다. 🙇 ps. 맨 처음에 작성했던 학습 로그 링크들이, 패키지 경로를 이동 한 후에 push하면서 존재하지 않는 링크가 되어, 업데이트 해 다시 올립니다!! 😄 인비의 학습로그[OOP] 추상클래스 - 5내용
링크[OOP] DTO - 2내용
링크[OOP] 의존성 주입 - 4내용
링크[Java] Enum - 5내용
링크[OOP] 일급 컬렉션 - 3내용
링크[OOP] Caching - 4내용
링크[JUnit] Nested 어노테이션 - 5내용
링크[Test] 테스트하기 쉬운 코드 - 5내용
링크[Java] Stream - 3내용
링크 |
…는 책임 View -> Controller 로 이동
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 인비! 제이입니다.
피드백 반영하시느라 정말 고생많으셨어요 🙏
의견이 다른 부분은 제가 코멘트로 남겨두었습니다.
그리고 추가로 몇가지 코멘트도 남겨두었습니다!
다음 미션때 뵐게요 🙇♂️
안녕하세요, 제이!! 😃
반갑습니다!!
체스 규칙이 복잡해 구현이 어려웠던 미션이었습니다. 😅
Pawn의 규칙이 가장 까다로웠습니다.
구조가 복잡해지다 보니, 각 객체들의 책임에 대해 고민을 많이 했어요.
체스 보드 세팅 정보를 외부에서 주입하도록 해, 매우 직관적이고 테스트에 용이한 코드를 작성했습니다.
로직들이 복잡하다 보니, 메서드들이 많아져 고민을 많이 했어요.
체스 보드 칸 위치와 기물들을 캐싱했습니다.
Direction 에서 방향을 판단하고, 각 기물의 이동 가능 방향들을 일일이 반환해 주다 보니, 클래스에 함수가 많아졌네요. 😭
어려운 만큼 많이 성장하고 있는게 느껴져요!! 💪
제이의 피드백을 통해 더 성장하고 싶습니다. 😄
감사합니다!! 🙇
인비의 학습로그
[OOP] 추상클래스 - 5
내용
링크
[OOP] DTO - 2
내용
링크
[OOP] 의존성 주입 - 4
내용
링크
[Java] Enum - 5
내용
링크
[OOP] 일급 컬렉션 - 3
내용
링크
[OOP] Caching - 4
내용
링크
[JUnit] Nested 어노테이션 - 5
내용
링크
[Test] 테스트하기 쉬운 코드 - 5
내용
링크
[Java] Stream - 3
내용
링크