Skip to content

Conversation

@chae-heechan
Copy link
Member

No description provided.

@chae-heechan chae-heechan changed the base branch from main to chae-heechan April 2, 2021 15:50
Copy link
Member

@oereo oereo left a comment

Choose a reason for hiding this comment

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

정말 고생많으셨습니다!!!!! 코드를 보면서 제 코드가 고칠 부분이 많이 보이는 것 같아요!! 정말 고생하셨습니다!!

this.isBonusMatch = isBonusMatch;
}
public boolean isUnderThree() {
return matchCount < 3;
Copy link
Member

Choose a reason for hiding this comment

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

전역상수로 빼는 방향은 어떠신가요??? 예를들어 로또의 규칙이 바뀌거나 범위가 1~45에서 바뀔 경우 상수로 빼는것도 좋을 것 같아요!!!

public class WinningResult {
private final int numberOfWinners;
private final GameResult gameResult;
public WinningResult(int numberOfWinners, GameResult gameResult) {
Copy link
Member

Choose a reason for hiding this comment

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

numberOfWinners 라고 하니까 살짝 이해가 안될 수도 있을 것 같아요!!! 읽었을 때 우승자의 수?? 이렇게 해석이 될 수도 있지 않을까 하는 조심스러운 리뷰 달겠습니다!!! 저의 지극히 개인적인 생각이옵니다!

import java.util.Comparator;
import java.util.List;
import java.util.stream.Collectors;
public class LottoTicket {
Copy link
Member

Choose a reason for hiding this comment

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

살포시 띄어주세요!!!

.collect(Collectors.toList());
return lottoNumbers;
}
public boolean contains(LottoNumber lottoNumber) {
Copy link
Member

Choose a reason for hiding this comment

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

이건 제가 궁금해서 그런 부분인데 contains 라는 method가 기존에 있는데 같은 이름의 method 를 구현하는게 괜찮은 방향인지 모르겠어요!!!

import java.util.List;
import java.util.function.Predicate;
import java.util.stream.Collectors;
public enum GameResult {
Copy link
Member

Choose a reason for hiding this comment

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

오오오오 enum 활용 좋습니다!!

public GameResult getGameResult() {
return gameResult;
}
} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

개행을 추가하면 될 거에요!!

return matchCount == 5 && isBonusMatch;
}
public boolean isAllMatch() {
return matchCount == 6;
Copy link
Member

Choose a reason for hiding this comment

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

isAllMatch 와 matchCount == 6 으로 했을 때 나중에 수정을 할 상황이 올 것 같아요!!!

Copy link

@begaonnuri begaonnuri left a comment

Choose a reason for hiding this comment

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

구현 잘해주셨네요 💯
몇가지 변경사항 남겼으니 반영해주시고, 궁금한점 DM주세요 :)

@@ -1,5 +1,47 @@
# 🚀 로또 1단계 - 자동

## 📋기능 구현 목록

Choose a reason for hiding this comment

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

기능 목록 👍🏻


version = '1.0.0'
sourceCompatibility = 1.8
sourceCompatibility = 14

Choose a reason for hiding this comment

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

프로젝트의 자바 버전을 올린 이유가 있을까요?

Comment on lines +31 to +32
LottoApplication app = new LottoApplication(new Printer(), new Receiver(System.in));
app.run();

Choose a reason for hiding this comment

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

Application이 Application을 생성해서 실행시키는 것이 어색하게 느껴지네요.
run()은 애플리케이션의 흐름을 제어하면서 로직을 진행시키는 것 같은데 이것을 따로 객체로 분리해보면 어떨까요?
MVC 패턴과 Controller의 역할에 대해 학습해보면 좋을것같아요.

https://m.blog.naver.com/jhc9639/220967034588

Comment on lines +57 to +59
if(manualTicketCount > purchaseAmount/LOTTO_TICKET_PRICE){
throw new IllegalArgumentException(LOTTO_TICKET_PRICE_RANGE_EXCEPTION_MESSAGE);
}

Choose a reason for hiding this comment

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

PurchaseAmount란 객체를 만들고 이 역할을 하도록 할 수 있지 않을까요?
main의 역할이 너무 큰 것 같아요.

Comment on lines +11 to +19
public LottoTicket() {
List<LottoNumber> lottoNumbers = LottoNumber.range();
lottoNumbers = generateLottoNumbers(lottoNumbers);
this.lottoNumbers = new ArrayList<>(lottoNumbers);
}
public LottoTicket(List<Integer> manualLottoNumbers){
List<LottoNumber> lottoNumbers = LottoNumber.setManualNumber(manualLottoNumbers);
this.lottoNumbers = new ArrayList<>(lottoNumbers);
}

Choose a reason for hiding this comment

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

이런 경우 사용자는 어떤 생성자가 수동 로또 생성인지, 자동 로또 생성인지 모를 것 같아요.
정적 팩토리 메소드로 구분해보는건 어떨까요?

Comment on lines +7 to +32
class MatchCountTest {

@Test
void isUnderThree() {
}

@Test
void isMatchThree() {
}

@Test
void isMatchFour() {
}

@Test
void isMatchFiveWithoutBonus() {
}

@Test
void isMatchFiveWithBonus() {
}

@Test
void isAllMatch() {
}
} No newline at end of file

Choose a reason for hiding this comment

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

내부를 구현해주세요!

Comment on lines +35 to +48
assertThat(winningResults.get(0).getGameResult()).isEqualTo(GameResult.THREE_MATCHED);
assertThat(winningResults.get(0).getNumberOfWinners()).isEqualTo(3);

assertThat(winningResults.get(1).getGameResult()).isEqualTo(GameResult.FOUR_MATCHED);
assertThat(winningResults.get(1).getNumberOfWinners()).isEqualTo(1);

assertThat(winningResults.get(2).getGameResult()).isEqualTo(GameResult.FIVE_MATCHED_WITHOUT_BONUS);
assertThat(winningResults.get(2).getNumberOfWinners()).isEqualTo(0);

assertThat(winningResults.get(3).getGameResult()).isEqualTo(GameResult.FIVE_MATCHED_WITH_BONUS);
assertThat(winningResults.get(3).getNumberOfWinners()).isEqualTo(0);

assertThat(winningResults.get(4).getGameResult()).isEqualTo(GameResult.ALL_MATCHED);
assertThat(winningResults.get(4).getNumberOfWinners()).isEqualTo(1);

Choose a reason for hiding this comment

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

이렇게 테스트가 많을 경우 assertAll을 활용할 수 있어요.
assertThat이 1부터 10까지 있을때 assertAll을 사용하지 않은 경우 4에서 실패하면 그대로 테스트가 끝나지만,
assertAll을 사용하면 4에서 실패해도 5부터 10까지 계속 진행을 한답니다 :)

https://javacan.tistory.com/entry/JUnit-5-Intro

FOUR_MATCHED(MatchCount::isMatchFour, "4개 일치", 50_000),
FIVE_MATCHED_WITHOUT_BONUS(MatchCount::isMatchFiveWithoutBonus, "5개 일치", 1_500_000),
FIVE_MATCHED_WITH_BONUS(MatchCount::isMatchFiveWithBonus, "5개 일치, 보너스 볼 일치", 30_000_000),
ALL_MATCHED(MatchCount::isAllMatch, "6개 일치", 2_000_000_000);

Choose a reason for hiding this comment

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

_ 구분 👍🏻

.isThrownBy(() -> new LottoNumber(lottoNumber))
.withMessage("로또 번호는 1 이상, 45 이하여야 합니다.");
}
@DisplayName("해시코드 테스트 커버리지 커")

Choose a reason for hiding this comment

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

테스트는 어떤 목적일까요?
그리고 테스트 커버리지는 무슨 의미를 가질까요?
equals()처럼 누구나 아는 api를 테스트할 필요가 있을까요?

Comment on lines +13 to +14
@MethodSource(value = "provideEvaluate")
@ParameterizedTest(name = "matchCount = {0}, isBonusMatch = {1}, expected = {2}")

Choose a reason for hiding this comment

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

MethodSource 👍🏻

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.

3 participants