[자판기] 박진훈 미션 제출합니다 (연습) #169
Conversation
8 -> 11
refactor: getter 함수 지양
| } | ||
|
|
||
| public int exchangeCoin(int money) { | ||
| return (money / amount); |
| } | ||
|
|
||
| private static void validateRange(int price) { | ||
| if (price < MIN_PRICE || price > MAX_PRICE) { |
There was a problem hiding this comment.
price < MIN_PRICE 와 price > MAX_PRICE 각각 메소드로 분리한다는 뜻인가요??
There was a problem hiding this comment.
private static boolean hasInvalidPriceRange(price) {
return price < MIN_PRICE || price > MAX_PRICE;
}조금 억지스럽다고 느껴질 수 도 있지만, 가독성 측면에서 차이가 꽤 있다고 생각합니다!
| } | ||
|
|
||
| public boolean isCheaperPrice(Price price) { | ||
| return this.price < price.price; |
There was a problem hiding this comment.
price.price 로 호출하는 게 어색하게 느껴집니다!
매개변수의 네이밍을 변경하거나 인스턴스 변수의 이름을 바꿔보셔도 괜찮을 것 같습니다!
| return new Money(money); | ||
| } | ||
|
|
||
| private static void validate(int money) { |
There was a problem hiding this comment.
처음에는 정적 팩토리 메소드 from 함수에서 확인을해서 확인함수에서 static 을 붙였는데 생성자로 옮기면서 필요가 없어졌네요,,, 감사합니다
| 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))); |
There was a problem hiding this comment.
coin.getKey() 가 아니라 그냥 coin 을 넘겨줘도 되지 않나요?
추가적으로 빈 맵을 선언하시는 게 거슬리시다면
Collectors 의 toMap 을 활용해보시는 것도 추천드려요!
There was a problem hiding this comment.
변수명 때문에 착각하신거 같아요 ㅠ coin 의 자료형은 coin 이 아닌 Entry<Coin, Integer> 여서 getKey() 로 Coin 을 넘겨주었습니다.. 네이밍 너무 어렵습니다..
toMap 에 대해 알려주셔서 감사합니다. 다음에 활용해보겠습니다!
| 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]))); |
| for (Map.Entry<Coin, Integer> coinEntry : coins.getCoins().entrySet()) { | ||
| System.out.printf(MESSAGE_COINS, coinEntry.getKey().getAmount(), coinEntry.getValue()); | ||
| System.out.println(); | ||
| } |
There was a problem hiding this comment.
Map 인터페이스의 forEach 메서드를 활용해보시는 것도 좋을 것 같습니다!
마침 제가 어제 자바 컬렉션 API 에 대해 학습했는데요! 참고해보시면 도움이 많이 될 것 같습니다 :)
| int minAmount = Coin.findMinAmount(); | ||
|
|
||
| while (money.isUseMoney(minAmount)) { | ||
| int number = numberGenerator.generate(coinAmounts); |
There was a problem hiding this comment.
number 라는 변수명은 너무 포괄적인 것 같습니다!
coinAmounts 를 기반으로 값이 생성되고 있으니 해당 맥락에 맞게 네이밍을 해주는 것도 좋을 것 같아요!
| this.coins = coins; | ||
| } | ||
|
|
||
| public void makeRandomCoins(NumberGenerator numberGenerator, Money money) { |
There was a problem hiding this comment.
사실 메서드 라인이 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();
}
}| try { | ||
| Money.from(input); | ||
| } catch (IllegalArgumentException exception) { | ||
| result = false; | ||
| } |
There was a problem hiding this comment.
예외가 발생하지 않는 정상적인 경우를 테스트하고 싶으신 것 같습니다!
assertThatNoException() 을 활용하시면 될 것 같아요!
| } | ||
|
|
||
| private void buyProduct() { | ||
| while (true) { |
There was a problem hiding this comment.
개인적으로 while(true)로 작성하면 읽는 도중에 종료조건을 한번 더 신경써야해서
저는 while의 조건구문에 항상 언제 종료된다는것을 표현하고는 합니다!
| this.products = products; | ||
| } | ||
|
|
||
| public static VendingMachine of(Coins coins, Products products) { |
There was a problem hiding this comment.
정적 팩토리 메서드 처음봤는데 좋네요..!
배워갑니다!
| public static Coin of(int amount) { | ||
| return Arrays.stream(values()) | ||
| .filter(coin -> coin.amount == amount) | ||
| .findAny() | ||
| .orElseThrow(CoinEmptyException::new); | ||
| } |
There was a problem hiding this comment.
오..! 이 부분에서 stream을 이용할 생각을 못했었는데 이렇게 사용할수 있겠네요
배워갑니다!
|
|
||
| import java.util.List; | ||
|
|
||
| public interface NumberGenerator { |
There was a problem hiding this comment.
interface 상단에 @FunctionalInterface 어노테이션을 선언하면 해당 인터페이스가 함수형 인터페이스임을 강조할 수 있습니다!
4주차 미션에서 참고했는데 이런 역할을 하더라구요!
| public class Price { | ||
|
|
||
| private static final int MIN_PRICE = 100; | ||
| private static final int MAX_PRICE = 1_000_000_000; |
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { |
There was a problem hiding this comment.
재정의하면서 기본적으로 o 라고 표현된거같은데
개인적으로 저는 equals를 재정의할때 파라미터 명을 other라고 네이밍을 지어줬습니다 ㅎㅎ
| return products.stream() | ||
| .filter(product -> product.isSameName(name)) | ||
| .filter(product -> product.purchasableProduct(money)) | ||
| .findAny() | ||
| .orElseThrow(ProductsNotFind::new); |
There was a problem hiding this comment.
이런 방식으로도 구매가능한 Product를 구분할 수 있군요!!
배워갑니다
|
|
||
| public boolean isAllQuantityZero() { | ||
| return products.stream() | ||
| .allMatch(Product::isSoldOut); |
| @@ -0,0 +1,10 @@ | |||
| package vendingmachine.exception; | |||
|
|
|||
| public class CoinEmptyException extends IllegalArgumentException { | |||
There was a problem hiding this comment.
우와.. IllegalArgumentException을 상속받아서 CustomException을 사용하는방식도 생각 못했던 부분인데
배워갑니다!
There was a problem hiding this comment.
커스텀 예외가 항상 좋은것은 아닙니다! 커스텀예외에 좋은글이있어서 링크남깁니다!
| .replaceAll(PRODUCT_DUPLICATED_DELIMITER_REGEX, PRODUCT_DELIMITER) | ||
| .split(PRODUCT_DELIMITER); | ||
|
|
||
| if (contents.length != 3) { |
No description provided.