[클린코드 2기 소인성] 로또 미션 STEP3 #112
Conversation
- 도메인을 분리하고 안정 장치를 마련한다.
- 유틸 함수는 도메인에 상관이 없어야 하므로 도메인 로직으로 분리한다.
- 컴포넌트는 일관된 동작만을 가진다.
- 함수형 서비스 로직을 클래스로 변경
- 폴더 변경 - App.actions > LottoResult.actions로 액션 변경 - 에디터 플러그인 적용
- 상수 추가 - 수동 구매 개수에 따라 폼을 그려주는 로직 추가
- 파일 추가 - App.actions > LottoCheck.actions로 액션 변경 - 에디터 플러그인 적용
- App.actions 간소화 - 수동 구매에 따른 로또 생성 기능 수정 - 에디터 플러그인 적용
|
@InSeong-So |
어제 새벽에 리뷰 요청이 없어서 한참 찾다가 리뷰어님께 DM 드렸었는데 지금 보니 있네요😨 요청했습니다! |
src/js/components/App.actions.js
Outdated
| $modal.replaceChildren($lottoResultModal); | ||
| } | ||
| $modal.classList.toggle('open'); | ||
| const count = LottoService.validAmount($amountInput.value); |
There was a problem hiding this comment.
메서드 이름이 validCount에서 validAmount로 바뀐 것 같은데 맞나요??
이름과 반환값이 잘 호응하지 않는 것 같아요!
There was a problem hiding this comment.
그리고 input에 대한 접근은 event.target을 통해 하는게 적절해 보입니다.
event 변수를 받고 있음에도 특정 dom 엘리먼트를 의존하면 핸들러로서 재사용 성이 크게 떨어질 것 같아요.
더불어 inputAmount와 같이 dom 의존적인 네이밍보다 handleSubmitAmount처럼 이벤트 의존적인 네이밍을 사용했다면 이러한 문제가 더 잘 드러났을 것 같습니다!
There was a problem hiding this comment.
언급하신 대로 반환 값과 네이밍을 일치시키도록 수정해야겠습니다!
스탭2까지만 진행했을 때는 앱의 규모가 작아서 도메인과 유틸에 대한 분리를 느슨하게 했었는데, 스탭3를 수행하면서 비슷한 맥락의 로직이 모두 도메인에 포함되는 바람에 코드 중복이 발생하고 가독성이 떨어지면서 장황해진 것 같습니다.
도메인을 최대한 작게, 유틸을 풍부하게 가져가는 것이 옳아 보이네요! 스탭2 때 태은님이 해주셨던 리뷰를 다시 읽고 있습니다🥺 리팩토링 해볼게요!
src/js/components/App.actions.js
Outdated
| const count = LottoService.validAmount($amountInput.value); | ||
| const $lottoManualPurchase = LottoManualPurchase(count); | ||
| $modal.replaceChildren($lottoManualPurchase); | ||
| $elementToggleClass($modal, 'open'); | ||
| } catch (error) { | ||
| $amountInput.value = ''; | ||
| alert(error.message); | ||
| } | ||
| }; |
There was a problem hiding this comment.
후에 리팩터링 하실 때 고려해보면 좋으실 부분들 남깁니다.
- 추상화 수준
각 함수가 알아야 할 부분을 제한해 보면 어떨까 싶어요!
지금 이 핸들러는 dom에 대한 참조, 새롭게 렌더할 엘리먼트($lottoManualPurchase), 렌더 방식, 에러처리 방식 등 너무 많은 것을 알고 있는 것 같습니다.
const inputAmount = (event) => {
event.preventDefault()
try {
// html 상에서 input.name = 'amount-input'이라고 전제
const totalLottoCount = LottoService.validAmount(event.target.elements['amount-input'])
showLottoManualPurchaseModal(totalLottoCount)
} catch (error) {
handleFormError({ target: event.target, error })
}
}
const showLottoManualPurchaseModal = (totalLottoCount) => {
const $modal = $('.modal')
const $lottoManualPurchase = LottoManualPurchase(totalLottoCount)
openElement({ target: $modal, children: [$lottoManualPurchase] })
}
const openElement = ({ target, children }) => {
target.replacseChildren(..children)
$elementAddClass(target, 'open')
}
const handleFormError = ({ target, error }) => {
target.reset()
alert(error.message)
}이러한 방식으로 분리를 해보면 각 함수가 집중해야한 '하나의 일'이 무엇인지 보입니다!
위 코드는 예시일 뿐이니 더 좋은 방법 고민해보시면 좋겠습니다.
여기서 더 추상적으로 가려면 inputAmount를 고차함수로 만들어 inputName을 인자로 받도록 하는 등의 작업도 해볼 수 있을 것 같습니다.
const inputAmount = (inputName) => (event) => { ... }-
toggelClass vs addClass
개인적으로는 toggleClass가 적절한 선택이었는지 의문이듭니다.
제가 이해하기로 이 submit 이벤트핸들러는 modal을 보여주고자 하는 명확한 목적을 가지고 있습니다.
그렇다면 차라리 classname을 add하는 방식이 더 적절하지 않을까요?
toggle에 경우 이 함수의 내용만으로는 그 결과를 예측하기가 어려워보입니다! -
주어진 dom 객체 활용
inputAmount는 submit 이벤트임에도 불구하고 form 엘리먼트 객체를 충분히 활용하지 못하고 있습니다.
form 안에서 name속성을 통해 각 input을 특정한다던가, form 엘리먼트 객체가 지원하는 reset 메서드를 활용할 수 있는 점으로 고려해보시면 좋겠습니다.
지금과 같은 방식으로는 form안에 input이 늘어남에 따라 알아야할 dom 레퍼런스도 늘어날 것 같습니다.
특히 name을 잘 사용하면 dataset을 selector처럼 사용하는 일도 줄어들 것 같습니다.
There was a problem hiding this comment.
맞아요! 이벤트에 많은 역할을 너무 포함 시켰네요. 레이싱 게임 미션을 진행하면서 아키텍처를 먼저 설계하고 로또 미션을 보니 추상화라는 명목 하에 역할과 책임이 분리되지 않은 함수가 많이 보입니다.
이벤트 함수를 명확하게 분리하고 action 등을 가져다 사용하는 형태로 작업해보겠습니다🙌
classList를 제어하는 형태도 단순 기능 구현에만 눈이 멀었습니다😭 수정해볼게요!
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if (!event.target.matches('.modal-close')) return; | ||
| $elementRemoveClass($('.modal'), 'open'); |
There was a problem hiding this comment.
네이밍 일치를 하지 못했네요...! 일관성에 초점을 두고 리팩토링을 진행해보겠습니다🙇♂️
src/js/services/Lotto.service.js
Outdated
| ${Array.from({ length: count }) | ||
| .map( | ||
| (item, index) => /*html*/ ` | ||
| <div class="d-flex mb-3" data-props="lotto-purchase-numbers" key="${index}"> |
There was a problem hiding this comment.
이것저것 시도하다 남은 코드네요😨 지우겠습니다!
src/js/services/Lotto.service.js
Outdated
| @@ -20,6 +21,47 @@ class LottoService { | |||
| 6: 2000000000, | |||
| }; | |||
|
|
|||
| createManualPurchaseFormContent = count => { | |||
There was a problem hiding this comment.
거대한 앱의 모든 서비스를 해당 클래스에서만 담당하니 소위 주어진 일 뿐만 아니라 부업도 한 느낌입니다🤤 이 부분 정말 많이 고민했는데 리팩토링하면서 Lotto.service의 역할을 많이 덜어줘야겠네요😳
|
핸들러에 대한 추상화를 커링으로 구현할까 했지만 결국 고민에서 해결까지 이르는 시간이 너무 오래 걸리네요... 이사가 당장 다음주라 우선 분리하는 부분까지 피드백 적용해서 재요청 드립니다😭🙇♂️ |
| export const $handleDOMError = ({ $element, isReset, error }) => { | ||
| if ($element && $element instanceof HTMLElement && isReset) { | ||
| $element.value = ''; | ||
| $element.textContent = ''; | ||
| } |
There was a problem hiding this comment.
이 부분을 조금 고민했는데... 옵셔널 체이닝을 걸거나 다른 방안을 취해야겠네요! 수정하겠습니다😊
감사합니다, 태은님! 머지해주셔도 될 것 같아요🙇♂️ 고생하셨습니다😁 |
안녕하세요, 클린코드 2기 소인성 STEP 3 제출합니다.
시간이 부족해서 TDD를 적용하지 못했고 리팩토링을 많이 진행하지 못했습니다ㅠ 시간이 나면 계속 작업해보려고 합니다.
구현
실행화면 바로가기
웹 VSCode로 보기