Skip to content

Conversation

@aegis1920
Copy link

많은 피드백 부탁드립니다! 감사합니다!!

Copy link

@hongsii hongsii 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단계도 잘 구현해주셨네요! 👍
몇 가지 피드백 드렸으니, 확인하셔서 반영해주세요~

this.players = splitNames(names);
}

public void initMoney(List<String> moneys, MoneyResult moneyResult) {
Copy link

Choose a reason for hiding this comment

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

MoneyResult 를 파라미터로 전달하지 않고 여기서 생성해도 되지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

Players에서 MoneyResult를 생성해도 되지만 MoneyResult를 생성하는 것이 Players 클래스의 역할이라 보기 어렵다고 생각했습니다. 어디에 들어가도 애매할 것 같아 Controller로 뺐었는데 Players의 역할이라고 봐도 괜찮을까요?

Copy link

@hongsii hongsii Mar 25, 2020

Choose a reason for hiding this comment

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

말씀하신대로 역할이 애매하다면 스스로 초기화를 책임져도 좋을 것 같아요!
MoneyResult 의 정적 팩토리 메서드에 getter로 필요한 값(Players)을 파라미터로 전달해 생성했어도 좋았을 것 같습니다.

Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

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

피드백 반영을 잘 해주셨습니다! 👍
코멘트 남겼으니 확인해주세요. 이번 단계는 머지하겠습니다!
미션 진행하시느라 고생많으셨습니다 :)

@hongsii hongsii merged commit beb480d into woowacourse:aegis1920 Mar 25, 2020
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.

3 participants