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

[로또 미션] 이지은 미션 제출합니다. #13

Open
wants to merge 12 commits into
base: jelee2555
Choose a base branch
from

Conversation

jelee2555
Copy link

step1까지 진행하였고, step2 진행 중입니다.

아직 java가 익숙하지 않아 작은 기능을 구현하는 데에도 시간이 오래 걸려 많이 진행하지 못했습니다.

<신경 쓴 부분>

  • stream을 사용하여 코드 구현
  • 객체와 친해지기
  • test 코드 작성하기 (실패)

많이 부족한 코드지만 잘 부탁드립니다!

Copy link

@whxogus215 whxogus215 left a comment

Choose a reason for hiding this comment

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

안녕하세요 지은님! 리뷰어 조태현입니다. 한 주 동안 스터디와 함께 미션까지 진행하시느라 고생 많으셨습니다. 같이 배워나가는 과정이기에 저도 리뷰하면서 많이 배울 수 있을 것 같아요! 이번 미션이 다음 미션을 진행함에 있어서 많은 도움이 되길 바랍니다~!

- [x] 숫자들끼리 중복되지 않도록
- [x] 1 ~ 45 사이
- [x] 출력 시 숫자들이 오름차순으로 정렬

Choose a reason for hiding this comment

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

저도 리드미를 작성하면서 체크 리스트를 만드는데 지은님도 저와 같은 방식을 사용하시네요! 점점 미션에서 요구하는 사항이 많아지다 보니 하나하나 체크하는 습관을 들이는게 좋은 것 같습니다👍


public class InputView {

public static final Scanner SCANNER = new Scanner(System.in);

Choose a reason for hiding this comment

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

저는 View에서 구현한 모든 메서드마다 Scanner를 생성해서 사용하였는데요! 지은님처럼 static 객체를 생성하면 불필요한 객체 생성을 피할 수 있을 것 같네요! 좋은 내용 배우고 갑니다 👍

.map(LottoNumber::new)
.toList();
return new Lotto(lottoNumbers);
}

Choose a reason for hiding this comment

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

현재 이 코드는 입력받은 문자열(숫자와 콤마로 이루어진 입력 값)을 Lotto라는 객체를 생성하여 반환하는 코드네요.
스트림을 사용하여 코드를 간소화하고, 가독성이 좋아진 것 같습니다! List<Integer>를 생성하는 코드와 List<LottoNumber>를 생성하는 코드를 각각 메서드로 분리시키면 메서드의 역할을 좀 더 구분하기에 좋을 것 같아요!


public class Lotto {

public static final int LOTTO_SIZE = 6;

Choose a reason for hiding this comment

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

LOTTO_SIZE는 List<LottoNumber>를 생성하는 과정에서 검증할 때, 사용이 되는 것 같아요.
이 값이 외부에서 활용이 되지 않는거라면 private으로 선언하는건 어떨까요? 혹시 이 값이 외부에서도 쓰인다면 아예 별도의 클래스로 상수 값을 분리시키는 것도 생각해볼 수 있을 것 같습니다.


RandomLottoGenerator randomLottoGenerator = new RandomLottoGenerator();

for (int i = 0; i < price.getPrice() / 1000; i++) {

Choose a reason for hiding this comment

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

for문에 사용된 price.getPriceE() / 1000은 사용자가 입력한 금액에 따라 발행해야 할 로또 개수를 의미하는 것 같아요.
이 값을 별도의 변수로 빼내어 이름을 붙여준다면 이 코드를 처음 보는 사람도 쉽게 이해가 갈 것 같아요!


private void validateRange(int number) {
if (number < MIN_VALUE || number > MAX_VALUE) {
throw new IllegalArgumentException();

Choose a reason for hiding this comment

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

IllegalArgumentException을 던질 때, 해당 예외와 관련된 메시지를 같이 전달해보는건 어떨까요? 추후 LottoNumber라는 객체를 생성하는 과정에서 해당 메서드에서 예외를 발생했을 때, 예외 메시지가 출력된다면 코드를 수정하기 더 수월할 것 같아요!

@@ -0,0 +1,34 @@
public class LottoPrice {

public static final int MIN_PRICE = 0;

Choose a reason for hiding this comment

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

LottoPrice 내부에서 쓰이는 상수라면 private으로 선언해보는건 어떨까요?

public void printLotto(List<Lotto> lottos) {
System.out.println();
System.out.println(lottos.size() + "개를 구매했습니다.");
lottos.stream().map(Lotto::numbers).forEach(System.out::println);

Choose a reason for hiding this comment

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

스트림 문법을 적극적으로 활용하고 계시네요 👍

void 로또는_6개의_수로_이루어져야_한다() {
var numbers = List.of(new LottoNumber(1), new LottoNumber(2), new LottoNumber(3));

assertThatThrownBy(() -> new Lotto(numbers)).isInstanceOf(IllegalArgumentException.class).hasMessage("로또는 6개의 수로 이루어져야 한다. ");

Choose a reason for hiding this comment

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

메서드 체이닝으로 인해 .의 사용이 많아질 경우, 줄바꿈을 통해 코드의 가독성을 높일 수 있을 것 같아요!

assertThatThrownBy(() -> new Lotto(numbers))
                       .isInstanceOf(IllegalArgumentException.class)
                       .hasMessage("로또는 6개의 수로 이루어져야 한다. ");

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