[Step04] 로또(수동) PR by pbg0205 · Pull Request #2315 · next-step/java-lotto

pbg0205 · 2022-04-23T06:25:12Z

안녕하세요 TDD 14기 박병기 입니다
로또(수동) 미션 완료하여 리뷰 요청 드립니다 😃

이전에 피드백 주셨던 내용에 고민했던 부분은 아래와 같습니다!

1. RandomUtil 클래스 제거 여부

피드백에 주신 말씀에 대해 고민해보았습니다! 기존의 유틸 클래스로 분리한 의도는 난수 생성에 관련된 로직을 구분하기 위함이었습니다. 하지만 RandomUtil �메서드를 재사용하지 않고 메서드 호출 횟수를 증가시키는 것은 적합해보이지 않은 선택인 것 같아 RandomUtil을 �제거하고 RandomGenerationStrategy로 이동하였습니다 👍 👍

2. RandomGenerationStrategy에 관한 고찰

수동 미션을 진행하면서 LottoTicketGenerator에서 유연하게 로또 생성 방식(NumberGenerationStrategy)에 대해 고민해보았습니다.

1. 필드로 관리하여 setter를 사용하는 방법
2. 필드를 제거하고 파라미터로 NumberGenerationStrategy를 전달하는 방법

setter를 사용하는 방법은 �당장은 큰 변화로 보이지 않을 수 있으나 필드의 변경을 추적하는 어려움과 응집도를 낮출 수 있는 원인인 것 같았습니다. 그리고 �파라미터로 전달받는 것이 조금 더 적절한 범위에서 변수를 사용하고 유연하게 랜덤, 수동 생성 방식을 사용할 수 있는 방법으로 생각하여 2번 방법을 선택하게 되었습니다.
하지만 조금 고민되는 부분이 NumberGenerationStrategy에서 관리하는 필드가 존재하지 않아 적절한 객체의 책임을 할당한 것인지에 관한 의문이 듭니다... 이 부분에 관한 리뷰어님의 의견이 궁금합니다 🤔

3. ScannerUtil 클래스 생성
기존의 InputView의 책임에서 콘솔 값 입력 + 입력 안내 출력 의 책임이 있었습니다. 이번 미션을 진행하면서 수동 입력에 관한 요구사항에 따라 수동으로 로또를 생성하는 방식(ManualGenerationStrategy) 에서 콘솔 값 입력 로직(책임)을 사용해야 하는 경우가 발생하여 책임을 분리하고 코드 재사용을 위해 ScannerUtil 클래스를 분리하였습니다!

이번 미션을 진행하면서 변경에 유연한 설계에 관한 고민을 해볼 수 있는 기회였던 것 같습니다. 꼼꼼한 피드백 덕분에 다양한 사고로 고민해 볼 수 있었습니다. 👍 👍 👍 부족한 부분이나 첨언해주실 부분 있으시면 아낌없는 피드백 부탁드립니다 🙏
감사합니다!! 🙇‍♂️

- 5개가 일치하고 보너스 번호를 포함한다면 2등을 반환한다 - 보너스 번호가 기존에 포함되지 않았을 경우 예외 반환

- 수동 로또 번호 생성를 위한 입력 기능 분리

- model 내부 package : lotto, rank 로 분리

- 추가 숫자 문자열 반환 로직 InputUtil 이동

- 수동 입력을 위한 readManualTicketPrice 추가 - LottoGameController 로직 변경 - ManualPrice, RandomPrice 구분으로 별도 로또 생성 로직 구현 - 의미를 명확하기 위한 메서드 네이밍 재명명

aiden-sim

지난번에 드렸던 피드백도 잘 반영해 주시고,
로또 마지막 미션까지 멋지게 구현해 주셨습니다. 👍
기존에 구조를 잘 잡아주셔서 새로운 요구조건에 따른 변경점이 커보이진 않네요.

몇 가지 리뷰 남겨 놓았는데 확인 후, 재요청 부탁드리겠습니다.

그리고 RandomGenerationStrategy에 관한 고찰 에서 문의 하셨던 부분은
저도 2번 방식을 사용해도 괜찮은것 같은데, 아래 부분에 대한 이해도가 떨어져서 부연 설명 해주시면 감사하겠습니다! 😄

조금 고민되는 부분이 NumberGenerationStrategy에서 관리하는 필드가 존재하지 않아 적절한 객체의 책임을 할당한 것인지에 관한 의문이 듭니다.

로또 미션의 끝이 보이네요. 마지막까지 화이팅입니다 ❗❗❗

aiden-sim · 2022-04-23T09:06:37Z

-        return ticketMachine.generateWinningTicket(inputView.readWinningNumbers());
+        List<Integer> winningNumbers = inputView.readWinningNumbers();
+        LottoNumber bonusNumber = inputView.readBonusNumber();
+        return ticketMachine.generateWinningTicket(winningNumbers, bonusNumber);


당첨 번호와 보너스볼을 WinningTicket 에서 관리하도록 변경하셨군요 👍
WinningTicket 의 응집도가 좀 더 높아진것 같네요.

aiden-sim · 2022-04-23T09:22:05Z

    }

-    private LottoTicket generateLottoNumbers(Set<Integer> numbers) {
+    private LottoTicket generateLottoTicket(Set<Integer> numbers) {


참고만 해주세요~

로또 번호를 생성할때, 계속 new를 통해 새로운 객체를 생성하고 있어서 조금 비효율 적인것 같습니다.
만약 사용자가 100개의 로또를 구매했다면
6(로또번호 개수) * 100 = 600 개의 새로운 LottoNumber 객체가 생성될 것입니다.

로또 번호는 1~45 고정이기 때문에 미리 캐싱해서 사용하면 성능적으로 많은 향상이 될것 같습니다.

요런 기법을 flyweight pattern 이라고 하는데
java API 내부에서도 자주 사용되는 값을 새로 생성하지 않고 재사용하기 위해 종종 사용됩니다.

참고) https://sshplendid.github.io/blog/dev/designpattern/2020/02/12/flyweight-pattern/

시간이 되신다면 정적 팩터리 메서드 + flyweight pattern 을 적용해 보는것도 추천드립니다. 😄

aiden-sim · 2022-04-23T09:27:32Z

+    public Rank drawLotto(LottoTicket lottoTicket) {
+        int count = this.lottoTicket.countMatches(lottoTicket);
+        boolean matchBonus = lottoTicket.includeBonusNumber(bonusNumber);
+        count += addCountIfSecondPotential(count, matchBonus);


로또 규칙은 다음과 같은데

1등은 6개 번호 모두 일치 2등은 5개 번호 일치 + 보너스 번호 일치 3등은 5개 번호 일치 + 보너스 번호 불일치

기존에 작성하셨던 로직이 맞지 않나요 ?! 🤔

현재 로직은 4개 번호 일치 + 보너스 번호 일치 시 2등으로 처리되고 있습니다.
그리고 2등 출력 시, 보너스 볼 일치 문구 추가해 주시면 좀더 명확할것 같네요. 😄

다시 확인해보니까 이전 로직이 맞네요 😅
당첨된 적이 없다보니 2, 3등 규칙이 헷갈리네요...ㅎ
이전 로직으로 변경하도록 하겠습니다 !

aiden-sim · 2022-04-23T09:39:23Z

+
+    public WinningTicket(LottoTicket lottoTicket, LottoNumber bonusNumber) {
+        this.lottoTicket = lottoTicket;
+        validateBonusNumber(bonusNumber);


유효성 검사 잘 해주셨습니다 👍

보통 유효성 검사를 가장먼저 진행 하는데
lottoTicket 값을 선언 후 체크해야되기 때문에 흐름이 조금 어색한것 같습니다.

lottoTicket, bonusNumber 값을 넘겨서 유효성 체크하는건 어떻게 생각하시나요? 🤔

validateBonusNumber(lottoTicket, bonusNumber);

피드백 주시고 다시 확인해보니 어색한 것 같네요
윗 부분으로 검증 로직 변경해보겠습니다 😃

aiden-sim · 2022-04-23T09:46:27Z

+
+    private static final String DEFAULT_DELIMITER = ",";
+
+    private final static Scanner scanner = new Scanner(System.in);


Suggested change

private final static Scanner scanner = new Scanner(System.in);

private static final Scanner scanner = new Scanner(System.in);

보통 자바에서 상수 규칙은 static final로 사용되기 때문에 통일시키면 좋을것 같습니다 😄

aiden-sim · 2022-04-23T09:50:20Z

-
-    private long readNumber() {
-        return Long.parseLong(readLine());
+        return value % PRICE_PER_TICKET == 0;


가독성 있는 상수 처리 좋습니다. 👍

aiden-sim · 2022-04-23T09:54:44Z

+    }
+
+    private int generateNumber() {
+        return new Random().nextInt(MAX_LOTTO_NUMBER_BOUND);


generateNumber가 호출될때마다 Random 객체가 새로 생성될것 같네요.
한번 만, 선언해 놓고 사용해 볼 순 없을까요? 🤔

힌트) 문자열 덧셈 계산기 Pattern.compile 정적 선언

private static final Random RANDOM = new Random();

이렇게 한번 시도해보겠습니다 🤔

pbg0205 · 2022-04-23T11:48:19Z

제가 장황하게 말씀드렸네요. LottoTicketGenerator가 데이터를 관리하지 않고 로직만 담당하는 클래스 형태를 띄고 있어 역할이 조금 애매한 것 같아 이 클래스를 제거해야 해야하는지에 관한 고민이었습니다 🤔

public class LottoTicketGenerator {
    
    // 관리하는 데이터가 없고 단순히 로직만 처리하는 형식이라서 객체를 분리해야하지 않알까 하는 고민이 들었습니다..! 
    
    public List<LottoTicket> generateLottoTickets(int ticketCount, NumberGenerationStrategy numberGenerationStrategy) {
        return IntStream.range(0, ticketCount)
                .mapToObj(index -> generateLottoTicket(numberGenerationStrategy))
                .collect(Collectors.toList());
    }

   ...

}

- Random 객체 static final 로 변경 - WinningTicket validateBonus 위치 변경 - static final 순서 수정

aiden-sim · 2022-04-23T12:34:42Z

@pbg0205
아하 이해했습니다! LottoTicketGenerator의 역할에 대해서 고민을 하셨던 거군요.
생각해보면 LottoTicketGenerator는 현재 WinningTicket, LottoTicket 등의 객체를 생성하는
역할이라고 볼 수 있겠네요.

저는 보통 다른 객체를 생성하는 역할을 갖는 클래스를 Factory 로 사용하고 있습니다.
그런면에서 LottoTicketGenerator의 역할은 Factory 와 동일하다고 생각되며
적절한 역할을 수행하고 있다 생각합니다.

다만 저는 하나의 클래스에서 서로 다른 객체를 생성하게 하진 않고
명확하게 해당 객체만 생성하도록 역할을 나누긴 합니다.

예를 들어
WinningTicketFactory 는 WinningTicket 생성
LottoTicketFactory는 LottoTicket만 생성

List 형태가 필요하다면 아래와 같이 LottoTicketFactory를 이용하는 방식을 선호합니다.

public class TicketMachine {
    prviate LottoTicketFactory lottoTicketFactory;

    private List<LottoTicket> buyLottoTickets(int count, NumberGenerationStrategy numberGenerationStrategy) {
        return IntStream.range(0, count)
                .mapToObj(index -> lottoTicketFactory.generateLottoTicket(numberGenerationStrategy))
                .collect(Collectors.toList());
    }
}

링크 드린 부분도 참고하시면 좋을것 같습니다 😄

참고로 제가 얘기한 factory는 factory method pattern 보다는 좀 더 단순화된 형태입니다.

참고)
https://johngrib.github.io/wiki/pattern/factory/
https://stackoverflow.com/questions/13029261/design-patterns-factory-vs-factory-method-vs-abstract-factory

- 로또 번호 캐싱을 통한 메모리 누수 문제 보완 - 정적 팩토리 메서드를 통한 생성자 캡슐화 - flyweight pattern 으로 인한 동일성 보장으로 equals, hash 메서드 제거

- 2등 : 5개 포함 + 보너스 번호 포함 - 3등 : 5개 포함 + 보너스 번호 미포함

1. List<LottoNumber> 로직 제거 -> Set<LottoNumber> 로 대체 (LottoTicket 생성 로직에 List<Integer> -> List<LottoNumber> -> Set<LottoNumber> 로직 불필요) 2. LottoGenerator 삭제

pbg0205 · 2022-04-23T14:28:08Z

@SimJunBo 안녕하세요 리뷰어님!
피드백 주신 내용들 토대로 코드 수정 완료하여 재요청 드립니다ㅎㅎ

변경사항

flyweight pattern 적용

리뷰어님께서 언급해주신 내용대로 객체를 캐싱하는 방법이 효율적인 메모리 관리를 할 수 있는 역할이라 생각하여 flyweight pattern을 적용해보았습니다.
LottoNumber에 lottoNumbers HashMap 자료구조를 추가하고 정적 팩토리 메서드를 선언하여 기존에 존재하는 값일 경우 캐싱한 값을 반환합니다.

LottoTicketGenerator 제거

앞서 말씀해주신 팩토리 메서드 패턴 내용들을 적용한 후, 리팩토링을 진행해보니 LottoTicketFactory의 역할이 애매한 것 같아 LottoTicket 생성의 책임을 LottoMachine으로 이동하였습니다.

�flyweight pattern은 처음 들어본 생소한 개념이라 재밌게 학습하였습니다 😃
조금 더 메모리를 효율적으로 사용하는 방법을 하나 얻어가네요. 감사합니다 👍

aiden-sim

마지막 로또 미션 피드백 까지 아주 잘 반영해 주셨네요 👍

이번 미션을 하시면서 객체의 역할에 대해서 계속 고민하고 지속적으로 리팩토링
하시는 모습이 매우 인상 적이었습니다.
덕분에 저도 여러 고민을 하면서 재미있게 리뷰 한 것 같습니다.

한가지 당부 드리고 싶은 부분은 로또 미션 진행 하시면서
기능에 대한 요구사항을 햇갈려 하셨던 부분이 있었는데
앞으로 미션을 진행하시면서 README.md 에 todo list 를
작성해 보시면 좀 더 도움이 되실 것 같습니다. 😄

저와 함께한 시간이 조금이라도 도움이 되셨길 바라며
앞으로 남은 미션들도 지금처럼 잘 해내실 거라 믿고 열심히 응원하겠습니다. 💯

로또 미션 하시느라 고생하셨습니다.

aiden-sim · 2022-04-23T19:06:51Z

+import java.util.stream.Collectors;
+import java.util.stream.IntStream;

 public class TicketMachine {


LottoTicketGenerator 을 제거하고 TicketMachine가 factory 역할을 하게 되었군요.
지금 구조가 훨씬 간결하고 좋은것 같습니다 👍

pbg0205 · 2022-04-24T04:38:12Z

@SimJunBo 로또 미션 진행하면서 다양한 피드백 주신 덕분에 잘 마무리할 수 있었습니다 👍 👍

미션 진행하면서 요구사항에 관해 혼동하다 보니 리뷰해주실 때 많이 힘들셨을 것 같아요. 😢 이 부분은 미션 진행하기 이전에 To-Do-List를 작성해보면서 개선하도록 하겠습니다ㅎㅎ

리뷰어님께서 편하게 의사소통 해주시고 구조 뿐만 아니라 디테일한 개선점까지 꼼꼼하게 �리뷰 해주셔서 �미션하는 동안 재밌게 작업할 수 있었습니다. �편하게 이야기 할 수 있도록 많이 배려해주신 덕분에 정말 많이 얻어가네요. 저도 한번 더 질문하고 싶고 고민해볼 수 있는 기회였습니다. 의사소통 면에서도 배워가네요 👍 👍
감사합니다!! 💯 💯 💯

pbg0205 added 9 commits April 22, 2022 20:27

refactor: BonusNumber WinningTicket 필드 위치 변경

e2b0409

refactor: 2등 결정 정책 변경

755fdba

- 5개가 일치하고 보너스 번호를 포함한다면 2등을 반환한다 - 보너스 번호가 기존에 포함되지 않았을 경우 예외 반환

refactor: NumberGenerationStrategy 앞단에서 주입 받는 방식으로 변경

c8f6320

refactor: 난수 생성 책임 RandomGenerationStrategy 이동

8654942

refactor: ScannerUtil 분리

a27b3f7

- 수동 로또 번호 생성를 위한 입력 기능 분리

refactor: package 분리

4dbdcc7

- model 내부 package : lotto, rank 로 분리

refactor: InputUtil 추가 분리 작업 진행

32b48c4

- 추가 숫자 문자열 반환 로직 InputUtil 이동

refactor: NumberGenerationStrategy 파라미터 방식으로 변경

4142721

aiden-sim requested changes Apr 23, 2022

View reviewed changes

refactor: step04 피드백 내용 반영1

c85d85a

- Random 객체 static final 로 변경 - WinningTicket validateBonus 위치 변경 - static final 순서 수정

pbg0205 added 3 commits April 23, 2022 22:07

feat: flyweight pattern & 정적 팩토리 메서드 패턴 도입

b4dd2d2

- 로또 번호 캐싱을 통한 메모리 누수 문제 보완 - 정적 팩토리 메서드를 통한 생성자 캡슐화 - flyweight pattern 으로 인한 동일성 보장으로 equals, hash 메서드 제거

refactor: 로또 등수 판명 로직 변경

6c399c4

- 2등 : 5개 포함 + 보너스 번호 포함 - 3등 : 5개 포함 + 보너스 번호 미포함

refactor: 로또 생성 로직 리팩토링

f94e9c0

1. List<LottoNumber> 로직 제거 -> Set<LottoNumber> 로 대체 (LottoTicket 생성 로직에 List<Integer> -> List<LottoNumber> -> Set<LottoNumber> 로직 불필요) 2. LottoGenerator 삭제

aiden-sim approved these changes Apr 23, 2022

View reviewed changes

aiden-sim merged commit b5f06b2 into next-step:pbg0205 Apr 23, 2022


		private static final String DEFAULT_DELIMITER = ",";

		private final static Scanner scanner = new Scanner(System.in);

	private final static Scanner scanner = new Scanner(System.in);
	private static final Scanner scanner = new Scanner(System.in);

Uh oh!

Conversation

pbg0205 commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aiden-sim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiden-sim Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbg0205 Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbg0205 commented Apr 23, 2022

Uh oh!

aiden-sim commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbg0205 commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

변경사항

Uh oh!

aiden-sim left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiden-sim Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbg0205 commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pbg0205 commented Apr 23, 2022 •

edited

Loading

aiden-sim Apr 23, 2022 •

edited

Loading

pbg0205 Apr 23, 2022 •

edited

Loading

aiden-sim commented Apr 23, 2022 •

edited

Loading

pbg0205 commented Apr 23, 2022 •

edited

Loading

aiden-sim left a comment •

edited

Loading

aiden-sim Apr 23, 2022 •

edited

Loading

pbg0205 commented Apr 24, 2022 •

edited

Loading