move : 프로젝트 변경으로 인한 코드 이동 #187
Conversation
| EnumMap<Coin, Integer> returnCoin = vendingMachine.returnCoin(money); | ||
| outputView.printReturnCoins(returnCoin); | ||
| } | ||
|
|
There was a problem hiding this comment.
혁수님 로직을 보며, 제 코드에 문제점을 발견하게 되었네요.
제 코드는 구매할 상품명을 입력할 때, 없는 메뉴인 경우 무한루프가 정상 동작하지만, 재고가 떨어진 메뉴는 프로그램이 종료하는 문제가 있었습니다. 😂😂
미션을 진행할 때, ExceptionHandler(RetryHandler)에 태울 로직의 묶음 단위를 꼼꼼히 파악하는게 정말 중요할 것 같습니다,, 👍
| String inputMenu = inputView.purchaseMenu(); | ||
| vendingMachine.purchase(inputMenu, remainCash); | ||
| } | ||
|
|
There was a problem hiding this comment.
함수형 인터페이스에 대해, 람다식으로 전달하는 방법보다 메서드 참조로 전달하는 방식이 정말 깔끔하고 좋은 것 같습니다.
- 무한 루프의 단위를 메서드로 분리하여 구현한다.
- 메서드 참조로 Handler에 태운다
정말 명확하고 관리하기 용이한 코드인 것 같습니다 👍👍
| } | ||
| return cash.canPurchase(this.price); | ||
| } | ||
|
|
There was a problem hiding this comment.
추가적으로, 해당 로직이 물품 구매에 사용될 로직이기에 준기님도 VendingMachine.sell()에서 물품이 구매 가능한지를 isEmpty()와 isGreaterThanPrice() 메서드를 호출하여 물품이 구매 가능한 상황에서만 minusCount()를 호출하고 있는데요.
그러나 minusCount() 자체가 메서드 호출 시점을 검증할 수 있도록 minusCount() 내부에서 잘못된 상태의 호출에서 예외를 발생시킬 수도 있을 것 같아요.
코멘트 남겨주신 부분이 67 line의 soldOut 재차 검증 로직을 말씀하신건지 여쭙고 싶습니다!
There was a problem hiding this comment.
네 맞습니다!
특정 메서드는 호출되기 위한 전제 조건이 존재하는데요.
구매 로직이 호출되기 위해선 재고가 있어야 하는 것 처럼요!
이럴 때 저는 메서드 내부에서 전제조건을 위반하는 상태를 확인할 수 있다면 이를 확인하고 예외를 던져주려고 노력하고 있습니다.
이는 우리가 코드를 짤 때도 조금 더 안전한 코드를 작성할 수 있게 해주며 추후 협업을 진행할 때 팀원이 놓칠 수 있는 실수를 예방해 주기도 한다고 생각하기 때문이에요!!
| this.coins = coins; | ||
| this.menus = menus; | ||
| } | ||
|
|
There was a problem hiding this comment.
남은 금액이 상품의 최저 가격보다 적거나, 모든 상품이 소진된 경우 바로 잔돈을 돌려준다.
를 확인하기 위한 메서드인 것 같아요!
VendingMachine -> Menus (canPurchase): 구매 가능한지 확인한다.
Menus -> Menu (canPurchase): 메뉴가 소진되었는지 확인한다.
Menu -> Cash (canPurchase): 현재 금액으로 구매할 수 있는지 확인한다.
최종적으로, 자판기에서 상품을 구매할 수 있는지 검증하기 위한 협력은 아래와 같은 흐름인 것으로 판단됩니다.
VendingMachine -> Menus -> Menu -> Cash
객체지향 관점에서 역할 분리가 잘 되어 있는 코드라 느껴졌습니다 👍
하지만 모든 메서드 네이밍이 canPurchase이기 때문에, 흐름을 하나씩 따라가며 세부 구현을 확인해야 정확히 어떤 로직인지 파악이 가능한 아쉬움도 느껴졌어요! 메서드 네이밍을 의도에 명확하게 수정하시면 더 파악하기 쉬울 것 같습니다 :)
There was a problem hiding this comment.
위에 코멘트 드린 내용과 이어지는 내용입니다. 아래의 코멘트는 정답이 없는 영역이라, 가벼운 제안 + 혁수님 생각이 궁금하여 남겨드립니다.
최근에 우아한 형제들 세미나를 통해 이런 내용을 들은적이 있어요.
주제: Validation 위치
Validate 로직이 적다면 객체 안에서 수행하는게 맞는데, 그 객체를 Validate 하기 위해 여러 객체를 참조해야 한다면, 응집도가 확 떨어져버리는 트레이드 오프가 있다. 역할 분리가 잘 되어 있지만, 어떤 유효성 검증인지 파악하기 위해 여러 클래스를 가봐야 알 수 있기 때문이다. 따라서 때로는 절차지향적인 방식도 도움이 될 수 있다. ~~~
말씀드린 내용은 객체 유효성 검증에 대한 내용이라 조금 다른 주제이긴 하지만, 자판기에서 상품을 구매할 수 있는지 검증하기 위한 협력도 뭔가 같은 결이라고 문득 느껴졌습니다.
자판기에서 상품을 구매할 수 있는지 확인하는 검증로직에 대해, 여러 클래스들을 오가며 확인해야하다보니 응집도가 떨어지는 것 아닌가?라는 생각이 들었어요.
따라서 해당 검증 로직은 자판기에서 모두 수행하도록 하고(약간의 절차지향), 검증을 위해 필요한 기능들만 관련 객체들이 API로 제공해주는 방식으로 리팩터링한다면 응집도를 높여주고, 한 눈에 확인할 수 있는 장점도 있을 것 같습니다. 이 부분에 대해 혁수님 의견이 궁금합니다 😊
흥미로운 주제라 생각되어, 다른 분들도 의견 있으시다면 편히 코멘트 남겨주세요!
There was a problem hiding this comment.
위에서 언급해 주신 네이밍과 관련된 내용은 저 역시 구현하면서 많이 아쉬웠던 부분이에요.
다만 보시는 것 처럼 VendingMachine이 구매 가능 여부를 알 수 있는 방법은 menus에게 물어보는 방법밖에 없기에 menus에 똑같은 질문을 다시 전달해 주는 느낌인데요. 이 상황에서 어떤 식으로 네이밍을 구분해 줄 수 있을 지 고민이네요.
지금의 제 생각으론 제 코드를 리팩토링 한다고 해도 canPurchase() 로직은 지금의 형태에서 크게 바꿀 것 같진 않아요.
저의 코드에선 상품을 Cash로 구매할 수 있을 지 확인하기 위해선 각각의 판매 항목들을 순회하면서 하나라도 구매 가능한 항목이 존재하는지를 확인해야 해요.
그리고 이 각각의 항목들은 List<>를 래핑한 일급 컬렉션 Menus가 가지고 있고요.
Menus를 필드 값으로 가지고 있는 VendingMachine이 구매 가능 여부를 알 수 있는 방법은 Menus에게 구매 가능 여부를 물어보는 방법 이외엔 없다고 생각합니다.
더군다나, Menus.canPurchase()를 호출하는 행위 자체가 해당 행동을 위한 필요 API를 Menus가 적절히 제공해 주고 있는 것이라고 생각했어요.
다만, 이렇게 생각한 이유는 제가 제 코드를 많이 보면서 어느 정도 제 코드에 가스라이팅 당한 것도 있다고 생각하기에 준기님의 생각이 다르다면 얼마든지 편하게 말씀해 주시면 좋을 것 같습니다 ㅎㅎㅎ
There was a problem hiding this comment.
@june-777
준기님 의견에 동의합니다. 실제 자판기를 가정하면 저희가 인지하는 부분은 버튼을 누르면 음료가 나온다. 뿐이니까요.
menu라는 객체를 생성했다면 구매할 수 있는지 검증은 메뉴의 책임이라고 생각합니다.
| this.amount = amount; | ||
| } | ||
|
|
||
| // 추가 기능 구현 |
There was a problem hiding this comment.
👍👍
참고로 Optional의 값이 없는 경우 orElseThrow로 예외를 발생시킬지, 비어있는 Optional을 반환할지 고민했었는데
- 없는 값일 때 추가적인 로직이 필요하다 -> 후자
- 없는 값이면 그냥 예외상황이다 -> 전자
로 기준을 잡게 되었습니다 :) 참고하시면 좋을 것 같아요.
There was a problem hiding this comment.
저도 해당 기준이 적절하다고 생각합니다!! 좋은 것 같네요 👍 ㅎㅎ
| List<Integer> coinAmounts = Arrays.stream(Coin.values()) | ||
| .map(Coin::getAmount) | ||
| .toList(); | ||
|
|
There was a problem hiding this comment.
혁수님의 구현을 몇 번 봐보니, Java 8 부터 도입된 인터페이스 default 메서드를 적극 활용하신다고 느껴졌어요!
제가 알기로 default 메서드 도입 배경은
자바 라이브러리, 스프링 등 자바 기반 생태계에서 인터페이스A, 구현체1, 구현체2, ...구현체10 인 상황일 때, 인터페이스A에 대한 오퍼레이션 시그니쳐를 새롭게 추가하기 부담되니 (수십개의 구현체에서 추가 오버라이딩하여 구현해야 하므로) default 메서드를 도입한 것으로 알고 있습니다.
저는 그래서 아직까지는 인터페이스의 default 메서드를 활용해야겠다는 순간은 못느낀 것 같아요! 혁수님은 어떤 장점이 있을 것으로 판단하여 default 메서드를 적극 활용하시는지 궁금해요! 😊
There was a problem hiding this comment.
사실 이 부분은 default 메서드가 리팩토링 시점이거나, 구현 끝자락 즈음에 작성된 코드였던 것으로 기억하는데요.
default 부분을 작성한 이유는 모든 CoinGenerator가 코인을 선택하는 방법만 다르지, 결국 코인을 생성하는 전체 로직은 공통될 것이라고 생각했기 때문이에요.
그리고 이렇게 작성을 하는 와중에 "CoinGenerator는 인터페이스가 아니라 추상클래스로 제공되는 것이 맞았겠다" 라는 생각을 하게 되었습니다.
default 메서드로 구현하기 보단 그냥 부모 객체가 제공하는 공통 메서드 인 편이 더 자연스럽겠다고 생각했기 때문이에요.
저 역시 아직 default 메서드를 활용하는 기준은 세우지 못했을 뿐더러, 인터페이스에서의 default 메서드를 사용하는 부분은 많이 부족하다고 생각해요.
혹시 이 부분에서 인사이트를 얻고 싶으시다면 제 의도는 "추상 클래스"의 사용이었으나 시간의 한계로 인터페이스를 추상클래스로 변경하지 못했다는 점을 알아주셨으면 좋겠습니다!!
| package vendingmachine.domain.money; | ||
|
|
||
| import vendingmachine.exception.VendingMachineException; | ||
|
|
There was a problem hiding this comment.
어제 스터디 후 리뷰 시간에서 Money는 불변 객체로, Cash는 가변 객체로 활용하기 위해 나눠서 구현하신 것으로 기억합니다!
해당 미션을 진행하며, 어느 포인트에서 분리의 필요성을 느끼신건지 궁금해요!
그리고 Spendable 인터페이스 도입 이유도 여쭙고 싶습니다!
There was a problem hiding this comment.
우선 Money와 Cash로 나누어 준 이유는 다음과 같습니다.
가장 먼저, 저는 Money 객체를 불변성을 가진 객체로 만들고 싶었어요.
이 부분은 이번 미션을 구현하면서 들었던 생각은 아니고 "로또" ~ "크리스마스 프로모션" 사이의 시점에서 들었던 생각이에요.
아무래도 민감한 관심사를 다루는 객체이기에 값의 변경은 정말 조심스럽게 진행되어야 한다고 생각했거든요.
예를 들면 모든 주문이 이루어진 이후의 주문 금액은 그 결과가 변경될 일이 없기 때문에 런타임 상황에서 혹여나 값이 변경될 가능성을 차단하고 싶었어요.
그런데, 이렇게 Money를 불변객체로 사용하는 순간 우리는 "금액이 변경되어야 하는" 상황에서 큰 오버헤드가 발생하게 되는데요.
이번 미션에서처럼 사용자의 투입 금액을 사용해 잔액이 감소해야 하는 상황에선 잔액이 감소할 때 마다 새로운 인스턴스를 생성해야 하기 때문이에요.
그래서 값의 변경이 필요한 부분에선 한정적으로 가변 클래스 Cash를 사용하자 라고 생각했습니다.
( 이 부분에 대해 불변객체와 쌍을 이루는 가변 객체를 제공하라는 글을 본 적이 있었습니다. String과 StringBuilder를 예시로 들고 있었는데, 찾아보니 Effective Java에서 나온 이야기인 것 같네요.
Effective Java를 아직 읽어보진 않아서 관련 아티클을 첨부해 드립니다.
https://jyami.tistory.com/79)
이제 우리는 금액과 관련된 관심사 중 값이 변해야 하는 상황에서만 한정적으로 Cash를 사용하고, 값의 변경이 끝났거나 변경될 필요가 없는 상황에선 Money 클래스를 사용할 수 있습니다.
이제 우리는 돈과 관련된, 같은 값을 가지는 두 개의 객체 Money와 Cash를 가지고 있습니다.
그런데, 결국 두 클래스는 다른 클래스이지만 같은 관심사를 가지고 있기에 두 클래스를 서로 비교를 하는 것은 매우 자연스러운 상황입니다.
때문에 두 클래스를 같은 인터페이스 Spendable을 구현하도록 만들어 주어 쉽게 두 객체를 비교하는 로직을 작성할 수 있게 되었습니다.
+) 추가로, 준기님께 질문을 받은 이후 제 코드를 조금 만져보다가 Spendable 인터페이스를 아래와 같이 사용할 수 있음을 알게 되었어요.
public interface Spendable extends Comparable<Spendable> {
@Override
default int compareTo(Spendable o){
return this.getPrice() - o.getPrice();
};
int getPrice();
}이 경우는 default 메서드의 사용이 필요한 경우라 위에서 해 주신 질문에 대해서도 적절한 답변이 될 것 같네요!!!
There was a problem hiding this comment.
@zangsu
아무래도 사용자의 입력과 자판기의 입력을 분리하고 반환 값이 같은 get메서드가 필요하기에 하나의 get메서드를 오버라이드 하는 방식을 사용하셨다고 이해했어요.
제대로 이해했다면, 의도는 정말 좋지만 효율적인 사용인지는 의문이 들어요. interface를 활용하는 가장 큰 장점은 확장성 이라고 생각하는데 Spendable을 통해 어떤 확장 대비할 수 있을까요?
간단하게 생각해 보면, 별도 디렉터리에 할당해서 3개 이상 선택 시 구매 가능. 같은 옵션을 넣어줄 수 있겠네요. 고민해 보면 좋을 것 같아 코멘트 남깁니다. 😄
추가로, Cash, Money 처럼 기능이 비슷하지만 다른 도메인 소속의 클래스는 별도의 영역에 분리하면 어떨까요?
There was a problem hiding this comment.
@Hugh-KR
음, 사용자와 자판기의 입력을 분리한다는 의도는 아니었습니다.
둘 다 공통적으로 "금액" 이라는 정보를 표현한다는 공통된 특징을 가지고 있어요.
다만 Money는 불변 객체, Cash는 가변 객체라는 점만 달라요. (String과 StringBuilder를 생각해 주시면 좋을 것 같습니다.)
결국 두 클래스는 모두 똑같은 값을 똑같은 형태로 나타내기 위한, 불변성에 대한 특징만 아주 조금 차이가 나지만 거의 같은 객체라고 생각했어요.
그리고 이 같은 책임을 가질 수 있는 객체들은 서로 비교가 가능해야 한다고 생각합니다.
이를 위해 두 클래스가 하나의 공통된 관심사를 가진다는 것을 나타내기 위해 Spendable 인터페이스를 구현하게 된 거에요.
이제 우리는 굳이 equals나 compareTo를 따로 오버라이딩 해 주지 않더라도 Cash cash와 Money money를 cash.compareTo(money)와 같이 비교할 수 있게 되었습니다!
Cash와 Money를 같은 패키지에 두게 된 것도 비슷한 이유입니다. 두 클래스는 논리적으로 거의 동일한 책임을 가지고 있기 때문에 package-private 접근 제어자에 접근할 수 있도록 만들어 주고 싶었어요.
| import vendingmachine.view.io.Reader; | ||
|
|
||
| public class InputView { | ||
| //regex = [글자,숫자,숫자] |
There was a problem hiding this comment.
[글자,숫자,숫자] 포맷 중 글자는 한글만 검증해도 된다는 생각이실까요? 영어 특수문자 등은 domain에서 검증이 이루어질테니까요!
제가 입력에 대한 검증을 너무 복잡하게 생각하는 것 같긴하네요,,
There was a problem hiding this comment.
앗, 저는 글자를 무조건 한글만 받도록 만든 것이 아래의 정규표현식입니다.
영어나 특수문자 등의 입력은 예외를 발생하도록 만들었어요!
도메인에서 이루어지는 이름에 대한 검증은 아마 빈 문자열에 대한 검증만 진행되지 싶어요.
| import vendingmachine.domain.coin.generator.CoinGenerator; | ||
| import vendingmachine.domain.money.Money; | ||
| import vendingmachine.exception.VendingMachineException; | ||
|
|
There was a problem hiding this comment.
제 코드를 리마인드하며 새로운 궁금증이 생겨 추가로 코멘트 남깁니다.
제 코드는 Coins을 생성하기 위해 Service -> CoinChanger를 거칩니다.
CoinChanger에서
- 보유금액으로 동전을 교환할 수 있는지 검증하고
- 보유 금액에 대한
EnumMap<Coin, Integer>를 생성하여 반환합니다. - Coins의 생성자 파라미터는
EnumMap<Coin, Integer>만을 받음으로써 결과적으로
public class Coins {
private final EnumMap<Coin, Integer> coinCounts;
public Coins(EnumMap<Coin, Integer> coinCounts) {
this.coinCounts = coinCounts;
}
// something..와 같은 형태를 띄고 있습니다.
제 구현 방식의 장점은
- Domain 객체가 갖고 있는 public API에 대해, 테스트하기 굉장히 용이하다고 느껴졌습니다. (생성자 파라미터가 EnumMap만을 인자로 받고 있기 때문입니다.)
Coins의 핵심 역할인남아있는 금액을 동전으로 거슬러준다는 꽤나 까다로운 구현이 요구되었고, 혁수님과 비슷한 방식으로 2중 Loop문에 무한 반복까지 발생 가능한 형태가 되어, 반드시 테스트가 필요한 로직이었습니다.- 여기서 1번이 큰 강점으로 다가왔습니다.
단점으로는
보유금액으로 동전을 교환할 수 있는지 검증하는 로직이 서비스 계층 (CoinChangerImpl)로 위치하게 되어, 다른 개발자가 봤을 때 검증로직을 추적하기 어려울 수도 있겠다는 생각이 들었습니다.- Coins 객체에서 Coins생성 (
EnumMap<Coin, Integer>) 에 대한 책임을 지지 않는 것이 좋은 코드인지는 잘 모르겠네요.
혁수님의 구현 방식은 Coins에서 CoinsGenerator를 호출하여 생성하는 로직이다 보니, 위에 열거드린 제 구현 방식의 장 / 단점이 뒤바뀌어진다고 느껴졌습니다.
야구 게임 미션과 같이 Generator 로직이 간단한 경우 아래와 같이
// Domain
public class BaseballCollection {
private final List<Integer> baseball;
public static BaseballCollection ofComputerBaseball(NumberGenerator numberGenerator) {
return new BaseballCollection(numberGenerator);
}
// something
}
// Test Code
@Test
@DisplayName("[성공 테스트] 3스트라이크")
void getHintSuccessTest1() {
// given
List<Integer> computerNums = Lists.newArrayList(1, 3, 7);
// BaseballCollection 내부에서 Generator를 통해 생성하지만, 테스트에 큰 지장 없음
BaseballCollection computerBalls = BaseballCollection.ofComputerBaseball(() -> computerNums.remove(0));
// something test테스트 코드에는 큰 지장이 없어서, 지금까지 생각 못했던 부분인 것 같습니다.
하지만 자판기 미션은 Generator로직이 꽤나 복잡하여, Coins객체에서 Generator를 호출하여 생성하는 방식은 테스트하기 어려울 수 있겠다는 생각도 들었습니다 🤔
쓰다보니 내용이 길어져,, 요약 드리면
- Domain 객체는 최대한 순수하게 유지한다. (검증 / public API / 내부 구현만 존재)
- Domain 객체를 생성하기 위한
Generator를 통해 생성한다. 그 외 1번과 동일
두 가지로 나뉘는 것 같고, 이 부분에 대해 혁수님의 생각이 궁금합니다 🤔🤔🤔
There was a problem hiding this comment.
오, 이 부분에 대해서 구현할 때 깊이 생각해 보지 않은 것 같은데요.
결국 변경될 수 있는 객체 생성 방식에 대해 이미 생성한 객체를 파라미터로 전달 받느냐 VS 객체 생성 전략 자체를 파라미터로 전달 받아 내부에서 값을 생성하느냐 의 차이가 되겠네요.
준기님이 말씀해 주신 테스트 관점에서 먼저 생각을 해 봤어요.
준기님의 경우 테스트 결과를 확인하기 위해 단순히 CoinGenerator가 생성해 줬을 EnumMap만을 전달해 주면 되는 반면, 저는 EnumMap의 결과를 반환하는 CoinGenerator 클래스를 별도로 구현해 주어야 했겠네요.
확실히 테스트 하기 좋은 코드가 될 것 같아요.
그리고, 준기님의 코드에서 검증 로직을 찾는 것도 그렇게 어렵진 않았고요.
그리고, 마지막으로 Coins 객체가 객체 생성에 대한 책임을 가져야 하는가? (must) 라고 묻는다면 저는 아니다. 라고 대답할 것 같습니다. 우리가 작성한 Coins는 결국 EnumMap을 래핑한 일급 컬렉션의 역할을 잘 수행하는 것이 가장 중요하다고 생각해요.
Coins의 책임은 외부에 Coin와 관련된 API를 제공해 주는 것이지, 객체 생성은 어떤 식으로 진행하든 문제는 없을 것 같은데요!
제 코드를 리팩토링 한다면 Coin을 생성하는 로직을 조금 더 밖으로 분리해 낼 것 같습니다.
준기님의 구현처럼요!
There was a problem hiding this comment.
@june-777
저는 거스름돈을 거슬러 주는 자판기, 거스름돈을 받는 사용자로 큰 틀의 도메인을 분리했어요. 각자 돈을 담을 수 있는 주머니 혹은 지갑이 있다고 가정하고 Coins 객체를 각각 할당했습니다.
자판기에서 거슬러진 돈을 바로 사용자에게 할당하는 방식으로 구현했고, 조금 더 단순하게 접근할 수 있었던 부분이 장점이라고 여겨져요.
| private final InputView inputView = new InputView(); | ||
| private final OutputView outputView = new OutputView(); | ||
| private final RetryHandler handler = new RetryHandler(); |
There was a problem hiding this comment.
@zangsu
해당 객체들을 주입받아서 가지고 오는 방법은 어떻게 생각하시나요?
There was a problem hiding this comment.
저 역시 인스턴스 필드로 가지고 있는 부분들을 생성 시점에서 의존성 주입을 받는게 좋을까? 라는 고민을 했었습니다!
그리고, 실제로 2주차였나 3주차 프리코스 미션을 수행할 때는 View의 부분을 추상화 하여 의존성을 주입받도록 만들어 보기도 하였고요.
사실, 우리가 항상 스터디때 말하는 것 처럼 "정답은 없"겠지만, 그래도 정답에 가까운 부분은 추상화 제공 후 의존성을 주입받는 것이라 생각해요.
아시는 바와 같이, 그리고 창혁님이 평소 의도하시는 바와 같이 혹시나 해당 부분에 다양한 변경 구현체가 필요할 때 손쉽게 구현 클래스를 갈아끼울 수 있다는 장점이 있죠. 그래서 창혁님의 방법처럼 AppConfig를 사용하는 것은 정말 좋아보입니다.
다만, 저는 이번 최종 코딩테스트를 진행하는 동안 "코딩테스트 5시간동안 변경이 되지 않을 부분들"에 대한 추상화 및 의존성 주입은 과감하게 타협하기로 하였습니다.
우리가 추상화를 제공하고 의존성을 주입받도록 하는 것은 어디까지나 "이후의 확장성"을 고려한 부분이기도 하고, 추후 확장 가능성이 생겼을 때 해당 부분에 추상화를 제공하도록 리팩토링 하는 것은 어려운 작업은 아니라고 생각했거든요.
그리고, 무엇보다도 제가 AppConfig를 사용해 의존성을 주입받는 방법을 많이 사용해 보지 않다 보니 의존성을 주입받는 코드를 작성하는게 조금 어색하기도 하고요.
간단하게 줄이자면, 해당 방법은 정말 좋아 보이지만, 이 부분을 타협하는 대신 다른 부분을 더 신경쓰기로 했다고 이해해 주시면 좋을 것 같습니다!
| private void purchase(VendingMachine vendingMachine) { | ||
|
|
||
| Cash remainCash = handler.getOrRetry(this::insertCash); | ||
|
|
||
| while (vendingMachine.canPurchase(remainCash)) { | ||
| handler.runOrRetry(() -> { | ||
| purchaseMenu(vendingMachine, remainCash); | ||
| }); | ||
| } | ||
| returnCoin(vendingMachine, remainCash.toMoney()); | ||
| } |
There was a problem hiding this comment.
직관적이고 반복문의 사용을 최소화한 로직으로 보입니다. 좋네요. 😄😄
| this.coins = coins; | ||
| this.menus = menus; | ||
| } | ||
|
|
There was a problem hiding this comment.
@june-777
준기님 의견에 동의합니다. 실제 자판기를 가정하면 저희가 인지하는 부분은 버튼을 누르면 음료가 나온다. 뿐이니까요.
menu라는 객체를 생성했다면 구매할 수 있는지 검증은 메뉴의 책임이라고 생각합니다.
| public static Coin from(int amount) { | ||
| return Optional.ofNullable(coins.get(amount)) | ||
| .orElseThrow(VendingMachineException.INVALID_COIN_PRICE::makeException); | ||
| } |
| List<Integer> coinAmounts = Arrays.stream(Coin.values()) | ||
| .map(Coin::getAmount) | ||
| .toList(); |
There was a problem hiding this comment.
구현방식이 신기해서 잠깐 고민했습니다. 테스트를 고려하면 지금처럼 분리된 방식이 현명할 듯 하네요.
| import vendingmachine.domain.coin.generator.CoinGenerator; | ||
| import vendingmachine.domain.money.Money; | ||
| import vendingmachine.exception.VendingMachineException; | ||
|
|
There was a problem hiding this comment.
@june-777
저는 거스름돈을 거슬러 주는 자판기, 거스름돈을 받는 사용자로 큰 틀의 도메인을 분리했어요. 각자 돈을 담을 수 있는 주머니 혹은 지갑이 있다고 가정하고 Coins 객체를 각각 할당했습니다.
자판기에서 거슬러진 돈을 바로 사용자에게 할당하는 방식으로 구현했고, 조금 더 단순하게 접근할 수 있었던 부분이 장점이라고 여겨져요.
| package vendingmachine.domain.money; | ||
|
|
||
| import vendingmachine.exception.VendingMachineException; | ||
|
|
There was a problem hiding this comment.
@zangsu
아무래도 사용자의 입력과 자판기의 입력을 분리하고 반환 값이 같은 get메서드가 필요하기에 하나의 get메서드를 오버라이드 하는 방식을 사용하셨다고 이해했어요.
제대로 이해했다면, 의도는 정말 좋지만 효율적인 사용인지는 의문이 들어요. interface를 활용하는 가장 큰 장점은 확장성 이라고 생각하는데 Spendable을 통해 어떤 확장 대비할 수 있을까요?
간단하게 생각해 보면, 별도 디렉터리에 할당해서 3개 이상 선택 시 구매 가능. 같은 옵션을 넣어줄 수 있겠네요. 고민해 보면 좋을 것 같아 코멘트 남깁니다. 😄
추가로, Cash, Money 처럼 기능이 비슷하지만 다른 도메인 소속의 클래스는 별도의 영역에 분리하면 어떨까요?
| //IO | ||
| INVALID_INPUT_FORMAT("잘못된 입력 형식입니다."), | ||
| INVALID_NUMBER_FORMAT("숫자를 입력해 주세요"), |
| public int getMachineMoney() { | ||
| printer.printMessage("자판기가 보유하고 있는 금액을 입력해 주세요."); | ||
| return reader.getInteger(); | ||
| } | ||
|
|
||
| public List<String> getMenus() { | ||
| printer.printMessage("상품명과 가격, 수량을 입력해 주세요."); | ||
| List<String> menus = reader.getStringsWithDelimiter(";"); | ||
| validateMenuFormat(menus); | ||
| return menus; | ||
| } |
There was a problem hiding this comment.
자판기가 보유하고 있는 금액을 입력해 주세요., 상품명과 가격, 수량을 입력해 주세요. 에러 메시지처럼 상수로 관리하는데 확실한 이점을 가진다고 생각합니다. 변할 수 있는 값이니까요.
There was a problem hiding this comment.
맞아요...! 아마 시간이 충분했다면 이 부분을 InputView 내부의 inner enum으로 관리하지 않았을까 싶습니다!
어쩌면 모든 출력 메시지, 에러 메시지들은 구현 시작과 동시에 바로 enum으로 생성하고 시작할 수도 있을 것 같고요!
이 부분은 아직 전략을 세우지 못했네요 ㅎㅎ,,,
| public void printMachineCoin(Coins coins) { | ||
| printer.printMessageUsingFormat(""" | ||
|
|
||
| 자판기가 보유한 동전 | ||
| 500원 - %d개 | ||
| 100원 - %d개 | ||
| 50원 - %d개 | ||
| 10원 - %d개""", | ||
| coins.getCoinCount(Coin.COIN_500), | ||
| coins.getCoinCount(Coin.COIN_100), | ||
| coins.getCoinCount(Coin.COIN_50), | ||
| coins.getCoinCount(Coin.COIN_10)); | ||
| } |
There was a problem hiding this comment.
@zangsu
굉장히 간편한 방식이지만, Coins클래스에서 반환할 잔돈을 계산하고있고 CoinGenerator클래스에서 초기 보유 동전을 계산하고 있으니까, 해당 부분은 생략하고 아래의 printReturnCoins메서드 하나로 출력이 가능할 듯 합니다.
물론 저도 중복된 부분이 있어요.🥲 최대한 빠르게 리팩토링 하려고 합니다.
There was a problem hiding this comment.
동의합니다! 이 부분도 구현 초기에 작성한 코드로 기억하는데 구현이 완성될 때 까지 얼마나 걸릴지를 어림잡기 힘들어 최대한 빠르게 구현하고 넘어가기 위해 이런 방식을 선택한 것으로 기억하네요 ㅎㅎ
| this.coins = coins; | ||
| this.menus = menus; | ||
| } | ||
|
|
There was a problem hiding this comment.
위에서 언급해 주신 네이밍과 관련된 내용은 저 역시 구현하면서 많이 아쉬웠던 부분이에요.
다만 보시는 것 처럼 VendingMachine이 구매 가능 여부를 알 수 있는 방법은 menus에게 물어보는 방법밖에 없기에 menus에 똑같은 질문을 다시 전달해 주는 느낌인데요. 이 상황에서 어떤 식으로 네이밍을 구분해 줄 수 있을 지 고민이네요.
지금의 제 생각으론 제 코드를 리팩토링 한다고 해도 canPurchase() 로직은 지금의 형태에서 크게 바꿀 것 같진 않아요.
저의 코드에선 상품을 Cash로 구매할 수 있을 지 확인하기 위해선 각각의 판매 항목들을 순회하면서 하나라도 구매 가능한 항목이 존재하는지를 확인해야 해요.
그리고 이 각각의 항목들은 List<>를 래핑한 일급 컬렉션 Menus가 가지고 있고요.
Menus를 필드 값으로 가지고 있는 VendingMachine이 구매 가능 여부를 알 수 있는 방법은 Menus에게 구매 가능 여부를 물어보는 방법 이외엔 없다고 생각합니다.
더군다나, Menus.canPurchase()를 호출하는 행위 자체가 해당 행동을 위한 필요 API를 Menus가 적절히 제공해 주고 있는 것이라고 생각했어요.
다만, 이렇게 생각한 이유는 제가 제 코드를 많이 보면서 어느 정도 제 코드에 가스라이팅 당한 것도 있다고 생각하기에 준기님의 생각이 다르다면 얼마든지 편하게 말씀해 주시면 좋을 것 같습니다 ㅎㅎㅎ
| this.amount = amount; | ||
| } | ||
|
|
||
| // 추가 기능 구현 |
There was a problem hiding this comment.
저도 해당 기준이 적절하다고 생각합니다!! 좋은 것 같네요 👍 ㅎㅎ
| import vendingmachine.domain.coin.generator.CoinGenerator; | ||
| import vendingmachine.domain.money.Money; | ||
| import vendingmachine.exception.VendingMachineException; | ||
|
|
There was a problem hiding this comment.
오, 이 부분에 대해서 구현할 때 깊이 생각해 보지 않은 것 같은데요.
결국 변경될 수 있는 객체 생성 방식에 대해 이미 생성한 객체를 파라미터로 전달 받느냐 VS 객체 생성 전략 자체를 파라미터로 전달 받아 내부에서 값을 생성하느냐 의 차이가 되겠네요.
준기님이 말씀해 주신 테스트 관점에서 먼저 생각을 해 봤어요.
준기님의 경우 테스트 결과를 확인하기 위해 단순히 CoinGenerator가 생성해 줬을 EnumMap만을 전달해 주면 되는 반면, 저는 EnumMap의 결과를 반환하는 CoinGenerator 클래스를 별도로 구현해 주어야 했겠네요.
확실히 테스트 하기 좋은 코드가 될 것 같아요.
그리고, 준기님의 코드에서 검증 로직을 찾는 것도 그렇게 어렵진 않았고요.
그리고, 마지막으로 Coins 객체가 객체 생성에 대한 책임을 가져야 하는가? (must) 라고 묻는다면 저는 아니다. 라고 대답할 것 같습니다. 우리가 작성한 Coins는 결국 EnumMap을 래핑한 일급 컬렉션의 역할을 잘 수행하는 것이 가장 중요하다고 생각해요.
Coins의 책임은 외부에 Coin와 관련된 API를 제공해 주는 것이지, 객체 생성은 어떤 식으로 진행하든 문제는 없을 것 같은데요!
제 코드를 리팩토링 한다면 Coin을 생성하는 로직을 조금 더 밖으로 분리해 낼 것 같습니다.
준기님의 구현처럼요!
| List<Integer> coinAmounts = Arrays.stream(Coin.values()) | ||
| .map(Coin::getAmount) | ||
| .toList(); | ||
|
|
There was a problem hiding this comment.
사실 이 부분은 default 메서드가 리팩토링 시점이거나, 구현 끝자락 즈음에 작성된 코드였던 것으로 기억하는데요.
default 부분을 작성한 이유는 모든 CoinGenerator가 코인을 선택하는 방법만 다르지, 결국 코인을 생성하는 전체 로직은 공통될 것이라고 생각했기 때문이에요.
그리고 이렇게 작성을 하는 와중에 "CoinGenerator는 인터페이스가 아니라 추상클래스로 제공되는 것이 맞았겠다" 라는 생각을 하게 되었습니다.
default 메서드로 구현하기 보단 그냥 부모 객체가 제공하는 공통 메서드 인 편이 더 자연스럽겠다고 생각했기 때문이에요.
저 역시 아직 default 메서드를 활용하는 기준은 세우지 못했을 뿐더러, 인터페이스에서의 default 메서드를 사용하는 부분은 많이 부족하다고 생각해요.
혹시 이 부분에서 인사이트를 얻고 싶으시다면 제 의도는 "추상 클래스"의 사용이었으나 시간의 한계로 인터페이스를 추상클래스로 변경하지 못했다는 점을 알아주셨으면 좋겠습니다!!
| } | ||
| return cash.canPurchase(this.price); | ||
| } | ||
|
|
There was a problem hiding this comment.
네 맞습니다!
특정 메서드는 호출되기 위한 전제 조건이 존재하는데요.
구매 로직이 호출되기 위해선 재고가 있어야 하는 것 처럼요!
이럴 때 저는 메서드 내부에서 전제조건을 위반하는 상태를 확인할 수 있다면 이를 확인하고 예외를 던져주려고 노력하고 있습니다.
이는 우리가 코드를 짤 때도 조금 더 안전한 코드를 작성할 수 있게 해주며 추후 협업을 진행할 때 팀원이 놓칠 수 있는 실수를 예방해 주기도 한다고 생각하기 때문이에요!!
| import vendingmachine.view.io.Reader; | ||
|
|
||
| public class InputView { | ||
| //regex = [글자,숫자,숫자] |
There was a problem hiding this comment.
앗, 저는 글자를 무조건 한글만 받도록 만든 것이 아래의 정규표현식입니다.
영어나 특수문자 등의 입력은 예외를 발생하도록 만들었어요!
도메인에서 이루어지는 이름에 대한 검증은 아마 빈 문자열에 대한 검증만 진행되지 싶어요.
| package vendingmachine.domain.money; | ||
|
|
||
| import vendingmachine.exception.VendingMachineException; | ||
|
|
There was a problem hiding this comment.
@Hugh-KR
음, 사용자와 자판기의 입력을 분리한다는 의도는 아니었습니다.
둘 다 공통적으로 "금액" 이라는 정보를 표현한다는 공통된 특징을 가지고 있어요.
다만 Money는 불변 객체, Cash는 가변 객체라는 점만 달라요. (String과 StringBuilder를 생각해 주시면 좋을 것 같습니다.)
결국 두 클래스는 모두 똑같은 값을 똑같은 형태로 나타내기 위한, 불변성에 대한 특징만 아주 조금 차이가 나지만 거의 같은 객체라고 생각했어요.
그리고 이 같은 책임을 가질 수 있는 객체들은 서로 비교가 가능해야 한다고 생각합니다.
이를 위해 두 클래스가 하나의 공통된 관심사를 가진다는 것을 나타내기 위해 Spendable 인터페이스를 구현하게 된 거에요.
이제 우리는 굳이 equals나 compareTo를 따로 오버라이딩 해 주지 않더라도 Cash cash와 Money money를 cash.compareTo(money)와 같이 비교할 수 있게 되었습니다!
Cash와 Money를 같은 패키지에 두게 된 것도 비슷한 이유입니다. 두 클래스는 논리적으로 거의 동일한 책임을 가지고 있기 때문에 package-private 접근 제어자에 접근할 수 있도록 만들어 주고 싶었어요.
| private final InputView inputView = new InputView(); | ||
| private final OutputView outputView = new OutputView(); | ||
| private final RetryHandler handler = new RetryHandler(); |
There was a problem hiding this comment.
저 역시 인스턴스 필드로 가지고 있는 부분들을 생성 시점에서 의존성 주입을 받는게 좋을까? 라는 고민을 했었습니다!
그리고, 실제로 2주차였나 3주차 프리코스 미션을 수행할 때는 View의 부분을 추상화 하여 의존성을 주입받도록 만들어 보기도 하였고요.
사실, 우리가 항상 스터디때 말하는 것 처럼 "정답은 없"겠지만, 그래도 정답에 가까운 부분은 추상화 제공 후 의존성을 주입받는 것이라 생각해요.
아시는 바와 같이, 그리고 창혁님이 평소 의도하시는 바와 같이 혹시나 해당 부분에 다양한 변경 구현체가 필요할 때 손쉽게 구현 클래스를 갈아끼울 수 있다는 장점이 있죠. 그래서 창혁님의 방법처럼 AppConfig를 사용하는 것은 정말 좋아보입니다.
다만, 저는 이번 최종 코딩테스트를 진행하는 동안 "코딩테스트 5시간동안 변경이 되지 않을 부분들"에 대한 추상화 및 의존성 주입은 과감하게 타협하기로 하였습니다.
우리가 추상화를 제공하고 의존성을 주입받도록 하는 것은 어디까지나 "이후의 확장성"을 고려한 부분이기도 하고, 추후 확장 가능성이 생겼을 때 해당 부분에 추상화를 제공하도록 리팩토링 하는 것은 어려운 작업은 아니라고 생각했거든요.
그리고, 무엇보다도 제가 AppConfig를 사용해 의존성을 주입받는 방법을 많이 사용해 보지 않다 보니 의존성을 주입받는 코드를 작성하는게 조금 어색하기도 하고요.
간단하게 줄이자면, 해당 방법은 정말 좋아 보이지만, 이 부분을 타협하는 대신 다른 부분을 더 신경쓰기로 했다고 이해해 주시면 좋을 것 같습니다!
| public int getMachineMoney() { | ||
| printer.printMessage("자판기가 보유하고 있는 금액을 입력해 주세요."); | ||
| return reader.getInteger(); | ||
| } | ||
|
|
||
| public List<String> getMenus() { | ||
| printer.printMessage("상품명과 가격, 수량을 입력해 주세요."); | ||
| List<String> menus = reader.getStringsWithDelimiter(";"); | ||
| validateMenuFormat(menus); | ||
| return menus; | ||
| } |
There was a problem hiding this comment.
맞아요...! 아마 시간이 충분했다면 이 부분을 InputView 내부의 inner enum으로 관리하지 않았을까 싶습니다!
어쩌면 모든 출력 메시지, 에러 메시지들은 구현 시작과 동시에 바로 enum으로 생성하고 시작할 수도 있을 것 같고요!
이 부분은 아직 전략을 세우지 못했네요 ㅎㅎ,,,
| public void printMachineCoin(Coins coins) { | ||
| printer.printMessageUsingFormat(""" | ||
|
|
||
| 자판기가 보유한 동전 | ||
| 500원 - %d개 | ||
| 100원 - %d개 | ||
| 50원 - %d개 | ||
| 10원 - %d개""", | ||
| coins.getCoinCount(Coin.COIN_500), | ||
| coins.getCoinCount(Coin.COIN_100), | ||
| coins.getCoinCount(Coin.COIN_50), | ||
| coins.getCoinCount(Coin.COIN_10)); | ||
| } |
There was a problem hiding this comment.
동의합니다! 이 부분도 구현 초기에 작성한 코드로 기억하는데 구현이 완성될 때 까지 얼마나 걸릴지를 어림잡기 힘들어 최대한 빠르게 구현하고 넘어가기 위해 이런 방식을 선택한 것으로 기억하네요 ㅎㅎ
No description provided.