[🚀 사이클1 - 미션 (보드 초기화 + 기물 이동)] 제이콥 미션 제출합니다.

[khcho96]

체크 리스트

• 미션의 필수 요구사항을 모두 구현했나요?
• Gradle test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?
• 애플리케이션이 정상적으로 실행되나요?

어떤 부분에 집중하여 리뷰해야 할까요?

웨지 안녕하세요!
초기 설계와 기물 이동에 대한 전략 패턴에 대한 고민으로 PR이 많이 늦어졌습니다.
페어의 도움으로 작성된 코드인지라 아직 이해하는 과정인 코드가 존재하지만, 리뷰를 받으면서 배워가려고 리뷰 요청했습니다.
잘 부탁드립니다. 감사합니다:)

1. 기물 이동 전략 패턴 적용

기물 이동 전략 구현 과정에서 전략패턴을 적용하기 위한 설계가 떠오르지 않아 굉장히 많은 if문과 while문으로 구현해 depth와 메서드 길이가 길어졌습니다. 이를 상, 하, 좌, 우 등의 좌표 증감값을 Direction Enum으로 도출하고, 고정적인 공통 탐색 알고리즘을 OneStepStrategy, SlideStrategy, JumpSlideStrategy, SequenceStrategy로 추상화하여 기물에 주입하는 방법을 페어의 도움을 통해 구현할 수 있었습니다.

2. getter 사용

Board, Player, Position에서 getter를 사용하고 있는데, getter없이 구현할 방법을 찾지 못해 우선 구현했습니다.

[sihyung92]

안녕하세요 제이콥, 리뷰어 웨지입니다.

리뷰들은 잘 반영되었네요. 모호했던 리뷰들에 대해선 다시 코멘트를 남겼으니 확인해보셔요.

리뷰 시점에 submit 한 것과 기존 리뷰에 댓글로 그때그때 단 코멘트가 혼재되어 있으니 (죄송..) 확인해보시고 또 요청 주셔요. 감사합니다

[sihyung92]

조금 더 구체적인 리뷰를 드리자면, board와 Piece의 양방참조가 조금 불편해요.

메서드 내 로컬 변수로 선언하거나, 메서드 시그니처에서 인자나 응답 형태로 가지고 있는 것은 약한 의존이고
필드로 가지고 있는 것은 강한 의존에 해당하는데요

객체 내 에서 board에 대한 참조를 많이 필요로 한다는 건 의존관계를 바꾸어야 한다는 신호이기도 합니다.
(객체지향은 프로세스와 프로세스가 필요로하는 데이터를 클래스라는 형태로 모으는 거니까요)

그런데 Piece에 Board를 필드로 놓는 순간, 양방 참조가 되죠 (Board는 piece의 맵, Piece는 Board를 참조..)
물론 지금처럼 메서드에 인자 형태로 있는 순간 양방 참조입니다만.

대부분의 filterValidPositions에서 board를 필요로 하는 이유는 position에 대한 점유를 확인하기 위함인 걸로 보이는데, piece는 position과 move strategy를 가지고 Path 까지만 생산하고,
position을 이동 할 때 이미 객체가 있는지 여부는 Position과 Piece의 맵핑을 관리하는 board에서 하는게 각 객체의 책임으로 (필드로 가지고 있는 데이터들을 보았을 때) 적절할거 같아요.
-> 이 "책임"이라는 건 제이콥의 클래스의 분할 수준과 데이터를 어떻게 배치했냐를 보고 떠올린 것이어요. 책임의 분배라는 건 정답이 있는 문제가 아니라, 어떤 행위가 필요하고 누가 할 것이며 데이터는 어디에 두어야 할까? 라는 발상 끝에 배치되는 것이에요.

이번 미션의 가장 중추인 핵심 구조에 대한 부분이고, 제이콥의 고민이 담겨있는 부분이기 때문에 생각이 다르시다면 반드시 반영할 필요는 없습니다.
한번 고민해보시고 반영여부를 결정해보세요.

[sihyung92]

네네, toString은 무조건 제공되는 코드인데 기본제공되는 hashcode는 쓸모가 없기 때문에 항상 재정의됩니다. 근데 toString 메서드의 모호성으로 인해 (말씀주신것처럼 '읽기 쉬운 형태' 라는게 얼마나 모호한가요)
라이브러리의 매 버전마다 하위호환을 지키지 않는 경우가 많습니다.

애초에 다른 개발자가 만든 toString을 로직에 쓸 때는 이 지점을 고려해 매우 신중하게 사용해야하며, (시간과 관련된 java SDK 클래스 들은 ISO 규약에 맞춰 toString을 응답하는데, 이렇게 정해진 포맷으로 약정된 클래스의 경우 괜찮습니다. 버전에 따라 변경되지 않을 것이라 예상할 수 있으니까요.)

스스로도 개발할 때 toString이 변하지 않는 도메인 양식으로 제공하겠다는 확신이 없으면 toString을 비즈니스 목적으로 사용하면 안 됩니다. (이 확신이라는건 이 객체 유지보수를 3년 해도 toString을 변치않고 그대로 제공할 수 있다 급입니다)

[sihyung92]

기존보다 훨씬 마음에 듭니다만, 대신 parser의 command를 의존하고 있는 점이 아쉽습니다. command가 Formation을 알고 Formation은 command를 모르게 해주세요.

[sihyung92]

경계값을 기준으로 한 테스트가 잘 구성되어 있는게 좋습니다.