-
Notifications
You must be signed in to change notification settings - Fork 38
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
[1단계 - 자판기] 호프(김문희) 미션 제출합니다. #4
[1단계 - 자판기] 호프(김문희) 미션 제출합니다. #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 호프! 앞으로 미션동안 잘 부탁드립니다 :)
전체적으로 코드를 보면서 놀랐네요! 구조도 좋고 가독성도 좋아서 읽기 좋았습니다 👍
세부적인 것은 코멘트로 남겼구요, 전체적으로 궁금한 것이 모든 함수를 typing 하셨는데 반환값만을 typing 하는 방식이 아니라 파라미터 없는 함수들까지 inteface 파일에 추가하는 방식을 선택한 이유가 궁금합니다! 예를 들어 GenerateCoins
같은 함수들이요!
<질문>
- 질문을 읽으면서 역질문을 드리고 싶은데요! hash history 와 history api 를 이용해서 history 를 관리하는 것의 차이는 무엇일까요?
- 호프의 코드를 예로 들면, ChangePageView 와 ProductPageView 모두 this.$page 의 submit 이 일어날 때 각자의 핸들러를 실행시키고 있는데요! event 를 해제하지 않으면 상품 페이지에서 잔액 페이지로 갔을 때 두 핸들러 모두 실행되겠죠? 물론 호프의 코드는 target 으로 return 해주고 있지만, 실행되지 않아야할 핸들러가 실행되는 것 자체가 문제입니다. 이런 관점에서 생각해보고 반영되면 좋겠네요!
src/js/moderator/productModerator.js
Outdated
this.productPageView.renderInputForm(); | ||
this.productPageView.initDOMS(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 순서상 Init 이 먼저 된 다음에 render 를 해야겠죠?
this.productPageView.renderInputForm(); | |
this.productPageView.initDOMS(); | |
this.productPageView.initDOMS(); | |
this.productPageView.renderInputForm(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금보니 정말...순서가 말도 안되네요 😭
이 부분은 코드에서 Dom Element를 다루는 방법을 바꾸면서 수정되었습니다 :)
감사합니다!
src/js/ui/productPageView.js
Outdated
|
||
class ProductPageView { | ||
constructor() { | ||
this.$page = document.querySelector("#page"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드는 아래 17라인인 initDOMS 에서 실행하지 않는걸까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러게요! 지금보니 이것도 Dom 을 초기화해주는건데 initDoms 에 넣어놨어야했네요!
지금은, init 메서드 내부에서 init Doms , bindEvents 를 모두 처리하도록 수정했습니다.
지적 감사드려요 👍
src/js/ui/productPageView.js
Outdated
on(this.$page, "click", this.onClick); | ||
}; | ||
|
||
initDOMS = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOMS 는 왜 대문자일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분에 대해서 페어와 깊게 상의하지 않고 네이밍한것같아요.
해당 함수는 지워졌지만, 만약 다시 필요하게 된다면 initDoms
혹은 initDomElements
정도로 네이밍을 변경할 것 같아요.
src/js/moderator/productModerator.js
Outdated
try { | ||
this.productProcessMachine.add({ name, price, count }); | ||
const products = this.productProcessMachine.getProducts(); | ||
this.productPageView.renderProductStatus(products); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
product 하나를 추가할 때마다 모든 products 를 다시 그리는군요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 이 부분 문제점 인지하고 수정했습니다! 아래 Pr 메시지에 자세하게 적어놓을게요 :) !
이게 문제라고 생각하지는 않았는데, 헤인티 리뷰를 받고나서보니..정말 비효율적이네요! 변경된 엘리먼트만 수정해주면 되니까요..?!
좋은 지적 정말 감사드려요! 👍
src/js/moderator/productModerator.js
Outdated
try { | ||
this.productProcessMachine.update(idx, name, price, count); | ||
const products = this.productProcessMachine.getProducts(); | ||
this.productPageView.renderProductStatus(products); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
업데이트도 마찬가지로 하나 업데이트 할 때마다 모든 products를 다시 그려야하네요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도, 위의 답변과 마찬가지 입니다 👍 👍
src/js/ui/changePageView.js
Outdated
this.$changesTableBody = document.querySelector("#changes-table-body"); | ||
}; | ||
|
||
renderHaveChanges = (changes) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have 를 붙이는건 매우 어색하네요! renderCurrentChanges 정도는 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 감사합니다 :) 지금 보니까 동사를 명사 앞에 붙이니 매우 부자연스럽네요 😭 currentChanges 로 수정했습니다 :D
src/js/ui/changePageView.js
Outdated
this.$haveChanges.innerText = `현재 보유 금액: ${changes}`; | ||
}; | ||
|
||
renderChangesTable = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음.. UI 의 종류를 template 이 아닌 view 까지 알고 있어야 할까요?
예를 들어 동전의 리스트를 표가 아닌 그래프로 그린다면 요런 메소드들은 Table -> Graph 로 전부 바꾸어줘야 하네요!
어떤 것을 렌더링하는지만 나타내면 어떨까 싶습니다. 예를 들어 renderCurrentCoins() 이런식으로요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI 가 바뀔 수도 있는데, UI 의 종류를 함수에 넣는게 옳지 않다는 의견에 동의합니다 👍
그래서 renderCoinStatus
으로 바꾸어봤습니다 ㅎㅎ
|
||
const newCoins = this.generateCoins(money); | ||
this.accumulateCoins(newCoins); | ||
console.log(this.coins); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 죄송합니다. 깜빡하고 안지웠네요 !! 이랑 같이 불필요한 주석도 제거했습니다 :) 👍 👍 짚어주셔서 감사해요!
src/js/moderator/changesModerator.js
Outdated
this.changePageView.renderInput(); | ||
const changes = this.changeProcessMachine.getTotalChanges(); | ||
const coinStatus = this.changeProcessMachine.getCoins(); | ||
this.changePageView.renderChangesTable(); | ||
this.changePageView.initDOM(); | ||
this.changePageView.renderHaveChanges(changes); | ||
this.changePageView.renderChangeStatus(coinStatus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 productModerator 와 같은 맥락인데요, 사소하지만 함수를 읽어가면서 어떤 흐름인지를 알 수 있도록 순서를 조정해보면 어떨까요?
// dom 을 먼저 초기화하고
this.changePageView.initDOM();
// input 을 렌더링하고
this.changePageView.renderInput();
// 현재 잔액과 동전값을 가져와서
const changes = this.changeProcessMachine.getTotalChanges();
const coinStatus = this.changeProcessMachine.getCoins();
// view 가 잔액표, 잔액, 동전 현황을 그리라고 한다.
this.changePageView.renderChangesTable();
this.changePageView.renderHaveChanges(changes);
this.changePageView.renderChangeStatus(coinStatus);
이렇게 읽으면서 흐름을 느낄 수 있다면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정말 좋은 포인트 감사드립니다! 아래와 같이 productModreator, changesModerator 를 수정했습니다 😄
init() {
// View 를 초기화하고
this.changePageView.init();
// 잔액과 동전값을 가져와서
const changes = this.changeProcessMachine.getTotalChanges();
const coinStatus = this.changeProcessMachine.getCoins();
// View 과 잔액표, 잔액, 동전 현황을 그리라고 한다.
this.changePageView.renderCurrentChanges(changes);
this.changePageView.renderCoinStatus(coinStatus);
}
init() {
// View 를 초기화하고
this.productPageView.init();
// 상품 리스트를 가져와서
const products = this.productProcessMachine.getProducts();
// view 가 초기 상품리스트를 그리라고 한다.
this.productPageView.renderProductsStatus(products);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
척하면 척이네요! 👍
src/js/constant/index.js
Outdated
export const ERROR_MESSAGE = { | ||
DUPLICATED_NAME: "중복된 상품은 입력 할 수 없습니다.", | ||
MAXIMUM_NAME_LENGTH: "상품명은 10자이하로 입력해주세요", | ||
VALID_PRICE: "유효한 가격을 입력해주세요", | ||
MINIMUM_COUNT: "추가하는 수량은 0이하가 될수가 없습니다.", | ||
MAXIMUM_COUNT: "수량은 최대 20개까지 추가 가능합니다.", | ||
DIVIDED_BY_MINIMUM_COIN: "투입된 금액은 10으로 나누어 떨어져야합니다.", | ||
MAXIMUM_CHANGES: "최대 잔액은 100000원 입니다.", | ||
MINIMUM_CHANGES: "금액은 0원보다 높아야합니다.", | ||
}; | ||
|
||
export const VENDING_MACHINE_NUMBER = { | ||
MAXIMUM_CHANGES: 100000, | ||
MAXIMUM_PRICE: 10000, | ||
MINIMUM_PRICE: 100, | ||
MINIMUM_COIN: 10, | ||
MAXIMUM_COUNT: 20, | ||
MINIMUM_COUNT: 0, | ||
MAXIMUM_NAME_LENGTH: 10, | ||
}; | ||
|
||
export const EVENT_TYPE = { | ||
CHARGE: "@charge", | ||
ADD: "@add", | ||
DELETE: "@delete", | ||
EDIT: "@edit", | ||
}; | ||
|
||
export const CONFIRM_MESSAGE = "정말로 삭제하시겠습니까?"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typescript 를 배웠다면 여기 상수들도 변경될 수 없도록 type 을 설정해봤으면 좋겠는데요, 지금은 충분히 ERROR_MESSAGE. DUPLICATED_NAME = '~~~'
로 메세지를 수정할 수 있겠죠?
typescript 를 이용해서 어떻게 수정되지 않도록 type 을 설정할 수 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 이 부분 학습해서 타입스크립트의 enum 을 활용해서 아래와 같이 수정했습니다.
확실히 enum으로 정의하니, 타입정의할 때에도 편하고 좋네요 👍 👍
export enum ERROR_MESSAGE {
DUPLICATED_NAME = "중복된 상품은 입력 할 수 없습니다.",
MAXIMUM_NAME_LENGTH = "상품명은 10자이하로 입력해주세요",
VALID_PRICE = "유효한 가격을 입력해주세요",
MINIMUM_COUNT = "추가하는 수량은 0이하가 될수가 없습니다.",
MAXIMUM_COUNT = "수량은 최대 20개까지 추가 가능합니다.",
DIVIDED_BY_MINIMUM_COIN = "투입된 금액은 10으로 나누어 떨어져야합니다.",
MAXIMUM_CHANGES = "최대 잔액은 100000원 입니다.",
MINIMUM_CHANGES = "금액은 0원보다 높아야합니다.",
}
export enum VENDING_MACHINE_NUMBER {
MAXIMUM_CHANGES = 100000,
MAXIMUM_PRICE = 10000,
MINIMUM_PRICE = 100,
MINIMUM_COIN = 10,
MAXIMUM_COUNT = 20,
MINIMUM_COUNT = 0,
MAXIMUM_NAME_LENGTH = 10,
}
export enum EVENT_TYPE {
CHARGE = "@charge",
ADD = "@add",
DELETE = "@delete",
EDIT = "@edit",
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오! enum 을 적용하셨군요 👍
그치만 보통 이런 경우엔 const 를 쓰는게 맞습니다! 2단계에서 아래 리스트에 대해 공부하고 결과를 알려주시면 좋겠네요!
- enum 을 사용하면 안되는 이유
- readonly 타입이란?
- as const 란?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
학습 키워드 알려주셔서 감사해요! 다음 미션까지 학습해서 알려드리겠습니다 👍 👍 헤인티 짱짱..감사해용 ❤️
- SingleProduct 클래스 작성 - ProductProcessMachine Interface 수정
안녕하세요 헤인티! 먼저 리뷰 반영이 늦어진 점 정말 죄송해요. 타입스크립트 이것저것 공부해보고 적용해보느라 늦어졌습니다 ㅠ_ㅠ 헤인티가 주신 질문에 대한 저의 생각이랑, 코드를 어떻게 변경했는지 간략하게 설명해보았습니다. 1. 전체적으로 궁금한 것이 모든 함수를 typing 하셨는데 반환값만을 typing 하는 방식이 아니라 파라미터 없는 함수들까지 inteface 파일에 추가하는 방식을 선택한 이유가 궁금합니다! 예를 들어 GenerateCoins 같은 함수들이요! 2. 질문을 읽으면서 역질문을 드리고 싶은데요! hash history 와 history api 를 이용해서 history 를 관리하는 것의 차이는 무엇일까요? Hash api 사용법
History api 사용법
제가 이렇게 두 가지 방법을 찾아보고 깨달은것은, 지금 현재 서버에서 서버사이드렌더링 방식으로 HTML을 그려서 응답해줄게 아니면 지금 Histroy Api 를 사용하는 것과 hash api 를 사용하는 것에 성능상 차이가 없다고 판단했습니다. 그래서 해시뱅 방식으로 유지했습니다! 3. 호프의 코드를 예로 들면, ChangePageView 와 ProductPageView 모두 this.$page 의 submit 이 일어날 때 각자의 핸들러를 실행시키고 있는데요! event 를 해제하지 않으면 상품 페이지에서 잔액 페이지로 갔을 때 두 핸들러 모두 실행되겠죠? 물론 호프의 코드는 target 으로 return 해주고 있지만, 실행되지 않아야할 핸들러가 실행되는 것 자체가 문제입니다. 이런 관점에서 생각해보고 반영되면 좋겠네요!
수정한 점
그럼 다시 한번 늦게 리뷰요청 보내드려서 죄송하구, ㅠ_ㅠ 정말정말 감사드립니다. 많은 도움이 되었어요 🙇 |
`<td>${name}</td> | ||
<td>${price}</td> | ||
<td>${count}</td> | ||
<td> | ||
<button class="edit-button process-button">수정</button> | ||
<button class="delete-button process-button">삭제</button> | ||
</td>`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 친구도 template 이겠죠?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 지적해주셔서 감사합니다 :) 따로 빼도록 할게요~~!
export const $ = ( | ||
selector: string, | ||
target: Element | Document = document | ||
): Element => target.querySelector(selector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 함수가 항상 Element 를 return 하는게 맞을까요?
그리고 아래 src/js/util/event.ts 파일에서 generic 을 사용하셨는데요, 요 함수도 generic 으로 표현해보면 좋겠네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호...넵 감사합니다! 사실 별 생각 없이 쿼리셀렉터로 엘리먼트를 가져오니까, Element를 리턴할거라고 생각했어요! HTML 엘리먼트 관련 타입 더 공부하고 적용해보겠습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 호프! 정리해주신 것과 수정사항 모두 확인했습니다. 자세히 적어주셔서 감동받았네요 👍
2단계에서 #4 (comment), #4 (comment) 이렇게 두 개 피드백 주시면 좋겠습니다!
2단계에서 봬요~ 고생 많으셨습니다!
✨ 데모페이지
안녕하세요 헤인티 ! 만나뵙게 되어 영광입니다 :) 리뷰이 호프라고 합니다 ~! 이번 미션동안 잘부탁드려요 !
이번에는 특별한 패턴을 정하지 않고 페어와 인터페이스 작성 -> 도메인 구현 -> 도메인 단위테스트 작성 -> UI 구현 순으로 코드를 작성했습니다.
인터페이스 먼저 작성하고, 도메인을 구현하니 도메인 구현할 때 훨씬 메서드의 역할들이 분리가 잘 되었고, UI 와 도메인의 분리도 쉬웠습니다.
그리고 UI 구현 할 때 도메인 코드를 직접 참고하지 않고 인터페이스 부분만 참고해도 되었기에 굉장히 편리했어요.
그리고 단위테스트를 바로바로 구현하니, 구현 할 때 놓쳤던 예외사항들을 잘 처리 할 수 있었기에 좋았습니다.
✅ Step1 구현 목록
✅ src 파일 구조
이번 스텝은 다음과 같은 고민들을 하면서 진행했습니다.
1. 도메인과 UI 를 분리하기
2. 커스텀 이벤트를 사용해서 뷰-컨트롤러의 의존성을 분리하기
✅ 고민하고 있는 점
1. SPA 구현 - 해쉬뱅 사용
2. 이벤트 해제
그럼 감사합니다 👍 👍