Skip to content
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

step1 완료 #98

Merged
merged 14 commits into from
Mar 22, 2019
Merged

step1 완료 #98

merged 14 commits into from
Mar 22, 2019

Conversation

CheolHoJung
Copy link

도메인

LotteryNumber

로또 번호 값 포장 클래스입니다. 당첨번호 비교를 위해 equals와 hashCode 메소드를 구현, toString으로 번호를 반환합니다.

LotteryTicket

로또 번호 컬렉션 클래스입니다. 당첨번호 비교를 위해 getWinningRank 메소드를 제공, toString으로 번호목록을 반환합니다.

LotteryRank

당첨번호 순위 클래스입니다. generate 정적 메소드에서 당첨 갯수에 맞는 enum 인스턴스를 반환합니다.

랜덤 숫자 목록 생성

NumbersGenerator

List를 제공하는 인터페이스입니다(Supplier). LotteryGameBoard의 생성자로 구현체를 전달하여 지정번호로 로또를 생성할 수 있습니다.

BoundedUniqueNumbersGenerator

NumbersGenerator의 구현체로, 랜덤번호목록(1 ~ 65)을 생성합니다.

게임

LotteryGameBoard

로또 티켓 판매를 담당하는 보드입니다. 당첨번호를 전달하여 랭크목록을 가져오기 위해 checkWinningNumbers를, ResultView에서 번호목록을 출력하기 위해 getTickets를 제공합니다.

InputView

ResultView


안녕하세요 :) step1 구현에 대한 피드백 요청드립니다!

보드의 checkWinningNumbers, getTickets와 ResultView가 너무 얽혀있다는 생각이 드네요. 최대한 TDD로 해보려고 했지만, 보드에 대한 테스트를 작성하는데 LotteryTicket, LotteryRank 등의 다른 클래스가 계속 필요해지면서 왔다갔다 하느라 어떻게 진행했는지 기억도 잘 안나네요 ㅠㅠ

진행하면서 특히나 어려웠던 부분들 남겨봅니다.

  • 보통 TDD로 개발을 진행하면서 테스트를 작성하다가 다른 클래스가 필요해질 때는 현재 작성중인 클래스 개발을 멈추고 다른 클래스를 진행하나요??
  • 구조를 잡기 어려웠던 부분
    • GameBoard에서 티켓목록 반환 -> ResultView에서 티켓목록 출력
      • 티켓구매 메소드에서 생성된 티켓의 복사본을 반환하는게 더 나을까요?
    • Gameboard의 티켓구매 메소드에서 티켓의 갯수 반환, 당첨체크 메소드에서 List<랭크> 반환 -> ResultView에서 티켓의 갯수와 List<랭크>를 이용해 통계 출력
      • 별도의 통계 클래스를 만들어서 제공하는게 더 나을까요?

- 번호 생성 범위 테스트
- 동일성 테스트
- toString 테스트
- generate 테스트
- 티켓생성 테스트
- 1 ~ 4등 당첨 테스트
- 랜덤번호 생성 용도
- 테스트를 위한 지정번호 생성 용도
- 유니크 랜덤번호목록 생성 테스트
- 랜덤번호 로또구매 테스트
- 지정번호 로또구매 테스트
- 당첨 테스트
Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요. :) 이번에 리뷰를 담당하게된 리뷰어 박현식입니다.
클래스 분리 및 테스트 코드 작성 모두 깔끔하게 잘 하신 것 같아요.👍

첫번째 질문에 대한 답으로는 TDD로 일단 현재 개발하고 있는 클래스가 테스트를 통과하도록 작성
-> 리팩토링 과정에서 새로운 클래스 개발 이 과정으로 생각하시면 될 것 같아요! (Red-Green-Refactor)
개발하면서 새로운 클래스가 필요하다고 느끼는건 리팩토링을 같이 병행하고 있기 때문이라고 생각합니다. :)
물론 저걸 지키면서 개발하는게 쉽지는 않지만요. :)

두번째는 리스트 자체는 이뮤터블로 반환하고,
LotteryNumber과 LotteryTicket을 이뮤터블하게 작성하신다면 굳이 복사본은 필요 없을 것 같습니다. :)

마지막 질문 사항에 대해서는 피드백에 같이 추가했어요!
궁금하신 점 있으시면 언제든지 DM주세요. :)

src/main/java/lottery/domain/LotteryRank.java Outdated Show resolved Hide resolved
src/main/java/lottery/domain/LotteryRank.java Show resolved Hide resolved
src/main/java/lottery/domain/LotteryTicket.java Outdated Show resolved Hide resolved
src/main/java/lottery/domain/LotteryNumber.java Outdated Show resolved Hide resolved
InputView.viewTicketCount(count);
ResultView.viewTickets(board.getTickets());

final List<LotteryRank> lotteryRanks = board.checkWinningNumbers(InputView.inputWinningNumbers());
Copy link

Choose a reason for hiding this comment

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

앞서 레이싱게임에서 했던 것과 같이, 로또 결과를 반환해주는 Result 클래스를 만들어보면 어떨까요? :)

src/main/java/lottery/view/ResultView.java Outdated Show resolved Hide resolved
src/main/java/lottery/view/ResultView.java Outdated Show resolved Hide resolved
@CheolHoJung
Copy link
Author

피드백 감사합니다 :)

Result클래스 대신 통계 클래스를 추가했습니다.
통계 클래스의 단위 테스트는 등수별로 하나씩만 테스트 했는데, 더 많은 테스트가 필요한지 궁금합니다.

Copy link

@phs1116 phs1116 left a comment

Choose a reason for hiding this comment

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

안녕하세요. 리뷰어 박현식입니다. :)
리뷰 요청하신걸 놓쳐서 늦게 리뷰 드린 점 죄송합니다.
전체적으로 구현을 깔끔하게 잘하셨어요 💯
통계 테스트는 딱 최소한의 범위로 잘 테스트 하신 것 같습니다.
간단한 피드백 추가했구요 2단계 진행하시면서 수정 하시면 될 것 같아요. :)
1단계 개발하시느라 수고 많으셨습니다.

@phs1116 phs1116 merged commit 61e5603 into next-step:CheolHoJung Mar 22, 2019
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.

None yet

2 participants