[1단계 - 자판기] 밧드(감우영) 미션 제출합니다. #22

kamwoo · 2022-03-26T16:48:30Z

안녕하세요! 🙂 아래 데모페이지 링크입니다.
데모페이지

구조

vendingMachine은 상품과 돈을 저장, 삭제, 반환하는 역할을 하고, 각 페이지에 해당하는 UI를 담당하는 뷰들과 도메인으로 분리했습니다.
AppView에서는 상단의 네비게이션과 페이지를 렌더링할 빈 content 태그의 렌더하고, 네비게이션 탭 버튼 클릭 이벤트를 보내는 역할을 합니다. 그리고 AppController에서는 route와 탭 이벤트를 수신하여 라우팅하고 각 페이지뷰를 렌더하는 메소드를 실행시키는 역할을 합니다.

질문

이벤트가 발생할 때 window에서 dispatch를 시켜주고 도메인쪽에서 window를 통해 이벤트를 감지하는데요. 지금은 하나의 앱만 실행되서 window로 잡는 것과 앱 영역으로 잡는 것의 차이가 크지 않다고 생각했습니다. custom event를 사용해본 이유는 도메인과 UI로직을 더 분리시켜줄 수 있다고 생각했습니다. 그리고 다른 도메인에서도 직접적으로 뷰에 접근해서 연결시키지 않아도 쉽게 이벤트에 대한 액션을 할 수 있다고 생각했습니다.
사용해보면서 궁금했던 점은 앱 영역에서 이벤트를 발생시켰을 때, 이벤트 이름만 겹치지 않으면 상관없다고 생각하는데 다른 문제점이 있는지 그리고 이벤트를 dispatch하는 곳을 element로 한정시키는게 좋은지 궁금합니다.

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

- 삭제 버튼 클릭 시 확인 - 라우트 시 버튼 색 변경 - 예외처리 수정 Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

- 타입스크립트 타입 지정 - 상수화 - 메서드 분리 Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

EungyuCho

안녕하세요 밧드님 🙂

구조까지 코멘트에 적어주셔서 리뷰하는데 도움이 많이 됐어요. 코드가 일관성있게 작성되어있어서 좋았어요.

질문

이벤트를 dispatch하는 곳을 element로 한정시키는게 좋은지 궁금합니다.

이벤트를 상황따라 다를거같아요. window보다 Element 기준으로 한정시는편이 좋다고 생각하지만 현재처럼 전역적으로 이벤트를 걸 경우에는 window에 걸어도 무방하다고 생각되네요. 🙂

사용해보면서 궁금했던 점은 앱 영역에서 이벤트를 발생시켰을 때 이벤트 이름만 겹치지 않으면 상관없다고 생각하는데

말씀해주신대로 이벤트 이름만 안겹치면 된다고 생각해요. 추가적으로 생각나는 주의할만한 부분이 떠오르진않네요 🤔

피드백

figma시안에 맞게 구현해주셨고 상품 수정을 할 때 input요소에 outline이 적용되어서 수정을 할 수 있다는 느낌을 바로 받았어요.
각 view별로 템플릿을 분리해주신점이 인상적이었어요. 좋은 방향성이라고 생각합니다.
도메인을 조금 더 세분화해서 나눠보면 좋겠다고 생각했어요. 관련해서 코멘트에 남겨드렸어요.

나머지 코멘트들도 참고해주셔서 개선해보면 좋을거같아요. 요구사항 구현하시느라 고생하셨어요!
다음 리뷰요청때 봬요~

EungyuCho · 2022-03-27T06:04:21Z

src/js/controllers/AppController.ts

+  handleRouteChange(event: CustomEvent) {
+    const { $navButton }: RouteChangeDetailType = event.detail;
+
+    if ($navButton.id === SELECTOR.ID_STRING.ITEM_MANGE_TAB) {
+      window.history.pushState(null, null, URL.MANAGE_ITEM);
+    }
+
+    if ($navButton.id === SELECTOR.ID_STRING.MONEY_CHARGE_TAB) {
+      window.history.pushState(null, null, URL.CHARGE_MONEY);
+    }
+
+    if ($navButton.id === SELECTOR.ID_STRING.ITEM_PURCHASE_TAB) {
+      window.history.pushState(null, null, URL.PURCHASE_ITEM);
+    }
+
+    this.route();
+  }
+
+  route() {
+    const { pathname } = window.location;
+
+    if (pathname === `${URL.BASE_URL}/`) {
+      this.manageItemController.loadPage();
+      this.appView.changeButtonColor(SELECTOR.ID_STRING.ITEM_MANGE_TAB);
+      return;
+    }
+    if (pathname === `${URL.BASE_URL}/${URL.MANAGE_ITEM}`) {
+      this.manageItemController.loadPage();
+      this.appView.changeButtonColor(SELECTOR.ID_STRING.ITEM_MANGE_TAB);
+      return;
+    }
+    if (pathname === `${URL.BASE_URL}/${URL.CHARGE_MONEY}`) {
+      this.chargeMoneyController.loadPage();
+      this.appView.changeButtonColor(SELECTOR.ID_STRING.MONEY_CHARGE_TAB);
+      return;
+    }
+    if (pathname === `${URL.BASE_URL}/${URL.PURCHASE_ITEM}`) {
+      this.purchaseItemController.render();
+      this.appView.changeButtonColor(SELECTOR.ID_STRING.ITEM_PURCHASE_TAB);
+    }
+  }


switch문이 더 가독성이 좋을거같아요

넵 수정했습니다 d751ebd

EungyuCho · 2022-03-27T08:18:17Z

src/js/vendingMachine/vendingMachine.ts

+import { generateRandom } from '../utils/common';
+import { ItemType, CoinsType } from '../types/types';
+
+export default class VendingMachine {


다른 코멘트에서도 도메인을 만들어보시는걸 권해드렸지만 VendingMachine의 items을 도메인으로 변경해보면 좋을거같아요.
Manager를 생성하고 별도의 Product도 도메인으로 분리해보면 좋을거같아요

의도하신 것과 같을지 모르겠습니다.
vendingMachine <- itemManager <- item
_______________ <- coinManager

작성하면서 지금 요구 상황에서 이 정도로 분리하는게 과하지 않을까 생각을 했습니다. 요구사항이 많아질 때를 생각해서 처음부터 분리를 시키는게 좋을지 궁금합니다!
58f3b2d
2cfacab

제가 의도하던 방향성이 맞아요!
유효성 검증을 도메인의 역할과 책임에 추가해보면 적절한 범위라고 생각되어 드렸던 코멘트인데 새로운 방향성을 시도해보시는거라 기존 그대로 둬도 괜찮았을 것 같네요..

현재 검증부분과 요구사항의 범위만 보면 과한 느낌이 있는 것 같네요!..
작업을 할 때 개인적으로 현재 요구사항만 생각해서 구현하기보다 이후 추가 요구사항을 생각해서 작성을 하다보니 분리하는걸 추천드렸었어요 💦

아~ 그러면 안하는 것보다야 분리를 하는게 좋겠네요🙂 유효성 검증도 도메인 역할과 책임에 추가해보겠습니다!

EungyuCho · 2022-03-27T08:25:52Z

src/js/templates/chareMoneyTemplate.ts

@@ -0,0 +1,54 @@
+import { COINS } from '../constants/constants';
+import { CoinsType, TemplateType } from '../types/types';
+


template파일을 view마다 별도 파일로 만들어주셔서 깔끔하네요! 좋습니다 👍

EungyuCho · 2022-03-27T08:26:54Z

src/js/validates/validates.ts

+  if (price < ITEM.PRICE.MIN) {
+    throw new Error(ERROR_MESSAGE.ITEM_PRICE.UNDER_MIN);
+  }
+  if (price > ITEM.PRICE.MAX) {
+    throw new Error(ERROR_MESSAGE.ITEM_PRICE.OVER_MAX);
+  }
+  if (price % ITEM.PRICE.UNIT !== 0) {
+    throw new Error(ERROR_MESSAGE.ITEM_PRICE.INVALID_UNIT);
+  }
+
+  if (!Number.isInteger(quantity)) {
+    throw new Error(ERROR_MESSAGE.ITEM_QUANTITY.NOT_INTEGER);
+  }
+  if (quantity <= ITEM.QUANTITY.MIN) {
+    throw new Error(ERROR_MESSAGE.ITEM_QUANTITY.UNDER_MIN);
+  }
+  if (quantity > ITEM.QUANTITY.MAX) {
+    throw new Error(ERROR_MESSAGE.ITEM_QUANTITY.OVER_MAX);
+  }


input값을 검증할때 너무 많은 검증에 대한 책임이 있는거같아요.

도메인에서 검증해보시는건 어떨까요?
예를들어 Product라는 도메인이 있을 때 해당 도메인에서 price, quantity, name의 데이터을 생성자에서 검증하고 실패하면 지금처럼 예외를 throw하는식으로 작성하는 방식으로요.

도메인에 대한 데이터는 해당 도메인에서 검증하게 책임과 역할을 나눠주는게 좋다고 생각되어서 코멘트를 드렸어요.
그래서 validateAddItemInput에서는 input 값이 정상적인 데이터인지만 체크하는게 옳은 방향성이라고 생각이 되는데 밧드님의 의견을 여쭤보고 싶어요👀

개인적으로는 도메인에서 처리하는 것도 좋다고 생각하는데, 이 부분에서는 사람마다 달라서 매번 다르게 해보고 있습니다.
validate를 데이터가 정상적이지 않을 때도 했던 이유는 처음부터 어떻게 입력해라는 말이 없어서, 에러 메세지로 사용자가 어떤 부분을 잘못 입력했는지 알려줄 수 있다고 생각했습니다.
5cdf4ed

오.. 기존에 하던대로 하는 방법보다 여러 시도를 해보시는게 좋다고 생각되네요!
제가 너무 틀에 같혀 있었던거 같아요. 미션인만큼 여러 방향성을 직접 시도해보시는게 좋다고 생각해요!
좋습니다! 🙂

EungyuCho · 2022-03-27T08:32:34Z

src/js/vendingMachine/vendingMachine.ts

+  setCoins(newCoins: CoinsType) {
+    this.coins = newCoins;
+  }
+
+  getInputMoney(): number {
+    return this.inputMoney;
+  }
+
+  setInputMoney(inputMoney: number) {
+    this.inputMoney = inputMoney;
+  }


inputMoney, coins는 private로 관리되고 있는데 외부에 노출하지 않는게 좋지 않을까요?
외부에는 addItem, changeItem, deleteItem, chargeMoney만 노출되는게 맞다고 생각이들어요

어떤 의도이신지 정확히 파악하지 못했습니다. 😭 먼저 addItem, changeItem, deleteItem, chargeMoney를 제외하고 기존의 get, set 메소드는 제거를 했습니다.

inputMoney, coins는 private로 관리되고 있는데 외부에 노출하지 않는게 좋지 않을까요?

이 부분은 노출하지 않고, 데이터를 어떻게 가져다 쓸 수 있는지 잘모르겠습니다. 찾아보고 타입스크립트에서 get을 붙여서 쓰는 것을 적용했습니다.
b8d67d3

죄송해요 밧드님 제가 피드백이 자세하게 설명 못드린거같아요. 😢

설명드리고자 했던 바는 외부에 필요한 메서드만 노출해서 불필요한 내부 데이터 변경 메서드를 노출하지 않게 수정해보면 좋겠어서 드린 코멘트였어요. 예를들어

setItems는 addItem, changeItem, deleteItem에서만 사용하고 외부에 노출되지 않으므로 private로 관리한다

setInputMoney는 chargeMoney에서만 사용되므로 private로 관리한다 (setCoins도 동일한 맥락이다.)

위 setCoins, setInputMoney가 별도로 존재하므로 외부에서 돈만 추가하거나 코인의 개수만 수정해서 내부의 데이터가 일치하지 않을 가능성이 있다(코인가격의 합과 내부 inputMoney)가 불일치 할 가능성이 있음)

이러한 요소들로 코멘트를 드렸는데 지금봐도 불필요한 메서드에 대한 내용이 없었네요...
죄송합니다! 🙇

아! 아닙니다! 저가 어렵게만 생각을 했었네요. 전체적으로 외부로 노출하는 것 빼고 private을 추가해보겠습니다.

EungyuCho · 2022-03-27T12:07:55Z

src/js/vendingMachine/vendingMachine.ts

+export default class VendingMachine {
+  private items: ItemType[] = [];
+  private coins: CoinsType = { fiveHundred: 0, hundred: 0, fifty: 0, ten: 0 };
+  private inputMoney = 0;


Suggested change

private inputMoney = 0;

private money = 0;

money로 해도 되지않을까요??

넵 vendinMachine내부에서는 money를 사용하고, 입력해서 vendingMachine까지 들어갈 때까지는 inputMoney로 사용했습니다. 1c910dc

EungyuCho · 2022-03-27T12:09:07Z

src/js/controllers/chargeMoneyController.ts

+    this.vendingMachine = vendingMachine;
+    this.chargeMoneyView = new ChargeMoneyView();
+
+    this.bindEvents();


생성자에서 생성하는 이벤트를 별도의 메서드로 분리할 필요는 없어보여요

넵 수정했습니다! fc16c6d

EungyuCho · 2022-03-27T12:12:57Z

src/js/constants/constants.ts

+
+export const CONFIRM_MESSAGE = {
+  DELETE: '정말로 삭제하시겠습니까?',
+};
+
+export const ITEM = {
+  NAME: {
+    MAX_LENGTH: 10,
+  },
+  PRICE: {
+    MIN: 100,
+    MAX: 10000,
+    UNIT: 10,
+  },
+  QUANTITY: {
+    MIN: 0,
+    MAX: 20,
+  },
+};
+
+export const MONEY = {
+  MIN: 0,
+  MAX: 100000,
+  UNIT: 10,
+};


요구사항이 늘어나는걸 고려한다면 view나 domain 폴더별로 constants를 관리하는게 맞는 방향성이라고 생각이되요.

money나 item은 도메인에 종속되는 상수고 confirm message같은 경우에도 특정 view에 종속될 가능성이 높다고 생각해요.
템플릿을 분리해주신거처럼 상수영역에 대한 분리도 한번 생각해보시면 좋을거같아요.

도메인, 뷰에 관한 상수를 분리했고 에러, 앱의 대해서 추가로 분리했습니다. 764eb6a

EungyuCho · 2022-03-27T12:34:15Z

src/js/controllers/AppController.ts

+
+  handleRouteChange(event: CustomEvent) {
+    const { $navButton }: RouteChangeDetailType = event.detail;
+


항상 history api에 쌓이는거같아요.

상품 관리탭에서 잔돈충전을 2번 누르고 뒤로가면 상품관리 탭으로가는게 아니라 이전 history인 잔돈충전에 머물러있어요.
예외처리를 해봅시다!

탭을 누를 때 클릭 되어있으면 pushState를 하지 않는 방식으로 수정했습니다. 607b4c0

EungyuCho · 2022-03-27T13:19:16Z

src/js/views/mangeItemView.ts

+  }
+
+  handleTableItemDelete(item: ItemType) {
+    window.dispatchEvent(new CustomEvent(CUSTOM_EVENT.TABLE_ITEM_DELETE, { detail: { item } }));


Custom Event Dispatch는 함수로 분리해보면 좋을거같아요.
매번 이벤트를 dispach할때 Custom Event를 생성해야해서 아래처럼 분리하면 간편하게 재사용할 수 있을거같아요.

export const emit = ({ target = window , eventName, detail }) => { const event = new CustomEvent(eventName, { detail }); target.dispatchEvent(event); };

넵 적용했습니다! 가독성이 훨씬 좋네요🙂 30955d2

EungyuCho

안녕하세요 밧드님~ 반영해주신 부분 확인했습니다.

반영하느라 고생많으셨습니다.
익숙한 방법을 사용하시지 않고 다른 방법을 시도해보시고 경험해보려고 하시는점이 인상깊었어요! 🙂

추가적으로 코멘트를 남긴 부분이 있어 한번 더 확인부탁드릴게요!

EungyuCho · 2022-03-29T15:13:31Z

src/js/vendingMachine/vendingMachine.ts

+  private itemManager: ItemManager = new ItemManager();
+  private coinManager: CoinManager = new CoinManager();
+
+  get items(): Array<ItemType> {
+    return this.itemManager.items;
+  }
+
+  get coins(): CoinsType {
+    return this.coinManager.coins;
+  }
+
+  get money(): number {
+    return this.coinManager.money;
+  }


Suggested change

private itemManager: ItemManager = new ItemManager();

private coinManager: CoinManager = new CoinManager();

get items(): Array<ItemType> {

return this.itemManager.items;

}

get coins(): CoinsType {

return this.coinManager.coins;

}

get money(): number {

return this.coinManager.money;

}

private itemManager = new ItemManager(); // 클래스는 선언할 때 자동으로 변수에서 타입을 직접적으로 알 수 있어 명시해줄 필요가 없어요

private coinManager = new CoinManager();

get items() {

return this.itemManager.items; // 이 부분도 불필요합니다 🙂

}

get coins() {

return this.coinManager.coins;

}

get money() {

return this.coinManager.money;

}

타입스크립트에 대해서 거의 몰라서 궁금한 점이 있습니다! 타입을 명시하는게 마우스를 올려 보지 않아도 무엇을 받고, 리턴하는지 알려주면 코드를 읽으면서 이해하기 쉽다고 생각했는데 이런 부분은 굳이 생각하지 않아도 되는지 궁금합니다! 🙂
말해주신 부분은 수정했습니다 1c5ecd7

ItemManager, CoinManager 타입의 경우 결국 마우스를 올려봐야 알수있어서 크게 다른점이 없다고 생각해요.
원시 타입의 경우에는 유의미할수도 있겠다고 생각이 드네요 🤔

저는 작업할때 번거로운점도 있고 IDE상에서 타입을 따라가서 확인해보는데 익숙해지다보니 이런 부분에 대해 깊게 생각해보진 않았네요 🙂
케이스마다 다를거같아요! 네이밍이 타입을 유추하기 힘들 것 같다고 생각될때는 명시하는 편도 좋아보입니다 ㅎㅎ

EungyuCho · 2022-03-29T15:24:34Z

src/js/constants/errorConstants.ts

+    UNDER_MIN: '100원보다 낮은 가격은 입력할 수 없습니다.',
+    OVER_MAX: '10,000원 보다 큰 가격을 입력할 수 없습니다.',


100, 10000등 설정값에 의존하는 텍스트네요 👀

앗 constant로 설정한 값들로 수정했습니다! dd91d78

EungyuCho · 2022-03-29T15:28:16Z

src/js/controllers/chargeMoneyController.ts

+  handleChargeMoney(event: CustomEvent) {
+    const { inputMoney }: ChargeMoneyDetailType = event.detail;


Suggested change

handleChargeMoney(event: CustomEvent) {

const { inputMoney }: ChargeMoneyDetailType = event.detail;

handleChargeMoney(event: CustomEvent<ChargeMoneyDetailType>) {

const { inputMoney } = event.detail;

CustomEvent에 제네릭으로 넣어주면 event.detail이 넣어준 타입으로 받아줄 수 있어요.
제네릭을 지정안했을때의 기본값이 CustomEvent이기 때문에 동작은 하지만 any를 최소화하는걸 좋아해서 추천드려요.

혹시 변경하신다면 다른 CustomEvent에도 적용부탁드릴게요 🙂

타입 스크립트 공부 많이 해야겠네요 😭 컨트롤러 없애면서 다른 곳에도 적용할게 있는지 찾아보겠습니다!

EungyuCho · 2022-03-29T15:33:14Z

src/js/types/types.ts

+  item: ItemType;
+};
+
+export type TemplateType = string;


이 타입은 지정할 필요가 있었는지 의문이 드네요?.. 🤔

작성해주신 template는 모두 문자열을 리턴해서 타입 명시를 안해줘도 string 타입으로 인식하기때문에 타입을 만들필요도 명시할 필요도 없다고 생각돼요.

이 부분도 위에서 얘기한 것과 같이 단순히 string이 아니라 template라는 것을 알려줄 수 있다고 생각했습니다. 이제는 굳이 할 필요가 없는 것 같다는 생각이 드네요! 59a2425

EungyuCho · 2022-03-29T15:34:08Z

src/js/validates/validate.test.ts

+  });
+
+  test('돈이 100,000보다 크다면 에러를 throw한다.', () => {
+    const money = 100001;


설정에 있는 값을 이용하는게 좋은거같아요. (MONEY.MAX)

변경하신다면 나머지 상품에 관련해서도 변경해보면 좋을거같아요

넵 수정했습니다! 33dbf54

kamwoo · 2022-03-30T11:24:57Z

안녕하세요! 너무너무 좋은 피드백 감사합니다!
이전 피드백에서 이해하지 못했던 부분들을 다시 설명해주셔서 이번에 적용해봤습니다.

유효성 검증을 도메인의 역할과 책임에 추가해보면 적절한 범위라고 생각되어 드렸던 코멘트
Vending Machine을 직접 생성자로 받아서 처리하면 어떨까요?

두 피드백을 적용해보니깐 컨트롤러에 남아있는게 없었습니다🤔 불필요한 컨트롤러는 지워보니 아래와 같은 구조로 정리가 됐습니다.

수정하면서 궁금했던 점들 입니다.

vendingMachine을 뷰에서 직접받아서 사용하는데 vendingMachine을 사용하는

this.vendingMachine.changeItem(targetRowIndex, item);

이런 것들도 비즈니스 로직이라고 생각하는데, 수정된 뷰가 비즈니스 로직과 분리가 되어있는 것이 아니지 않을까하는 생각이 들었습니다. 단순히 사용하는 구문은 도메인 로직에 해당하지 않을까요?

위에서 연장되는 얘기인데, 개인적으로 컨트롤러만 봐도 어떤식으로 전개되는지 알 수 있게 하면 흐름을 파악하는데 편하지 않을까 생각해서, vendingMachine에서 데이터를 가져오고 뷰에 메소드를 이용하는 부분을 컨트롤러에서 보여주도록 하는데, 굳이 그럴 필요는 없을까요?

EungyuCho · 2022-03-30T16:11:43Z

수정된 뷰가 비즈니스 로직과 분리가 되어있는 것이 아니지 않을까하는 생각이 들었습니다. 단순히 사용하는 구문은 도메인 로직에 해당하지 않을까요?

네! 도메인 로직이 맞고 결합되어있는 상태가 맞습니다!

개인적으로 컨트롤러만 봐도 어떤식으로 전개되는지 알 수 있게 하면 흐름을 파악하는데 편하지 않을까 생각해서, vendingMachine에서 데이터를 가져오고 뷰에 메소드를 이용하는 부분을 컨트롤러에서 보여주도록 하는데, 굳이 그럴 필요는 없을까요?

말씀해주신대로 MVC가 처음에 흐름 파악하는데는 더 나을 수 있다고 생각이 들어요.
하지만 현재처럼 view에 model이 결합되어있어 작업속도가 더 빠르다고 생각하는데요.

controller에서 인자를 넣고 받을 view에서 해당 데이터와 타입을 명시해줘야하고... 이러한 과정들이 번잡하다보니 중간에 controller를 두기보다 직접 model을 참조시키는걸 선호해요 🙂

EungyuCho

밧드님 남겨주신 피드백 모두 확인했습니다.

반영하시느라 정말 고생하셨어요! 저도 피드백을 주고 받으면서 많은 생각을 하게돼서 좋았습니다 🙂
혹 궁금한점이 남아있으시다면 코멘트 주시면 감사하겠습니다~~

Approve하겠습니다. 스텝 2에서 뵐게요!~

kamwoo · 2022-03-30T18:08:53Z

추가로 궁금한 점이 있습니다!

네! 도메인 로직이 맞고 결합되어있는 상태가 맞습니다!

MVC를 사용한 이유가 ui로직이랑 도메인 로직을 분리하는게 구조를 만들 때 가장 중심적으로 생각을 했고 필요성을 느껴서 입니다!
분리를 하는 이유가 UI적인 기능들을 변경할 때 도메인 로직이 섞여있으면 수정할 때 많은 부분을 신경써야 된다고 생각합니다.
뷰 모듈 하나는 해당하는 UI적인 요소들을 동적으로 보여주고, 이벤트를 넘겨주는 역할로 충분하다고 생각하는데, vendingMachine에 데이터를 가져오고 넘겨주는 역할까지 해서 하는 일이 많아진게 아닌가 생각도 들었습니다.

지금은 단순히 vendingMachine.addItem()정도만 사용하고 있는데, 컨트롤러에 들어갔어야될 로직들이 많았다면 어떻게 되었을지 궁금합니다! 아니면 지금 이정도 도메인 로직은 괜찮다는 것인지도 궁금합니다.

고민이 많이 되는게 사실 요구사항이 늘어나도 지금 구조나 코드 상으로도 아무런 문제가 없고, 없을 것 같아서 입니다.😭 UI로직과 도메인 로직은 분리하기 위해서 컨트롤러를 쓴다는게 잘못된 생각이었나 해서 질문을 추가로 드렸습니다.

EungyuCho · 2022-03-31T13:50:26Z

지금 그대로 사용해도 무방해보여요.

컨트롤러에 들어갔어야될 로직들이 많았다면 어떻게 되었을지 궁금합니다! 아니면 지금 이정도 도메인 로직은 괜찮다는 것인지도 궁금합니다.

컨트롤러에 들어가는 로직이 복잡해진다는 경우를 말씀해주셔서 2가지 케이스를 생각해봤는데요.

연관성이 있는 도메인 로직이 많아질 경우
연관성 없는 도메인 로직이 많아질 경우

1번의 경우에는 추상화가 가능하다고 판단될 경우 연관된 도메인을 추상화해서 해결해볼 수 있을거같아요.
2번의 경우나 1번의 케이스에서 추상화를 하면 코드를 파악하는데 더 힘들어질 것 같은 경우에는 코드가 길어지더라도 도메인 로직을 그대로 사용해주는게 맞다고 생각해요.

도메인 로직이 정말 많아질 경우 도메인 로직만 묶어서 view 내부에서 함수를 분리해볼 수 있을수도 있을거같구요!
결론으로 가자면.. UI로직과 도메인 로직이 저는 함께 쓰여져도 무방하다고 생각해요. 지금은 로직 프로세스가 모여있는게 더 유지보수가 편할 것 같다고 생각이드네요 🙂

onschan and others added 27 commits March 22, 2022 16:47

docs: 기능 구현 목록 정리

487985e

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

chore: eslint + prettier 설정

afe895f

feat: 상품을 추가하고 상품의 정보를 수정할 수 있다.

d173d46

feat: 최초 자판기가 가지고있는 각 동전의 개수는 0개이다.

fc2a3af

feat: 자판기 보유 금액을 충전할 수 있다.

6b3eee8

chore: 디렉토리 구조 변경

4190492

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

design: 상품 관리 영역 UI

2031974

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

design: 잔돈 충전 영역 UI

3326144

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

design: 상품 구매 영역 UI

71ed881

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 디렉토리 구조 변경

6f75bcd

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 잔돈 충전 UI 템플릿 생성

e3d4b5d

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 잔돈 충전 페이지 라우팅

541832f

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 상품 구매 페이지 라우팅

8917342

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

chore: 파일명 변경

f299c9f

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

refactor: navigation 클래스 네이밍 변경

d188eb4

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 커스텀 이벤트 적용

304e615

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 네비게이션 버튼 클릭시 색깔 변경

9efad0e

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 상품 관리 페이지 수정, 삭제 버튼 기능 구현

99906f0

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 상품 관리 페이지 확인 버튼 UI 요소 구현

3ba52c9

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 상품 관리 페이지 확인 버튼 기능 구현

ec51122

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

fix: 상품 관리페이지 삭제 버튼 수정

a183cd9

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 잔돈 충전 페이지 기능 구현

490a31e

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

feat: 기능 요구 사항 구현

9ecd19b

- 삭제 버튼 클릭 시 확인 - 라우트 시 버튼 색 변경 - 예외처리 수정 Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

refactor: controller 영역 리팩토링

d79fed8

Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

refactor: 리팩토링

7ada089

- 타입스크립트 타입 지정 - 상수화 - 메서드 분리 Co-authored-by: kamwoo <kamwoo@users.noreply.github.com>

chore: eslint 중복 설정 제거

c5726b5

chore: github page deploy 설정

b4ddaf8

pocojang requested a review from EungyuCho March 27, 2022 00:14

pocojang assigned EungyuCho Mar 27, 2022

EungyuCho suggested changes Mar 27, 2022

View reviewed changes

kamwoo added 12 commits March 29, 2022 02:21

refactor: 중괄호 trailing comma 제거

c2c1a03

feat: getter 적용

b8d67d3

refactor: 도메인, 뷰, 메세지 별로 상수 파일 분리

764eb6a

refactor: bindEvents 메소드 constructor로 이동

fc16c6d

refactor: vendinMachine inputMoney에서 money로 수정

1c910dc

feat: table event binding 및 emit function 적용

30955d2

refactor: validate 과정 뷰에서 도메인으로 이동

5cdf4ed

feat: 클릭된 탭 중복 클릭 방지

607b4c0

refactor: item, coin 도메인 분리

58f3b2d

test: 도메인, 유효성 검사 테스트 작성

2cfacab

feat: 수정, 삭제 버튼에 같은 row 아이템 데이터 저장 및 사용

e73a9c2

feat: base url 수정

d695797

EungyuCho suggested changes Mar 29, 2022

View reviewed changes

kamwoo added 10 commits March 30, 2022 17:01

refactor: 유효성 검증 vendingMachine으로 위치 변경

6cf96b3

feat: view에서 vendingMachine을 직접 사용

809bf1b

refactor: controller삭제 및 AppController에서 App으로 변경

c29c64e

refactor: private 설정

6d7d49c

fix: 삭제 취소 클릭시 삭제되는 문제 해결

e7c13c0

refactor: 불필요한 타입 명시 삭제

1c5ecd7

refactor: 에러 메세지 상수 설정값 적용

dd91d78

refactor: templateType 삭제

59a2425

test: 지정된 상수값 사용으로 변경

33dbf54

feat: base url 수정

7c1a752

EungyuCho approved these changes Mar 30, 2022

View reviewed changes

EungyuCho merged commit b10bc8a into woowacourse:kamwoo Mar 30, 2022

kamwoo deleted the step1 branch April 2, 2022 06:47

		@@ -0,0 +1,54 @@
		import { COINS } from '../constants/constants';
		import { CoinsType, TemplateType } from '../types/types';


		handleRouteChange(event: CustomEvent) {
		const { $navButton }: RouteChangeDetailType = event.detail;

		UNDER_MIN: '100원보다 낮은 가격은 입력할 수 없습니다.',
		OVER_MAX: '10,000원 보다 큰 가격을 입력할 수 없습니다.',

		handleChargeMoney(event: CustomEvent) {
		const { inputMoney }: ChargeMoneyDetailType = event.detail;

[1단계 - 자판기] 밧드(감우영) 미션 제출합니다. #22

[1단계 - 자판기] 밧드(감우영) 미션 제출합니다. #22

Conversation

kamwoo commented Mar 26, 2022

구조

질문

EungyuCho left a comment

Choose a reason for hiding this comment

질문

피드백

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

EungyuCho Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 29, 2022 • edited

Choose a reason for hiding this comment

EungyuCho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo Mar 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamwoo commented Mar 30, 2022 • edited

EungyuCho commented Mar 30, 2022

EungyuCho left a comment

Choose a reason for hiding this comment

kamwoo commented Mar 30, 2022 • edited

EungyuCho commented Mar 31, 2022

kamwoo Mar 29, 2022 •

edited

kamwoo Mar 29, 2022 •

edited

kamwoo Mar 29, 2022 •

edited

EungyuCho Mar 29, 2022 •

edited

kamwoo Mar 29, 2022 •

edited

kamwoo Mar 29, 2022 •

edited

kamwoo Mar 29, 2022 •

edited

kamwoo Mar 29, 2022 •

edited

kamwoo Mar 29, 2022 •

edited

kamwoo Mar 29, 2022 •

edited

kamwoo Mar 30, 2022 •

edited

kamwoo Mar 30, 2022 •

edited

kamwoo Mar 30, 2022 •

edited

kamwoo commented Mar 30, 2022 •

edited

kamwoo commented Mar 30, 2022 •

edited