Skip to content

Conversation

@username0w
Copy link

안녕하세요.
3단계 - 로또(2등) PR 입니다.

Copy link
Contributor

@javajigi javajigi left a comment

Choose a reason for hiding this comment

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

2단계까지 객체 설계를 잘해놓다보니 3단계에 큰 변화 없이 기능 추가가 가능했네요. 👍
그럼에도 불구하고 약간은 아쉬움이 남는 부분이 있어 피드백 남겼어요.

특히 생성자에 부 생성자 추가, 메서드를 오버로딩을 고려하면서 구현해 보면 어떨가요?
이 두 가지를 고려하면서 구현할 경우 객체를 생성할 때와 메서드를 사용하는 개발자 입장에서 편리성을 얻을 수 있지 않을까 고민해 보는 시간을 가지기를 바랍니다.

Comment on lines +33 to +36
List<LottoNumber> lottoNumbers = numbers.stream()
.map(LottoNumber::new)
.toList();
return new Lotto(lottoNumbers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
List<LottoNumber> lottoNumbers = numbers.stream()
.map(LottoNumber::new)
.toList();
return new Lotto(lottoNumbers);
return new Lotto(toLottoNumbers(numbers));

Comment on lines +40 to +44
List<LottoNumber> lottoNumbers = new ArrayList<>();
for (Integer number : numbers) {
lottoNumbers.add(new LottoNumber(number));
}
return lottoNumbers;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
List<LottoNumber> lottoNumbers = new ArrayList<>();
for (Integer number : numbers) {
lottoNumbers.add(new LottoNumber(number));
}
return lottoNumbers;
return numbers.stream()
.map(LottoNumber::new)
.toList();

}

public LottoRank matchingRank(Lotto winningNumbers) {
public LottoRank matchingRank(WinningNumbers winningNumbers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

matchingRank를 Lotto와 WinningNumbers 중 어느 곳의 책임을 가지도록 구현하는 것이 좋을까?

Comment on lines +19 to +21
String bonusNumberString = InputView.readBonusNumber();
int bonus = Integer.parseInt(bonusNumberString);
LottoNumber bonusNumber = new LottoNumber(bonus);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
String bonusNumberString = InputView.readBonusNumber();
int bonus = Integer.parseInt(bonusNumberString);
LottoNumber bonusNumber = new LottoNumber(bonus);
LottoNumber bonusNumber = new LottoNumber(InputView.readBonusNumber());

LottoNumber에 String을 인자로 받는 부 생성자를 추가하면 어떨까?


import java.util.Objects;

public class LottoNumber implements Comparable<LottoNumber> {
Copy link
Contributor

Choose a reason for hiding this comment

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

LottoNumber로 원시값 포장 👍
단, 다음과 같은 상황을 위해 성능 개선을 하려면 어떻게 하면 좋을까?

상황

  • 1만명의 사용자가 동시에 당첨 여부를 확인할 수 있어야 한다.
  • 1명의 사용자는 평균 5장의 로또를 구매한 상태이다.

위 요구사항을 서버에서 생성되는 LottoNumber의 인스턴스의 갯수는?
1 * 5장 * 6개 숫자/1장 * 1만명 = 30만개이다.

동시에 생성되는 인스턴스 갯수가 너무 많다.
인스턴스 갯수를 줄일 수 있는 방법은?
로또 숫자 값을 LottoNumber 객체로 래핑하는 경우 매번 인스턴스가 생성되기 때문에 인스턴스의 갯수가 너무 많아져 성능이 떨어질 수 있다.
LottoNumber 인스턴스를 생성한 후 재사용할 수 있도록 구현한다.

힌트 : Map과 같은 곳에 인스턴스를 생성한 후 재사용하는 방법을 찾아본다.

return (long) prize * count;
}

public static LottoRank of(int matchCount, boolean hasBonus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

enum의 values() 메소드를 활용하면 모든 enum 값 목록을 가져올 수 있다.
for문을 이용해 구현해 본다.

private final Lotto winningNumbers;
private final LottoNumber bonusNumber;

public WinningNumbers(Lotto winningNumber, LottoNumber bonusNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

주 생성자 외에 여러 타입을 지원하는 부 생성자도 추가하면 얻을 수 있는 잇점이 있을까?

Comment on lines 24 to +32
Lotto lotto = new Lotto("1,2,3,4,5,6");
assertThat(lotto.numbers()).containsExactly(1, 2, 3, 4, 5, 6);
assertThat(lotto.numbers()).containsExactly(
new LottoNumber(1),
new LottoNumber(2),
new LottoNumber(3),
new LottoNumber(4),
new LottoNumber(5),
new LottoNumber(6)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Lotto lotto = new Lotto("1,2,3,4,5,6");
assertThat(lotto.numbers()).containsExactly(1, 2, 3, 4, 5, 6);
assertThat(lotto.numbers()).containsExactly(
new LottoNumber(1),
new LottoNumber(2),
new LottoNumber(3),
new LottoNumber(4),
new LottoNumber(5),
new LottoNumber(6)
);
Lotto lotto = new Lotto("1,2,3,4,5,6");
assertThat(new Lotto("1,2,3,4,5,6")).isEqualTo(new Lotto(1, 2, 3, 4, 5, 6));

위와 같이 테스트하는 건 어떨까?

Comment on lines +22 to +24
WinningNumbers wn = new WinningNumbers(new Lotto("1,2,3,4,5,6"), new LottoNumber(7));
assertThat(wn.contains(new LottoNumber(3))).isTrue();
assertThat(wn.contains(new LottoNumber(7))).isFalse();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WinningNumbers wn = new WinningNumbers(new Lotto("1,2,3,4,5,6"), new LottoNumber(7));
assertThat(wn.contains(new LottoNumber(3))).isTrue();
assertThat(wn.contains(new LottoNumber(7))).isFalse();
WinningNumbers wn = new WinningNumbers(new Lotto("1,2,3,4,5,6"), 7);
assertThat(wn.contains(7)).isTrue();
assertThat(wn.contains(7)).isFalse();

WinningNumbers에 부 생성자 추가
contains()에 int도 인자를 받을 수 있도록 메서드를 오버로딩하면 어떨까?

이 두 가지를 고려하면서 구현할 경우 WinningNumbers와 메서드를 사용하는 개발자 입장에서 편리성을 얻을 수 있지 않을까?

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