Skip to content

피드백적용 및 로또 2단계 구현 #139

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 29, 2019
Merged

피드백적용 및 로또 2단계 구현 #139

merged 5 commits into from
Mar 29, 2019

Conversation

yesjehmi
Copy link

  1. LottoNumbers.class, Number.class
  • 일급컬렉션 적용
  1. LottoResultStatus Enum bonus항목 추가
  2. Price 객체 더 적합한 클래스명으로 변경 > Money.class
  3. LottoResult.class
    • 멤버변수 2개 이하로 두기위해 불필요한 인스턴수 변수 (benefitRate) 삭제
  4. 당첨번호+보너스 번호 관리를 위해 WinningLotto.class 추가
  5. class구조 변경, 추가에 따른 test class추가.

@yesjehmi
1. LottoNumbers.class, Number.class
c5d4990 
   - 일급컬렉션 적용
2. LottoResultStatus Enum bonus항목 추가
3. Price 객체 더 적합한 클래스명으로 변경 > Money.class
4. LottoResult.class
   - 멤버변수 2개 이하로 두기위해 불필요한 인스턴수 변수 (benefitRate) 삭제
5. 당첨번호+보너스 번호 관리를 위해 WinningLotto.class 추가
6. class구조 변경, 추가에 따른 test class추가.
ssosso
ssosso requested changes Mar 25, 2019
View reviewed changes
Copy link

@ssosso ssosso left a comment

Choose a reason for hiding this comment

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

혜진님, 피드백이 다소 늦어져서 죄송합니다.
몇 가지 피드백 드리오니 확인부탁드립니다:)~

yesjehmi added 3 commits March 27, 2019 23:29
@yesjehmi
1. GenerateLotto LottoReturn으로 변경.
89cc8ba 
2. LottoResultStatus, description 추가.
3. Number compare > equals로 수정.
	- testClass로 수정.
4. test class명 setUp method 명 컨벤션 추가
@yesjehmi
1. lottoStatus Enum에 description 수정.
234e398 
	- resultView에 foreach로 수정.
2. Lotto.GenerateLotto > 생성자로 변경.
3. LottoGenerator > LottoNumberGenerator
	- method 쓰임새에 맞게 class 명 변경
@yesjehmi
test class 변경
04ac723
ssosso
ssosso requested changes Mar 28, 2019
View reviewed changes
Copy link

@ssosso ssosso left a comment

Choose a reason for hiding this comment

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

혜진님, 댓글 질문주신 부분까지 확인해서 피드백 드립니다~ 확인 부탁드려요 😉

src/main/java/domain/Lotto.java Outdated
Lotto lotto = new Lotto();
lotto.numbers = new Numbers(integers);
return lotto;
public Lotto (LottoNumbers integers) {
Copy link

Choose a reason for hiding this comment

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

코드 컨벤션

@yesjehmi yesjehmi mentioned this pull request Mar 28, 2019
Closed
@ausleaf @yesjehmi
1. Lotto, WinningLotto
10bd17e 
 > Lotto class 간소화 및 winning number 매칭등의 메소드는 winningLotto로 위임

2. LottoNumberGenerator, LottoNumbers
> 생성자로 방식에서 정적 팩토리 메소드로 instance 생성하도록 수정.

3. LottoResult
> winnint number 매칭, 주체 변경에 따른 소스 수정

4. Number : 내부에서 사용되는 수 (로또 최소값, 최대값) 상수화

5. InputView에서 String > String[]로 값 return 해 주도록 수정.
> 개인적으로 winningNumber의 타입 컨버팅, 유효성 체크 등은 비즈니스 내부 로직이라고 생각하여서 inputView에서는 최소한으로 split만을 한 채 값을 전달하도록 구현하였습니다.

4. TestClass 수정
yesjehmi
yesjehmi commented Mar 28, 2019
View reviewed changes
ssosso
ssosso approved these changes Mar 29, 2019
View reviewed changes
Copy link

@ssosso ssosso left a comment

Choose a reason for hiding this comment

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

혜진님, 제가 잘못 피드백 드린 부분까지 꼼꼼히 확인해주셔서 감사드립니다.
답변 및 몇 가지 피드백 더 드립니다. 이번 단계는 merge하고 다음 단계에서 피드백 반영 고려해봐주세요:)
감사합니다.

src/main/java/domain/WinningLotto.java
private LottoNumbers winningNumber;
private Number bonusNumber;

public WinningLotto(String[] inputWinnigNumber, int bonusNumber) {
Copy link

Choose a reason for hiding this comment

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

winningLotto에 String[]이 아닌 다른 형식의 inputWinningNumber를 받는 것은 어떨까요~?

@ssosso ssosso merged commit 98cfc62 into next-step:yesjehmi Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ssosso ssosso ssosso approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yesjehmi @ssosso @ausleaf