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

Step2 #3434

Open
wants to merge 9 commits into
base: chocheolhee
Choose a base branch
from
Open

Step2 #3434

wants to merge 9 commits into from

Conversation

chocheolhee
Copy link

No description provided.

Copy link
Member

@nooose nooose left a comment

Choose a reason for hiding this comment

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

안녕하세요 철희님
제가 남긴 코멘트 반영해 보면서 단계별로 개선해 나가면
요구사항을 잘 지킬 수 있을 것 같습니다.
기능 구현에 어려움이 있다면 중간중간 DM 남겨주세요

public static int inputPurchaseValue() {
System.out.println("구입 금액을 입력해 주세요.");
String value = SCANNER.nextLine();
return Integer.parseInt(value) / 1000;
Copy link
Member

Choose a reason for hiding this comment

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

1000이라는 숫자는 상수로 치환을 할 수 있을 것 같아요

Copy link
Member

Choose a reason for hiding this comment

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

금액은 그대로 입력받고 입력한 금액에 대해서 로또 장수를 판단하는 로직은 도메인에 위치해야 하지 않을까요?

Comment on lines +13 to +16
public static void printResultPurchaseValue(int value) {
System.out.println(value+"개를 구매했습니다");
System.out.println();
}
Copy link
Member

Choose a reason for hiding this comment

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

몇개를 구매했는지 정수를 전달하지 않고도
Lotto 컬렉션을 파라미터에 전달하면 로또가 몇개 구매했고, 어떤 로또들이 뽑혔는지
한번에 출력할 수 있을 것 같아요

Comment on lines +19 to +21



Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

의미 없는 개행은 제거하는 것이 좋아요

Comment on lines +10 to +12
public static List<Integer> execute() {
return generateLottoNumber();
}
Copy link
Member

Choose a reason for hiding this comment

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

LottoNumber는 1~45 숫자 중 하나를 의미하도록 만들고
Lotto는 LottoNumber를 리스트로 가져가도록 만들 수 있을 것 같아요

public class Lotto {
    private List<LottoNumber> numbers = new ArrayList<>();
    ...
    
    public Lotto(List<LottoNumber> numbers) {
         this.numbers.addAll(numbers);
    }

Copy link
Member

Choose a reason for hiding this comment

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

알고리즘을 풀듯이 한번에 처리하려고 하지말고
클래스별로 책임을 잘 분배하다보면
모든 기능을 테스트 케이스와 함께 작성할 수 있을 것 같아요

Comment on lines +17 to +18
List<Integer> value = execute(buyCount);
printResultLottoTicketValue(value);
Copy link
Member

Choose a reason for hiding this comment

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

로또 3개를 구매했다면 18개의 숫자가 value에 담겨있고
출력을 위해 6개씩 나눠 출력하는 것 같습니다.
만약 출력할 때 로또가 생성된 날짜같은 메타 데이터 정보를 함께 출력해야한다면 Integer만으로는 표현하기 힘들 것 같습니다.
Wrapper 클래스나 primitive 타입의 값들을 직접 사용하지 않고
객체 자체를 전달하면 좋을 것 같습니다.

Lotto는 로또 번호를 1~45 숫자 중 6개를 가져가고
List를 view에게 전달하면 되지 않을까요?


private static final int START_LOTTO_NUMBER = 1;
private static final int END_LOTTO_NUMBER = 45;
private static final List<Integer> lottoNumber = new ArrayList<>();
Copy link
Member

@nooose nooose Nov 19, 2023

Choose a reason for hiding this comment

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

List<Integer> lottoNumber 컬렉션을 멤버변수로 가지고 있다는 의미는
어떻게 보면 로또 번호가 아닌 1~45 숫자 리스트를 가진 객체로 해석될 수 있습니다.

로또 번호는 1~45 숫자 중 하나를 의미하는게 좀 더 자연스러울 것 같아요

class LottoNumber {
    private final int number;
    // 생성자
}

Comment on lines +19 to +29
lottoNumberReset();

List<Integer> values = new ArrayList<>();
makeStartToEndLottoNumber(values);
shuffle(values);

lottoNumber.addAll(values.subList(0, 6));

sort(lottoNumber);

return lottoNumber;
Copy link
Member

Choose a reason for hiding this comment

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

로또 번호들을 랜덤으로 생성하는 로직을 LottoNumber가 가져가는 것이 맞을까요?

Lotto lotto = Lotto.auto();

로또 생성을 로또가 직접 책임지도록 할 수 있을 것 같아요

Comment on lines +5 to +8
public class WinnerNumber {
public boolean compare(String prevWinnerNumber, String userLottoNumber) {
return Objects.equals(prevWinnerNumber, userLottoNumber);
}
Copy link
Member

@nooose nooose Nov 19, 2023

Choose a reason for hiding this comment

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

입력한 로또도 Lotto이고 구매한 로또도 Lotto로 볼 수 있겠네요

몇 개 일치하는 지 반환 = lotto.matches(우승로또);

이렇게 객체간 메시지를 전달하면 몇개가 일치하는지 쉽게 구성할 수 있을 것 같아요

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.

2 participants