Step3 - 볼링 점수판(리팩토링) 리뷰 요청드립니다.#44
Merged
javajigi merged 6 commits intonext-step:Integerousfrom Jul 26, 2019
Merged
Conversation
javajigi
approved these changes
Jul 26, 2019
Contributor
javajigi
left a comment
javajigi
left a comment
There was a problem hiding this comment.
2단계 구현을 너무 잘 구현해 리팩토링할 부분이 많지 않고 좋네요. 💯
상수 관리와 관련한 의견을 피드백에 간단하게 남겼어요.
3단계까지 완벽하게 구현했다면 4단계는 어렵지 않게 마무리할 수 있을 겁니다.
| public static final int STRIKE_PINS = 10; | ||
| public static final int GUTTER_PINS = 0; | ||
| public static final Pins STRIKE = Pins.from(STRIKE_PINS); | ||
| public static final Pins GUTTER = Pins.from(GUTTER_PINS); |
Contributor
There was a problem hiding this comment.
저는 이런 상수 값들이 지금과 같이 의존관계가 높은 Pins와 같은 객체에 있는 것이 더 좋다고 생각해요.
|
|
||
| public static Score ofStrike() { | ||
| return new Score(STRIKE_PINS, 2); | ||
| } |
Contributor
There was a problem hiding this comment.
이와 같은 정적 팩토리 메소드 또한 api를 사용하는 입장에서는 유용한 메소드라 생각해요. 👍
This file contains hidden or 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
2 participants
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.
안녕하세요!
2단계 빠르게 리뷰해주셔서 감사합니다!
3단계에서는 제 눈에 밞히는 것들을 수정했는데, 대부분 정적팩토리 메서드를 추가한 것입니다.
혹시나 더 개선할 부분이 있다면 조언부탁드리겠습니다-!!
그 외에 고민했던 것은,
프로젝트 이곳 저곳에 비슷한 상수들이 조잡하게(?) 사용되는 것 같아서
상수 유틸리티 클래스 혹은 열거 타입 사용을 고민했습니다.
하지만 해당 클래스와 강하게 결합되는 경우가 많은 것으로 판단해서 (틀린 판단일 수 있습니다.)
고민하다가 해당 클래스 내에 존재하도록 내버려두었습니다..
비즈니스에 따라 다르겠지만 볼링 게임만 고려할 경우에
각각의 클래스에 흩뿌려진 상수를 한 곳에 모아서 관리하는 것이 좋을지,
지금처럼 강한 결합을 보이는 각각의 클래스에 두는 것이 좋을 지 궁금합니다..!
바쁘시겠지만 리뷰 부탁드리겠습니다-! 감사합니다ㅎ