[서브웨이] 로또 2차미션 코드리뷰 요청입니다. #183

joseph415 · 2020-02-26T14:24:35Z

No description provided.

…임으로 변경

전체 로또 티켓 갯수에서 수동으로 발급 받을때 갯수가 줄지 않는 버그 수정

0개를 수동으로 입력 받을때에도 발급되는 버그 수정

타입을 잘못 설정해 수익률 계산시 overflow되는 버그 발견하여 long타입으로 수정함

dave915

서브웨이 구현하느라 고생하셨어요 👍
코멘트 남겼으니 확인 후 반영해보세요!

dave915 · 2020-02-28T04:26:30Z

src/main/java/lotto/view/InputView.java

    public static List<LottoBall> InputWinningTicket() {
        try {
            OutputView.printAnswerWinningBalls();
-            String winningTicket = scanner.nextLine();
-            LottoTicket winningTicketFactory = new LottoTicket(winningTicket);
+            LottoTicket winningTicketFactory = generateLottoTicket();


LottoTicket winningTicketFactory = generateLottoTicket(); 이부분은 우승 로또 Number 생성이라는 Domain로직으로 보는게 맞지 않을까요? View에서는 사용자에 대한 입력 및 출력 역활만 하면 좋을 것 같아요.

알겠습니다! 고치도록 하겠습니다.

dave915 · 2020-02-28T04:31:57Z

src/main/java/lotto/controller/LottoController.java

+        manualTicketingCount = InputView.inputManualTicketingCount(ticketingTotalCount);
+
+        for (int i = 0; i < manualTicketingCount.getTicketingCount(); i++) {
+            LottoTickets.insertLottoTicket(new LottoTicket(InputView.InputManualLottoTicket()));


LottoTicket들을 LottoTickets가 Static 변수로 관리하고 있네요.
이렇게 되면 추후에 LottoGame이 다중의 사용자가 이용 할 수 있게 요청사항이 왔을때 어떤 문제가 있을까요?

dave915 · 2020-02-28T04:34:54Z

src/main/java/lotto/domain/LottoTicket.java

@@ -28,15 +28,15 @@ public LottoTicket(List<LottoBall> lottoTicket) {
    public LottoTicket(String inputValue){


inputValue라는 파라미터 명보단 String LottoNumbers같은 내부에서 어떻게 쓰일지 알기쉽도록 변수명을 짓는게 좋을 것 같아요.

dave915 · 2020-02-28T04:37:43Z

src/main/java/lotto/view/InputView.java

+    }
+
+    private static LottoTicket generateLottoTicket() {
+        String winningTicket = scanner.nextLine();


InputWinningTicket메소드 뿐만아니라 InputManualLottoTicket에서도 사용하고 있어서 winningTicket변수명은 수정해야 할 것 같아요!

dave915 · 2020-02-28T04:47:58Z

src/main/java/lotto/controller/LottoController.java

+        }
+    }
+
+
    private WinningTicket generateWinningBalls() {
        try {
            List<LottoBall> winningBallsInput = InputView.InputWinningTicket();


WinningTicket 내부에 코멘트를 남길 수 없어서 여기에 남겨요.
List<LottoBall> winningTicket에 일급 컬렉션을 적용해보면 어떨까요?
또, 적용해 보면서 LottoTicket과 합쳐서 여섯개의 중복되지 않은 로또넘버를 가지는 객체로 합칠 수 있는지 고민해볼까요?

ps. 현재 객체 생성에 관한 validation이 흩어져있는데 객체 생성에 관한 로직이나 validation은 각 객체 내부에서 담당하도록 수정해보면 어떨까요?

1000원 미안 입력시 예외처리 정수 이외의 다른 값 입력할 경우 재입력 음수인 경우 예외처리 구현

총 몇장을 구매할수 있는지 로또 총 티켓의 개수 구하는 기능구현 천원 단위가 아닌 나머지 금액은 반환하는 기능구현

도메인과 의존성이 생길수 있는 validate가 생겨서 제거하고 객체 안에서 구현하였습니다.

joseph415 · 2020-03-01T17:59:05Z

-리뷰-
LottoTicket들을 LottoTickets가 Static 변수로 관리하고 있네요.
이렇게 되면 추후에 LottoGame이 다중의 사용자가 이용 할 수 있게 요청사항이 왔을때 어떤 문제가 있을까요?

-질문-
이 말이 헷갈렸는데 다중사용자가 사용할경우 LottoTickets를 공유해서 문제가 생길수 있겟네요 ,,
static의 남발이 문제를 가져온다는게 이런것인지 드디어 알았네요!

구글링해서 어떤 정보를 보았는데 domain에서의 static을 최대한 지양해야 한다는데 이게 맞는 정보인지 궁금합니다!
그렇다면 싱글톤 패턴 사용도 지양되나요? 아님 그냥 유연하게 경우에 따라 다 다른것인지 궁금합니다!

dave915 · 2020-03-02T14:59:17Z

구글링해서 어떤 정보를 보았는데 domain에서의 static을 최대한 지양해야 한다는데 이게 맞는 정보인지 궁금합니다!
Domian 객체의 상태를 나타내는 필드에는 static을 사용하면 여러 인스턴스가 서로의 상태에 영향을 미치기 때문에 static 으로 선언 하면 안됩니다. 하지만 객체의 정적인 정보를 나타낼때는 사용해도 무관 하다고 생각해요 :)
아래 링크도 읽어보세요!

https://unabated.tistory.com/entry/%EC%99%9C-%EC%9E%90%EB%B0%94%EC%97%90%EC%84%9C-static%EC%9D%98-%EC%82%AC%EC%9A%A9%EC%9D%84-%EC%A7%80%EC%96%91%ED%95%B4%EC%95%BC-%ED%95%98%EB%8A%94%EA%B0%80

그렇다면 싱글톤 패턴 사용도 지양되나요? 아님 그냥 유연하게 경우에 따라 다 다른것인지 궁금합니다!
싱글톤 패턴은 동일한 인스턴스를 재사용한다는 리소스적인 측면에서 이점이 있습니다. 하지만 싱글톤을 여러곳에서 호출하는 환경 (예를 들면 멀티쓰레드 환경) 등에서 사용할때 싱글톤이 상태를 지니게 되면 위 에서 이야기한 static 필드처럼 상태에 대해 무결성이 깨지기 때문에 주의해서 사용해야합니다.

dave915

처음 부터 다시 구현 하느라 고생 많으셨어요 👍
코멘트 몇개 남겼어요 확인 후 반영해주세요!
궁금한 점 있으시면 언제든 DM주세요 :)

dave915 · 2020-03-02T15:16:49Z

src/main/java/lotto/domain/LottoBalls.java

        return LOTTO_BALLS.stream()
-                .filter(x -> x.getLottoNumber() == lottoBallNumber)
+                .filter(Ball -> Ball.getLottoBall() == findLottoBall)


!!!! 🧐

ball -> ball.getLottoBall() == findLottoBall

dave915 · 2020-03-02T15:17:56Z

src/main/java/lotto/domain/LottoBalls.java

@@ -32,13 +34,16 @@ public static void shuffle() {
        for (int i = 0; i < LOTTO_TICKET_SIZE; i++) {
            lottoTicket.add(LOTTO_BALLS.get(i));
        }
+
+        Collections.sort(lottoTicket);
        return Collections.unmodifiableList(lottoTicket);


generateLottoTicket요 메소드도 테스트 추가했으면 좋겠어요!

dave915 · 2020-03-02T15:19:48Z

src/main/java/lotto/domain/LottoTicket.java

-        ValidateWinningTicketUtils.validateWinningBallsNumber(winningBalls);
-        ValidateWinningTicketUtils.validateWinningBallsLength(winningBalls);
-        ValidateWinningTicketUtils.validateDuplicatedWinningBalls(winningBalls);
+    //수동생성


LottoTicket입장에선 수동 생성인지, 자동생성인지 관심이 없어서 주석은 지워도 될 것 같아요 :)

dave915 · 2020-03-02T15:30:20Z

src/main/java/lotto/utils/ValidationUtils.java

+    public static void validateLottoUnit(String input) {
+        if (Integer.parseInt(input) < LOTTO_UNIT) {
+            throw new UnderLottoUnitMoney(String.format(UNDER_LOTTO_UNIT_RETURN_CHANGE, input));
+        }
+    }


이 클래스에서 error 메시지 변수명에 Lotto들어가는 Validation은 각 객체 안에서 해주면 좋을 것 같아요 :)

dave915 · 2020-03-02T15:34:41Z

src/main/java/lotto/view/OutputView.java

+        lottoTickets.getLottoTickets()
+                .forEach(lottoTicket -> System.out.println(lottoTicket.getLottoTicket()
+                        .stream()
+                        .map(LottoBall::getLottoBall)
+                        .collect(Collectors.toList())));


이 부분은 메소드 분리 해주는게 좋을 것 같네요 :)

System.out.println(lottoTicket.getLottoTicket() .stream() .map(LottoBall::getLottoBall) .collect(Collectors.toList()))

혹시 분리하는 이유를 알수 있을까요?? 저는 결과를 출력하는 메소드명을 printLottoTicketAndChangeMoney 라고 지어서 저 로직을 안에 넣었는데
가독성 측면에서 좋아지는 이점이 있어서 나누는 걸까요??
아니면 하나의 기능만 하게 나눠야해서 나누는것일까요?

꼭 메소드 분리가 아니더라도 람다식 안에 람다식이 중첩으로 들어가거나 함수 호출 시 람다식을 넣으면 다른 개발자가 봤을때 가독성이 떨어지게 됩니다 😭
적절하게 메소드분리 또는 변수 분리 해주세요.

그리고 또 이경우엔 lottoTicket에게 getLottoBallValues() 뭐 이런식으로 메세지를 보내서 Lotto의 숫자들을 제공하는 책임을 주는게 좋지 않을까요?

dave915 · 2020-03-02T15:47:12Z

src/main/java/lotto/controller/LottoController.java


 public class LottoController {
+    public void play() {
+        Money money = generateMoney();
+        LottoTicketCount autoTicketLottoTicketCount = new LottoTicketCount(money.generateLottoTicketCount());


autoTicketLottoTicketCount의 ticketCount가 생성 시점에 정해지는것이 아니라 generateAutoTicket메소드 호출 후에 정해지네요.
generateAutoTicket메소드 호출 전에 autoTicketLottoTicketCount을 사용하게 되면 원하는 의도와 다르게 동작 할 것 같네요.

autoTicketLottoTicketCount 를 allTicketLottoTicketCount 로 바꿔주어 의미를 좀 명확하게 나타냈습니다!

dave915 · 2020-03-02T15:49:00Z

src/main/java/lotto/domain/EarningRate.java

-        if (Double.isInfinite(calculateEarningRate(purchaseAmount))
-                || Double.isNaN(calculateEarningRate(purchaseAmount))) {
-            throw new IllegalArgumentException(ILLEGAL_PURCHASE_AMOUNT);
+        for (Rank r : rank) {


Rank r 축약어는 지양해 주세요!

List<Rank> rank 를 ranks로 선언 하면 될 것 같아요 :)

Suggested change

for (Rank r : rank) {

for (Rank rank : ranks) {

dave915

안녕하세요 서브웨이 :)
처음 PR주셨을때 보다 많이 깔끔해졌네요 💯
코멘트 몇개 남겼으니 확인 해 보세요!
수고하셨습니다. 머지할게요!

dave915 · 2020-03-03T17:15:56Z

src/main/java/lotto/domain/EarningRate.java

-        if (Double.isInfinite(calculateEarningRate(purchaseAmount))
-                || Double.isNaN(calculateEarningRate(purchaseAmount))) {
-            throw new IllegalArgumentException(ILLEGAL_PURCHASE_AMOUNT);
+        for (Rank r : rank) {


List<Rank> rank 를 ranks로 선언 하면 될 것 같아요 :)

Suggested change

for (Rank r : rank) {

for (Rank rank : ranks) {

dave915 · 2020-03-03T17:21:47Z

src/main/java/lotto/view/OutputView.java

+        lottoTickets.getLottoTickets()
+                .forEach(lottoTicket -> System.out.println(lottoTicket.getLottoTicket()
+                        .stream()
+                        .map(LottoBall::getLottoBall)
+                        .collect(Collectors.toList())));


꼭 메소드 분리가 아니더라도 람다식 안에 람다식이 중첩으로 들어가거나 함수 호출 시 람다식을 넣으면 다른 개발자가 봤을때 가독성이 떨어지게 됩니다 😭
적절하게 메소드분리 또는 변수 분리 해주세요.

그리고 또 이경우엔 lottoTicket에게 getLottoBallValues() 뭐 이런식으로 메세지를 보내서 Lotto의 숫자들을 제공하는 책임을 주는게 좋지 않을까요?

dave915 · 2020-03-03T17:23:07Z

src/test/java/lotto/domain/WinningTicketTest.java

-        WinningTicket winningTicket = new WinningTicket(winningBallValues, 7);
+    @DisplayName("몇개 맞추는지 test")
+    void hitLottoBallTest() {
+        LottoTicket lottoTicket = new LottoTicket("1,2,3,4,5,6");


전체적으로 new LottoTicket 대신 LottoTicket.of 로 해야 되는것 같은데요? :)

Suggested change

LottoTicket lottoTicket = new LottoTicket("1,2,3,4,5,6");

LottoTicket lottoTicket = LottoTicket.of("1,2,3,4,5,6");

dave915 · 2020-03-03T17:24:10Z

src/main/java/lotto/view/InputView.java

+    private static final Scanner SCANNER = new Scanner(System.in);
+
+    public static String inputMoney(){
+        OutputView.printInputMoney();


이경우는 InputView도 View이기 때문에 OutputView를 호출 안하고 바로 System.out.println을 사용해도 될 것 같아요!

joseph415 added 13 commits February 25, 2020 14:02

[docs] 수동으로 티켓 발급하는 기능 추가를 위해 to-do list 작성

297084a

[refactor] 기능 구현을 위해 inputWinningTicket 메소드 로직 캡술화

6ea9a93

[feat] 수동으로 로또 티켓 입력 받는 기능 구현

52febaa

[feat] 수동으로 몇개의 티켓을 발권할지 입력 받는 기능 구현

e158ae3

[refactor] PurchaseAmount lottoTicket 메소드명 lottoTicktetCount로 변경

57b32ff

[refactor] 수동 발급 티켓 개수 책임을 갖고있던 ManualTicketingCount 전체 티켓개수를 관리 하는 책…

66adfa1

…임으로 변경

[refactor] WinningTicket에 관한 validation을 Ticket에 관한 validation으로 명명 변경

455cd9e

[feat] 카운트를 리턴하는 기능 구현

e38fda4

[fix] 수동으로 몇개의 티켓을 발권할지 입력 받는 기능 버그 수정

1821fa9

전체 로또 티켓 갯수에서 수동으로 발급 받을때 갯수가 줄지 않는 버그 수정

[fix] 수동으로 몇개의 티켓을 발권할지 입력 받는 기능 버그 수정

c13027b

0개를 수동으로 입력 받을때에도 발급되는 버그 수정

[feat] 구매 가능 티켓보다 많은 수를 입력할 경우 재입력 예외처리 구현

b7d0ccb

[fix] 타입 오류로 overflow 되는 버그 수정

6c83a92

타입을 잘못 설정해 수익률 계산시 overflow되는 버그 발견하여 long타입으로 수정함

[fix] 쓰이지 않은 return 값을 발견해서 void로 수정

c150edb

dave915 suggested changes Feb 28, 2020

View reviewed changes

joseph415 added 16 commits February 28, 2020 22:39

[docs] README에 요구사항, 기능 구현 목록 작성

8d20546

[feat] 3가지 예외처리 기능 구현

0a6d32a

1000원 미안 입력시 예외처리 정수 이외의 다른 값 입력할 경우 재입력 음수인 경우 예외처리 구현

[feat] 구입 금액 클래스 생성

8c033fd

총 몇장을 구매할수 있는지 로또 총 티켓의 개수 구하는 기능구현 천원 단위가 아닌 나머지 금액은 반환하는 기능구현

[refactor] utils의 Validation을 제거하고 각 객체 안에서 구현하도록 리팩토링

9588d8d

도메인과 의존성이 생길수 있는 validate가 생겨서 제거하고 객체 안에서 구현하였습니다.

[feat] 수동으로 몇개의 티켓을 발권할지 입력 받는 기능 구현

558d190

[feat] 거스름돈을 출력하는 기능 구현

394eabd

[test] Count객체 단위 테스트

2a74fc7

[feat] 수동으로 로또 티켓 입력 받는 기능 구현

92b2e01

[feat] 개수에 맞춰 로또 발급하는 기능 구현

07d6584

[test] 수동으로 입력받을때 로또볼 개수 검증 단위테스트 추가

2a25bf4

[feat] 당첨번호와 로또 티켓을 비교하는 기능 구현

80c0455

[feat] LottoController 에서 당첨 번호 입력빋기 구현

eeb9455

[test] 보너스볼 판별하는 단위테스트 추가

7b001d2

[feat] 당첨 등수 구하는 기능 구현

55f1a3d

[feat] 각 랭크에 대한 개수 구하는 기능 구현

51950ba

[feat] 당첨 통계 출력구현

003cf67

joseph415 added 4 commits March 2, 2020 01:28

[feat] 수익률 계산하고 출력하는 기능 구현

50af921

[refactor] 전반적인 리팩토링

5148529

[refactor] LottoTicket 싱글톤 패턴을 멀티스레드 이용시 문제 가능성이 보여 인스턴스 변수로 변경

7ed9ea5

[fix] 수익율출력시 입력금액 출력되는 버그 수정

2fad47b

[refacotor] 변수명 수정

fd3092d

dave915 suggested changes Mar 2, 2020

View reviewed changes

joseph415 added 10 commits March 3, 2020 21:14

[refactor] 변수 명명 리팩토링

4cdfdfa

[test] generateLottoTicket() 단위 테스트 추가

bd333d2

[refactor] 매소드가 하나의 일만 하도록 분리

f150b77

[refactor] 변수명 리팩토링

83b0ee1

[refactor] 생성자2개 이상인 부분 of() 매소드 추가하여 정적 매소드로 부를수 있게함

75879c5

[refactor] validationutils 공통으로 샤용하지 않는 validation은 모두 객체 안으로 리팩토링

d39a52c

[refactor] 상속 구조 조합 구조로 변경

7e24d80

[refactor] 안쓰이는 메소드 제거

3a64e41

[refactor] 매소드 명명 리팩토링

837387c

[refactor] LottoTicket 리팩토링

30f8c42

dave915 approved these changes Mar 3, 2020

View reviewed changes

dave915 merged commit e809e23 into woowacourse:joseph415 Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[서브웨이] 로또 2차미션 코드리뷰 요청입니다. #183

[서브웨이] 로또 2차미션 코드리뷰 요청입니다. #183

joseph415 commented Feb 26, 2020

dave915 left a comment

dave915 Feb 28, 2020

joseph415 Feb 28, 2020

dave915 Feb 28, 2020

dave915 Feb 28, 2020

dave915 Feb 28, 2020

dave915 Feb 28, 2020

joseph415 commented Mar 1, 2020

dave915 commented Mar 2, 2020

dave915 left a comment

dave915 Mar 2, 2020

joseph415 Mar 3, 2020

dave915 Mar 2, 2020

dave915 Mar 2, 2020

dave915 Mar 2, 2020

dave915 Mar 2, 2020

joseph415 Mar 3, 2020 •

edited

Loading

dave915 Mar 3, 2020

dave915 Mar 2, 2020

joseph415 Mar 3, 2020

dave915 Mar 2, 2020

dave915 Mar 3, 2020

dave915 left a comment

dave915 Mar 3, 2020

dave915 Mar 3, 2020

dave915 Mar 3, 2020

dave915 Mar 3, 2020

		@@ -28,15 +28,15 @@ public LottoTicket(List<LottoBall> lottoTicket) {
		public LottoTicket(String inputValue){

	LottoTicket lottoTicket = new LottoTicket("1,2,3,4,5,6");
	LottoTicket lottoTicket = LottoTicket.of("1,2,3,4,5,6");

[서브웨이] 로또 2차미션 코드리뷰 요청입니다. #183

[서브웨이] 로또 2차미션 코드리뷰 요청입니다. #183

Conversation

joseph415 commented Feb 26, 2020

dave915 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseph415 commented Mar 1, 2020

dave915 commented Mar 2, 2020

dave915 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseph415 Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dave915 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseph415 Mar 3, 2020 •

edited

Loading