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

[로또 미션] 김진주 미션 제출합니다. #12

Open
wants to merge 1 commit into
base: nova0128
Choose a base branch
from

Conversation

nova0128
Copy link

@nova0128 nova0128 commented May 6, 2024

안녕하세요 리뷰어님!
저번 과제보다 mvc패턴에 더 신경써서 구현해보았습니다.
다만 public/private, 난잡한 메소드들을 다듬지 못한 채로 제출하여 고칠 부분이 아직 많은 것 같습니다.
부족한 부분은 많이 지적해주세요!

(중간에 메인브랜치로 잘못 커밋해 포크를 방금 새로 하여 커밋이 하나밖에 없습니다..)

@nova0128 nova0128 changed the title feat: 기능 구현 [로또 미션] 김진주 미션 제출합니다. May 7, 2024
Copy link

@zhy2on zhy2on left a comment

Choose a reason for hiding this comment

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

안녕하세요 진주님!! 제가 풀리퀘에서 진주님 이름을 찾지 못 해서 리뷰를 안 했다가 이름 바꾸신 걸 지금 발견해서 이제 리뷰 드려요ㅠㅠ 죄송합니다
우선 MVC 패턴에 맞게 패키지를 나누고 구현해주신 점 좋았습니다! 신경 쓰신 게 보였어요!

하나 최종적으로 코멘트 드릴 건 이번 과제에 원시값 포장에 대한 부분이 추가 됐는데 로또 번호, 로또 티켓같은 값들을 클래스로 만들어서 int 같은 원시값으로 바로 사용하는 게 아니라, LottoNumber, Lotto 객체로 감싸서 사용하는 것도 고민해보시면 좋을 것 같아요!

과제 너무 수고하셨습니다!

public class Controller {
public static void main(String[] args) {
OutputView.inputMessage();
int inputMoney = Input.inputMoney();
Copy link

Choose a reason for hiding this comment

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

이 부분을 int intputMoney = InputView.inputMoney(); 이런식으로 사용하는 건 어떨까요?
input에 담당하는 메서드를 InputView 클래스에서 따로 구현하는 것도 좋을 것 같습니다!

import java.util.List;

public class ReturnAutoLotto { //랜덤로또리스트를 반환
public static List<Integer> randomLotto(){
Copy link

Choose a reason for hiding this comment

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

이 부분은 stream 사용법을 찾아보시고 이용하시면 가독성이 더 좋아질 것 같습니다!

List<Integer> list = new ArrayList<>();
Random random = new Random();

while (list.size() < 6) {
Copy link

Choose a reason for hiding this comment

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

로또 개수와 로또번호의 범위를 지정하는 값들은 상수로 대체될 수 있는 값들인 것 같습니다! 스터디에서 배운대로 매직넘버를 제거하는 것도 고려해주시면 좋을 것 같습니다



OutputView.resultsMessage();
List<Integer> winnerCount = List.of(0, 6, 5, 5, 4, 3);
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 List<Integer> resultCount(List<List<Integer>> resultArray, List<Integer> answerNumbers, int bonusNumber) {
List<Integer> resultWinningList = initResultList();
Copy link

Choose a reason for hiding this comment

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

answerCount에 따른 인덱스를 미리 정의하면
if문을 각각 적어주지 않고도 구현이 가능할 것 같습니다!

import java.util.List;

public class TotalLotto {
public static List<List<Integer>> createResultArray () {
Copy link

Choose a reason for hiding this comment

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

로또를 Integer, List 객체로 관리하셨는데, LottoNumber, Lotto 클래스를 구현하셔서 사용하셨어도 좋았을 것 같습니다!

}

public static List<List<Integer>> createAddLotto (List<Integer> singleLotto, List<List<Integer>> resultArray) {
resultArray.add(singleLotto); // ?
Copy link

Choose a reason for hiding this comment

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

로또에 대한 예외 처리를 해주면 더 좋을 것 같습니다!

OutputView.printLottoCountInfo(manualCount, Input.inputAutoCount(inputMoney / 1000, manualCount));


for (int i = 0; i < Input.inputAutoCount(inputMoney / 1000, manualCount); i++) {
Copy link

Choose a reason for hiding this comment

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

매직넘버를 제거하면 더 좋을 것 같습니다!

List<Integer> winnerCount = List.of(0, 6, 5, 5, 4, 3);
List<Integer> winnerPrice = List.of(0, 2000000000, 30000000, 1500000, 50000, 5000);
long sum = 0;
for (int i = 5; i >= 1; i--) {
Copy link

Choose a reason for hiding this comment

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

for문을 사용하여 한 번에 출력을 잘 해주신 것 같습니다!

@nova0128 nova0128 changed the base branch from main to nova0128 May 22, 2024 11:07
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