[클린코드 4기 이철환] 자판기 미션 STEP 1 #61
Conversation
feat: step1 초안 완료
|
@go-lani |
cypress/e2e/change-charger.cy.js
Outdated
| it("잔돈은 10원 단위로 충전이 가능하다", () => { | ||
| cy.alert({ | ||
| action: () => { | ||
| cy.get(CHARGER_INPUT_SELECTOR).type("101"); | ||
| return cy.get(COIN_CHARGE_BUTTON_SELECTOR).click(); | ||
| }, | ||
| message: ERROR_MESSAGE.INVALID_UNIT, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
사소하긴 하지만 테스트 케이스랑 실제 테스트 코드랑 조금 상이해 보여요.
코드는 구체적인 반면 테스트케이스 텍스트가 조금 모호한 것 같아요.
잔돈은 10원 단위로 충전이 가능하다라는 텍스트만 봐서는 실제 view에 대한 기획이 잘 안그려집니다.
사용자가 실제로 10원을 입력했을 때 정확히 어떤 일이 일어나나요? 혹은 11원을 입력했을 때는 어떤 일이 일어나나요?
그리고 에러는 alert로 보여주는지, 토스트와 같은 UI로 보여주는지, input 밑에 에러를 보여주는지 테스트 케이스만 보고도 알 수 있었으면 합니다.
사용자가 어떤 행동(입력)을 했을 때 프로그램이 어떤 행동(출력)을 하는지 구체적으로 서술해주세요.
There was a problem hiding this comment.
그렇네요.. 허허 .. 10원 단위로 충전하다를 context로 그룹핑해서 11원, 101원 1001원 이런식의 단위를 전부 테스트를 해야하나 고민했으나 그렇게한다면 어디까지 커버해야될지에 대한 의문이 들어서 테스트 케이스 타이틀만 수정했습니다!
리뷰 반영 커밋
18ca601
cypress/e2e/change-charger.cy.js
Outdated
| it("최초 보유 금액은 0원이다.", () => { | ||
| cy.get(HOLDING_AMOUNT_SELECTOR).should("have.text", "0"); | ||
| }); |
There was a problem hiding this comment.
엇..🤦 그렇네요 ㅎㅎ... 삭제했습니다
리뷰 반영 커밋
bbee807
| }); | ||
|
|
||
| it("잔돈 입력후 충전하기 버튼을 눌러서 충전할 수 있다", () => { | ||
| cy.get(CHARGER_INPUT_SELECTOR).type(String(MINIMUM_CHARGE_PRICE)); |
There was a problem hiding this comment.
(마이너 코멘트)
String으로 캐스팅 안해도 type 메서드를 거치면 알아서 string으로 들어갈겁니다.
input value는 항상 string이니까요!
There was a problem hiding this comment.
@Tanney-102 공식 문서에 type 메소드의 argument로 string 값을 받는 걸로 표기되어 있고, IDE에서 경고를 줘서 String 생성자 함수로 강제 변환을 시켰습니다!
혹시 type에서 number 타입으로 입력되더라도 string 값으로 타입변환이 되는걸까요?? 별도로 문구가 안보여가지구요!
src/js/controller/vendingMachine.js
Outdated
| changeView($target) { | ||
| this.#models[$target.name].initialize(); | ||
| this.#handlers[$target.name](); | ||
| } |
There was a problem hiding this comment.
| changeView($target) { | |
| this.#models[$target.name].initialize(); | |
| this.#handlers[$target.name](); | |
| } | |
| changeView(menuName) { | |
| this.#models[menuName].initialize(); | |
| this.#handlers[meneName](); | |
| } |
changeView라는 이름도 조금 안어울리는 것 같네요
함수 내부 로직을 충분히 설명하고 있는지 고민이 필요해 보입니다.
src/js/view/changeCharger.js
Outdated
| clearForm() { | ||
| const $chargerInput = $("#coin-charging-form .charger-input"); | ||
| $chargerInput.value = ""; | ||
| $chargerInput.focus(); | ||
| } |
There was a problem hiding this comment.
form 엘리먼트가 가지고 있는 reset 메서드를 활용해보시면 좋을 것 같습니다.
There was a problem hiding this comment.
아하.. reset이 있었군요! ㅎㅎ
reset을 활용하다보니 공통적인 부분이 보여 별도 유틸 함수로 분리해서 작업해보았습니다!
리뷰 반영 커밋
9e27779
src/js/model/changeCharger.js
Outdated
| } | ||
|
|
||
| setState(state, newState) { | ||
| this.#state[state] += newState; |
There was a problem hiding this comment.
흠... 유효한 로직이 아닌 것 같아서 switch case문으로 분기처리를 해보았습니다.
리뷰 반영 커밋
d059ffb
src/js/model/changeCharger.js
Outdated
| initialize() { | ||
| this.#view.render(); | ||
|
|
||
| if (isInitialState(this.#state, CHANGE_CHARGER_INITIAL_STATE) === false) { | ||
| this.#view.update(this.#state); | ||
| } | ||
| } |
There was a problem hiding this comment.
initialize 메서드 안에서 isInitialState 여부를 두고 분기를 하는게 조금 어색하네요.
애초에 initialize와 Update 동작은 다른 메서드로 작성되어야하지 않았을까요?
There was a problem hiding this comment.
네 문제가 많았네요! ㅠㅠ render 메소드가 상속 받아 사용하다보니, update시에도 전처리로 레이아웃을 그려주는 render 메소드를 호출하려다 보니 저렇게 작업했던 것 같습니다.
저렇게 작업되다보니 초기 initialize 뿐만 아니라 update시에도 render 메소드가 호출되어 불필요하게 레이아웃을 그려줬었네요. 🤦
초기 렌더링 시(메뉴 이동 또는 최초 페이지 접근 시)에만 render 메소드를 호출해주고 Update일때는 내부 컨텐츠만 업데이트 되도록 분리시켜봤습니다!
리뷰 반영 커밋
617e1ee
src/js/model/changeCharger.js
Outdated
| const initialState = | ||
| getLocalStorage("charger") || CHANGE_CHARGER_INITIAL_STATE; | ||
| this.#state = { ...initialState }; | ||
| this.#view = new ChangeChargerView(); |
There was a problem hiding this comment.
모델에서 view을 의존, 직접 업데이트까지 해주는게 좋은 방법인지는 잘 모르겠습니다.
이 모델이 하나의 view랑만 연관된다는 보장이 있을까요?
이 앱이 실제 서비스였다면 기획에 따라 다른 페이지에서 이 모델의 데이터를 화면에 표현할 수 있는 여지는 충분이 있다고 생각됩니다.
만약 이 모델과 연관된 VIew가 늘어나면 매번 모델이 수정되어야 할까요?
모델은 데이터만 관리하고 데이터 변경에 따라 연관된 view이 반응하도록 하면 어떨까요?
proxy나 custom event를 사용하거나, 혹은 단순한 observer를 구현하여 view가 model의 변경을 구독하는 방식을 제안드려봅니다.
There was a problem hiding this comment.
패턴을 쓰는 방식보다 의존성을 떼어내보고자 Model선언시 View를 argument로 전해주는 방식은 어떻게 생각하시나요?? 이렇게 한다면 말씀하신대로 모델은 하나의 view에만 종속되지 않을 것 같다고 생각되는데 제가 잘못 생각하고 있는 걸까요??
view의 경우에는 update 메소드를 추상화하여 유연하게 작성할 수 있게 했는데.. 이부분에도 문제가 있을까요??
const managerModel = new ProductManagerModel(new ProductManagerView());
const chargerModel = new ChangeChargerModel(new ChangeChargerView());
const purchaseModel = new ProductPurchaseModel(new ProductPurchaseView());리뷰반영 코드
3d4b6ed
안녕하세요 태은님!
PR이 많이 늦어서 죄송합니다..!
이번 미션을 진행하면서 시도해본 것은 다음과 같습니다!
첫번째 접근으로는
이렇게 접근했을때 하나의 Model이 모든 View를 책임져야하므로 유지보수하기 어렵다고 판단했습니다.
선택한 방법은
Controller -> Model -> View 이렇게 단방향으로 흐름을 가져갈 수 있었습니다.
이렇게 하다보니 각각의 Model별로 View가 작성되어야해서 1대1 관계를 유지하게되어 파일 구조가 좀 지저분해진 것 같습니다.
사실상 활용을 잘했다고 생각하진 못했습니다.
좋았던 점은 디자인 시스템이 없는 상황에서 통일된 UI를 표현할 수 있어 좋았습니다!
아쉬웠던 점은 Webstorm을 사용하고 있어 플러그인이 따로 없어 일일히 공식사이트에서 검색해가며 작업하다보니 번거로움이 있었습니다.
여쭤보고 싶은 점은
위 4개 이외에도 태은님의 생각을 듣고 싶습니다!
사소한 것들까지 언급해주시면 감사하겠습니다!
다시 한번 미션 진행이 늦은 점 죄송합니다..! 잘 부탁드립니다! 🙏