[ 사이클1 - 미션 (보드 초기화 + 기물 이동)] 고래 미션 제출합니다.#269
[ 사이클1 - 미션 (보드 초기화 + 기물 이동)] 고래 미션 제출합니다.#269miniminjae92 wants to merge 66 commits intowoowacourse:miniminjae92from
Conversation
|
ai 활용에 대해 고민해주신것 보았는데요 저희팀에서는 개발자간에 토론을 할 때 사용하기도 하는데요 |
|
불변은 모든 상황에서 다 좋지만은 않았던것 같아요 값이 동시에 여러 스레드에 공유되는 경우가 아니면 저도 그렇게 큰 장점을 느끼며 개발한 기억은 없는것 같네요 지금은 교육기간이니 도전해보고 싶은 부분을 몸으로 체감해보셔도 좋을것 같아요 |
pci2676
left a comment
pci2676
left a comment
There was a problem hiding this comment.
리뷰어가 중간에 바뀌면서 리뷰가 늦었네요. 질문에 대한 부분은 DM으로 이야기했었고 PR에 복사해서 답변해두었어요.
개선할 포인트들에 대해 리뷰를 남겨두었으니 확인해주세요.
| import view.InputParser; | ||
| import view.InputView; | ||
| import view.OutputView; | ||
|
|
There was a problem hiding this comment.
GameManager가 domain 패키지에 위치하면서 view.InputParser, view.InputView, view.OutputView를 직접 의존하고 있는데요, 도메인 객체가 뷰 계층을 알고 있는 구조가 되어버립니다. 개선이 필요해보여요
| public Map<Position, Piece> getBoard() { | ||
| return board.getBoard(); | ||
| } | ||
|
|
There was a problem hiding this comment.
getBoard()가 Map<Position, Piece>를 그대로 반환하고 있는데요. 사용하는 곳은 view로 보이네요. 불변한 컬렉션을 반환해보면 어떨까요?
| return x >= MIN && x <= MAX_X && y >= MIN && y <= MAX_Y; | ||
| } | ||
|
|
||
| private static int generateKey(int x, int y) { |
There was a problem hiding this comment.
Position이 캐싱을 통해 동일 좌표에 대해 같은 인스턴스를 반환하고 있는데, equals()와 hashCode()를 별도로 오버라이드하고 계시네요. 캐싱 덕분에 == 비교만으로도 동등성이 보장되는 상황에서 필요한 이유가 있나요?
|
|
||
| Side(String name) { | ||
| this.name = name; | ||
| } |
There was a problem hiding this comment.
Side enum에 baseY(), generalY(), cannonY(), soldierY(), formationX() 같은 배치 좌표 정보가 들어있는데요, 진영(Side)이라는 개념이 "초/한 구분"이라는 본래 역할 외에 "보드 배치 좌표"라는 책임까지 가진것 같네요.
SRP를 위반한 것 아닌가요?
| public boolean isCurrent() { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
상태 패턴을 적용해서 턴을 관리하신 의도는 좋은데요, next()가 호출될 때마다 새로운 객체를 생성해야만 하나요?
| .hasMessageContaining("상대방의 기물은 움직일 수 없습니다."); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
choPlayer.validateAlly(choPiece)만 호출하고 별도의 단언이 없네요
| class FormationCommandTest { | ||
|
|
||
| @Test | ||
| void 포메이션_입력이_1_2_3_4_일때_정상_동작한다() { |
There was a problem hiding this comment.
하나의 테스트에서 4개의 입력에 대해 각각 단언하고 있는데요 간결하게 표현해주세요
ParameterizedTest 를 사용하시면 될 것 같네요.
| Direction(int dx, int dy) { | ||
| this.dx = dx; | ||
| this.dy = dy; | ||
| } |
| if (steps.size() <= 1) { | ||
| return false; | ||
| } | ||
| return steps.subList(0, steps.size() - 1).stream() | ||
| .anyMatch(pos -> !board.isEmpty(pos)); |
| public void validateDestinations(Position target) { | ||
| if (!positions.contains(target)) { | ||
| throw new IllegalArgumentException("이동할 수 없는 위치입니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
이런 메서드는 절차적으로 호출하지 않으면 의도한대로 사용되지 않을 우려가 있어보이는데요.
구조적으로 안전하게 설계를 개선해보면 좋을것 같네요.
2 participants
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
안녕하세요 웨지!
많이 늦었죠? 이번 리뷰 잘 부탁드립니다!
설계상 어려웠던 부분들을 지금 다시 떠올려보자면,
입출력 흐름
페어와 얘기를 하면서 기물을 선택하고 갈 수 있는 지점을 나타내주면 좋겠다고 의견을 나눴습니다.
실제 게임에서 기물을 클릭하면 이동 가능 경로가 보이고 이동할 곳을 클릭하는 것을 상상했고 진행했습니다.
이 과정에서 출발점, 도착점을 한 번에 진행할 로직을 분리해서 진행하다 보니 재시도 부분과 예외 처리들에서 고민되는 부분들이 많았던 것 같습니다.
게임 초기화
게임 초기화 관련해서 고민을 꽤 했었습니다.
처음에는 ' 게임 시작 시 장기판과 전체 기물을 올바른 위치에 초기화한다.'를 테스트를 해야 하는가에 대해서 고민했습니다.
올바른 위치에 초기화되는 것은 꼭 보장되어야 하는 도메인 규칙이라고 느껴졌고, 테스트를 해야 한다고 판단했습니다.
그 후에 테스트를 어떻게 진행할지, 초기화를 어떻게 할지에 대해서 오래 고민했습니다.
고민을 오래 했지만, 결과적으로는 구현은 하드코딩, 테스트는 Mock으로 해보려 도전했다가 실패, 테스트를 위한 getter 사용을 고려하다가 좋은 방법이 떠오르지 않아서 패스했습니다.
결국은 페어가 팩토리를 만들자는 의견을 해서 하드 코딩 부분들을 하나의 팩토리에 응집시킬 수 있었습니다.
보드
저는 사전미션에서 위치가 총 90개밖에 안 되기 때문에 모든 위치를 가지고 빈 기물 객체를 가지는 방법을 떠올렸습니다. 그룹별로 토론을 진행했고, 대화를 나누면서 존재하는 기물에 대해서만 데이터로 보관하는 방법이 더 구현이 쉽게 떠올라서 해당 방법으로 진행했습니다.
그 후 보드와 기물 간에 협력을 어떻게 진행할지에 대해서 고민했습니다.
페어와 그림을 그리면서 얘기를 하면서 보드는 기물에게 위치를 주면, 기물은 해당 위치로부터의 모든 이동 가능한 위치들을 반환하고 보드는 그 위치들을 순회하면서 기물이 존재하는 것만 위치별 기물
Map에 담아서 다시 기물로 보내고 기물을 해당 데이터들을 이용해서 최종 목적지를 담아서 보드로 보내고 보드는 바깥으로 응답하는 식으로 설계했습니다.추후 계속해서 구현하다가 실제로 Map이라는 자료구조를 기물이 알게 되는 것도 강한 의존성이라고 느껴져서 BoardReader라는 인터페이스 사용을 떠올렸고, 적용을 해봤습니다.
이 과정에서 순환 참조를 생각해 봤고, 어디서 처리하는 게 좋은가? 고민을 해보게 됐습니다.
저는 결과적으로 잘 변하지 않는 기물의 공통된 이동 전략을 추상화하고 게임 진행 시 계속 바뀌는 진영, 턴에 대해서는 상태 패턴을 적용해 봤습니다.
불변
직관적으로는 불변이 좋다고만 생각하지, 아직 제대로 경험을 해본 적이 없는 것 같습니다.
멀티 스레드 환경이나 많은 동료와 협업 등 여러 변수가 생겨야 느낄 수 있을까요?
불변성을 잘 지키면 예상치 못하는 동작을 방지하고 동시성에 대해서 에너지 소모를 아낄 수 있고, 부수 효과를 방지할 수 있다고 들었습니다. 다만 제가 시작부터 불변성을 유지하려 노력해서인지 현실적인 체감을 못 했는데 실무에서는 많이 다른지 궁금합니다.
이번 보드(장기판)가 가지고 있는 필드인 맵에 대해서 상태가 가변이었는데 보기가 불편하고, 위에 설명한 이유들을 익히 들어와서 불변성을 지니도록 보드를 설계해서 결국 보드를 가지고 있는 게임에서 final 할당 방지를 제거하게 됐는데 이 부분에서도 근거와 직접 느껴보는 경험?이 부족하다 보니 필요한 일이었는지 궁금합니다.
정적 메서드
언제 한 번 재활용 가능한, 캐싱을 사용할 수 있을 때 정적 팩토리 메서드를 사용해 보고 싶었는데 이번 장기에서 고정된 위치와 기물들을 초기에 생성하기 때문에 활용해 봤습니다.
이번 미션에서 페어는 AI 활용을 자제하는 것으로 알고 저도 노력을 해봤습니다.
어떠한 글을 작성하거나 예를 들어서 피드백을 받았을 때 먼저 내 생각을 적어 보고 AI와 문답을 해보고 답변을 가다듬는 것과 그러지 않고 순수한 제 생각을 작성하는 것 중 무엇이 지금 우테코를 할 때 좋을지 고민입니다.
과하지 않게 적당히 잘 활용하는 것이 좋다는 판단이 있을 수 있을 텐데 이번 미션에서 아예 금지를 해두고 진행해 보니 생각보다 강한 의존성이 생겼다는 것을 느꼈습니다.
웨지의 AI 활용에 대한 의견이 궁금하네요, 더 고생한 만큼 기억에 강하게 남는 것처럼 최대한 사용을 자제하는 것과 어느 정도 고민을 하고 시간을 무리하게 지체되지 않도록 인터넷 서칭, AI, 동료와의 토론을 적극 활용하는 것이 더 추천될까요?
마지막으로 아래 요구 사항들을 지키는 것이 어려웠습니다.
for문 안에서 발생하는 if 조건문들을 depth를 위해서 줄이는 과정
모든 기능을 TDD로 구현한다. << 모든 public method를 테스트한다고 받아들이면 될까요? (getter, UI 제외)
모든 원시값을 포장하고 일급컬렉션을 사용한다.
디미터의 법칙
게터를 사용하지 않는다
이 부분들에 대해서는 아직 부족한 부분이 많지만 시간을 너무 소모하기엔 늦은 것 같아 부족하지만 PR 보냅니다, 잘 부탁드립니다.