-
Notifications
You must be signed in to change notification settings - Fork 168
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
[2단계 - 행운의 로또 미션] 곤이(김기융) 미션 제출합니다. #40
Conversation
0d5364c
to
d69f3c1
Compare
- rename file (camelCase) - dependency injection - DOM selector (class -> id, dataset) - form tag event - private field
- test valid winning numbers
- check winning numbers
- render winning result - reset
- remove Parentnode
- check winning numbers
- check modal - check reset
- change earning rate
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 라이브러리를 만들어 사용해보았습니다. 커스텀 DOM 라이브러리를 직접 만들어서 사용하니 이전보다 더 코드를 깔끔하고 효율적으로 짤 수 있다는 생각을 했습니다. 그러나, 같은 DOM을 조회해도 생성자를 이용하기 때문에 인스턴스가 새로 생성된다는 점에서 더 좋은 방법이 있을지 고민하고 있습니다. 이 외에도 코드를 보시면서 보이는 문제점에 대해서 말씀해주시면 많은 도움이 될 것 같습니다!
-
좋은 시도입니다. 👍
-
다만 이렇게 많은 동작들이 추상화 되게 되면, 다양한 trade off가 있습니다.
당장 작성하는 어플리케이션쪽 코드는 짧고 깔끔해질지 몰라도, 추상화 수준이 높아지면 높아질수록 이 DOM 라이브러리 코드는 더 복잡해지고 비대해질 것이구요. 심할경우 이러한 추상 인터페이스들을 정말 잘 문서화해놓고 사용법을 잘 정리해놓고 유지보수해주지 않으면, 당장 이 DOM 라이브러리를 사용할줄 모르는 다른 팀원이나 신규팀원들은 이 인터페이스를 익히지 못하면 코드를 작성하지 못하는 수준에 이를 수 있습니다. -
이러한 라이브러리를 만들어보는 것은 굉장히 좋은 시도이지만, 실제 실무에서 적용해본다면 위에서 말씀드린 것과 같은 비용도 충분히 고민해보셔야합니다. (그리고 대부분의 경우에는 직접 만들기보다 잘 만들어진 jQuery, React 같은 라이브러리를 쓰는 경우가 많은데, 왜 직접 만들어 쓰지않고 이런 잘 만들어진 라이브러리를 쓸까? 를 고민해보시면 좋을 것같아요.)
-
같은 DOM을 조회했을때 querySelect를 새로하는 문제는 캐시로 해결할 수 있습니다.
-
selector key값을 받아서, 해당 키값으로 select된 DOM ref를 저장해두고, 다음에 같은 key값을 받았을때 다시 querySelect하는 것이 아닌, 캐시에 저장해둔 값을 반환하면 됩니다. (단, 이러한 방법은 모든 key가 변하지않는 유니크한 DOM ref를 반환한다는 보장이 있어야 안전합니다.)
-
Lodash 와 같은 유틸 라이브러리에 포함되어있는 memoize 함수가 비슷한 동작을 합니다. 어떻게 구현되어있는지 알아보고 memoization에 대해 공부해보세요. 👍
이전 미션에서 DOM을 중복해서 조회하는 것보다 저장을 해놓는 편이 좋다는 피드백을 주셨습니다. 당시에는 말씀해주신대로 중복된 DOM 사용을 피하는 것이 당연하다고 생각하여 코드를 수정했습니다. 그러나 DOM을 공부하다보니 DOM의 속도는 자바스크립트 엔진과 비교해서 느리지 않다는 사실을 알게 됐습니다. DOM 조작의 경우에는 리플로우가 발생하여 최대한 DOM 조작을 최소화하는 것이 맞지만 단순히 DOM을 조회하는 것 역시 과연 비용이 클까? 라는 의문이 생겼습니다.
- '단순히 DOM을 조회하는 것이 비용이 클까?' 라는 질문에는 그럴수도 있고 아닐 수도 있습니다. 비용이 큰지 작은지를 비교하기 위해서는 전제조건이 필요합니다.
- MDN querySelector 문서를 보시면 DOM tree를
depth-first pre-order traversal
(깊이 우선 preorder 탐색) 한다고 되어있는데요. 이말은 즉, querySelector의 성능은 DOM tree의 크기와 어느정도 비례한다는 것입니다. - 전체 문서의 크기가 굉장히 커서 탐색해야할 노드가 많다면, 성능은 더 나빠지겠죠.
- 반대로말하면 전체 DOM tree가 작음을 보장할 수 있다면, 매번 querySelect하더라도 큰 성능차이를 느끼지 못할 수도 있습니다.
cypress/utils/testInputValue.js
Outdated
alertMessage && checkAlert(alertMessage); | ||
cy.get(input).should('have.value', alertMessage ? '' : value); | ||
// cy.get(input).should('have.value', alertMessage ? '' : value); |
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/constants/alertMessage.js
Outdated
@@ -1,3 +1,6 @@ | |||
import { UNIT_AMOUNT } from './standard.js'; | |||
|
|||
export const MSG_INVALID_PURCHASE_AMOUNT = `금액을 ${UNIT_AMOUNT}원 단위로 입력해주세요!`; | |||
export const MSG_OVERLAPPED_LOTTO_NUMBERS = `중복된 숫자를 입력하셨습니다. 다시 입력해주세요!`; | |||
export const MSG_OUT_RANGED_LOTTO_NUMBERS = `범위 밖의 숫자를 입력하셨습니다. 1부터 45 사이의 숫자를 입력해주세요!`; | |||
export const MSG_BLANK_INPUT = `빈 칸을 입력하셨습니다. 다시 입력해주세요!`; |
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.
end of file에 대한 내용은 공통 피드백 시간에서 다뤘음에도 불구하고 놓쳐버렸네요 😂
vscode setting에서 "files.insertFinalNewline": true를 추가했습니다.
src/js/model/Lotto.js
Outdated
} | ||
|
||
get numbers() { | ||
return [...this._numbers]; | ||
return [...this.#numbers]; |
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.
그냥 return this.#numbers
하지 않고 spread해서 새로운 배열로 반환하는 이유가 있을까요? 👀
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.
최초의 목적은 배열은 객체이기 때문에 배열 자체를 넘겨주는 경우, 값의 변경이 가능하다고 판단하였습니다. 따라서 이를 외부에서 수정하지 못하도록 배열을 복사하여 은닉화를 하려고 했습니다. 그러나 getter만 있는 경우, 값을 재할당하려는 시도가 있을 때 에러를 띄우므로 불필요한 것 같습니다!
src/js/util/DOM.js
Outdated
@@ -19,6 +19,22 @@ export const $ = (() => { | |||
return this; | |||
}; | |||
|
|||
constructor.prototype.map = function (callBack) { | |||
if (!callBack || typeof callBack !== 'function') { | |||
return; |
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.
라이브러리 사용자가 라이브러리 제작자가 의도하지 않은 방향으로 사용할 때에는
undefined를 리턴하는것 보다는, 명시적으로 자바스크립트가 에러를 뱉고 동작을 정지할 수 있도록 throw new Error
를 하는 것이 좋아보입니다.
undefined를 리턴할 경우, 사용부에서 에러가 나지않으면, 이 라이브러리를 잘못 사용했는데도 그 원인이 어디에있는지 파악하기 어려울 수 있습니다.
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/util/validator.js
Outdated
MSG_INVALID_PURCHASE_AMOUNT, | ||
UNIT_AMOUNT, | ||
} from '../constants/index.js'; | ||
|
||
const isRangeOf = (min, max, numbers) => numbers.every(number => min <= number && number <= max); | ||
|
||
const isOverlapped = numbers => numbers.length !== new Set(numbers).size; |
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.
isDuplicated
라는 이름이 더 적절해보여요! 👍
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.
수정했습니다!
안녕하세요 리뷰어님 :) 리뷰어님께서 말씀해주신 Lodash의 memoize 함수를 참고하면서 많은 부분을 새롭게 알 수 있었습니다. Object 보다 해싱에 최적화된 Map이라는 객체가 존재한다는 사실과 캐싱을 통해 성능을 끌어올릴 수 있다는 것을 알게 됐습니다. Lodash의 memoize 함수를 참고하여 좀 더 단순한 memoize 함수를 만들어봤습니다. 부족한 부분 알려주시면 감사하겠습니다 🙋♂️ 이 외에도 정말 많은 부분에서 인사이트를 얻을 수 있었습니다. 항상 감사드립니다 😄 |
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.
고생하셨습니다! 👍
안녕하세요! 곤이입니다.
항상 바쁘신 와중에도 소중한 시간 내주셔서 감사합니다 😀😀
이전 미션에서 추가로 다음과 같은 기능과 테스트를 추가했습니다.
🎯🎯 step2 당첨 결과 기능
🔗 기능 상세
🎞 테스트 케이스
🙋♂️ 궁금한 점
카일과 미션을 진행하면서 커스텀 DOM 라이브러리를 만들어 사용해보았습니다. 커스텀 DOM 라이브러리를 직접 만들어서 사용하니 이전보다 더 코드를 깔끔하고 효율적으로 짤 수 있다는 생각을 했습니다. 그러나, 같은 DOM을 조회해도 생성자를 이용하기 때문에 인스턴스가 새로 생성된다는 점에서 더 좋은 방법이 있을지 고민하고 있습니다. 이 외에도 코드를 보시면서 보이는 문제점에 대해서 말씀해주시면 많은 도움이 될 것 같습니다!
이전 미션에서 DOM을 중복해서 조회하는 것보다 저장을 해놓는 편이 좋다는 피드백을 주셨습니다. 당시에는 말씀해주신대로 중복된 DOM 사용을 피하는 것이 당연하다고 생각하여 코드를 수정했습니다. 그러나 DOM을 공부하다보니 DOM의 속도는 자바스크립트 엔진과 비교해서 느리지 않다는 사실을 알게 됐습니다. DOM 조작의 경우에는 리플로우가 발생하여 최대한 DOM 조작을 최소화하는 것이 맞지만 단순히 DOM을 조회하는 것 역시 과연 비용이 클까? 라는 의문이 생겼습니다.
🧶데모 사이트