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

[로또 미션] 오지현 미션 제출합니다. #20

Open
wants to merge 7 commits into
base: zhy2on
Choose a base branch
from

Conversation

zhy2on
Copy link

@zhy2on zhy2on commented May 6, 2024

늦어서 죄송합니다.. 자바에 익숙하지 않아 구현이 힘들었습니다
예외 처리 부분이 부족한 것 같습니다ㅠㅠ 잘못된 점이나 개선할 사항이 있으면 알려주시면 감사하겠습니다!

@che7371
Copy link

che7371 commented May 7, 2024

안녕하세요 지현님! 지현님 코드 리뷰를 맡은 오채현입니다. 저도 자바에 대한 지식이 많은 것이 아니기 때문에 다른 분들처럼 수준 높은 리뷰에는 미치지 못할 수 있습니다. 감안하고 봐주시면 감사하겠습니다! :-)

Copy link

@che7371 che7371 left a comment

Choose a reason for hiding this comment

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

코드 구조를 매우 잘 짜신 것 같습니다. 안에도 관련 메소드들이 잘 들어가 있고 한 가지 일만 하도록 구현이 된 것 같아요. 테스트 코드,예외 처리도 잘 구현하신 것 같아요. 단계별로 점점 올라갈 때 앞에서 아쉬웠던 부분이 수정이 되어 좀 더 좋은 코드로 변한 부분이 많이 보였습니다. 코드 보면서 저도 많은 걸 배워갔습니다. 문자열만 포장하시면 완벽한 코드가 될 것 같아요. 정말 수고 많으셨습니다 :-)

Copy link

Choose a reason for hiding this comment

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

원시 값 포장을 잘 하신 것 같습니다.

OutputView.displayLottoTickets(tickets);
public static void main(String[] args) {
LottoController controller = new LottoController();
controller.run();
}
Copy link

Choose a reason for hiding this comment

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

1단계에서 구현을 하신 것처럼 Controller 안에서 말고 Application 안에서 컨트롤러 객체 생성 후 controller.run() 사용하시면 좋을 것 같습니다.

Copy link

Choose a reason for hiding this comment

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

원시 값 포장을 잘 사용하셨네요! 저는 그냥 1000으로 나눠버렸는데 하나 배워갑니다 :-)

Copy link
Author

Choose a reason for hiding this comment

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

아하 감사합니다! 그런데 저 부분은 매직넘버를 상수로 선언해서 제거한 부분이고 이번 과제에서 말한 원시값 포장이랑은 다른 개념인 것 같아요..!!

저도 잘은 모르지만
int money; 로 바로 사용하는 게 아니라
Money 클래스를 따로 구현하고 Money money = new Money(int amount);
이런식으로 원시 값을 객체로 포장해서 사용하는 게 원시값 포장인 것 같습니다..!!

돈에 대해선 제가 원시값 포장을 못 했어요🥲 이 부분은 추가로 더 고민해보고 수정하겠습니다!

Copy link

Choose a reason for hiding this comment

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

죄송해요 저도 어제 스터디에서 설명해주시는 거 듣고 제가 틀렸단 걸 깨달았습니다..전 단순히 포장(감싼다)인 줄 알았어요ㅠㅠ저도 아직 배워야할 게 많은 것 같습니다..ㅠㅠ

Copy link

Choose a reason for hiding this comment

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

잘 하신 것 같습니다. PRIZES를 배열로 선언하지 않고 따로 클래스를 생성해서 관리하고(Map을 사용한다던지) 로또 수익 계산을 해도 괜찮을 것 같습니다!

Copy link

Choose a reason for hiding this comment

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

문자열도 포장을 하시면 좋을 것 같아요. 위에서 원시 값을 포장하신 것처럼요!

Copy link

Choose a reason for hiding this comment

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

문자열을 포장하시고 반복문으로 출력하시면 코드가 훨씬 더 간결해질 것 같습니다.

throw new IllegalArgumentException("로또 숫자는 6개여야 합니다.");
}
if (numbers.stream().distinct().count() != LOTTO_NUMBER_COUNT) {
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.

예외 처리도 잘 하신 것 같아요.

Copy link

Choose a reason for hiding this comment

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

enum을 잘 활용하신 것 같습니다. 저는 enum을 활용할 때 많이 헤맸는데 또 하나 배워갑니다!

@zhy2on
Copy link
Author

zhy2on commented May 8, 2024

꼼꼼하게 리뷰해주셔서 감사합니다! 신경 쓴 부분도 알아봐 주셔서 감사했습니다🥲
말씀해주신 문자열을 상수로 관리하는 부분을 신경 써서 수정해보겠습니다!!

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