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

[2단계 - 블랙잭 베팅] 모코(송선권) 미션 제출합니다. #889

Open
wants to merge 16 commits into
base: songsunkook
Choose a base branch
from

Conversation

songsunkook
Copy link

안녕하세요, 모코입니다. 🌱
2단계 요구사항인 "베팅" 기능을 적용하여 리뷰 요청드립니다.
2단계도 잘부탁드립니다!

체크 리스트

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

객체지향 생활체조 요구사항을 얼마나 잘 충족했다고 생각하시나요?

1~5점 중에서 선택해주세요.

  • 1 (전혀 충족하지 못함)
  • 2
  • 3 (보통)
  • 4
  • 5 (완벽하게 충족)

선택한 점수의 이유를 적어주세요.

기능 추가 및 리팩토링 과정에서 BlackjackGame(구 BlackjackController) 클래스가 3개의 필드를 가지게 되어버렸습니다. 줄일 수 있는 마땅한 방법을 모르겠습니다.

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

controller 제거

1단계 코드에서 controller가 반드시 필요한가 라는 다른 크루들의 질문에 마땅한 답변을 내지 못했습니다.

1단계에서 service의 책임이 작고, (Y, N 입력을 통한) 카드 추가 뽑기 단계가 controller에게 도메인 로직에 대한 강한 의존성이 생겨버려 service와 controller를 분리하는 메리트가 적어보였고, 결국 둘을 합쳤습니다. 하지만 이렇게 되니 controller가 MVC에서 도메인 로직을 최소화하고 정말 Controller의 역할만 하고있는가?라는 질문에 적절한 답을 내놓을 수 없었고, 그럴거면 controller가 무슨 의미가 있나. 차라리 controller를 지워보는 것은 어떠냐?라는 이야기를 접했습니다.

이야기가 괜찮은 접근인 것 같아서 한 번 적용해보았습니다. 기존의 BlackjackController를 BlackjackGame으로 네이밍을 변경했습니다. 사실 네이밍을 변경한 게 전부인 느낌이지만 더이상 controller로써 바라보지 않고자 했습니다.

GameManager의 구현체를 Dealer에서 BlackjackGame으로 변경

변경된 BlackjackGame 클래스는 더이상 controller가 아니라 블랙잭 게임을 관리하는 클래스가 되었습니다. 이렇게 보니 GameManager라는 책임은 Dealer보다 BlackjackGame에 더 어울려보였고, GameManager의 구현체를 Dealer에서 BlackjackGame으로 변경했습니다.

변경된 후에는 GameManager를 사용하는 곳이 테스트코드밖에 남지 않았습니다. 하지만 GameManager로 접근하는 기존 테스트코드가 잘못되었다고 느껴지지는 않아서 테스트코드까지 변경하지는 않았습니다. 얼핏보면 테스트코드만을 위한 GameManager가 되어버린 느낌도 있는데 여기에 대해 이스트는 어떻게 생각하시는지 궁금합니다.

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.

1 participant