Conversation
|
|
||
| public class Change { | ||
| private final int amount; | ||
| private final int[] coins; |
There was a problem hiding this comment.
Map을 이용하면 더 좋을것 같습니다.
예를들면
Map<Coin, Integer> coins 처럼 구성할 수 있을 것 같아요
| } | ||
|
|
||
| private int[] makeCoins(int amount) { | ||
| int[] coins = new int[4]; |
There was a problem hiding this comment.
Enum으로 관리하면 좋을것 같아요
만약에 1000원단위가 생기면 손대야할 부분이 너무 많아질것 같아요
| this.amount = amount; | ||
| } | ||
| public static int getRandomCoin(){ | ||
| List<Integer> coins = new ArrayList<>(); |
There was a problem hiding this comment.
호출 할때마다 List를 새로 생성하면 자원 낭비가 될것같네요
상수로 빼서 관리해도 될거같아요.
private final `List<Integer>` coins = List.of(
COIN_10.amount,
COIN_50.amount,
COIN_100.amount,
COIN_500.amount
)
| coins.add(COIN_500.amount); | ||
| return Randoms.pickNumberInList(coins); | ||
| } | ||
| public static int getIndex(int coin){ |
|
|
||
| // 추가 기능 구현 | ||
| public static boolean isDivideMinCoin(String input) { | ||
| if (Integer.parseInt(input) % COIN_10.amount == 0) return true; |
There was a problem hiding this comment.
try catch가 없어서 NumberFormat Exception에 대처하기 어려워보이네요
해당 메소드가 숫자를 검증하는 역할까지 가진다는 것에서 책임이 너무 많아지는것 같아요
차라리 파라미터로 primitive type (int)으로 받고 처리하는건 어떨까요?
| this.amount = amount; | ||
| } | ||
|
|
||
| public int buyProduct(String name){ |
There was a problem hiding this comment.
buyProduct 함수가 isExistProduct의 역할을 같이 수행하는것 같네요
아래에서 isExistProduct로 상품잔여개수에 대한 판별을 하는데 굳이 RUN_OUT을 추가로 반환해줄 필요가
해당 부분에서는 개수가 부족할때 예외를 던지는 방식으로 시도해보는것을 어떨까요
| int buyPrice = 0; | ||
| for(int index = 0 ; index < products.size();index++){ | ||
| int buy = products.get(index).buyProduct(buyProduct); | ||
| if(buy!=RUN_OUT){ |
| public boolean checkContinue(int amount){ | ||
| if(minAmount > amount) return false; | ||
| for(int index = 0; index < products.size(); index++){ | ||
| if(!products.get(index).isExistProduct())return false; |
There was a problem hiding this comment.
products.get(index).isExistProduct() 부분에서 products.get(index) 부분을 한번 변수로 분리해주는것이 좋아보여요.
| amountPrice -= products.buy(buyProduct); | ||
| } | ||
| public boolean isFinish(){ | ||
| return products.checkContinue(amountPrice); |
There was a problem hiding this comment.
getter를 통해 상태를 가져오고 처리하는 방식은 어떨까요?
|
|
||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
lastPrint()의 내용이 VendingMachine.toString()에 더 적합해보입니다.
아래 구문은 getAmountPrice()정도로 사용할 수 있을것 같네요
| try { | ||
| productsInput = Console.readLine(); | ||
| String[] splitProduct = productsInput.split(";"); | ||
| for (String product : splitProduct) { |
There was a problem hiding this comment.
함수내용이 너무 길어지면 메소드 분리를 생각해볼 수 있습니다.
인텔리제이의 Ctrl + Alt + M 기능을 이용해 메소드를 쉽게 분리할 수 있습니다.
| if (!Coin.isDivideMinCoin(inputChange)) throw new IllegalArgumentException("[ERROR] 잔돈이 나눠지지 않습니다."); | ||
| } | ||
|
|
||
| private void inputDigitValidation(String input) { |
There was a problem hiding this comment.
입력 값에 대한 오류처리가 많다면 따로 validator 클래스를 생성해서 분리해놓는게 좋을 것 같습니다.
| } | ||
|
|
||
| private void validateProductQuantity(int quantity) { | ||
| if (quantity <= 0) throw new IllegalArgumentException("[ERROR] 수량은 0보다 커야합니다."); |
|
|
||
|
|
||
| private void validateProductPrice(int price) { | ||
| if (price > 100 && Coin.isDivideMinCoin(String.valueOf(price))) return; |
There was a problem hiding this comment.
Magic Number도 상수로 치환해주실거죠??🥺
| int minPrice = Integer.MAX_VALUE; | ||
| while (productsInput.equals("")) { | ||
| outputView.askProductPrint(); | ||
| try { |
There was a problem hiding this comment.
우테코 피드백에서 함수의 길이가 길어지면 기능을 분리해보라고 하셨는데, 함수를 생성해 분리해보는건 어떨까용
|
|
||
| public int inputChange() { | ||
| outputView.askPricePrint(); | ||
| String inputChange = ""; |
| buyPrice = buy; | ||
| } | ||
| } | ||
| if(buyPrice == 0)throw new IllegalArgumentException("[ERROR] 해당 제품이 존재하지 않습니다."); |
There was a problem hiding this comment.
구매하는 역할과 예외처리하는 역할을 buy메서드가 동시에 하고 있는 것 같아요. 역할을 분리해보는게 어떨까용
| import java.util.List; | ||
|
|
||
| public class InputView { | ||
| private final OutputView outputView = new OutputView(); |
There was a problem hiding this comment.
생성자를 사용해서 외부에서 outputview클래스를 받아오면 객체 생성 횟수를 줄일 수 있을 거 같아요.
VendingMachineService 클래스에서도 outputview클래스를 생성하셨더라구요. 그 객체를 받아 와서 그대로 사용하는게 좋을 것 같아요.
| for(int index = 0; index < products.size(); index++){ | ||
| if(!products.get(index).isExistProduct())return false; | ||
| } |
There was a problem hiding this comment.
for (Product product : products) return product.isExistProduct()
이런식으로 정리하면 깔끔할꺼 같아요!
5 participants
No description provided.