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

[로또 미션] 한상우 미션 제출합니다. #17

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

Conversation

sangu1026
Copy link

처음에는 Lotto안의 숫자들을 int값으로 설정하여 하였다가
Lotto안의 숫자들을 LottoNumber로 한번 더 감싸서 만들어보았습니다.
이 과정에서 기초적인 자바문법으로는 한계가 있다는 것을 느꼈고,
고급 자바문법(stream,forEach..)의 필요성을느꼈습니다.

예외처리까지는 했는데, 시간이 부족해 테스트 로직을 짜지 못했습니다.
부족한점이 많지만 리뷰 부탁드립니다!🙇‍♂️

같이 생각해보고 싶은점

  • 메인함수에서 컨트롤러가 뷰를 포함하여 뷰들을 호출하는 것이 맞을까, 아니면 컨트롤러와 뷰를 따로 호출 하는 것이 맞을까?(컨트롤러의 역할)

@sangu1026
Copy link
Author

간단한 테스트 로직을 추가했습니다!

Copy link

Choose a reason for hiding this comment

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

예쁘게 잘 짜신 것 같아요.

Set<Integer> nonDuplicateNumbers = new HashSet<>(numbersList);
return nonDuplicateNumbers.size();
}

Copy link

@GoToBILL GoToBILL May 7, 2024

Choose a reason for hiding this comment

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

    Set<Integer> seen = new HashSet<>();
    if (lottoNumbers.stream()
            .anyMatch(n -> !seen.add(n.getNumber()))) throw new IllegalArgumentException("로또는 중복돼서는 안됩니다.");

스트림 문법을 쓰시면 더 깔끔하게 하실 수 있어요!

Copy link
Author

Choose a reason for hiding this comment

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

private void validateDuplication(List<LottoNumber> numbers) {
        Set<Integer> seenNumbers = new HashSet<>();
        if (numbers.stream()
                .anyMatch(n -> !seenNumbers.add(n.getLottoNumber()))) throw new IllegalArgumentException("로또는 중복돼서는 안됩니다.");
    }

감사합니다 👍

}
return false;
}
}
Copy link

Choose a reason for hiding this comment

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

스트림 문법의 contains를 사용하면 더 깔끔해 질 것 같아용.

Copy link
Author

@sangu1026 sangu1026 May 21, 2024

Choose a reason for hiding this comment

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

public boolean iscontained(int number) {
        return lotto.stream()
                .map(LottoNumber::getLottoNumber)
                .collect(Collectors.toList())
                .contains(number);
    }
``` 훨씬 깔끔하네용👍 

throw new IllegalArgumentException("로또 숫자는 숫자형식이어야 합니다.");
}
}
}
Copy link

Choose a reason for hiding this comment

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

꼼꼼하게 예외처리를 하신 것 같아요

lottos.add(lotto);
}

}
Copy link

Choose a reason for hiding this comment

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

저랑 비슷한 구조로 하셔서 잘 읽히네요

public int ticket() {
return money / 1000;
}
}
Copy link

Choose a reason for hiding this comment

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

제가 놓친 부분을 예외 처리를 하셔서 잘 배우고 갑니다

public static void printPercent(double profitPercent) {
System.out.println("총 수익률은 " + profitPercent + "%입니다.");
}
}
Copy link

Choose a reason for hiding this comment

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

view 폴더는 너무 이쁘게 짜신 것 같아요.

}

Collections.shuffle(numbers);

Copy link

Choose a reason for hiding this comment

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

전 랜덤으로 6개 뽑았는데 suffle을 이용하는 방법도 있네요!

compareUserLotto(lastWeekLotto,lotto);
checkBonusNum(lastWeekLotto, bonusNum);

checkBingoList(correctNum, bonusFlag);
Copy link

Choose a reason for hiding this comment

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

메소드 순서대로 아래에 구현돼있어서 읽기 편했어요

public void findBingo6(int correctNum) {
if (correctNum == 6) Bingo.getBingo6().addCorrectLottoNum();
}
}
Copy link

Choose a reason for hiding this comment

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

따로 클래스를 파셔서 하셨는데 ENUM에서 내부 매소드를 이용해서 enum의 정보를 이용하는 방법도 있어요.
예시로 ordinal이라는 함수를 이용하면 몇번째로 구현됐는 지도 알려줘서 따로 리스트를 만들어서 index로 저장하는 방법도 있습니다.

return bingo6;
}

}
Copy link

Choose a reason for hiding this comment

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

enum 메소드를 사용하면 더 깔끔하게 할 수 있어요.

public static void showProfit(int money) {
OutputView.printPercent(calculate(money));
}
}
Copy link

Choose a reason for hiding this comment

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

다 좋은데 controller가 너무 비대한 것 같아요.
몇몇을 분리해 줄 필요가 있을 것 같아요

Copy link

@GoToBILL GoToBILL left a comment

Choose a reason for hiding this comment

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

잘 하셨어용!

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