-
Notifications
You must be signed in to change notification settings - Fork 137
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
[부산대 BE_정지민] 미션 제출합니다. #109
Open
stopmin
wants to merge
43
commits into
next-step:main
Choose a base branch
from
stopmin:stopmin
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 유지보수를 위해서는 나누는게 맞지만 프로젝트가 작아서 합쳐둠.
- 재시도 가능성을 생각하여 게임 자체에 대한 서비스로직으로 게임 추상화
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
다들 수고하셨습니다 🔥🔥
PR이 조금 심심해서 구현하면서 고민했던 부분이랑, 미래의 누군가가... 제 코드를 리뷰한다면 조금 도움이 됐으면 해서 정리해봤습니다 :D
짜다보니 계속 부족한 부분이 보여서 계속 고치고싶네요.
다음 미션은... 가능하면... 빨리 시작해서 이틀~사흘 정도는 고민해서 더 좋은 코드 짜보겠습니다!!!!!
다들 다음미션도 파이팅합시다💪
코드 리뷰시 참고사항
1.
Player
추상화HumanPlayer
,ComputerPlayer
도 일종의Player
라고 판단하였고 동작이Player
라는 하나의 인터페이스로 정의될 수 있다고 판단하여Player
로 추상화하여 각자HumanPlayer
,ComputerPlayer
로 구현체로 두었습니다.2. 난수 생성 모듈
3. 입출력 핸들러
infra
계층, 도메인/어플리케이션의 외부에 맞닿아있는 부분이라고 판단하여 DB나 다른 ORM과 맞닿아있을 수 있는infra
계층에 두었습니다.4. 입/출력 메시지 및 에러 메시지
domain
내에서Enum
으로 관리하였습니다.format()
메서드를 활용하면Enum
내의 message에 각종 값들을 추가해줄 수 있습니다. 이를 통해서 게임 결과(스트라이크, 볼)의 개수도 유연하게 대처할 수 있었습니다.ErrorType
도 둘 수 있는걸로 아는데 (Illegal~~~Error
같은 것들이나 뭐 404나..) 새벽감성으로 생각이 안나서 일단 message만 다루도록 했습니다.5. 야구게임에 관련된 매직넘버들
BaseballGame
도메인에서 관리하도록 하였습니다. 예외 문구나, 타 클래스에서 validation을 할 때도 해당 상수를 참고하도록 작성하였습니다.6. GameService를 왜 별도로 두었는가?
stack
이 쌓이도록 구현했던 기억이 있습니다.7. 테스트코드를 위해서 원래 코드를 양보해도 되는가? -
reflection
테스트코드에 대해서는 고민이 많았습니다.
굳이,
private
로서 동작해도 되는 것들이 테스트코드를 위해서 과연public
이나protect
같은 접근제어자로 바꿔도 될까? 고민하다가 테스트코드를 위해서 코드를 수정한다?라는 것은 솔직히 유지보수성은 좋다고 한들, 전체적인 코드 품질을 위해서는 조금 좋지 않을 수 있다고 판단하였습니다.따라서 테스트코드에서
reflection
를 활용하여 해당 메서드에 일시적으로 접근할 수 있도록 조작했습니다.그런데 좋은 방법이라고 생각하진 않습니다.
예제코드는 아래와 같습니다 :D
다른 선배한테 조언을 구해봤는데 객체로 빼서
public
으로 여는 방법이 좋다고 하는데 다음 미션에서 한번 활용해보겠습니다 :D