Conversation
whxogus215
left a comment
whxogus215
left a comment
There was a problem hiding this comment.
안녕하세요 지은님! 리뷰어 조태현입니다. 한 주 동안 스터디와 함께 미션까지 진행하시느라 고생 많으셨습니다. 같이 배워나가는 과정이기에 저도 리뷰하면서 많이 배울 수 있을 것 같아요! 이번 미션이 다음 미션을 진행함에 있어서 많은 도움이 되길 바랍니다~!
| - [x] 숫자들끼리 중복되지 않도록 | ||
| - [x] 1 ~ 45 사이 | ||
| - [x] 출력 시 숫자들이 오름차순으로 정렬 | ||
|
|
There was a problem hiding this comment.
저도 리드미를 작성하면서 체크 리스트를 만드는데 지은님도 저와 같은 방식을 사용하시네요! 점점 미션에서 요구하는 사항이 많아지다 보니 하나하나 체크하는 습관을 들이는게 좋은 것 같습니다👍
|
|
||
| public class InputView { | ||
|
|
||
| public static final Scanner SCANNER = new Scanner(System.in); |
There was a problem hiding this comment.
저는 View에서 구현한 모든 메서드마다 Scanner를 생성해서 사용하였는데요! 지은님처럼 static 객체를 생성하면 불필요한 객체 생성을 피할 수 있을 것 같네요! 좋은 내용 배우고 갑니다 👍
| .map(LottoNumber::new) | ||
| .toList(); | ||
| return new Lotto(lottoNumbers); | ||
| } |
There was a problem hiding this comment.
현재 이 코드는 입력받은 문자열(숫자와 콤마로 이루어진 입력 값)을 Lotto라는 객체를 생성하여 반환하는 코드네요.
스트림을 사용하여 코드를 간소화하고, 가독성이 좋아진 것 같습니다! List<Integer>를 생성하는 코드와 List<LottoNumber>를 생성하는 코드를 각각 메서드로 분리시키면 메서드의 역할을 좀 더 구분하기에 좋을 것 같아요!
|
|
||
| public class Lotto { | ||
|
|
||
| public static final int LOTTO_SIZE = 6; |
There was a problem hiding this comment.
LOTTO_SIZE는 List<LottoNumber>를 생성하는 과정에서 검증할 때, 사용이 되는 것 같아요.
이 값이 외부에서 활용이 되지 않는거라면 private으로 선언하는건 어떨까요? 혹시 이 값이 외부에서도 쓰인다면 아예 별도의 클래스로 상수 값을 분리시키는 것도 생각해볼 수 있을 것 같습니다.
|
|
||
| RandomLottoGenerator randomLottoGenerator = new RandomLottoGenerator(); | ||
|
|
||
| for (int i = 0; i < price.getPrice() / 1000; i++) { |
There was a problem hiding this comment.
for문에 사용된 price.getPriceE() / 1000은 사용자가 입력한 금액에 따라 발행해야 할 로또 개수를 의미하는 것 같아요.
이 값을 별도의 변수로 빼내어 이름을 붙여준다면 이 코드를 처음 보는 사람도 쉽게 이해가 갈 것 같아요!
|
|
||
| private void validateRange(int number) { | ||
| if (number < MIN_VALUE || number > MAX_VALUE) { | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
IllegalArgumentException을 던질 때, 해당 예외와 관련된 메시지를 같이 전달해보는건 어떨까요? 추후 LottoNumber라는 객체를 생성하는 과정에서 해당 메서드에서 예외를 발생했을 때, 예외 메시지가 출력된다면 코드를 수정하기 더 수월할 것 같아요!
| @@ -0,0 +1,34 @@ | |||
| public class LottoPrice { | |||
|
|
|||
| public static final int MIN_PRICE = 0; | |||
There was a problem hiding this comment.
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); |
| void 로또는_6개의_수로_이루어져야_한다() { | ||
| var numbers = List.of(new LottoNumber(1), new LottoNumber(2), new LottoNumber(3)); | ||
|
|
||
| assertThatThrownBy(() -> new Lotto(numbers)).isInstanceOf(IllegalArgumentException.class).hasMessage("로또는 6개의 수로 이루어져야 한다. "); |
There was a problem hiding this comment.
메서드 체이닝으로 인해 .의 사용이 많아질 경우, 줄바꿈을 통해 코드의 가독성을 높일 수 있을 것 같아요!
assertThatThrownBy(() -> new Lotto(numbers))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("로또는 6개의 수로 이루어져야 한다. ");
2 participants
step1까지 진행하였고, step2 진행 중입니다.
아직 java가 익숙하지 않아 작은 기능을 구현하는 데에도 시간이 오래 걸려 많이 진행하지 못했습니다.
<신경 쓴 부분>
많이 부족한 코드지만 잘 부탁드립니다!