[클린코드 4기 방현경] 자판기 미션 STEP 1 #54
Conversation
There was a problem hiding this comment.
안녕하세요
@hyeonbang 👋
리뷰가 이미 된 줄 알았는데 누락되었네요 ㅠㅠ
정말 죄송합니다.
코멘트 리뷰하나하나 다 남겨놨어요!
피드백
1. 테스트
꼼꼼히 하는게 굉장히 보기 좋았습니다
하지만 정말 단순히 렌더링만 테스트하는 경우가 있으시던데 그런 경우는 스냅샷 테스팅도 고려해보시면 좋아요
2. 구조
View가 따로 존재하지 않아서 그럴까요? 컨트롤러가 너무나 무겁고 비대합니다.
또한 추상 클래스가 그 역할을 다 하기에는 계층이 애매한 구간이 계속 보이네요
가장 중요한 포인트는 Proxy 객체 활용도입니다.
Proxy는 정말 강력한 API인데 활용을 제대로 못하고 있네요.
제대로 활용하시면 코드량이 확 줄어들겁니다.
3. 그 외
- 랜덤으로 선택하신 점 매우 좋습니다 👍
- 유틸은 잘 활용하시는 것 같네요.
- 다만 무의미한 유틸도 눈에 띄어서 꼭 필요한지 다시 한번 정의해보셨으면 좋겠어요
고민 사항 (eventListener)
고민하신 것에 비해 큰 효과는 없는 부분인 것 같아요�.
단지 코드가 나뉘어져있을뿐 어딘가에 위임하거나 격리하거나 효율적으로 나눠서 재활용하거나 어떠한 느낌은 못받겠어요.
스마트하게 처리하는 방법은 아닐 수 있지만 이벤트를 더 다룰 수 있는 방법을 굳이 꼽아보자면
이벤트 위임이나 커스텀 이벤트를 Dispatch 하는 방법도 한번 시도해보세요 :)
이것저것 실험해보는게 중요하죠.
그렇게하다보면 @hyeonbang 님이 생각하는 스마트 아이디어가 언젠가는 떠오를거에요.
마지막 미션 첫 스텝 고생하셨습니다.
리뷰 반영 후 다시 리뷰 요청해주세요 👏
| homeSpec(); | ||
| }) | ||
|
|
||
| function homeSpec() { |
There was a problem hiding this comment.
Spec이라는 의미는 기능 단위로 나눴다고 보시면 될까요?
| }) | ||
|
|
||
| function rechargeSpeck() { | ||
| const amount = 100; |
There was a problem hiding this comment.
사소하지만 변경되지 않는 값이라면 상수로 명시해주는게 어떨까요?
매개변수로 넘어온다고 생각할 수 있거든요!
| rechargeSpeck(); | ||
| }) | ||
|
|
||
| function rechargeSpeck() { |
There was a problem hiding this comment.
rechargeSpec을 의도하신 네이밍 맞을까요?
| import { ERROR_MESSAGE } from "../../src/js/common/error.js"; | ||
| describe('잔돈 충전하기 테스트', () => { |
There was a problem hiding this comment.
Import 다음에는 개행을 하나 추가하는게 어떨까요?!
| describe('사이트 홈 테스트', () => { | ||
| beforeEach('페이지 방문', () => { |
There was a problem hiding this comment.
방문보다는 렌더링에 초점이 있는 것 같은데 렌더링 테스트 네이밍은 어떠세요?
| import { RECHARGE_CONTAINER, STOCK_CONTAINER } from './common/template.js'; | ||
| import { qs, setTemplate } from './utils/view.js'; | ||
|
|
||
| export class App { |
There was a problem hiding this comment.
App이라는 클래스를 바라보는 관점에서는 이미 자판기의 역할만 수행할 수 있는 앱 메인 느낌이 강하네요
확장성을 가지기 어렵다면 VendingMachineApp 으로 명시해도 좋겠는걸요?
| #initStock() { | ||
| setTemplate(qs($.APP), STOCK_CONTAINER); | ||
| this.#container.view = qs($.STOCK.CONTAINER); | ||
| if (!this.#container.view) return; | ||
|
|
||
| new StockComponent(this.#container); | ||
| } | ||
|
|
||
| #initRecharge() { | ||
| setTemplate(qs($.APP), RECHARGE_CONTAINER); | ||
| this.#container.view = qs($.RECHARGE.CONTAINER); | ||
| if (!this.#container.view) return; | ||
|
|
||
| new RechargeComponent(this.#container); | ||
| } |
There was a problem hiding this comment.
이미 규칙을 가지고 있어서 상위 계층을 하나 만들고 확장하거나 수동으로 초기화하지 않는 방법도 고려해볼 수 있겠어요!
| #initStock() { | ||
| setTemplate(qs($.APP), STOCK_CONTAINER); | ||
| this.#container.view = qs($.STOCK.CONTAINER); | ||
| if (!this.#container.view) return; |
There was a problem hiding this comment.
view가 없을때는 에러를 더닞고 다른 조치를 할 수 있어야하지 않을까요?
| import { getObjectLength } from '../utils/util.js'; | ||
|
|
||
|
|
||
| export class Validator { |
There was a problem hiding this comment.
Validator의 if문이 계속 늘어지고 있어 다른 리팩터링 방법도 고민해보면 좋겠어요 :)
| import { ERROR_MESSAGE, InstanceOfAbstractError } from '../common/error.js'; | ||
| import { displayNone, setEventListeners } from '../utils/view.js'; | ||
|
|
||
| export class Component { |
There was a problem hiding this comment.
어차피 단일 파일 단일 클래스라면 export default를 고려해도 되지 않을까요?
안녕하세요 리뷰어님
어느덧 마지막 미션인 자판기 미션까지 오게 되었네요.
지난 미션동안 학습해온 것들을 토대로 작업을 진행해보았습니다.
구조
처음 자판기 미션을 구현하기 전 구조에 대해서 많은 고민을 했었는데,
자판기가 여러대가 된다면, 잔돈의 타입이 추가된다면과 같은 범용성을 고려한 코드를 작성하고 싶었습니다.
결론적으로 자판기 모델 (vendinMachinModel)이라는 클래스를 작성하고,
재고관리(stockComponent)와 돈통충전(rechargeComponent) 컨트롤러를 작성하여
모델은 상태값만 설정하고, 컨트롤러는 모델에 상태값을 제어하는 역할을 하는 방식으로 작업했습니다.
View
view와 관련해서는 컨트롤러가 인스턴스를 생성할 때
자판기 모델을 여러개 생성한다면이라는 조건 하에 제어될 view의 부모 element를 부여해주고 있습니다.
Controller
컨트롤러는 모델과 뷰을 통제하고, 모델과 뷰는 결합도를 낮추기 위해 노력했습니다.
이전 미션까지는 view라는 클래스를 생성하여 view와 관련된 모든 부분을 view.js에서 제어하는 방식으로 작업을 취했는데,
view에서 결합도를 낮추려하다보니 dom api를 단순하게 나열하는 방식의 메소드의 집합이 되어서
클래스로 관리할 목적이 없다고 판단하여 utils/view.js에 사용될 메소드를 정의하여 컨트롤러에서 제어하고 있습니다.
컨트롤러는 Component 클래스를 통해 추상화를 진행하였고,
추상클래스에 인스턴스를 생성할 수 없도록 예외처리 하였습니다.
common과 utils
common에는 목적에 따라 관리가 필요한 어플리케이션의 기능이나 로직과 관련이 있는 모듈을,
utils은 어플리케이션의 기능 사항과는 전혀 관련이 없는 개발의 편의를 위한 모듈을 관리하고 있습니다.
작업 상황
돈통 충전에서 잔돈을 생성하는 경우에 최적화 방식(제일 높은 잔돈부터 차례로 생성)과 무작위 방식(랜덤 생성)을 고민했는데,
아무래도 자판기의 돈통은 높은 잔돈만 많이 생성되는 방식은 옳지않다고 판단하여 요구사항에 쓰인대로 무작위방식을 택했습니다.
고민
기존에 eventListener를 컨트롤러에서 일일이 직접 호출 하는 방식을 사용하고 있었는데,
이번에는 객체로 만들어 관리를 해보았습니다.
코드의 길이가 괜히 방대해지는 느낌이 있기도 해서 이걸 좀 더 스마트하게 처리하는 방법이 있을지 고민 중입니다.
리뷰어님께 조언을 부탁드립니다!
마지막 미션인 만큼 이제껏 배운 것을 잘 적용해보고 더 배워가면서 마무리할 수 있으면 좋겠습니다. 잘부탁드립니다!