[자판기] 조나현 미션 제출합니다. #165
[자판기] 조나현 미션 제출합니다. #165
Conversation
자판기 게임에서 구현할 기능 목록입니다.
자판기 게임에서 사용할 메세지들을 GameMessage 클래스에 정의합니다.
금액에 숫자 이외의 문자가 입력된 경우 IllegalArgumentException을 발생시킨다.
상품 추가 입력 양식이 잘못됬을 때 IllegalArgumentException을 발생합니다. Regex를 이용하여 입력 양식에 부합하는지 판단했습니다.
vendingMachineException의 validGoodsInputForm을 이용하여 입력 양식에 부합하는지 판단한 후 부합한다면 반환합니다.
사용자가 등록한 상품 금액이 100원부터 시작하지 않고, 10원 단위가 아니라면 예외를 발생시킵니다.
사용자의 투입 금액을 입력 받습니다. vendingMachineException의 validMoneyInputForm을 이용하여 사용자의 투입 금액 입력 양식이 어긋나면 예외를 발생시킵니다.
상품 가격 예외 처리 완료하였으므로, readme 업데이트 합니다.
자판기 금액을 생성할 때 Coin 클래스를 이용하기 위해 , amount 변수를 도입하여 자판기에 생성된 코인의 개수를 Enum Class에 등록합니다.
자판기가 보유하고 있는 금액으로 동전을 무작위로 생성하는 함수입니다. 요구사항에 적힌대로 랜덤 값 추출은 pickNumberInList 함수를 사용하였습니다.
코드에서 중복되어 불필요한 부분을 제거합니다.
기능 목록 추가합니다.
가독성을 위해 기존 moneyGenerator에서 vendingmachine으로 패키지 명 변경합니다.
기능 구현 마무리합니다. 커밋이 많이 밀려서 부득이하게 한번에 올립니다 🤯
불필요한 import문을 제거하고, if문을 간결화합니다.
goods 클래스는 초기에 상품 리스트를 등록할 때만 값을 변경합니다. 따라서 불필요한 set은 제거하고 상품 등록시 필요한 makeGoodsInfo 함수에서 set기능을 대신합니다.
GoodsManager 클래스를 사용하기 위해선 상품리스트와 상품의 최소 가격은 필수이므로 생성자를 통해 상품 리스트와 최소 가격 설정 후 사용할 수 있도록 제한합니다. 따라서 기존 makeGoodList에서 사용한 minPriceGoodsList 함수를 생성자로 옮깁니다.
금액이 10원 미만일 때 예외 처리 추가합니다.
함수명 수정합니다.
Exception처리를 위해 단순히 클래스를 불러오는 메서드를 삭제합니다.
기존에 append 내부에서 +를 통해 문자열을 합쳤습니다. 가독성을 위해 stringbuilder의 append를 이용하여 분리합니다.
자판기 게임 예외처리 테스트 완료하였습니다.
상품 등록 양식 Regex가 제대로 처리되지않아 수정합니다.
상품 구매와 관련된 GoodsManager 클래스 테스트 완료했습니다.
| } | ||
|
|
||
| public void makeVendingMachineCoin(String money) { | ||
| VendingMachineManager vendingMachineManager = new VendingMachineManager(); |
There was a problem hiding this comment.
VendingMachineManager를 필드로 빼거나 상속받아서 returnCoin에서도 중복된 생성을 없애는 것이 좋아보입니다.
|
|
||
| import java.util.*; | ||
|
|
||
| public class VendingMachineManager { |
There was a problem hiding this comment.
클래스명은 ...Manager, ...Info 등의 이름형식은 의도가 명확하지 않습니다.
make라는 접두어가 있으니 두 메소드의 make를 없애고 클래스명을 CoinMaker나 Initialization 같은 이름이 어떨까요.
| while (true) { | ||
| int num = Randoms.pickNumberInList(vendingMoneyList); | ||
| if (vendingMoney == 0) break; | ||
| if (vendingMoney - num < 0) continue; // 450 - 500이 -50이면 다른 동전으로 | ||
| vendingMoney -= num; | ||
| Coin.getCoin(num).addCoin(); | ||
| } |
There was a problem hiding this comment.
while (vendingMoney != 0) {
int num = Randoms.pickNumberInList(vendingMoneyList);
if(vendingMoney - num >= 0) {
vendingMoney -= num;
Coin.getCoin(num).addCoin();
}
}
반복조건이 더 명확한게 좋을거 같아요
| InputUserMoney(); | ||
| } | ||
|
|
||
| public void InputUserMoney() { |
There was a problem hiding this comment.
gameStart 메소드처럼 외부에서 호출될 일이 없는 메소드들의 접근제어자를 수정하는것이 좋아보입니다.
|
|
||
|
|
||
| public void purchaseGoods(int money) { | ||
| /** |
There was a problem hiding this comment.
주석보단 소스자체로 메소드의 기능을 설명하는 것이 좋아보입니다. (우테코 피드백)
| } | ||
|
|
||
| public HashMap<Integer, Integer> makeRemainCoinList(int money) { | ||
| HashMap<Integer, Integer> coinList = new LinkedHashMap<>(); |
There was a problem hiding this comment.
가장 큰 단위의 동전부터 보여주기 위해 LinkedHashMap 을 활용한 게 너무 좋은것 같습니다.
| for (Coin c : Coin.values()) { | ||
| if (money <= 0) break; | ||
| int require = money / c.getAmount(); | ||
| if (require > c.getCount()) { | ||
| money -= (c.getCount() * c.getAmount()); | ||
| coinList.put(c.getAmount(), c.getCount()); | ||
| continue; | ||
| } | ||
| money -= (require * c.getAmount()); | ||
| coinList.put(c.getAmount(), require); | ||
| } |
There was a problem hiding this comment.
프리코스 피드백을 참고해보면 개행도 피드백이라고 합니다.
제어문 사이에 개행시키면 훨씬 보기 좋을것 같습니다.
| vendingMachineException.validMoneyInputForm(vendingMoney); | ||
| vendingMachineException.validMoneyType(vendingMoney); |
There was a problem hiding this comment.
자판기의 소유금액 입력을 확인할때, 하나의 메서드로 처리하고 vendingMachineException 클래스에서 메서드를 분리하는게 좋을꺼 같아요
| return price; | ||
| } | ||
|
|
||
| public void deleteAmount() { |
| try{ | ||
| vendingMachineController.gameStart(); | ||
| }catch (Exception e){ | ||
| System.out.println(e.getMessage()); |
There was a problem hiding this comment.
exception시 error 메시지 출력 후 재입력을 받아야 하는데 출력 후에 앱이 끝날 것 같아요
| private void makeGoodsInformation(String goodsList) { | ||
| String[] itemList = goodsList.split(";"); | ||
| for (String item : itemList) { | ||
| String[] goodsInformation = item.replace("[", "").replace("]", "").split(","); |
There was a problem hiding this comment.
replace 두번 쓰는 것 보다 replaceAll 에 "[]"를 한번에 넣으면 좋을 것 같아요
| } | ||
|
|
||
| void makeGoodsInfo(String[] goodsInformation) { | ||
| price = Integer.parseInt(goodsInformation[1]); |
| makeGoodsList(); | ||
| } | ||
|
|
||
| public void makeGoodsList() { |
| public HashMap<Integer, Integer> makeRemainCoinList(int money) { | ||
| HashMap<Integer, Integer> coinList = new LinkedHashMap<>(); | ||
|
|
||
| for (Coin c : Coin.values()) { |
There was a problem hiding this comment.
c라는 변수 네이밍이 어떤 것을 뜻하는지 명확하게 바뀌면 좋을 것 같아요
|
|
||
| private final HashMap<String, Goods> goodsInfo = new HashMap<>(); | ||
| private int minPrice = Integer.MAX_VALUE; | ||
| VendingMachineException vendingMachineException = new VendingMachineException(); |
There was a problem hiding this comment.
Exception이라는 네이밍보다는 Validation이라느 네이밍이 더 어울릴것 같아요
| String[] itemList = goodsList.split(";"); | ||
| for (String item : itemList) { | ||
| String[] goodsInformation = item.replace("[", "").replace("]", "").split(","); | ||
| vendingMachineException.validGoodsMoneyType(goodsInformation[1]); |
There was a problem hiding this comment.
goodsInformation[1]를 goodsInformation[PRICE_POSITION]처럼 상수로 값을 표현하는게 좋을것 같네요
| } | ||
|
|
||
| public static Coin getCoin(int amount) { | ||
| return Arrays.stream(values()).filter(a -> a.getAmount() == amount).findAny().orElse(null); |
There was a problem hiding this comment.
orElse 내부의 구문은 항상 호출됩니다.
지금은 문제가 없지만 orElseGet을 사용하는 습관이 필요합니다.
문제가 된 상황 예시
| while (true) { | ||
| int num = Randoms.pickNumberInList(vendingMoneyList); | ||
| if (vendingMoney == 0) break; | ||
| if (vendingMoney - num < 0) continue; // 450 - 500이 -50이면 다른 동전으로 |
There was a problem hiding this comment.
랜덤으로 생성된 num이 계속해서 vendingMoney - num < 0이 true인 값을 생성하면 무의미한 루프가 계속 돌게됩니다.
vendingMoney의 값에 때라 생성되는 num을 달리 하면 좋을 것 같네요
| * -> 잔돈 반환 | ||
| */ | ||
|
|
||
| while(true){ |
There was a problem hiding this comment.
while(true) 보다는 goodsManager에 상태를 참고할 수 있는 메소드를 두고 while의 조건에 넣는건 어떨까요?
예를들면 while(goodsManager.isEnd())와 같은 느낌이 될것같네요
| } | ||
|
|
||
| public void validGoodsInputForm(String goods) { | ||
| final String GOODS_INPUT_FORM = "\\[[0-9가-힇A-Za-z]+,?[0-9]*,?[0-9]*\\];?"; |
There was a problem hiding this comment.
상품을 입력받는 부분에 대한 검증은 정규식이 제일 깔끔하게 처리될 것 같네요 👍
| } | ||
|
|
||
| public void validGoodsName(String goods, HashMap<String, Goods> goodsInfo){ | ||
| if(goodsInfo.get(goods)==null) |
returnCoin 메서드와 makeVendingMachineCoin 두 개의 메서드에서 중복된 VendingMachineManager를 생성하고 있습니다. 해당 클래스는 중복 선언이 불필요하므로 제거합니다.
자료형이 들어간 메서드, 변수명을 개선합니다. goodsList -> goodsCataLog / makeGoodsList -> makeGoodsCataLog로 수정합니다.
클래스명의 의도가 불분명하여, CoinMaker로 수정합니다.
반복 조건이 while문 조건이 아닌 메서드 내부에 선언되어있어 가독성이 떨어집니다. 따라서 반복 조건 가독성 향상을 위해 분할하였습니다.
controller 내부에서만 사용하는 메서드들의 접근 제어자를 변경합니다.
주석을 제거하고 while(true)안의 if문으로 반복 조건을 명확히 하였습니다. 또한 0원일 때 잔돈 반환이 되는 오류가 발생하여 수정 하였습니다.
가독성을 위해 코드 간격 재조정을 합니다.
입력된 금액에 대해 공통적으로 검사하는 로직을 묶어 Exception 클래스에서 함수로 선언하였습니다.
count 하나를 줄이는 기능이지만 delete는 전부 지우는 느낌이어서 가독성이 떨어지므로 reduce로 변경합니다.
replace를 2번 사용하는 것 보다 regex를 사용한 replaceAll을 이용하여 한번에 제거하였습니다.
Coin c 라는 변수명을 Coin coin 으로 변경하여 가독성을 높였습니다.
클래스명을 Exception에서 Validator로 변경한다.
orElse의 문제점이 있어 orElseGet 메서드로 전환합니다.
No description provided.