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

[Rix01]

안녕하세요 피케이! 이산입니다🏔️

우선, 시간 내서 리뷰를 해주시는 것에 정말 감사합니다. 이번 장기 미션 사이클1을 시작하면서 처음에는 다형성을 고려한 완벽한 설계를 꿈꿨지만, 오히려 그 생각에 갇혀 코드를 한 줄도 쓰지 못하는 경험을 했습니다. 그래서 이후에는 “일단 돌아가는 코드를 구현하자”는 목표로 우선순위를 변경하여 구현을 마쳤습니다.

현재 코드는 기능은 작동하지만, 스스로 보기에도 책임 분리가 제대로 안 되어 있고 if문이 너무 많고 복잡하다고 생각합니다. 마음 같아서는 지금 코드를 최대한 리팩토링을 한 후에 PR을 올리고 싶었지만 현재로서도 사이클1의 PR이 많이 늦어졌다고 판단하여 '돌아가는 코드'를 먼저 공유하고 올바른 방향 설정을 위해 용기 내어 제출합니다.

현재 코드를 리팩토링하고 싶긴 하지만 어디에서부터 손을 대야 할지도 감이 잘 안 잡혀서 리뷰어님께 이러한 코드에서 객체지향적으로 리팩토링을 진행하기 위해서는 어디에서부터 방향을 잡아야할지 여쭤보고 싶습니다. 지금까지의 제 코드 경험을 봤을 때 처음부터 깔끔한 코드를 작성한 경험을 거의 드물어서 이렇게 복잡한 코드를 제대로 리팩토링하는 경험이 필요하다고 느껴졌습니다.

보드 초기화의 경우 페어와 구현 방식에 있어서 차이가 좀 있어서 각자 PR을 올린 이후에 각자 리팩토링 해보기로 했습니다.

체크 리스트

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

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

1. 패턴 사용에 대한 의문

저는 디자인 패턴에 대해 잘 모릅니다. 하지만 블랙잭에서도 그렇고 이번 미션에서도 디자인 패턴에 있는 특정 패턴을 사용한다면 객체지향적으로 좋은 코드가 될 수 있다고 생각합니다. 하지만 패턴 사용에 있어서 학습 방향을 ‘패턴 학습 → 코드에 적용’ 순서가 맞는 건지에 대한 의문이 들었습니다. 객체지향적인 설계를 하다보니 “아 이 설계가 이 패턴을 사용한 거구나”를 깨닫는 방향이 더 학습 방향에 맞다고 생각했습니다. 피케이의 경우 어떠한 학습 방향이 맞다고 생각하시는지 궁금합니다.

2. 기물 추상 클래스화

기물을 현재는 Piece에 Team과 PieceType을 가지고 있는 클래스로 만들었습니다. 하지만 이렇게 하다보니 후에 기물들의 이동을 구현할 때 수많은 if문을 가지게 하는 것이 이것 때문이라는 생각이 들었습니다. 그래서 지금 드는 생각으로는 Piece를 추상 클래스로 만들고 그에 따른 기물들을 자식 클래스로 만들어서 리팩토링을 해야한다고 생각하는데 이렇게 하지 않고도 객체지향적으로 설계할 수 있는 방법이 있을지 궁금합니다.

3. 움직임 규칙을 누가 가져야 하는가

현재 코드에서는 움직임 규칙을 MoveRule 인터페이스로 만들고 각 기물들의 MoveRule을 구현하는 방식으로 한 후에 해당 MoveRule을 PieceType 필드에 넣어주는 방식으로 했는데 이러한 방식이 맞는 방식인지 잘 모르겠습니다.

4. TDD에 있어서 어디까지 설계를 하고 구현을 해야하는가

이번 장기 미션을 시작함에 있어서 다형성을 잘 적용하고 싶다는 마음에 설계를 시작할 때 다형성을 모두 생각하고 진행을 하려고 했었는데 오히려 그것들을 다 생각하고 TDD 방식으로 구현을 하려고 했을 때 어떤 것부터 테스트 코드를 작성해야할지 의문이 많이 들었습니다. 이번 경험을 통해 TDD는 애초에 설계를 다 한 후에 하는 방식이 아니라 테스트 코드를 통해 작은 것부터 완성해 나간 후에 리팩토링을 하는 과정에서 다형성을 적용해 나가는 것 같다는 생각이 들었습니다. 사이클1 PR을 빨리 올린 다른 크루에게 물어봤을 때도 처음에는 Game, Board, Piece 정도만 생각하고 테스트 코드를 작성하기 시작했다고 들었습니다. TDD 방식을 사용할 때 어느 정도까지 설계를 생각하고 구현을 하는 것이 맞는지 궁금합니다.

5. 보드의 책임이 너무 막중한 것 같은데 어떤 식으로 분산할 수 있을지

현재는 보드가 해야하는 책임이 너무 큰 것 같은데 이것을 어떻게 책임을 분산시킬 수 있을지 궁금합니다. List<Direction>을 List<Position>의 형태로 변환하는 과정 같은 경우는 유틸성 클래스를 따로 빼서 하는 것이 적절한 것인지 궁금합니다.

6. getter 사용에 대한 의문

상태를 꺼내지 말고 메시지를 던지라는 원칙은 머리로는 이해하지만, 실제 구현 단계에서는 자꾸 getter를 호출해 외부에서 로직을 처리하게 됩니다. 어떤 기준으로 로직을 객체 내부로 밀어넣어야 getter를 최소화할 수 있을지 궁금합니다.

[pkeugine]

안녕하세요 이산, PK 입니다.
현재 실패하는 테스트가 있습니다.

PR 을 올리고 리뷰 요청을 하기 전에

• 해당 프로젝트가 제대로 compile 되는지,
• 테스트가 통과 되고 있는지,
• 그리고 요구사항을 만족시키기 위해 노력했는지

꼭 한 번씩 확인해주세요.

그리고 현재 객체지향적으로 감이 잘 잡히지 않아 리팩터링이 힘들다고 해주셨는데,
우선 요구사항인 10줄 제한을 맞추기 위한 메서드 분리부터 해보면 좋을 것 같습니다.

일단 request changes 로 두겠습니다.

• 실패하는 테스트 통과하도록 변경
• 10줄 제한 지키기

위의 이 두가지 한 번 도전해보죠.
만약 분리하기가 쉽지 않다면 솔직하게 PR 에 그 부분을 inline comment 또는 DM으로 남겨주세요.
그 때는 오래 걸리더라도 함께 해봅시다.

[pkeugine]

안녕하세요 이산, PK 입니다.
로직이 정말로 읽기가 편하네요! (뷰 빼고요 ㅎ)
고민을 많이 하신게 느껴졌습니다.

DM으로 얘기하고 리뷰를 하며, 일단 저희는 천천히 진행해보면 좋겠다는 생각을 했어요.
그래서 이번 리뷰에서는 테스트나 비교적 덜 중요한 코멘트는 빼고, 다음의 내용에 집중했습니다 :

1. 책임
2. 책임
3. 책임

사실 이산이 제게 남긴 질문의 절반은 다 이 주제인 것 같아서요,
이 주제로 어려움을 겪고 있다고 말해주시기도 했고요.

그래서 시간을 투자해서 이산의 코드로 저 스스로도
책임을 옮겨보기도 하고, 리팩터링을 하면서 시간을 좀 가졌습니다.

우선 명심해야 할 건,
제가 말한 그대로 하지 않아도 됩니다.
단지 어려워하는 상황에서 한가지 경로를 제안하는 것일 뿐이니
너무 제 예시에 매몰되어 그것만 생각하지는 말아주세요.

또한, 리팩터링하는데 시간이 너무 오래걸린다 싶으면
일단 생각나는대로 하고 다시 리뷰 요청 주셔도 됩니다.
질문이 있다면 언제든 물어봐도 좋고요.
정 안되겠다 싶으면 안해도 돼요.
지금 이 상태로도 읽기 편하고 좋습니다.

그래도 일단 이번 리팩터링 때 신경 썼으면 하는 부분들 나열해볼게요 :

1. 제가 남긴 예시 말고도 이산이 getter 를 줄일 수 있는 방향이나 책임을 옮기기 좋은 부분이 보이면 적극적으로 리팩터링해보세요.
2. view 챙기세요. 다른건 다 좋은데 현재 view 상태 때문에라도 머지 못 합니다.
3. 이번엔 10줄 요구사항에 이어 indent 요구사항까지 챙겨봅시다.

같이 한 번 파이팅해봅시다 😁
해피해킹하세요~!! 🍀 🍀 🍀

[pkeugine]

기능 목록을 체크리스트로 정리하고, 구현 완료 여부를 표시한 점 좋습니다.

v1.2로 "돌아가는 코드를 우선 만들어보기"에 집중한 것도 좋았다고 생각해요.
처음부터 완벽하게 설계하려다 한 줄도 못 쓰는 것보다,
일단 동작하는 코드를 만들고 그 위에 리팩터링하는 경험이 훨씬 값지거든요.

이제 "돌아가는 코드"는 만들어졌으니, 리팩터링의 차례입니다 :)

[pkeugine]

이동 규칙을 인터페이스로 분리하고,
각 구현체가 굉장히 깔끔하네요. 👍
이해하기도 쉽고 뭔가 추가해야 할 때 그리 어렵지 않게 관리 할 수 있을 듯 해요.

[pkeugine]

Q1. 패턴 사용에 대한 의문

저도 이산의 생각에 동의합니다.
패턴을 먼저 생각해내고 그걸 바로 도입하는 경우도 물론 있겠지만,
제일 자연스러운건 구현하다보니 '어, 이 패턴으로 리팩터링 하면 더 깔끔하겠는데?' 라는 생각에 의해
도입하는게 훨씬 자연스러워요.
처음부터 패턴을 너무 의식해서 코드를 작성하다보면 오히려 읽기 힘든 로직이 나올 수도 있고요.
그리고 그러다가 패턴이 이상하게 도입되는 경우도 있습니다.

그런데 공부 방식은 저는 양방향으로 하는 것을 추천드려요.
진짜 필요할 때 도입하기도 해보고,
이론적으로 'A 디자인 패턴, B 디자인 패턴이 있구나~', 그리고 '이럴 때 쓰는 구나~' 를 학습하며
한 번 간단하게 구현해보는겁니다.
그래야 진짜 필요할 때 그걸 떠올릴 수 있으니까요 ㅋㅋㅋ

이번 사이클에서 리뷰를 확인해보고 리팩터링을 해보며
이산이 생각했을 때 시도해볼만한 패턴이 있다면 한 번 적용해보세요.
저도 확인해보고 너무 이상하다 싶으면 짚고 넘어갈게요 👍

[pkeugine]

Q3. 움직임 규칙을 누가 가져야 하는가

두번째와 세번째 질문이 비슷한 내용이네요.
그래서 일단 더 넓은 범위로 보이는 3번부터 얘기해볼게요.

앞서 남긴 코멘트에서 저는 이 구조가 굉장히 깔끔하다고 했죠.
읽기도 편하고, 기물별 특징을 파악하는데 큰 어려움이 없었습니다.

그런데 이산은 왜 이 방식이 맞는지 잘 모르겠다고 느끼시나요?
이왕 이 주제를 던져보셨으니, 한 번 깊게 파보고싶네요 :)
다른 방식을 시도해보셨다면 어떤 것을 해봤고, 그 방식의 장단점도 알려주세요.
같이 얘기해보죠.

[pkeugine]

Q2. 기물 추상 클래스화

우선 저는 if 문이 그리 많다고 느끼진 않았습니다.
그리고 필요하다면 필요한 만큼 써야한다고 생각해요.
오히려 지나치게 그 수를 의식하다가 더 복잡한 로직이 생길 수도 있거든요.

그럼에도 이산의 접근 자체는 좋아요.
분명 기물의 종류를 구분해서 Piece 의 자식 클래스로 만들면 if 문을 줄일 수 있을 것 같거든요.
정확히 말하면 줄인다기보단 퍼져있는 것들을 더 적절한 곳으로 옮겨줄 수 있을 듯 해요.

그런데 이미 너무나도 잘 만들어주신 MoveRule과 그 구현체들이 있습니다.
MoveRule의 구현체가 Board 상태, 현 Position, 그리고 기물의 팀 정보를 받아서
직접 판단해볼 수도 있지 않을까요?
이러면 굳이 기물마다 추가 클래스를 만들지 않고도 지금 구조에 자연스럽게 적용 할 수 있을 것 같아요.

예를 들어, Piece 클래스에 다음과 같은 메서드를 추가하고 PieceType 에서 구현해보면 어떨까요? :

    public List<Position> findMovablePositions(Board board, Position position) {
        return pieceType.findMovablePositions(board, position, team);
    }

참고로 한 번에 완벽하게 못 할 수도 있고, 꼭 저 방식 그대로 하지 않아도 돼요.
그리고 앞서 말했듯 저는 현재 if 문 상황이 그리 심각하다고 보지 않습니다.
그러니 리팩터링하는데 너무 시간이 오래 걸린다 싶으면 제게 추가 질문을 하셔도 좋고, 넘어가도 괜찮아요. :)

[pkeugine]

10줄 요구사항 지켜보자는 목표 잘 지켜주셨어요 👍
(아직 좀 넘는 곳들이 보이긴 하는데, 그래도 충분히 고민해주신 것으로 보여서 넘어갑니다 ㅎ)

지금부터는 메서드 길이 10줄 외에도 다른 요구사항들을 한 번 챙겨보면 좋을 것 같아요.
특히 indent 요구사항을 중점으로 챙겨보죠.

[pkeugine]

아 그리고 view 쪽은 요구사항 적용을 크게 고려를 안해주신 것 같은데,
저는 view도 요구사항에 맞게, 그리고 가독성 좋게 리팩터링 해주셨으면 합니다.

백엔드 개발자라도 프로젝트 내에서 view 를 관리하는 상황이라면,
백엔드 로직과 동일하게 잘 관리해줘야 한다고 생각하거든요. 예외를 두면 안돼요.

실제로 백엔드인데도 프론트엔드를 다뤄야하는 경우가 굉장히 자주 발생하고 (생각보다 더 자주 발생합니다)
지금처럼 TUI 를 만들어야하는 경우도 생기거든요.
그 때 가서 '아 view 는 중요한 백엔드 로직이 아니니까' 라고 말할 수는 없잖아요 :)

근데 view 다뤄야 할 일이 진짜 자주 있을겁니다. 강조하는거예요.
우테코 크루들 보면 선을 완전히 그어버리는 경향이 있는 것 같아서요.

뷰에 indent 외에도 else 사용과 public 상수 등이 있는데,
전체적으로 리팩터링 해주세요.

[pkeugine]

앞서 말했던 team.isCho() 를 응용해봅시다.
현재 이 두 if 문들은 y 좌표를 구하는 것 외에는 완전히 동일해요.
그렇다면 y 좌표 구하는 걸 team 에게 맡겨서 이 중복을 줄여보면 어떨까요? :

int y = team.calculateY(defaultYPosition.get(pieceType));

생각해보면 team 에 따라 y 좌표 구하는 로직이 달라지는건 당연하잖아요.
그러니 team 이 그 역할을 담당해도 될 것 같은데 이산은 어떻게 생각하세요?

심지어 이산이 비슷한 시도를 했던 흔적이 있더라고요 : 2111fbf 😁

[pkeugine]

파일 끝에 빈 줄(trailing newline)이 없어요.

POSIX 표준에서는 텍스트 파일이 개행 문자로 끝나야 하고, Git diff에서 \\ No newline at end of file 경고가 뜨면서 diff가 지저분해져요.

IntelliJ 설정에서 Editor → General → Ensure every saved file ends with a line break를 켜두면 자동으로 처리돼요 👍

[pkeugine]

반복 입력을 추출하려는 시도 자체는 좋습니다.

다만 호출부가 두 곳인데 둘 다 Position을 반환해서 <T> 제네릭이 실질적으로 하는 일이 없고,
안에서 OutputView.printErrorMessage()를 직접 호출하고 IllegalArgumentException만 잡고 있어서
다른 맥락에서 재사용하기도 어려워요.

이 정도 규모에서는 오히려 풀어 쓰는 게 읽기 좋지 않을까요?

private Position askMovePiecePositionUntilValid(Board board) {
    while (true) {
        try {
            outputView.printMoveInfo();
            Position position = inputView.readPosition();
            routeChecker.findAvailablePositions(board, position);
            return position;
        } catch (IllegalArgumentException e) {
            OutputView.printErrorMessage(e.getMessage());
        }
    }
}

이러면 메서드 이름이 "뭘 하는지"를 말해주고,
doLoop의 정의를 찾아가지 않아도 바로 이해할 수 있어요.

[pkeugine]

더 나아가 while (true) 문도 명시적인 flag 를 두는 방식으로 바꾸면 어떨까 해요.
while (true) 는 정말 위험한 로직이거든요.

지금 당장 문제를 일으키지 않더라도
유지보수 하거나 기능이 추가되는 과정에서 의도치 않게 이 루프를 빠져나가지 못하는 문제가 생길 가능성이 높아요.
그러면 애플리케이션 자체가 제대로 동작하지 않게 되는건데요,

명시적인 flag를 넣는다고 그 문제가 완전히 해결되지는 않지만
적어도 안전장치 하나는 넣어준다고 보면 좋을 것 같네요.
그리 어렵지도 않으니 한 번 도전해봅시다!

[pkeugine]

패키지명이 moveRules로 camelCase인데,
Google Java Style Guide에서 패키지명은 전부 소문자여야 해요. (해당 컨벤션을 따르는건 저희 요구사항이기도 합니다)
moverules로 바꿔보면 어떨까요?

참고로 Google Java Style Guide 는 시간을 내서 꼭 한 번 전부 다 읽어봤으면 해요.
앞서 말한 것처럼 요구사항이기도 하고, 저 컨벤션을 따르는 곳이 많거든요.
실제로 다 읽으면 생각보다 그리 어렵지는 않을 거예요.