Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2단계 자판기] 밧드(감우영) 미션 제출합니다 #50

Merged
merged 64 commits into from
Apr 13, 2022

Conversation

kamwoo
Copy link

@kamwoo kamwoo commented Apr 6, 2022

안녕하세요! 고생 많으십니다. 🙂
데모 페이지

구조

vendingMachine-step2

  • 메인 페이지(상품 관리, 잔돈 충전, 상품 구매), sign 페이지(sign in, sign out, edit profile)로 나눴습니다. 이렇게 나눈 이유는 라우팅에서 새로 url을 주는 방식으로 해보니깐 메인 페이지에서 탭을 클릭할 때도 전부 다시 렌더링해야 되서 spa에 맞출려고 큰 범위로 먼저 나눴습니다.

  • url 변경, 페이지 렌더링을 App에서 이벤트를 받고 페이지를 렌더링하고, 페이지에서 섹션들을 렌더링하는 방식으로 만들었습니다.

  • 유저 정보, 상품 데이터, 잔돈을 서버에 저장하고 수정,삭제를 하고 도메인에서도 저장해서 사용했습니다. 도메인에도 놔둔 이유는 수정하거나 추가할 때 기다리지 않고 빠르게 렌더링할 수 있다고 생각했습니다.

  • 상품 구매 페이지에서 새로고침했을 때 상품 데이터나 잔돈이 들어가 있지 않은게 보기 불편해서 서버 사용하는 겸 도전해봤습니다. 괜히 한 것 같아서 하면서 후회 많이했습니다. 😭

  • 2단계에서는 e2e 테스트만 작성했습니다. 일반 유저일 경우 메인 페이지에서 기능, 관리자일 경우 메인 페이지 기능, 관리자 로그인, 회원가입, 정보 수정 3개 파일로 나누어서 작성했습니다.

질문

  • 지금 라우팅하는 방식에서 만약에 페이지가 더 늘어난다면 감당하기 어렵겠다고 느꼈습니다. 보시기에 지금 방식에서 어떻게 수정하면 좋을지 아사님 의견이 궁금합니다.
  • 처음에 상품 데이터나 잔돈을 서버에서 가져와서 초기화 시키는 부분이 itemManager나 coinsManager에서 이루어지면 좋을지, main페이지 렌더링 시키는 부분에 있어야 할지, 어디에서 실행되어야 할지 궁급합니다.
  • 네이밍 부분에서 input이나 충전한 잔돈, 동전, 반화해야 되는 잔돈이 겹치는게 많아서 뭐라고 지으면 좋을지 잘모르겠습니다. 저가 봐도 헷갈리는 수준이라.. 지금 상태에서 추천해주실 이름이나 평소에 사용하시는 네이밍 팁이 있으신지 궁금합니다. 🙂

로딩 시간이 조금 길 수 있습니다.. 😓

kamwoo added 30 commits April 2, 2022 15:55
- 구입 상품의 가격이 남아있는 투입한 금액보다 작을 경우
- 수량이 1개 이상일 경우
- main과 sign으로 분류 후 세부 페이지 렌더링
- url 해쉬 추가
- 클래스로 변경
- 싱글톤 적용
- accessToken과 userData를 private하게 관리한다.
- 유일한 객체로 사용한다.
Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 밧드님~

step2 분량이 상당한데도 복잡도가 그리 높지않아보이네요.. 파일 분리를 잘해주신거 같습니다 👏

유저 정보, 상품 데이터, 잔돈을 서버에 저장하고 수정,삭제를 하고 도메인에서도 저장해서 사용했습니다. 도메인에도 놔둔 이유는 수정하거나 추가할 때 기다리지 않고 빠르게 렌더링할 수 있다고 생각했습니다.

좋은 고민을 하신거같아요. 미션을 할때마다 heroku 서버가 느려서 저도 이런 고민을 하고 동일한 솔루션을 적용했었어요. 잘 적용해주셨습니다! 👍

2단계에서는 e2e 테스트만 작성했습니다. 일반 유저일 경우 메인 페이지에서 기능, 관리자일 경우 메인 페이지 기능, 관리자 로그인, 회원가입, 정보 수정 3개 파일로 나누어서 작성했습니다.

테스트가 실패하는 케이스도있고 이전 jest로 진행했던 부분이 실패하는 케이스도 있어서 코드 코멘트에 내용을 담아드렸어요~

지금 라우팅하는 방식에서 만약에 페이지가 더 늘어난다면 감당하기 어렵겠다고 느꼈습니다. 보시기에 지금 방식에서 어떻게 수정하면 좋을지 아사님 의견이 궁금합니다.

폴더별로만 관리가 잘 된다면 문제는 없을거같아요. main페이지에서 페이지를 라우팅하는 부분에 대해서는 개선할만한 부분이 있다고 생각되어 코멘트를 달아드렸어요.

처음에 상품 데이터나 잔돈을 서버에서 가져와서 초기화 시키는 부분이 itemManager나 coinsManager에서 이루어지면 좋을지, main페이지 렌더링 시키는 부분에 있어야 할지, 어디에서 실행되어야 할지 궁급합니다.

현재 작성해주신 구조에서는 manager에서 하는게 제일 맞는 방향성이라고 생각이 들어요.
별도로 메서드를 만들어두면 재사용하기도 편하구요 🙂

네이밍 부분에서 input이나 충전한 잔돈, 동전, 반화해야 되는 잔돈이 겹치는게 많아서 뭐라고 지으면 좋을지 잘모르겠습니다. 저가 봐도 헷갈리는 수준이라.. 지금 상태에서 추천해주실 이름이나 평소에 사용하시는 네이밍 팁이 있으신지 궁금합니다.

자판기 미션이 특히 네이밍에 대한 부분을 설정하기 어려운거같아요 😅
보통 돈에 관련된 네이밍은 amount를 즐겨사용하고 동전은 사용해주신대로 coins로 충분하다고 생각해요. 반환에 대한 부분도 change가 자주 사용하는 표현이 맞기도하고 문제없어보입니다 :)
불필요한 수식어가 없는지만 한번 확인해보면 좋을거같아요. 관련해서도 코멘트로 드렸어요~

진행하시느라 정말 고생많으셨어요!
코멘트 확인해주시고 다시 리뷰요청 부탁드릴게요 🙂

Comment on lines 10 to 12
constructor(private readonly vendingMachine: VendingMachine) {
this.vendingMachine = vendingMachine;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constructor(private readonly vendingMachine: VendingMachine) {
this.vendingMachine = vendingMachine;
}
constructor(private readonly vendingMachine: VendingMachine) {}

생략하셔도 암묵적으로 초기화를 진행합니다~
참고

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정했습니다!

Comment on lines +19 to +22
handlePageChange(event: CustomEvent<RouteChangeDetailType>) {
this.handleRouteChange(event);
this.renderPage();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

타입스크립트에서 CustomEvent를 적용해보지 않아서 몰랐는데 제네릭을 사용할 수 있네요?..
타입지정을 잘해주신거같네요 👍 👍

import UserMenuView from './main/userMenuView';
import Storage from '../api/storage';

export default class MainView {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

최상위 view(main, sign)별로 폴더를 구분해주셨으니 해당 폴더 안에 최상위 View(mainView.ts, signView.ts)를 옮겨주면 좋을거같아요. (view가 많아질걸 대비해서)

그리고나서 최상위 View의 파일명을 index.ts로 바꿔서 관리하면 더 깔끔할지도 모르겠다는 생각이 들었습니다 😅

Copy link
Author

@kamwoo kamwoo Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mainView와 signView 역할에도 더 맞는것 같고, 보기도 훨씬 깔끔하다고 느꼈습니다. 🙂


constructor(name: string, price: number, quantity: number) {
constructor(name: string, price: number, quantity: number, id: number) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인자가 4개나 되면 헷갈릴거같아요 👀
생성할 때 객체형태로 받아보시는건 어떨까요?

ItemManager에서 setItem, changeItem을 할때에도 그대로 넣어주면 돼서 수월할거라고 보이기도하구요

Copy link
Author

@kamwoo kamwoo Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 수정했습니다.!

Comment on lines 11 to 13
const item = new Item(name, price, quantity, id);

itemManager.addItem({ name, price, quantity });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const item = new Item(name, price, quantity, id);
itemManager.addItem({ name, price, quantity });
const item = new Item(name, price, quantity, id);
itemManager.addItem({ name, price, quantity, id });

id가 undefined네요~

새로운 기능을 추가할 때 기존 테스트가 동작하는지 테스트해보시면 좋을거같아요.
변경할때 테스트가 실패한 상태로 기존의 테스트를 방치한다면 테스트를 하는 이유가 없어보이네요
테스트를 하는 이유에 대해 한번 더 생각해볼 수 있으면 좋겠습니다 🙂

Copy link
Author

@kamwoo kamwoo Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기능 추가를 테스트를 하면서 했어야 했는데, 구현하는 것에 급했던 것 같다고 생각했습니다.😓
각 테스트에 id 부분을 추가했습니다.

return this.moneyManager.money;
}

chargeCoins(inputMoney: number) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chargeCoins인데 인자가 inputMoney네요??.. 🤔
동전으로 모두 넣었다고 가정하는게 맞는거같고 input이라는 수식어도 필요없을거같아요.
coins라는 인자명이 적절할거같아요.
(validateChargeCoins도 인자가 money네요 👀 )

작성해주신 메서드에 맞게 인자의 네이밍도 변경해보는게 좋다고 생각해요.
메서드명이 chargeCoins면 인자로 들어오는 coin은 자판기에서 입력받은 코인이란걸 유추할 수 있어서 수식어가 없어도 자연스럽다고 생각이 들었어요.

밧드님은 어떻게 생각하실까요?

Copy link
Author

@kamwoo kamwoo Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 이부분에서 money로 하는게 고민이었습니다. coins가 동전 각 개수를 가지고있는 객체로 많이 사용해놔서, 동전들의 합이라는 의미로 coinsSum이라고 변경했습니다.
유추할 수 있는 input은 뺐고, validate하는 인자들을 모두 input을 붙여서 사용하고 있어서 유효성 검증하는 인자에서는 input 수식어를 붙였습니다.

import { SELECTOR } from '../../constants/viewConstants';
import { CUSTOM_EVENT, URL } from '../../constants/appContants';
import showSnackbar from '../../utils/snackbar';
import AuthAPIAPI from '../../api/authAPI';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엇?.. 이름이 뭔가 이상한데요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😭 수정했습니다.

@@ -28,3 +31,11 @@ export type TableItemChangeDetailType = {
export type TableItemDeleteDetailType = {
item: ItemType;
};

export type userDataType = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type userDataType = {
export type UserDataType = {

나머지는 파스칼 케이스를 사용해주셨는데 이 타입은 놓치신거 같아요

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 수정했습니다.

fixture: 'moneyDataResponse.json',
}).as('moneyDataRequest');

cy.intercept('POST', ProductAPI.BASE_URL + ProductAPI.TYPES.PRODUCTS + '/*', {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제 호출되는건 patch 메서드를 사용하네요~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제 요청되는 것과 겹쳐서 틀린 부분을 찾기가 어려웠던것 같습니다. intercept하는 부분들을 custom command로 모아서 관리하도록 해봤습니다.

Comment on lines 15 to 17
cy.intercept('POST', AuthAPI.BASE_URL + AuthAPI.TYPES.SIGN_IN, {
fixture: 'userData.json',
}).as('signInRequest');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그인이 intercept가 잘 안되는거같아요.
왜 그런지 잘 모르겠네요 😓

동일현상 있으신지 한번 확인부탁드릴게요! 만약 맞다면 저도 시간날때 한번 찾아보겠습니다~
스크린샷 2022-04-09 오전 5 29 43
 

Copy link
Author

@kamwoo kamwoo Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그인, 정보 수정 request에서는 intercept가 안되고 있었네요 🥲 무슨 이유인지는 모르겠지만 각 테스트에 intercept를 추가하니깐 됐습니다. 다른건 되는데 이것만 안되는 것 같아서 조금더 알아보겠습니다.

@kamwoo
Copy link
Author

kamwoo commented Apr 13, 2022

현재 작성해주신 구조에서는 manager에서 하는게 제일 맞는 방향성이라고 생각이 들어요.

이 부분에서 manager에 두고 처음 manager들이 초기화될 때, 탭을 클릭할 때 업데이트를 시키고 싶었는데 적용이 안되서, 생성자에 async를 하는 것과 같이 동작하도록 하는 방법을 찾고 적용해보겠습니다.

@EungyuCho
Copy link

이 부분에서 manager에 두고 처음 manager들이 초기화될 때, 탭을 클릭할 때 업데이트를 시키고 싶었는데 적용이 안되서, 생성자에 async를 하는 것과 같이 동작하도록 하는 방법을 찾고 적용해보겠습니다.

생성자에서 async는 동작하지 않을거에요 👀
생성자는 바로 해당 클래스의 인스턴스를 반환해야해서 불가능한걸로 알고있어요.

loading 상태를 내부적으로 만들고 서버에 데이터 요청을 보내는 경우 로딩중이라는 피드백을 주는건 어떨까요?

Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 밧드님

피드백 반영하신 부분 확인했어요 피드백이 이래저래 많았던 것 같은데 반영하신다고 정말 고생많으셨어요 👍 👍

ItemManager관련해서 한가지 추가 코멘트를 남겨드렸는데 한번 확인부탁드릴게요!
머지후라도 추가적으로 코멘트 주셔도 무방합니다!~

이번 미션에서 API도 정말 다양하게 정의해서 사용해주셨고 테스트 또한 꼼꼼하게 작성해주셨어요.
코드도 짜임새있게 작성해주셔서 정말 완성도가 높게 구현됐다고 생각해요 🙂
타입스크립트 또한 최대한 사용하려고 노력해주셨고 전체적으로 잘 녹여내주셨어요.

이번 미션은 여기서 승인해도 충분해보여요.
추가적으로 개선해보고 싶으신 부분이 있다면 시도해보시고 질문해주셔도 좋습니다~

방학중에도 피드백 반영하시느라 고생 많으셨어요!
앞으로 있을 레벨2과정도 잘 해내실거라고 생각해요. 밧드님 화이팅입니다! 😄

this._items.push(new Item(item));
}

changeItem(index: number, item: ItemType) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._items를 배열이 아닌 Map 자료구조를 사용하면 어떨까요??

Comment on lines +7 to +12
mainView: MainView;
signView: SignView;

constructor() {
this.appView = new AppView();
this.vendingMachine = new VendingMachine();
this.manageItemView = new ManageItemView(this.vendingMachine);
this.chargeMoneyView = new ChargeMoneyView(this.vendingMachine);
this.purchaseItemView = new PurchaseItemView(this.vendingMachine);
this.mainView = new MainView();
this.signView = new SignView();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mainView: MainView;
signView: SignView;
constructor() {
this.appView = new AppView();
this.vendingMachine = new VendingMachine();
this.manageItemView = new ManageItemView(this.vendingMachine);
this.chargeMoneyView = new ChargeMoneyView(this.vendingMachine);
this.purchaseItemView = new PurchaseItemView(this.vendingMachine);
this.mainView = new MainView();
this.signView = new SignView();
constructor(private mainView: MainView, private signView: SignView) {

이렇게 초기화하면 자동으로 this에 생성자로 받은 인자들이 할당돼요~
참고

@EungyuCho EungyuCho merged commit 964c913 into woowacourse:kamwoo Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants