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

[로또 미션] 김수민 미션 제출합니다. #6

Open
wants to merge 2 commits into
base: boyekim
Choose a base branch
from

Conversation

boyekim
Copy link

@boyekim boyekim commented May 6, 2024

4단계까지 미션제출합니다.

@boyekim boyekim changed the title Boyekim [로또 미션] 김수민 미션 제출합니다. May 6, 2024
@boyekim
Copy link
Author

boyekim commented May 6, 2024

enum을 어떻게 효율적이게 활용할 수 있을지 고민하였습니다. -> 계산식을 enum에서 활용하고 싶었으나 활용방법이 잘 떠오르지 않았습니다. 추가적 학습이 필요하다고 생각됩니다. 우선은 같은 의미를 묶는 용도(3개일치와 5000원 등)로 사용하였습니다.
일급컬렉션의 조건이 필드값이 하나만 있어야 하는 것 같은데 이를 적용하면서 코드를 구성하는것이 힘들었어서 결국 다른 필드값을 추가하였습니다. 이를 중점으로 봐주시면 감사하겠습니다.

Copy link

@TaeyeonRoyce TaeyeonRoyce left a comment

Choose a reason for hiding this comment

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

안녕하세요, 수민님.
미션 진행하느라 고생하셨습니다!
꼼꼼하게 요구사항 반영하시고 계산 로직을 잘 처리하신 것 같네요.

몇몇 코드들에서 이해하는데 오래 걸리는 부분들이 있었어요.
다른분들 코드 많이 보시면서 적용하면 좋을 것 같습니다.

일급 컬렉션 관련 외에도 몇가지 코멘트 남겼으니, 확인해주세요.


public class LottoMain {

public static void main(String[] args) {

Choose a reason for hiding this comment

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

하나의 메서드에서 너무 많은 로직이 수행되고 있어요.
메서드의 역할이 가중되면, 해당 메서드의 의존도가 높고 다양한 요구사항 변경에 영향을 받을 수 있습니다. 또, 테스트 하기도 어렵고 무슨 역할을 하는지 파악하기 어려워요.

main에서 꼭 이루어져야 하는 로직은 무엇일까요?
입력을 받고, 로또를 생성하고, 계산이 이루어지는 구체적인 로직들은 해당 역할을 하는 객체에 위임 해도 좋을 것 같아요.

public class LottoMain {

public static void main(String[] args) {
int manual, automatic;

Choose a reason for hiding this comment

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

처음부터 선언하지 말고, 생성과 함께 사용하면 어떨까요?
manual이나 automatic 을 처음에 선언할 필요가 있나요?


import java.util.List;

public class Lottos {

Choose a reason for hiding this comment

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

일급컬렉션으로 사용하고자 했는데 필드가 늘어난 이유는 객체의 행동이 없어서 인 것 같아요.
로또의 집합의 개념을 넘어서, [예산], [일치 개수], [보너스 볼] 까지 의미를 포함하는 것 같네요.
로또의 집단이 어떤 의미를 담고 있는지 전혀 표현이 되지 않은 것 같습니다.

예를 들어 당첨 로또와 결과를 맞추어본다고 했을 때,

public class Lottos {
    private final List<OneLotto> myLottos;

    // ....

    public int calculateMatchNumberCount(Lottos other) {
        // ....
        return matchNumberCounts;
    }
}

이렇게 객체가 로직을 포함한다면 필드에 count는 불필요하게 될 것 같아요.
객체에 필드, 값이 아닌 행동을 통해 의미를 표현해보세요


import java.util.List;

public class OneLotto {

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 +9
private boolean bonusCheck;
private CorrectNum correctCnt;
private int correctNum;

Choose a reason for hiding this comment

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

[보너스 여부]도 포함하여 하나로 관리하면 어떨까요.
CorrectNumbounsCheck를 통해 결정되는 [등수] 라는 객체로 분리해도 괜찮을 것 같네요.

ex)

public class OneLotto {
    private final List<Integer> lottoNumbers;
    private final Prize prize;
}

Comment on lines +27 to +37
public void setBonusCheck(Lottos lottos) {
if (correctNum == 4 && lottoNumbers.contains(lottos.getBonusBall()))
bonusCheck = true;
}

public void setCorrectCnt() {
if (bonusCheck && correctNum == 4) {
correctCnt = CorrectNum.FIVE_NOT;
return;
}
List<CorrectNum> nums = CorrectNum.getList();

Choose a reason for hiding this comment

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

setCorrectCnt가 정상적으로 동작하려면 bonusCheck가 이미 지정되어야 하네요. (setBonusCheck()를 통해)

이런 의도대로 항상 사용된다고 보장할 수 있을까요?
setCorrectCnt()가 단독적으로 실행 될 수 있는 점도 고려해보세요.

추가로 setXxx처럼 관례적으로 사용하는 네이밍과 역할이 다른데, 이 부분도 고민해보면 좋겠습니다.
값을 지정(set)하는 것 이외에도 행동이 포함 되는데, 이를 표현해보세요.

Comment on lines +15 to +18

Collections.shuffle(numList);

return numList.subList(0, 6);

Choose a reason for hiding this comment

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

👍

public class LottoInput {

public static int inputBalance() {
Scanner sc = new Scanner(System.in);

Choose a reason for hiding this comment

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

상수로 선언해서 한번만 초기화 할 수 있습니다.

Comment on lines +21 to +25
String[] ans = str.split(", ");
List<Integer> result = new ArrayList<>();
for (int i = 0; i < ans.length; i++) {
result.add(Integer.parseInt(ans[i]));
}

Choose a reason for hiding this comment

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

"1,2,3,4,5,6"
"일, 이, 삼, 사, 오, 육"
과 같은 입력은 오류가 나겠네요.

}

private static String makeLottoStr(OneLotto oneLotto) {
StringBuffer sb = new StringBuffer();

Choose a reason for hiding this comment

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

StringBuffer를 사용한 이유가 있나요?

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