Skip to content
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

[1단계 - 행운의 로또 미션] 호프(김문희) 미션 제출합니다 #89

Merged
merged 29 commits into from
Feb 27, 2022

Conversation

moonheekim0118
Copy link

@moonheekim0118 moonheekim0118 commented Feb 24, 2022

안녕하세요 리뷰어님 :) 로또 미션 1단계 제출합니다.

데모페이지

파일 구조

 ┣ 📂src
 ┃ ┣ 📂css
 ┃ ┃ ┗ 📜index.css
 ┃ ┣ 📂js
 ┃ ┃ ┣ 📂__tests__
 ┃ ┃ ┃ ┗ 📜app.test.js
 ┃ ┃ ┣ 📂utils
 ┃ ┃ ┃ ┣ 📜common.js
 ┃ ┃ ┃ ┣ 📜dom.js
 ┃ ┃ ┃ ┗ 📜validation.js
 ┃ ┃ ┣ 📂view
 ┃ ┃ ┃ ┣ 📜LottoList.js
 ┃ ┃ ┃ ┗ 📜PurchaseForm.js
 ┃ ┃ ┣ 📜app.js
 ┃ ┃ ┣ 📜constants.js
 ┃ ┃ ┣ 📜controller.js
 ┃ ┃ ┗ 📜model.js
 ┃ ┗ 📜index.js
 ┣ 📜.eslintrc.json
 ┣ 📜.gitignore
 ┣ 📜.prettierrc.json
 ┣ 📜LICENSE
 ┣ 📜README.md
 ┣ 📜babel.config.js
 ┣ 📜index.html
 ┣ 📜package-lock.json
 ┣ 📜package.json
 ┗ 📜webpack.config.js

1단계 요구사항

  • 로또 구입 금액을 입력하면, 금액에 해당하는 로또를 발급해야 한다.
  • 로또 1장의 가격은 1,000원이다.
  • 소비자는 자동 구매를 할 수 있어야 한다.
  • 번호 보기 토글 버튼을 클릭해 로또 번호를 볼 수 있어야 한다.

궁금한 점

  • 이번에 처음으로 단위 테스트를 작성하게 되었는데요, 궁금한 점이 있어 질문드립니다.
    작성 기준으로 하나의 함수의 동작을 검증한다 를 삼았습니다. 그런데 예를들면 onSubmitCash라는 함수에서 validation해당 뷰의 이벤트 핸들링두 가지 함수를 호출하고, 또 onSubmitCash 라는 함수 자체의 역할을 실행한다면, onSubmitCash라는 함수를 단위테스트하는게 아니라, onSubmitCash 함수내부에서 호출하는 모든 함수에 대한 단위테스트를 각각 작성하는게 맞을까요?
    그렇게 된다면 만약에 onSubmitCash 함수 내부에서 호출되는 함수가 요구사항에 따라서 바뀐다면 테스트 코드도 다 바꾸게 될텐데, 그렇게 검증하는게 옳을지 고민입니다!
    단위테스트에서 한 단위의 테스트를 나누는 기준이 아직 명확하지 않아 질문 드립니다!

그럼, 감사합니다 :)

euijinkk and others added 23 commits February 22, 2022 14:51
Co-authored-by: moonheekim0118 hellomooneekim@gmail.com
Co-authored-by: moonheekim0118 hellomooneekim@gmail.com
Co-authored-by: moonheekim0118 hellomooneekim@gmail.com
Co-authored-by: moonheekim0118 hellomooneekim@gmail.com
Co-authored-by: moonheekim0118 hellomooneekim@gmail.com
Co-authored-by: moonheekim0118 hellomooneekim@gmail.com
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Co-authored-by: moonheekim0118 <hellomooneekim@gmail.com>
Copy link

@lsw1164 lsw1164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 호프님, 코드 잘봤습니다.
access modifier를 적절히 사용해서 캡슐화에 신경써주셨군요 👍

테스트 코드에 제 생각을 말씀드리자면 정답이 없다고 생각합니다.
개인적인 기준은 public 메서드는 인터페이스라는 측면에서 상대적으로 잘안바뀌고 외부에 직접 노출하는 기능이기에 테스트를 작성하는 게 좋다고 생각합니다.
반면에 private 메서드는 바뀔 여지가 많고 외부에서 직접 접근하지 않는 기능이라 특별한 경우가 아니라면 작성하지 않는 편인데요.

만약 메서드들이 전부 public이면 전 각각도 테스트를 작성하고 상위 메서드에 대한 테스트도 작성해 볼 거 같습니다.

그리고 요구사항이 바뀐다면 테스트도 바뀔 수 있습니다.
테스트 코드를 작성하는 이유는 지금까지의 요구사항을 자동으로 검증해주기에
앞으로의 코드 추가/수정을 도와준다고 생각합니다.
만약 그 요구사항의 변경이 기존의 검증 방식을 바꿀 정도라면? 당연히 테스트 코드도 바꿔야된다 생각합니다 :)

Comment on lines 1 to 2
export const $ = (selector, target = document) => target.querySelector(`#${selector}`);
export const $$ = (selector, target = document) => target.querySelectorAll(`#${selector}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수명은 selector라고 돼있지만, 실제로는 id string을 의미하는 거 같습니다.
파라미터로 받은 id에 # prefix 붙여서 selector로 바꿔주고 있군요
id가 아니라 className이 파라미터로 전달되면 찾을 수 없을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일부러 class는 CSS측에서만 사용하고, 자바스크립트 측에서는 id로만 엘리먼트를 선택하도록 사용해서 저렇게 구현하게 되었습니다만, 지금 보니 selector라는 이름과 맞지 않아서 혼동을 주네요.. 😢

src/js/constants.js Outdated Show resolved Hide resolved
src/js/view/LottoList.js Outdated Show resolved Hide resolved
src/js/model.js Outdated
@@ -0,0 +1,24 @@
import { LOTTO_RULE } from './constants.js';
import { generateRandomNumber } from './utils/common.js';
export default class Model {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Model은 어떤 의미로 작성한 네이밍일까요?

Copy link
Author

@moonheekim0118 moonheekim0118 Feb 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MVC 디자인패턴으로 구현하다보니 , 데이터를 관리해주는 클래스 이름을 Model 이라고 짓게 되었습니다. 그런데 생각해보니 갖고 있는 데이터가 로또 리스트 딱 하나라서, 네이밍을 그냥 LottoList 혹은 LottoData 라고 해주는게 더 좋을지 고민입니다. 이에 대한 리뷰어님의 의견은 어떠실까요..?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네, 그냥 Model 보다는 어떤 의미를 담은 객체인지 알기 쉬운 네이밍을 고민해보면 좋겠습니다 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 의견 감사드립니다 :)

@moonheekim0118
Copy link
Author

moonheekim0118 commented Feb 27, 2022

안녕하세요 리뷰어님 :) 좋은 답변 그리고 리뷰와 의견 감사드립니다. 1단계 리팩토링 결과는 다음과 같습니다.

1. Selector 상수 및 유틸 함수 수정

  • 말씀해주신 대로 Selector는 앞에 '#' 혹은 '.'가 붙여야 한다고 판단했고, querySelector 함수 역시 앞에 '#'을 붙여서 사용하면 범용성이 떨어진다고 생각하여서 변경해주었습니다 :)

2. View 라는 추상 클래스 생성

  • onClickToggle 메서드 이름을 수정하다가, 뷰에서 돔을 설정해주고 이벤트를 바인딩 해주는 메서드를 호출하는게 반복되어서 추상화 시켜보았습니다.

3. Model 이름 수정

  • LottoData 라는 이름으로 수정하여서 로또에 관한 데이터를 담고 있음을 명시해주었습니다 :)

4. 로또 번호 생성 함수 유틸화

  • 해당 함수는 굳이 lottoData 클래스에 있을 필요가 없다고 생각하여 유틸 함수로 따로 분리했습니다.

4. 테스트 코드 추가

  • 로또 번호 생성 함수에 대한 테스트 코드를 추가했습니다. 반환된 값이 1부터 45사이인지에 대한 테스트를 작성했습니다:)

감사합니다 :)

Copy link

@lsw1164 lsw1164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 호프님, Approve 하겠습니다. 다음 스텝 진행해주세요 :)

@lsw1164 lsw1164 merged commit d34815d into woowacourse:moonheekim0118 Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants