[자판기] 전하영 리팩토링합니다. #172
[자판기] 전하영 리팩토링합니다. #172
Conversation
- 남은 투입금액 계산 - 남은 상품 수량 계산
- 남은 금액이 남은 상품의 최저 가격보다 적은 경우 - 모든 상품이 소진된 경우 잔돈 반환
juno-junho
left a comment
juno-junho
left a comment
There was a problem hiding this comment.
사실 저는 어떻게 하셨나 궁금해서 봤는데 예외도 엄청 꼼꼼히 체크하시고, 그 자신만의 논리 체계가 있으신것 같아요
사실 뭐 진행한 프리코스에 대해서 정답이 있는 구조가 있는 것도 아니고, 어떻게 구현하셨는지가 메서드를 한가지 역할을 하도록 분리해라, 메서드와 변수명을 명확하게 하라 같은 리팩토링 기술보다 훨씬 중요하니까요. 역시 전공자답습니다 ㅋㅋ
그 특히나 마지막에 동전 Math.min으로 할수 있다는게 왜 전 생각을 못했는지 모르겠어요 ㅋㅋ
역시 코드 보니 저는 5시간 내에 최대한 빨리 구현하려고 해서 빼먹은 부분이 많이 보이네요
하영님 최종 코테 준비만 하면 될것 같습니다ㅋㅋ
| public static void main(String[] args) { | ||
| // TODO: 프로그램 구현 | ||
| VendingMachineController vendingMachineController=new VendingMachineController(); | ||
| vendingMachineController.setVendingMachine(); |
There was a problem hiding this comment.
main 메서드에서 set -> runVendingMachine() 이라는 함수를 통해서 어떻게 보면 두가지 일을 하고 있는것 같아요. 코드를 봤는데, controller에서 run메서드 실행 전에 set은 반드시 되어야 하니까 run 메서드 안에서 setVendingMachine 메서드를 호출하는 방식이 좋지 않을까 생각해봐요!
| private static int amountOfInput; | ||
|
|
||
| public void setVendingMachine() { | ||
| balance = new Balance(InputView.readBalance()); |
There was a problem hiding this comment.
그리고 static을 통해 클래스 변수로 지정하신 특별한 이유가 있나요?
balance를 인스턴스 변수로 선언하고 setter를 통해서가 아니라 생성자를 통해서 this.balance = new Balance(InputView...) 로 설정해주면 setVendingMachine 메서드를 없앨수도 있을것 같아요.
static 변수(클래스 변수)를 피해야 하는 이유는 여러가지지만 제가 명확히 기억하는 것은 2가지 인데,
- 사용하지 않더라도 프로그램 시작과 끝까지 메모리 내에 존재해 메모리 효율성이 떨어진다.
- 동시성 문제를 고려해줘야 한다. -> 한 클래스에 여러 곳에서 접근해 로직을 실행하는 도중 다른 곳에서 또 접근한다면 thread safe하지 않은 문제가 발생해서 Map도 ConcurrentMap 타입을 사용하는 것으로 저는 알고 있어요!
추가적인 자료는 구글에 찾아봐도 나오지만 첨부해 두겠습니다!
static 사용을 피해야 하는 이유
**추가
InputView랑 OutputView를 생성자로 받아 생성한다면 STATIC 메서드를 없앨 수 있을거같아요
다시 보니 setVendingMachine이 자판기 설정을 하는 메서드네요.. 나름 의미가 있는 메서드라는걸 코드 보면서 느꼈습니다. 단순히 객체를 초기화해주는게 아니었네요 (자판기 보유 동전 설정 + 상품 설정 역할을 하네요..)
| this.message = message; | ||
| } | ||
|
|
||
| public String fullMessage() { |
There was a problem hiding this comment.
getMessage라 해도 괜찮을것 같아요!
메서드는 동사로 만드는게 의미 전달에 좋을것 같아요.
fullMessage대신 getFullMessage라 하는게 좋지 않을까요?
|
|
||
| public static int validateBalance(String input) throws IllegalArgumentException { | ||
| int balance = validateNum(input); | ||
| validateNegative(balance); |
| public static int readAmountOfInput() { | ||
| System.out.println(AMOUNT_OF_INPUT.fullMessage()); | ||
| try { | ||
| return validateAmountOfInput(Console.readLine()); |
There was a problem hiding this comment.
저는 투입금액 도 10으로 나눠 떨어지는것 validate를 해줬는데 꼭 해줘야할 필요는 없는것 같네요 ㅋㅋ
| } | ||
| } catch (IllegalArgumentException e) { | ||
| System.out.println(e.getMessage()); | ||
| saveProducts(); |
|
|
||
| public void runVendingMachine() { | ||
| while (canBuy()) { | ||
| buy(); |
There was a problem hiding this comment.
이렇게 받으면 buy()애서 구매할 상품명을 입력 먼저 받고 아래 OutputView.printAmountOfInput(amountOfInput); 통해서 투입금액이 출력되지 않나요? 순서가 바뀐것 같은데 실행은 잘 되시나보네요
그 위에서 한번 출력을 하고 시작하네요
| import static vendingmachine.model.Validator.validateNum; | ||
| import static vendingmachine.model.Validator.validatePrice; | ||
|
|
||
| public class Price { |
There was a problem hiding this comment.
따로 클래스 뺀 이유가 있으신가요? Product안에서 int로 처리해줘도 될것 같은데 특별한 이유가 있는지 궁금해요
There was a problem hiding this comment.
특별한 이유가 있다기 보다는 얼마 전에 원시 타입 포장 관련 글(https://tecoble.techcourse.co.kr/post/2020-05-29-wrap-primitive-type/)을 보고 처음 설계할 때 적용해 보려고 분리를 했었는데..
리팩토링 하다 보니 결국 검증 과정도 Validator 클래스로 다 넘겨서ㅋㅋㅋ 저도 끝에는 굳이 분리할 필요가 있었나 생각하긴 했어요ㅎㅎ
| private final int balance; | ||
| List<Integer> numbers = new ArrayList<>(); | ||
| private final Map<Coin, Integer> coins = new EnumMap<>(Coin.class); | ||
| private final Map<Coin, Integer> changeCoins = new EnumMap<>(Coin.class); |
There was a problem hiding this comment.
오 이런게 있는 줄 몰랐어요 ㅋㅋ 저는 LinkedHashMap 써서 Coin enum 순서대로 넣어줬거든요
There was a problem hiding this comment.
저도 처음 써봤어요ㅋㅋㅋㅋ 오히려 LinkedHashMap은 한번도 안 써봤네요..
| for (Coin coin : Coin.values()) { | ||
| int numberOfBalanceCoin = coins.get(coin); | ||
| int neededCoins = change / coin.get(); | ||
| int inputCoins = Math.min(neededCoins, numberOfBalanceCoin); |
There was a problem hiding this comment.
전 왜 이 생각을 못했죠
사실 이거 어떻게 구현하셨는지가 제일 궁금했는데 진짜 간단하게 하셨네요
전 neededCoin과 numberOfBalanceCoin 을 if문으로 경우 나눠서 처리해줬거든요
|
하영님 혹시나 해서 하는 말인데 저는 자판기 미션 구현한게 리팩토링도 거의 안했고, 정말 한 7시간? 정도만에 테스트 통과되게끔만 만든거라서 코드가 너무 지저분해요. 민정님이랑 같이 하영님을 맨 처음 봐서 그런지 잘 되셨으면 좋겠다는 마음이 커요 ㅋㅋ 그리고 하영님은 지금까지 꼼꼼하게 잘 하시는거 같아서 저는 합격하실거라 믿어의심치 않습니다! (심지어 stream 없이 10줄 규칙을 지키신거면 진짜 대단하다 생각해요) 리펙토링 하다 테스트가 깨지는 경우가 있다고 했는데, intellij가 사용하는 기능을 통해서 리팩토링을 하시면 오류 범위를 낮추실수 있을거에요. 메서드 분리 하고 싶은 부분 : 추가적으로 제가 자주 쓰는 intellij 단축키는 이번 기회를 통해서 만날수 있게 되어서 정말 반가웠고, 주변에 같이 준비하는 사람 한명 없이 혼자 개발공부를 해오면서 정말 혼자 사막 위를 걷는 기분이었는데, 전 스터디 시작하고 정말 삶이 즐거워 진것 같아요. |
2 participants
No description provided.