Step2 - 볼링 점수판(점수 계산) 리뷰요청 드립니다.#43
Merged
javajigi merged 66 commits intonext-step:Integerousfrom Jul 23, 2019
Merged
Conversation
javajigi
approved these changes
Jul 23, 2019
Contributor
javajigi
left a comment
There was a problem hiding this comment.
특별히 피드백할 부분이 없을 정도로 구현 잘 했네요. 💯
- 상태 객체를 추상화하고, 이 객체의 계층 구조를 만듦으로써 전체적인 객체의 역할이 명확해 지는 느낌이 나네요.
- 위와 같은 상태 객체의 추출이 처음부터 가능했다면 좋았겠지만 그렇지 않더라도 TDD를 하다보니 자연스럽게 등장했다면 더 좋은 경험이 될 수 있었을 것 같아요. 지속적인 리팩토링을 통해 자연스럽게 객체를 분리하는 연습을 하면 한단계 더 나은 연습을 할 수 있을 것 같아요.
- 테스트 코드 구현한 것으로 보니 TDD 방식으로 구현하려고 노력했다는 흔적이 보이는데 맞나요?
3단계가 리팩토링인데 딱히 리팩토링할 부분을 찾을 수 있을지 모르겠네요.
3단계 힌트와 javajigi 브랜치에 제가 구현한 코드가 있는데 참고해서 개선할 부분을 찾아보면 좋겠어요.
|
|
||
| public List<Frame> getFrames() { | ||
| return Collections.unmodifiableList(frames); | ||
| return frames.getFrames(); |
Contributor
There was a problem hiding this comment.
List<Frame>을 노출하지 않고 구현할 수는 없을까?
| import domain.Score; | ||
| import domain.state.State; | ||
|
|
||
| public class FrameResult { |
| import domain.Pins; | ||
| import domain.state.State; | ||
|
|
||
| abstract class Closed implements State { |
Contributor
There was a problem hiding this comment.
추상 클래스로 Closed 상태 객체 추출 👍
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.
안녕하세요!
우선, 1단계에서 피드백 받은 내용들을 반영하였습니다.
2단계는 저의 예상보다 더 어려웠고, 3단계가 리팩토링 단계이므로
2단계에서는 기능 구현을 우선시하며 구현했습니다..
때문에 개선해야 할 점들이 많을 것으로 예상됩니다..!
바쁘시겠지만 피드백 주시면 감사하겠습니다-!!
** step1 커밋들까지 따라온 것 보니 제가 step1 브랜치에서 step2 브랜치를 생성했나봅니다.. 코드 보기에 불편하실 것 같네요 죄송합니당..ㅠ