[클린코드 4기 우경준] 자판기 미션 STEP2 #60
Conversation
There was a problem hiding this comment.
저번주에 리뷰를 해드리기로 했었는데 깜빡하고 있었네요 😭 🙇
질문사항
- 최대한 한가지 역할만을 하는 함수를 만들기 위해 노력했습니다. 어떻게 생각하시나요?
- 직관적으로 읽힐만큼
객체의 메세지(메소드)를 적극 활용해 복잡한 명령형 구문들을 캡슐화 했고, 가독성을 최대한 높였습니다. 어떤 것 같으신가요?
- 직관적으로 읽힐만큼
- 객체의 메소드를 적절하게 분리해주신 것 같아요. 다만 코멘트에도 남겨드렸는데 외부에서 조작하면 의도하지 않은 동작을 할 여지가 있는 속성이나 메소드를 적절한 접근지정자로 설정할 필요가 있어보여요.
네이밍이 직관적이었나요? (참고로 전 길고 자세하게 영문법과 단어들을 최대한 살려 네이밍을 쓰는 편입니다.) - 네이밍이 구현에 치중된 나머지 어떤 동작을 할지 예측을 못한 메소드도 있었어요. 하나의 역할만 하는게 아닌 경우에는 구현에 치중된 네이밍보다 어떤 행동을 하는지 추상적으로 네이밍을 하는게 더 쉬울수도 있다고 생각합니다 👀
구조가 크게 달라지지 않아서 많은 코멘트는 없는데 위에 말씀드린 것 위주로 리팩터링해보면 좋을 것 같아요.
리뷰가 늦어 죄송합니다 😢 😢
고생하셨어요!
| input = 0; | ||
| totalAmount = 0; | ||
|
|
||
| constructor({ input = 0, totalAmount = 0 }) { | ||
| super(); | ||
| this.input = input; | ||
| this.totalAmount = totalAmount; | ||
| } |
There was a problem hiding this comment.
필드를 선언을 private field로 선언해둬야할 것 같아요
이렇게 사용하게되면 별도로 필드를 내부에서 조작하려고 만든 메소드가 무의미해지는 것 같아요.
외부에서 실수로라도 필드를 조작하게되면 의도한대로 프로그램이 동작하지 않을 수 있으니 접근지정자를 지정해줍시다 :)
| @@ -0,0 +1,10 @@ | |||
| import { getCustomElementClass, getCustomElementCreator } from "../../core/CustomComponent.js"; | |||
There was a problem hiding this comment.
사소하긴한데 이 파일위치를 바꿔봐도 좋을 것 같아요
view 파일이랑 위계가 똑같은것보다 element 폴더를 별도로 만들어두는게 깔끔할수 있겠다 싶었어요
views/productManagerView/customElements/ProductInventoryContainer.js
| alert('상품 구입에 금액이 부족합니다!'); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| #checkProductQuantity() { | ||
| if (this.quantity <= 0) { | ||
| alert('상품 재고가 모두 떨어졌습니다!'); | ||
| return false; |
There was a problem hiding this comment.
실패했을 때 어떻게 동작할지는 모델 밖에서 정의하는게 좋아보여요.
경고창에서 Toast를 띄우는 방식으로 바뀐다고 쳤을 때 모든 모델에 있는 에러를 핸들링해야해서 번거로울 수 있다고 느꼈어요 🤔
저는 외부에서 에러 핸들링을 하고싶다면 Error를 반환하고 아니라면 객체형태로 반환하고 외부에서 컨트롤해주는게 좋다고 생각해요.
전역적으로 Error를 받아서 meesage를 toast나 경고창을 띄우는식으로 설정을 해두고 개별적으로 에러를 관리하고 싶을 때 해당 함수에서 try catch를 추가적으로 작성해서 외부에서 에러케이스에 대한 동작을 핸들링하는게 더 자연스럽다고 생각했어요
| export function mergeArrayObjectToOneObject(arrayObject) { | ||
| return arrayObject.reduce((prev, curr) => { | ||
| for (const key in curr) { | ||
| const val = curr[key]; | ||
| prev[key] = val; | ||
| } | ||
|
|
||
| return prev; | ||
| }, {}); | ||
| } |
There was a problem hiding this comment.
어떤 용도로 사용되는것인지는 모르겠지만 사용하지 않는 것 같아요 👀
| const res = this.#divideNumberInDivideLevels(restAmount, COINS); | ||
| this.#setVendingMachineInLocalStorage(); | ||
|
|
||
| return res; |
There was a problem hiding this comment.
COINS는 인자로 주입해주지 않아도 될 것 같고 네이밍도 조금 바꿔보면 좋을 것 같아요
divideNumberInDivideLevels가 상세하지만 어떤 동작을 하는지 유추하기 힘들 것 같아요
| const res = this.#divideNumberInDivideLevels(restAmount, COINS); | |
| this.#setVendingMachineInLocalStorage(); | |
| return res; | |
| const coins = this.#exchangeCoins(restAmount); | |
| this.#setVendingMachineInLocalStorage(); | |
| return coins; |
안녕하세요! 마지막 미션 제출합니다!
그동안 정말 유익했던 프로젝트였습니다! 🙌
리뷰해주셔서 진심으로 감사드립니다!
마지막 리뷰도 잘 부탁드리겠습니다!
MVVM 패턴을 채택한 이유와 그 효용
STEP1 -> STEP2로 진행 될 때 View가 통째로 바뀌기 때문에 View 이외에 부분은 그대로 사용하기 위해서 STEP1에서 MVVM 패턴을 적용해 과제를 구현했습니다.
이번 STEP2에서 , STEP1에서 진행했던 부분의 View를 교체했는데, 이번 미션을 진행하면서 View 이외의 부분들을 변경없이 지킬 수 있었습니다. 🙌
그 외 궁금한 점들
감사합니다!! 😁