Step2 - 로또 게임 2단계 #15

pageprologue · 2021-07-17T10:04:46Z

학습로그

객체 지향 생활 체조 원칙
Java - collect(), Collector, Collectors
Java 단일 요소의 배열 Collections.singletonList vs Arrays.asList
메모리 절약을 위해서 요소가 없거나(empty) 하나인 경우에는 Collections.emptyList() 또는 Collections.singletonList()를 사용
GitHub의 Merge, Squash and Merge, Rebase and Merge 정확히 이해하기
cherry-pick으로 원하는 커밋 가져오기

jjj0611

희진 선생님 안녕하세요 2단계 구현해주신 내용 잘 봤습니다.
생각보다 코드리뷰가 많이 늦어져서 죄송합니다 😢

간단하게 고민해볼만한 부분들 코멘트 남겼으니 확인 부탁드려요! 🙏

jjj0611 · 2021-07-22T13:44:05Z

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

 public class LottoController {
-    public void run() {
+
+    private static final LottoTicketVendingMachine lottoTicketVendingMachine = new LottoTicketVendingMachine();


클래스 내부에서 new로 선언하며 의존하는 것과 주입받아서 사용하는 것에 대해서 고민해보면 좋을 것 같아요.
벤딩머신은 변경점이 그렇게 많지는 않겠지만, 주입해주는 방향에 대해 고민해보면 좋을 것 같아요.

넵~! LottoService 객체를 새로 만들었고, 여기에서 LottoTicketVendingMachine을 생성자로 주입받도록 변경하였습니다.

jjj0611 · 2021-07-23T10:12:36Z

src/main/java/lotto/LottoApplication.java

    public static void main(String[] args) {
-        LottoController lottoController = new LottoController();
-        lottoController.run();
+        LottoController.run();


LottoController에서 run하는 것도 instance 생성에서 static으로 변경한 이유가 있을까요?
이전 리뷰에서 제가 질문을 드린건 무조건 변경하라는 것이 아니라 어떤 차이가 있을까에 대해 고민해보시면 좋을 것 같다는 부분이었어요!
저는 둘 다 괜찮다고 생각하는데 '왜 그렇게 선택했는가'에 대한 기준은 중요하다고 생각합니다!

이 부분에 대해서 static 키워드와 정적 팩터리 메서드 부분을 다시 살펴보았습니다.

  static 영역에 할당된 메모리는 모든 객체가 공유하고, 프로그램의 종료시까지 메모리가 할당된 채로 존재하게 됩니다. 모든 객체가 공유한다는 점에서 사이드 이펙트 영향이 커 질 수 있습니다. 또, 프로그램이 종료되기 전에 항상 메모리에 상주하고 있어서 자주 사용하지 않는 메서드가 누적된다면 GC에 수거되지 못하므로 메모리 낭비가 발생할 수 있습니다.  
  
  인스턴스 생성과 비교하여 정적 팩터리 메서드의 장점에 대해 정리해 보았습니다.
   1. 생성자는 의미있는 명명을 하기 어렵지만, 정적 팩토리 메소드는 의미있는 명명이 가능하다. 
  2. 정적 팩토리 메소드는 메소드 리턴 타입 범위 내에서 다양한 리턴 타입의 정의가 가능하다.
   3. 정적 팩토리 메소드는 인스턴스를 생성하기 위한 로직을 캡슐화 할 수 있다.  
   
   여기에서는 LottoController 에서 메소드를 작게 분리하고, 의미 있는 명명을 하기 위해 인스턴스를 생성하여 메서드를 호출하는 방식으로 변경하였습니다~!

jjj0611 · 2021-07-23T10:15:27Z

src/main/java/lotto/domain/lotto/LottoGenerator.java

 public class LottoGenerator {

-    private static final List<LottoNumber> lottoNumberContainer = create();
+    private static final List<LottoNumber> LOTTO_NUMBER_CONTAINER = create();


LottoNumber에 대한 cache가 이곳에 위치해야 하는지 고민해보면 좋을 것 같아요.
지금 LottoGenerator를 통해서 Lotto를 생성하고 있는데, 코드 곳곳에 보면 new LottoNumber()로 생성하는 곳도 있고,
LottoGenerator를 통해서 caching된 객체를 받는 곳도 있어요.
이 부분에 대한 일관성(모두 new 를하여 사용할지, 어짜피 lottoNumber는 1~45만 필요하니 어디서든 caching된 것을 사용할지)을 유지하면 좋지 않을까 싶습니다!

LottoGenerator 의 역할을 LottoTicketVendingMachine 로 옮겼습니다~!  그리고 로또 번호 인스턴스를 생성하는 부분은 로또 티켓이 자동 생성인지, 수동 생성인지에 따라 차이가 생기게 되었습니다.

자동 생성에서는 LottoGenerator를 통해 전체 로또 번호를 미리 생성하여 중복되지 않은 로또 번호를 반환해 주고 있습니다.

수동 생성인 경우에는 수동 번호를 입력 받기 때문에 유효성 검증과 일관된 자료형을 사용할 목적으로 LottoNumber로 생성하고 있습니다.

수동 생성의 경우도 고민이 되는 부분이 있네요.
중복되지 않은 번호여야 한다는 것은 로또 티켓이 가져야 하는 role이지 않을까요?
수동으로 생성한다하더라도 티켓을 만들 때 유효성 검증에서 걸릴텐데
한 애플리케이션 안에서 어떤 부분은 new를 통해 새롭게 생성하고, 어떤 부분은 caching을 사용하는 구조보다는
일관성을 가져가는게 어떨까 싶네요 :)

넵 이 부분 다시 수정하였습니다~!

jjj0611 · 2021-07-23T10:15:52Z

src/main/java/lotto/domain/lotto/LottoGenerator.java

+        Collections.shuffle(LOTTO_NUMBER_CONTAINER);
+        return LOTTO_NUMBER_CONTAINER.subList(0, 6).stream()


0과 6이 의미하는 바가 무엇일까요?

jjj0611 · 2021-07-23T10:17:02Z

src/main/java/lotto/domain/lotto/LottoNumber.java

+    public LottoNumber(String lottoNumber) {
+        this(Integer.parseInt(lottoNumber));
+    }


Integer.parseInt(lottoNumber)를 했는데 lottoNumber가 숫자가 아닌 문자열이면 어떤 일이 벌어질까요?
무엇이 잘못되었는지 사용자가 잘 알 수 있게 하려면 어떻게 하는 것이 좋을까요?

LOTTO_NUMBER_CONTAINER 에서 6개의 볼을 꺼내기 위해 ArrayList.subList 를 사용하였습니다. 파라미터로 받는 fromIndex와 toIndex를 따로 상수화 하지 않았었는데, 6이라는 숫자는 LOTTO_NUMBER_SIZE 상수로 대체 하였습니다. 0도 상수로 빼는 것이 좋을까요?
 또한 ArrayList.subList 사용시 메모리 누수 위험성이 있다고 하여 수정하였습니다. ( ArrayList.subList 주의할 점)

엇 제가 남긴 코멘트는 IntegerParseInt에 대해서 valid하지 않은 String이 들어오면 어떻게 되는지에 대한 물음이었는데,
혹시 그에 대한 답변이 맞을까요??

아 이 부분 바로 위에 남긴 코멘트였는데 잘못 달았네요..ㅎ
IntegerParseInt 하는 부분 처리 했습니다.

jjj0611 · 2021-07-23T10:26:02Z

src/main/java/lotto/domain/vending/BuyingPrice.java

+        if (buyingPrice < TICKET_PRICE) {
+            throw new IllegalArgumentException("구입 금액은 " + TICKET_PRICE + "원 이상이여야 합니다.");
        }
    }

-    private void validateFactor(int buyingPrice) {
-        if ((buyingPrice % LottoTicketVendingMachine.TICKET_PRICE) != 0) {
-            throw new IllegalArgumentException("구입 금액은 1,000원 단위여야 합니다.");
+    private void validateDivisible(int buyingPrice) {
+        if ((buyingPrice % TICKET_PRICE) != 0) {
+            throw new IllegalArgumentException("구입 금액은 " + TICKET_PRICE + "원 단위여야 합니다.");


BuyingPrice가 여전히 TICKET_PRICE가 있는 TicketAmount에 의존하고 있고,
TicketAmount도 BuyingPrice를 받는 메서드가 존재해서 양방향 의존이 되고 있네요.
예전 리뷰에서도 말씀드렸다시피 '구매금액'이라는 용어가 가지는 방향에서 저는 티켓 금액인 1,000원에 대한 고민이 자꾸 드는 것 같아요.
정의한 용어와 그 용어의 역할에 대해 같이 고민해보면 좋을 것 같아요.
더불어 양방향 의존에 대한 고민도 해보시면 좋을 것 같습니다!

이 부분은 TicketAmount 생성자에서 BuyingPrice 객체를 받지 않고 totalTicketAmount 를 받도록 수정하였습니다. TICKET_PRICE 상수도 BuyingPrice 로 옮겼습니다. 양방향 의존에 대해서는 더 학습해 보도록 하겠습니다~!

jjj0611 · 2021-07-23T10:28:08Z

src/main/java/lotto/domain/vending/LottoTicketVendingMachine.java

+
+public class LottoTicketVendingMachine {
+
+    private static final LottoGenerator LOTTO_GENERATOR = new LottoGenerator();


여기서도 static하게 필드 선언을 하는 것과 주입받아 사용하는 것은 어떤 차이가 있을까요?

사실 로또 게임에서 LottoGenerator도 크게 변하지 않을 도메인이긴 하지만 주입 받을 때와 필드로 선언했을 때의 차이에 대해서는 고민해보시면 좋을 것 같아요

넵! 이번 미션에서 static 키워드에 대해 많이 생각해 보게 되네요 :)  생성자에서 로또 번호를 generate 해주고, shuffle 해주도록 수정하였습니다.

jjj0611 · 2021-07-23T10:28:48Z

src/main/java/lotto/domain/vending/TicketAmount.java

@@ -0,0 +1,36 @@
+package lotto.domain.vending;
+
+public class TicketAmount {


위에 BuyingPrice에서도 말씀드렸지만, BuyingPrice와 TicketAmount의 관계에 대해 고민해보시면 좋을 것 같아요!

jjj0611 · 2021-07-23T10:40:03Z

src/main/java/lotto/domain/winning/WinningLottoRank.java

+
+import java.util.*;
+
+public class WinningLottoRank {


WinningLottoRank가 하고 있는 일이 너무 많은 것 같아요.
처음에 LottoTikcet들과 당첨 번호를 받아서 랭크가 생성이 되고,
lottoTicket 한개를 받아서 일치하는 숫자가 몇개인지도 확인할 수 있고(이건 WinningLottoRank가 아닌 WinningNumbers의 역할이죠)
LottoRank의 Key들을 반환하는 역할도 하고 있네요.

이 WinningLottoRank를 만든 목적은 어떤거였을까요?

WinningLottoRank는 당첨 번호와 일치하는 개수에 따른 순위를 계산해 주는 역할을 하기 위해서 만들었습니다. 로또 티켓에서 당첨번호와 일치하는 개수를 반환하는 메서드는 WinningNumbers 로 옮겼습니다!

jjj0611 · 2021-07-23T10:41:35Z

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

    private static final String BONUS_NUMBER_MESSAGE = "보너스 볼을 입력해 주세요.";
-    private static final String DELIMITER = ",";

    private InputView() {


static으로 사용하는 것들 중 이것은 생성자를 막아두고, 어떤 것들은 생성자를 열어두었네요!

넵 수정했습니다 :)

pageprologue added 14 commits July 17, 2021 18:54

refactor : 티켓 장 수 계산 메소드 수정, static 키워드 수정

ceb411e

refactor : 로또 당첨 순위 클래스명 변경 (LottoPrize -> LottoRank), 코드 이동 및 테스트 코드 수정

250b0a0

docs : 구현 목록 추가

5079c3d

style : 패키지 정리

76114b5

feat : 티켓 장 수 테스트 및 기능 구현, 관련 소스 수정

7889a2c

feat : InputView 수동 티켓 입력, 티켓 장 수 예외 처리 추가

3ce0976

feat : 로또 수동 생성 테스트 및 기능 추가

0f9d2a0

feat : 전체 로또 티켓 생성 테스트 및 기능 추가, 관련 소스 수정

7f7ae6c

refactor : OutputView 정리

4f4b103

feat : 로또 번호가 중복될 경우 예외 발생 테스트 및 기능 구현

1fe76e0

feat : 당첨 결과 테스트 및 기능 구현, 관련 소스 수정, 코드 정리

6983ea5

fix : 보너스 번호가 일치하는 경우 당첨 순위 버그 수정 및 테스트 코드 작성

2094b29

refactor : 요소가 없는 경우 Collections.emptyList() 로 변경

4089514

feat : 로또 티켓 유효성 검사 테스트 및 기능 구현

ab32b7c

jjj0611 self-requested a review July 17, 2021 11:29

jjj0611 requested changes Jul 23, 2021

View reviewed changes

pageprologue added 3 commits August 2, 2021 02:01

refactor : static 키워드 제거, 생성자 주입으로 변경

b2792af

refactor : 사용자 입력 에러를 Controller 가 담당하도록 수정, 클래스 변수를 생성자 주입으로 수정

2a6e9d8

refactor : 당첨 번호 일치 개수 기능 WinningLottoRank -> WinningNumbers 로 이동

af61ec9

jjj0611 self-requested a review August 10, 2021 11:06

pageprologue added 5 commits August 18, 2021 03:27

refactor : LottoNumber 캐시 적용

75f5df1

refactor : LottoService, LottoController, StringUtil 정리

491f190

style : rank 패키지 생성 및 Rank 관련 클래스 이동

a01d40a

refactor : 테스트 클래스 패키지 이동, Rank 2등인 경우 수정

db7c747

chore: root project name 수정

7bb61e0

		Collections.shuffle(LOTTO_NUMBER_CONTAINER);
		return LOTTO_NUMBER_CONTAINER.subList(0, 6).stream()


		public class LottoTicketVendingMachine {

		private static final LottoGenerator LOTTO_GENERATOR = new LottoGenerator();

		@@ -0,0 +1,36 @@
		package lotto.domain.vending;

		public class TicketAmount {

Step2 - 로또 게임 2단계 #15

Are you sure you want to change the base?

Step2 - 로또 게임 2단계 #15

Uh oh!

Conversation

pageprologue commented Jul 17, 2021

학습로그

Uh oh!

jjj0611 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!

Choose a reason for hiding this comment

Uh oh!

pageprologue Aug 3, 2021 • 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!

pageprologue Aug 3, 2021 • 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!

pageprologue Aug 3, 2021 • 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!

pageprologue Aug 3, 2021 • 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!

pageprologue Aug 3, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pageprologue Aug 3, 2021 •

edited

Loading

pageprologue Aug 3, 2021 •

edited

Loading

pageprologue Aug 3, 2021 •

edited

Loading

pageprologue Aug 3, 2021 •

edited

Loading

pageprologue Aug 3, 2021 •

edited

Loading