Skip to content
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단계 - 체스] 헌치(박지우) 미션 제출합니다. #286

Merged
merged 132 commits into from Apr 3, 2022

Conversation

BETTERFUTURE4
Copy link

@BETTERFUTURE4 BETTERFUTURE4 commented Mar 27, 2022

안녕하세요 카일!😊
현재 페어 프로그래밍을 끝낸 후
기본적인 기능 구현 및 리팩토링만 진행한 상태로 제출하려 합니다!
리뷰 잘 부탁드려요ㅎㅎ
덧붙여 이번 미션에서 적용하려 했던 부분들 입니다!

적용사항

  • commend를 통한 이동(stateLauncher)을 상태패턴으로 구현
  • piece 전략을 strategy로 따로 빼서 구현

이번 미션은 많이 고되었네요^^;;
이외의 질문사항들은 생각나는대로 코멘트로 남기겠습니다!

wishoon and others added 30 commits March 22, 2022 17:43
@BETTERFUTURE4
Copy link
Author

BETTERFUTURE4 commented Mar 30, 2022

안녕하세요 카일😄
피드백 주신 부분들에 대한 개선을 적용하였습니다!

피드백 적용사항

  • 체스게임과 무관한 파일들이 많이 포함되어 있는 것 같아요🙂 -> 앞으로는 .gitgnore에 추가
  • 줄임말보단 풀네임을 작성해주는게 더 잘 읽힐 것 같습니다.
  • 전략패턴을 통해 말들의 이동을 잘 구현해주셨네요.
    • 해당 전략패턴이 왜 필요하다고 생각하시나요?
    • 말들별로 달라지는 값들을 각자 구현한다는 것은 좋지만, 각 말들의 역할은 무엇이라 정의하셨나요?
  • get이라는 표현이 너무 범용적으로 사용된다고 생각이 들어요.
    • 어떤 필드값을 가져오는 것은 get도 적절하지만
    • 내부적으로 연산을 통해 어떤 값을 계산하는 로직은 어떤 네이밍이 적절할지 고민해보아요.
  • OutputView에서 도메인으로 직접적인 의존을 가지고 있네요.
    • 요 부분은 어디서 담당해서 전달해줘야할까요?
  • 값을 재할당하는 구조가 아닌 불변으로 설계해보는건 어떨까요?
  • Command라는 객체가 도메인에서 사용되어도 될까요?
    • 패키지구조상 도메인이 컨트롤러 레이어에 의존하고 있네요.
  • 도메인인 포지션이 뷰의 입력방식인 move source target을 도메인이 매우 잘 알고 있는 것 같아요.
    • 추가로 0과 1이라는 인덱스가 무엇을 의미하는지도 상수화해주면 좋을 것 같아요.
  • 네이밍규칙을 지켜주세요🙂 추가로 두 가지를 얘기해보면 좋을 것 같아요.
    • TotalStatus를 통해서 위너를 찾을 수 있을 것 같은데, 해당 값을 추가로 인스턴스변수로 둔 이유가 있을까요?
    • 결과를 구하는 로직은 모두 Board에 있고 해당 객체는 단순히 데이터덩어리로 보여요. 이렇게 구성한 이유가 있을까요?
  • 테스트에서만 사용하는 전략을 전략패턴으로 구현한 이유가 있을까요?
    • 추가로, 실제 게임에서 사용되는 보드와 다른 형태의 판을 생성해서 테스트하는 것은 어떤 의미를 가지나요?
  • 팀과 관계없이 절대값을 만들어낼 수 있지 않을까요? -> 질문
  • 경기 중에서도 status를 통해 결과를 확인할 수 있어야 하는데 해당 부분이 구현되지 않은 것 같아요. -> 질문
  • **체스게임에 대한 상태를 상태패턴으로 관리하고 있는 것 같아요.
    • 혹시 ChessGame과 컨트롤러에서 따로 상태를 관리하는 이유가 있을까요?** -> 답변
  • pawn의 기본 점수는 1점이다.
    • 하지만 같은 세로줄에 같은 색의 폰이 있는 경우 1점이 아닌 0.5점을 준다.
    • 해당 요구사항이 포함되어있지 않은 것 같아요.
    • 요 부분에 추가로 각 말들이 자신의 점수를 가지고 있지 않고 별도의 객체를 둔 이유가 있을까요?

추가 구현사항

  • 상태 패턴(State), 말을 Interface 로 구현

이전 코멘트 중 추가 질문하고 싶은 부분들은 👀 이모지를 달았으니 확인해주세요!
항상 감사합니다~!

Copy link

@KIMSIYOUNG KIMSIYOUNG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 헌치!
변경해주신 부분 확인했습니다!
코멘트에 대댓글 남겨두었는데 확인 부탁드려요!

@BETTERFUTURE4
Copy link
Author

BETTERFUTURE4 commented Apr 2, 2022

안녕하세요 카일😄!
PR요청이 늦었네요ㅠㅠ
피드백 주신 부분들에 대한 개선을 적용했고, 따로 몇가지 리팩토링 작업도 진행했습니다!

피드백 적용 사항

  • status 커멘드 관련 피드백
    • 점수를 통해 판단하는 것이고,
    • 동점일 때는 누구를 이긴것으로 간주할지도 고민해봐야할 것 같아요.
    • 경기상황으로 돌아갈 수 있어야 합니다. 현재는 경기가 종료되어요.
    • status는 확실한 경기 승패를 드러내기보단, 점수기반으로 현황을 보여주는 기능
  • 이동을 통해, 왕이 잡히는 경우에 게임의 결과값(Finished)를 반환하는 방법은 어떨까요?
  • Command 객체 관련 피드백
    • 도메인으로 이동했지만 move, start 와 같은 입력값에 대한 의존이 그대로 들어온다.
    • 더 고민해보자 -> 입력값과 관련된 부분 -> view 패키지로 이동!
  • 루트 반환 메서드 관련 피드백
    • 목적지까지 반복을 통해 이동한 새로운 포지션을 매번 생성
      • for문 사용하는 방법이 생각나지 않아서, 대신 리스트에 담긴 이전 포지션 값을 통해 루트 생성

이외 적용사항

  • Abstract -> Default 이름변경
  • state.go 메소드를 컨트롤러로 이동
  • is...(isPawn() 등) 메서드를 enum으로 변환 후 저장, 반환!
  • 가능하면 추상클래스 제거

항상 감사합니다!

Copy link

@KIMSIYOUNG KIMSIYOUNG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 헌치!
리뷰 반영해주신 부분 확인하였습니다🙂
1단계는 여기서 머지하면 좋을 것 같아요!
고생하셨습니다 2단계도 잘 부탁드리겠습니다.

@KIMSIYOUNG KIMSIYOUNG merged commit d793574 into woowacourse:betterfuture4 Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants