[자판기] 박진훈 미션 제출합니다 (연습) by jinhoon227 · Pull Request #169 · woowacourse/java-vendingmachine-precourse

jinhoon227 · 2022-12-09T19:09:13Z

No description provided.

8 -> 11

refactor: getter 함수 지양

khj1

안녕하세요 진훈님! 코드 잘 보고 갑니다!
저번에 리뷰 할 때보다 실력이 더 늘으신 것 같아요 👍
제 역량으론 슬슬 아쉬운 부분을 집어내기가 힘들어지는 것 같습니다 :)
항상 화이팅입니다!

khj1 · 2022-12-10T04:24:04Z

src/main/java/vendingmachine/domain/coins/Coin.java

+    }
+
+    public int exchangeCoin(int money) {
+        return (money / amount);


괄호로 묶어주지 않아도 될 것 같습니다!

khj1 · 2022-12-10T04:24:53Z

src/main/java/vendingmachine/domain/products/Price.java

+    }
+
+    private static void validateRange(int price) {
+        if (price < MIN_PRICE || price > MAX_PRICE) {


조건문을 메서드로 분리해주신다면 가독성이 더 좋아질 것 같아요!

price < MIN_PRICE 와 price > MAX_PRICE 각각 메소드로 분리한다는 뜻인가요??

private static boolean hasInvalidPriceRange(price) { return price < MIN_PRICE || price > MAX_PRICE; }

조금 억지스럽다고 느껴질 수 도 있지만, 가독성 측면에서 차이가 꽤 있다고 생각합니다!

khj1 · 2022-12-10T04:26:27Z

src/main/java/vendingmachine/domain/products/Price.java

+    }
+
+    public boolean isCheaperPrice(Price price) {
+        return this.price < price.price;


price.price 로 호출하는 게 어색하게 느껴집니다!
매개변수의 네이밍을 변경하거나 인스턴스 변수의 이름을 바꿔보셔도 괜찮을 것 같습니다!

khj1 · 2022-12-10T04:27:52Z

src/main/java/vendingmachine/domain/Money.java

+        return new Money(money);
+    }
+
+    private static void validate(int money) {


static 으로 선언해주신 이유가 있나요?

처음에는 정적 팩토리 메소드 from 함수에서 확인을해서 확인함수에서 static 을 붙였는데 생성자로 옮기면서 필요가 없어졌네요,,, 감사합니다

khj1 · 2022-12-10T04:41:10Z

src/main/java/vendingmachine/domain/coins/Coins.java

+        EnumMap<Coin, Integer> changes = new EnumMap<>(Coin.class);
+        coins.entrySet().stream()
+                .sorted(Comparator.comparingInt(o -> o.getKey().getAmount()))
+                .forEach(coin -> changes.put(coin.getKey(), calculateCoin(coin.getKey(), money)));


coin.getKey() 가 아니라 그냥 coin 을 넘겨줘도 되지 않나요?

추가적으로 빈 맵을 선언하시는 게 거슬리시다면
Collectors 의 toMap 을 활용해보시는 것도 추천드려요!

변수명 때문에 착각하신거 같아요 ㅠ coin 의 자료형은 coin 이 아닌 Entry<Coin, Integer> 여서 getKey() 로 Coin 을 넘겨주었습니다.. 네이밍 너무 어렵습니다..

toMap 에 대해 알려주셔서 감사합니다. 다음에 활용해보겠습니다!

khj1 · 2022-12-10T04:46:49Z

src/main/java/vendingmachine/util/ProductsConvert.java

+        String[] contents = product.replaceAll(PRODUCT_START_END_REGEX, BLANK)
+                .replaceAll(PRODUCT_DUPLICATED_DELIMITER_REGEX, PRODUCT_DELIMITER)
+                .split(PRODUCT_DELIMITER);
+
+        if (contents.length != 3) {
+            throw new ProductsConvertException();
+        }
+
+        return new Product(contents[NAME],
+                Price.from(IntConvert.convert(contents[PRICE])),
+                Quantity.from(IntConvert.convert(contents[QUANTITY])));


기능을 더 분리해볼 수 있지 않을까요?

예외처리 던지는 부분은 분리가 가능했네요..!

khj1 · 2022-12-10T04:50:57Z

src/main/java/vendingmachine/view/OutputView.java

+        for (Map.Entry<Coin, Integer> coinEntry : coins.getCoins().entrySet()) {
+            System.out.printf(MESSAGE_COINS, coinEntry.getKey().getAmount(), coinEntry.getValue());
+            System.out.println();
+        }


Map 인터페이스의 forEach 메서드를 활용해보시는 것도 좋을 것 같습니다!

마침 제가 어제 자바 컬렉션 API 에 대해 학습했는데요! 참고해보시면 도움이 많이 될 것 같습니다 :)

컬렉션 API 개선 학습 내용 정리

유용한글 감사합니다! 정리를 잘하셨네요!

khj1 · 2022-12-10T04:53:38Z

src/main/java/vendingmachine/domain/coins/Coins.java

+        int minAmount = Coin.findMinAmount();
+
+        while (money.isUseMoney(minAmount)) {
+            int number = numberGenerator.generate(coinAmounts);


number 라는 변수명은 너무 포괄적인 것 같습니다!
coinAmounts 를 기반으로 값이 생성되고 있으니 해당 맥락에 맞게 네이밍을 해주는 것도 좋을 것 같아요!

khj1 · 2022-12-10T05:16:27Z

src/main/java/vendingmachine/domain/coins/Coins.java

+        this.coins = coins;
+    }
+
+    public void makeRandomCoins(NumberGenerator numberGenerator, Money money) {


기능을 더 분리해볼 수 있을 것 같습니다!

어떤식으로 분리하실지 여쭤봐도될까요?!

사실 메서드 라인이 10 라인을 넘어가서 기능 분리를 해보면 어떨까 싶어 제안 드린건데

생각보다 기능 분리가 쉽지 않네요... :(

public void makeRandomCoins(NumberGenerator numberGenerator, Money money) { List<Integer> coinAmounts = Coin.makeAmountList(); while (money.isConvertable()) { int amount = numberGenerator.generate(coinAmounts); if (money.isUseMoney(amount)) { convertToCoin(money, amount); } if (!money.isUseMoney(amount)) { coinAmounts.remove((Integer) amount); } } } private void convertToCoin(Money money, int amount) { money.useMoney(amount); Coin coin = Coin.of(amount); coins.put(coin, coins.get(coin) + 1); } --- public class Money { public boolean isConvertable() { return money >= Coin.findMinAmount(); } }

너무어렵네요.. 좋은 코드 감사합니다!

khj1 · 2022-12-10T05:21:53Z

src/test/java/vendingmachine/MoneyTest.java

+        try {
+            Money.from(input);
+        } catch (IllegalArgumentException exception) {
+            result = false;
+        }


예외가 발생하지 않는 정상적인 경우를 테스트하고 싶으신 것 같습니다!
assertThatNoException() 을 활용하시면 될 것 같아요!

맞습니다.. 좋은걸 알려주셔서 감사합니다!!

Choi-JJunho

배우기만 하고가서 도움이 되실지 모르겠네요ㅠㅠ
정말 많이 배워갑니다🙇‍♂️

Choi-JJunho · 2022-12-11T06:58:02Z

src/main/java/vendingmachine/controller/VendingMachineController.java

+    }
+
+    private void buyProduct() {
+        while (true) {


개인적으로 while(true)로 작성하면 읽는 도중에 종료조건을 한번 더 신경써야해서
저는 while의 조건구문에 항상 언제 종료된다는것을 표현하고는 합니다!

Choi-JJunho · 2022-12-11T07:09:17Z

src/main/java/vendingmachine/domain/VendingMachine.java

+        this.products = products;
+    }
+
+    public static VendingMachine of(Coins coins, Products products) {


정적 팩토리 메서드 처음봤는데 좋네요..!
배워갑니다!

Choi-JJunho · 2022-12-11T07:12:07Z

src/main/java/vendingmachine/domain/coins/Coin.java

+    public static Coin of(int amount) {
+        return Arrays.stream(values())
+                .filter(coin -> coin.amount == amount)
+                .findAny()
+                .orElseThrow(CoinEmptyException::new);
+    }


오..! 이 부분에서 stream을 이용할 생각을 못했었는데 이렇게 사용할수 있겠네요
배워갑니다!

Choi-JJunho · 2022-12-11T07:17:22Z

src/main/java/vendingmachine/domain/coins/NumberGenerator.java

+
+import java.util.List;
+
+public interface NumberGenerator {


interface 상단에 @FunctionalInterface 어노테이션을 선언하면 해당 인터페이스가 함수형 인터페이스임을 강조할 수 있습니다!

4주차 미션에서 참고했는데 이런 역할을 하더라구요!

Choi-JJunho · 2022-12-11T07:20:07Z

src/main/java/vendingmachine/domain/products/Price.java

+public class Price {
+
+    private static final int MIN_PRICE = 100;
+    private static final int MAX_PRICE = 1_000_000_000;


_로 구분해주니까 보기가 편하네요!
배워갑니다

Choi-JJunho · 2022-12-11T07:22:10Z

src/main/java/vendingmachine/domain/products/Product.java

+    }
+
+    @Override
+    public boolean equals(Object o) {


재정의하면서 기본적으로 o 라고 표현된거같은데
개인적으로 저는 equals를 재정의할때 파라미터 명을 other라고 네이밍을 지어줬습니다 ㅎㅎ

Choi-JJunho · 2022-12-11T07:25:28Z

src/main/java/vendingmachine/domain/products/Products.java

+        return products.stream()
+                .filter(product -> product.isSameName(name))
+                .filter(product -> product.purchasableProduct(money))
+                .findAny()
+                .orElseThrow(ProductsNotFind::new);


이런 방식으로도 구매가능한 Product를 구분할 수 있군요!!
배워갑니다

Choi-JJunho · 2022-12-11T07:28:32Z

src/main/java/vendingmachine/domain/products/Products.java

+
+    public boolean isAllQuantityZero() {
+        return products.stream()
+                .allMatch(Product::isSoldOut);


오 생각해보지 못한 방식이였는데 배워갑니다.

Choi-JJunho · 2022-12-11T07:30:08Z

src/main/java/vendingmachine/exception/CoinEmptyException.java

@@ -0,0 +1,10 @@
+package vendingmachine.exception;
+
+public class CoinEmptyException extends IllegalArgumentException {


우와.. IllegalArgumentException을 상속받아서 CustomException을 사용하는방식도 생각 못했던 부분인데
배워갑니다!

커스텀 예외

커스텀 예외가 항상 좋은것은 아닙니다! 커스텀예외에 좋은글이있어서 링크남깁니다!

Choi-JJunho · 2022-12-11T07:32:00Z

src/main/java/vendingmachine/util/ProductsConvert.java

+                .replaceAll(PRODUCT_DUPLICATED_DELIMITER_REGEX, PRODUCT_DELIMITER)
+                .split(PRODUCT_DELIMITER);
+
+        if (contents.length != 3) {


이 부분도 상수로 분리해주면 좋을것같아요!

이런 놓치고있었네요..ㅜ

BuildTools added 13 commits December 7, 2022 21:57

fix: 자바 버전설정

33bb09e

8 -> 11

docs: 추가 기능 목록

8a82769

feat: 추가 skeleton

14fcd46

feat: 추가 자판기가 보유하고 있는 금액으로 동전을 무작위로 생성하는 기능

c72a783

test: 추가 자판기가 보유하고 있는 금액으로 동전을 무작위로 생성하는 기능테스트

fe08b52

feat: 추가 상품명, 가격, 수량을 입력하여 상품을 추가하는 기능

a029d98

test: 추가 상품명, 가격, 수량을 입력하여 상품을 추가하는 기능테스트

02196fd

feat: 추가 사용자가 투입한 금액으로 상품을 구매하는 기능

c2f7005

test: 추가 사용자가 투입한 금액으로 상품을 구매하는 기능테스트

786b4ca

feat: 추가 남은 금액이 상품의 최저 가격보다 적거나, 모든 상품이 소진된 경우 잔돈 반환 기능

05ec395

test: 추가 남은 금액이 상품의 최저 가격보다 적거나, 모든 상품이 소진된 경우 잔돈 반환 기능테스트

aa7e517

fix: 추가 출력문추가

2059c15

refactor: 수정 클린코드, 필요없는 변수제거

9f81ddd

refactor: getter 함수 지양

khj1 reviewed Dec 10, 2022

View reviewed changes

Choi-JJunho reviewed Dec 11, 2022

View reviewed changes

		@@ -0,0 +1,10 @@
		package vendingmachine.exception;

		public class CoinEmptyException extends IllegalArgumentException {

Conversation

jinhoon227 commented Dec 9, 2022

Uh oh!

khj1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choi-JJunho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!