[2단계 - 행운의 로또 미션] 서니(박선희) 미션 제출합니다. #53
Conversation
- 당첨 번호를 입력하는 form에 `min`, `max` 속성을 통한 validation 추가 - `hidden` 클래스의 동작을 `display: none`으로 변경 - 이미 기본값으로 적용되는 CSS 구문 삭제 - 이벤트 핸들러 함수들의 이름 변경 - `handleSubmitMoney`에서 돈의 입력값을 이벤트 객체를 통해 받아오도록 수정 - `LottoView`에서 요소를 보여주거나 비활성화 하는 메소드들을 `utils` 함수로 이동 - `show`, `hide` 메소드 이름 변경
- 로또 이모지와 번호 사이에 padding 추가
- 당첨 개수는 0 이상의 정수여야 함 - 수익률은 소수점이 포함된 0 이상의 실수여야 함
- 검증 실패 시 alert 메시지를 확인함 - `constants`에 alert 메시지 추가
- Cypress 문제로 인해, autofocus가 포함된 요소에 수동으로 focus해주는 테스트 코드 추가
- 당첨 번호의 입력 범위와 중복값 확인 - 입력 후 결과 모달이 뜨도록 구현 - 두 자리 수 입력 시 다음 폼으로 자동 포커스 이동되도록 구현
- 당첨 개수를 표시하는 `span`에 `data-rank` 속성 추가 - utils 함수에 `getRandomNumber`를 랜덤 숫자 배열을 리턴하는 함수로 변경 - 당첨 금액 상수화
- 가독성을 위한 코드 수정
- LottoView 에서 Lotto의 멤버 변수를 참조하는 부분을 getter 를 이용하여 변경함. - 상수화 한 부분에 맞추어 변경된 코드 부분 수정
결과 확인하기 버튼을 누를 때 마다 개수별 당첨 로또 개수를 세로 새야 하므로, this.data 에서 관리하지 않고, getResult에서만 이용하는 값으로 사용함.
There was a problem hiding this comment.
안녕하세요 서니님!! 이번 2단계 로또 미션도 수고 많으셨습니다! 👍
상세히 피드백을 드릴만한 흠잡을 코드는 없고, 설명도 자세히, 어떤 이유로 그렇게 만드셨는지 이해가 되었습니다!
그래서 다음 단계를 진행하시면서, 코멘트에 대해 한번 생각해보시고 적용해보시면 될 것 같습니다!
아래는 전반적인 피드백입니다!
-
앱 구조도를 활용해 현재 서로간의 의존도가 어떻게 되어있는지를 지속적으로 체크하시면서 작업하시는 것 같아 대단하신 것 같습니다! 다만, 코드상에서 어떤 함수가 외부로 노출되는지, 노출이 되지 않는지를 구별하기는 쉽지않아서 구별할 수 있도록 내장 메서드나 함수의 경우에는 _ prefix를 붙여서 구분하고, constructor - public - protected - private 순으로 메서드 정렬을 하면 조금 더 구별하기가 쉬울 것 같습니다!
-
전반적으로 1단계와 동일하게 분리가 잘 되어있어서 보기 좋은 코드였습니다! 다만, 점점 View나 App의 코드 줄 수가 많아지고 도메인이 많아질텐데요! 3단계에서는 이러한 View, App(Controller)를 어떻게 분리를 할 지도 고민을 해보시면 좋을 것 같습니다! 현재는 모든 View에 대한 역할을 View에서 하고 있는데, View는 전체 View에 대한 역할을 갖고, 그 안에 세부적인 Module을 분리하여 파일 단위로 쉽게 볼 수 있게 적용을 해보는 경험을 해보시는 것도 좋아보입니다! 👍
아래는 질문주신 부분에 대한 피드백입니다!
- 현재 등수 판단과 관련해서 해당 개수에 따른 등수를 return 해주는 getRankByMatchCount, 와 등수에 따른 상금을 return해주는 getPriceByRank를 lottoUtils로 분리하여 두었습니다. 이렇게 분리한 이유는 해당 로직이 Lotto, LottoApp, LottoView 와 크게 연관성이 없고, 어디에서나 사용 될 수 있으나, 로또 앱에 한정된 유틸성 함수라고 생각했기 때문입니다.
그런데 이렇게 작성하다보니, 현재 앱에서는 getRankByMatchCount는 Lotto 클래스에서만, getPriceByRank는 LottoApp에서만 이용되어서 불필요한 분리가 아니었나 하는 생각이 들었습니다. 이런식으로 조건이나 키에 대한 값을 리턴하는 함수를 만들고, 이용할 때 어느 위치에 있는 것이 더 좋은걸까요?
도메인 단위로 분리하신 부분에 대해 처리를 잘 하신 것 같아요! 이런 친구를 매핑 테이블이라고 하곤 해요. 이런 형태의 함수는 지금 관리하시는데로 도메인에 특화된 친구이기 때문에 서니님이 하신대로 처리하는게 좋아보입니다! 💯
- label을 이용할 때 Navigating 문제가 있어 h 태그는 label tag안에 사용하지 않는 것을 권장한다고, MDN 문서에 작성되어있었습니다. 그래서 현재 index.html에 h4 태그로 되어있는 부분을 스타일 적용을 위해 p 태그로 변경해 둔 상태입니다. 그렇게 변경한 이유는 당첨번호와 보너스번호 부분이 검색과는 크게 연관이 없고, h4 태그가 적용되어 있었기 때문에 그 중요성이 낮다고 판단했기 때문입니다.
이렇게 하는 과정에서 궁금증이 들었습니다. 만약 h태그가 꼭 이용되어야 하는 부분이라면 label로 변경하기 보다, h태그를 이용한 후, 따로 이벤트를 js를 이용하여 걸어주는 것이 맞다는 생각이 들었는데요. 네이버 로고 부분을 확인해 보니 h1 태그내에 img, a 태그 등을 이용하는 것을 확인할 수 있었습니다. 그럼 h 태그 안에 label 태그를 이용할 수 있지 않을까라는 생각도 들었는데, 실제로는 어떻게 이용하는지 궁금합니다.
관점의 차이일 것 같아요. label은 dom에서 쉽게 내부의 Input 이벤트 같은 이벤트를 묶어서 한번에 처리하기위해 사용하는 친구이고, h 태그는 semantic 과 같은 어떤 역할을 하는 지 지정할 때 사용하는 친구이고... 보통 h와 같은 (heading) 태그를 사용하실때는 이벤트 관점보다는 html상의 위상, 이 element가 heading을 해야하는지 등에대한 처리가 필요할 때 사용하는 경우가 많습니다. 저희 프로덕트 같은 경우에는 h 태그 안에 label 태그를 사용하고 있습니다. 위상 상으로, heading tag는 위치나 역할을 부여하는 tag이고, label은 기능에 대한 묶음이기 때문에 heading tag 안에 있는게 맞다고 생각되었기 때문입니다!
- 결과를 계산하는 과정에서, winningRankCounts(당첨 결과에 따른 로또 개수 object)를 메서드 안에서 선언하는 것이 좋은 방법은 아니라고 오늘 수업시간에 피드백을 받았습니다. 그래서 제출 전 수정을 할 까 했으나, 해당 메서드 외부에서 선언하고 이용할 경우 모달에서 취소를 누르고 다시 누르게 되면 해당 object 내의 value가 변경되지 않게 되어서, 변경하지 않고 메서드 내부에 선언하도록 했습니다. 메서드 안에서 선언되지 않으면, 매번 해당 메서드가 실행 될 때마다 object 내부의 value를 초기화해주어야 하는데, 그렇게 해야하는것인지 잘 감이 오지않아서 리뷰어님이라면 어떻게 처리하셨을지 혹은 참고해보면 좋을 키워드 등을 추천해주시면 감사하겠습니다!
아무래도 메서드 내에 상태와 비스무레한게 들어있다면 관리가 어려워지고 객체를 새로 만드는게 되니 관리 포인트가 늘어나므로 좋은 방법이 아니라고 피드백을 주셨던 것 같습니다! 어차피 로또의 갯수나, 입력받는 로또의 위치는 동일하므로 lotto 객체를 배열 7개로 가지고 있고, 배열의 객체에 접근해서 lotto 내에 winningCount 멤버변수를 만들어 그 값을 증가시키는 건 어떨까요? 아니면, class WinningCount를 만들어서 WinningCount 내에 addCount 함수, count 변수, Count getter, 를 만들어서 lotto에서 확장할 수 있게 구현한 다음, 해당 기능을 구현할 때는 class WinningLotto extends WinningCount, Lotto 와 같이 한 다음, 두 기능을 구현하고 new WinningCount 객체를 7개 만들어서 구현할 수도 있겠네요. 저라면 후자의 방법으로 객체지향적으로 구현 했을 것 같습니다!
점점 코드가 성숙해져가고 계시네요! 3단계도 화이팅입니다~!
| @@ -1,22 +1,28 @@ | |||
| import { ALERT_MESSAGE } from '../../src/js/constants.js'; | |||
| import { ALERT_MESSAGE, VALUE } from '../../src/js/constants.js'; | |||
| import Lotto from '../../src/js/models/Lotto.js'; | |||
|
|
|||
| describe('LOTTO 테스트', () => { | |||
| beforeEach(() => { | |||
| cy.visit('http://localhost:5500/'); | |||
| }); | |||
|
|
|||
| it('사용자가 로또 구입 금액을 입력하고 확인 버튼을 누르면 금액에 맞는 로또가 화면에 보여진다.', () => { | |||
There was a problem hiding this comment.
테스트 로직이 점점 길어지고 있는데, 테스트 로직을 분리해보는 건 어떨까요?
가령,도메인 단위로 나눠볼 수도 있겠구요! UI, 기능테스트로 분리해볼 수도 있겠구요!
There was a problem hiding this comment.
이 부분은 기능이 증가함에 따라서 점점 길어지고 있는 것 같아 분리를 해볼 계획이었습니다 : ) 이전에는 에러처리와 메인 기능으로 분리했었는데, 도메인이나 UI/기능으로 구분하는 것도 좋네요!
| <input type="number" class="winning-number mx-1 text-center" /> | ||
| <input type="number" class="winning-number mx-1 text-center" /> | ||
| <input type="number" class="winning-number mx-1 text-center" /> | ||
| <input type="number" name="winning-number" aria-label="winning-number1" id="first-winning-number" |
There was a problem hiding this comment.
해당 부분은 label에서 for을 이용하기 위해서 속성을 주었습니다. 지금 보니 이렇게 사용할거면 js 내에서 focus를 줄 때 이용했어도 좋을 것 같네요.
| </div> | ||
| </div> | ||
| <div class="bonus-number-container flex-grow"> | ||
| <h4 class="mt-0 mb-3 text-center">보너스 번호</h4> | ||
| <label for="bonus-number" class="mt-0 mb-3 text-center text-base d-block font-semibold">보너스 번호</label> |
There was a problem hiding this comment.
중복된 클래스를 사용하지 않기 위해서, class style을 잘 활용하고 계시는 것 같습니다!
| @@ -8,3 +9,66 @@ export const LOTTO = { | |||
| NUMBER_COUNT: 6, | |||
| PRICE: 1000, | |||
| }; | |||
|
|
|||
| export const VALUE = { | |||
There was a problem hiding this comment.
하위의 constant는 복수형이고, 여기는 단수인데 어떤 차이가 있을까요?
There was a problem hiding this comment.
이부분은 제가 작성할 때 일관성체크를 못한 부분인것 같습니다. 수정하겠습니다!
|
|
||
| this.bindEvents(); | ||
| generateLottos(lottoCount) { |
There was a problem hiding this comment.
App Class 내부에서만 쓰이는 Method면 _ prefix를 붙여서 구분하는 편이 좋을 것 같아요! 그리고, class 내에서 순서를 constructor - public method - protected - private 순으로 지정하면 좋을 것 같습니다! 외부에서 app을 사용할 때 public method만을 사용하므로, 확인할 때 위에 있을 수록 더 쉽게 찾을 수 있거든요!
There was a problem hiding this comment.
여기서부터 아직 체크하지 못했습니다 ㅠㅠㅠㅠㅠ 엉엉...
| return this.numbers.includes(bonusNumber); | ||
| } | ||
|
|
||
| getWinningRank(winningNumber, bonusNumber) { |
There was a problem hiding this comment.
복잡한 로직이 있다면 get이 아닌 get- function 형태로 분리하신 걸까요? 좋은 구분처럼 보입니다 👍
| @@ -0,0 +1,28 @@ | |||
| import { VALUE } from '../constants.js'; | |||
|
|
|||
| export const getPriceByRank = (rank = VALUE.RANK.LOSE) => { | |||
| $element.disabled = true; | ||
| }; | ||
|
|
||
| export const getMatchCount = (array1, array2) => { |
There was a problem hiding this comment.
array1, array2가 array가 아닌 무언가가 들어왔을 때에 대한 예외가 없어서 위험해보입니다!
유조님께 드렸던 피드백을 참고하셔서 함수를 만들어보시면 좋을 것 같습니다!
There was a problem hiding this comment.
아무래도 자바스크립트다보니.. 이러한 처리가 필요할 것으로 보입니다!
| hide($element) { | ||
| $element.classList.add('hidden'); | ||
| } | ||
| const lottoFragments = lottos.map((lotto) => { |
|
코맨트를 확인하시면 머지 하도록 하겠습니다! |
|
수업시간에 작성하는 self 리뷰 |
There was a problem hiding this comment.
- 내가 고민한 구조는 1달이 지난 지금 봐도 명확해 보이는가?
- yes. 사실 뭐가 없다. Dom elem은 따로 두고 사용하고 싶다고 생각하다보니 자연스레 MVC 패턴으로 작성이 된 것 같다. 요즘은 component식으로 구현을 하고있는데, 실제로 로또 앱 자체의 크기가 작다보니 적절한 구조였다고 생각한다.
- 테스트 코드는 너무 길어져서, 기능이나 테스트에 대한 소주제로 분리할 수 있을 것 같다.
- 내가 설계한 구조를 지금도 다시 설명할 수 있는가?
- yes. .
- 한 단계 더 추상화해서 분리할 수 있는 부분은 없는가?
- util에서 getNumber로 올리기.
- 아직 추상화가 뭔지 모르겠다 사실.
- 상수화가 적절히 이루어졌는가?
- 중간중간 이건 왜 상수화 안했지? 싶은 것들이 보였다.
- 개발 후에 작성하려니 헷갈려서, 개발하는 도중에 맞춰가며 상수화를 하는 것이 맞다고 생각한다.
- 예외를 위한 고민이 적절한가?
- util 함수에서 예외를 위한 고민이 조금 더 필요했겠다는 생각이 든다.
- 네이밍에서 아쉬운 부부은 없는가?
- get NumbersString 같은 경우에는 toString 처럼 앞에 To 가 붙었어도 좋았겠다는 생각이 드는데, Lotto.toString인거라 이 부분은 아직 좀 고민이 되는 부분이다. Lotto 가 가진 멤버 변수는 numbers 뿐이라 Lotto.toString으로 표현하는 것도 좋았을 것 같은데, 외부에서 보기에는 모르니 현재 네이밍도 괜찮은 것 같기도 하고. 고민이다.
- 한 함수가 한가지 기능만 하고 있는가?
- getRandomNumberArray 같은 경우, 랜덤으로 수를 생성하는 것을 외부로 추출해도 좋았을 것 같다.
- 내장 객체 메서드를 이용해서 구현할 수 있는 부분은 없는가?
- 잘 모르겠다.
- 웹 접근성 부분에서 개선할 부분은 없는가?
- 모달 외부를 클릭했을 때 닫히게 만들기.
| @@ -8,3 +9,66 @@ export const LOTTO = { | |||
| NUMBER_COUNT: 6, | |||
| PRICE: 1000, | |||
| }; | |||
|
|
|||
| export const VALUE = { | |||
There was a problem hiding this comment.
상수화를 할 때 네이밍에 대한 일관성이 없었던 것 같다. 상수화를 할 때 더 명확한 기준을 가지고 네이밍을 하는 습관을 가지면 좋을 것 같다.
| WINNING_COUNT_TEXT: '.winning-count', | ||
| WINNING_RATE_TEXT: '.winning-rate', | ||
| } | ||
| } |
There was a problem hiding this comment.
EOL X(
이때도 prettier 설정이 잘못되어있거나, 적용이 안되어있었던 것 같은데, 도구를 이용할 땐 도구가 제대로 작동되고있는지 확인이 필요할 것 같다.
|
|
||
| this.bindEvents(); | ||
| generateLottos(lottoCount) { | ||
| return Array.from({ length: lottoCount }, () => new Lotto()); |
There was a problem hiding this comment.
array가 가진 멤버 변수를 잘 활용한 것 같다.
| disableElement($(SELECTORS.MONEY_INPUT.INPUT)); | ||
| disableElement($(SELECTORS.MONEY_INPUT.SUBMIT_BUTTON)); | ||
|
|
||
| $(`${SELECTORS.WINNING_NUMBER_INPUT.INPUT}:first-child`).focus(); | ||
| } |
There was a problem hiding this comment.
여기에서는 firstchild를 이용했는데, html 에는 first-winning-number가 존재한다. form label에 연관해 주기 위해 Id를 지정해 놓았는데, 이 경우에는 first-child로 찾지 말고, id 를 이용해 찾거나, form label을 이용할 때 aria-label을 이용하는 두가지 개선점이 있을 것 같다.
| handleInputWinningNumbers(event) { | ||
| if (!event.target.classList.contains('winning-number')) return; | ||
|
|
||
| if (event.target.value.length >= 2) { |
There was a problem hiding this comment.
2와 같은 숫자도 LOTTO_NUMBER_MAX_LENGTH 정도로 상수화 해주었으면 활용이 더 좋았을 듯!
| getWinningRank(winningNumber, bonusNumber) { | ||
| const matchCount = getMatchCount(winningNumber, this.numbers); | ||
|
|
||
| return matchCount === 5 && this.isIncludeBonus(bonusNumber) |
| const numberArray = []; | ||
|
|
||
| while (numberArray.length < length) { | ||
| const number = Math.floor(Math.random() * (max - min + 1) + min); |
There was a problem hiding this comment.
랜덤 숫자를 뽑아주는 건 getRandomNumber 정도로 추출해줄 수 있을 것 같다.
| export const getRandomNumberArray = (min, max, length) => { | ||
| const numberArray = []; | ||
|
|
||
| while (numberArray.length < length) { |
There was a problem hiding this comment.
여긴 그때도 고민했었는데, 중복되지 않고 Array를 만들고자 할 때 while문으로 계속 돌리는게 맞는가 하는 생각이 든다. 계속 1 1 1 1 1 .. 이런식으로 나오면 무한루프에 빠질수도 있을 것 같은데, 이 외에는 해결책을 아직 찾지 못했다. 어떻게 해야할까..
| $element.disabled = true; | ||
| }; | ||
|
|
||
| export const getMatchCount = (array1, array2) => { |
There was a problem hiding this comment.
array가 아닐때 발생할 수 있는 예외처리. 유틸로 사용하는 함수들은 외부에서 가져다 사용하는 함수들이므로 사용자가 잘못 작성했을 때에대한 예외처리에 더 신경쓸 필요가 있다.
|
|
||
| $lottoList.append(...lottoFragments); | ||
| $(SELECTORS.LOTTO_LIST.CONTAINER).append($lottoList); | ||
| $(SELECTORS.LOTTO_LIST.LOTTO_COUNT_TEXT).textContent = lottos.length; |
There was a problem hiding this comment.
돔 접근은 한 번만 할 수 있게 최적화는 잘 되었다고 생각한다.
학습로그[보안] XSS attack
[UI/UX] 웹 접근성과 웹 표준
[마크업] class name 을 이용한 스타일 지정
[DOM] Document Fragment
|
안녕하세요! Vallista님! :)
행운의 로또 미션 2단계 제출합니다.
지난 번 리뷰가 많은 도움이 되었습니다.
고민되는 부분과 생각해 볼 부분들이 있어서 리뷰요청이 늦어진 점 죄송합니다. 💦
이번에도 리뷰 잘 부탁 드립니다.🙏
데모 페이지
1단계 PR
간단히 정리해본 1단계 PR 리뷰에 대한 답변
2단계 진행시 중점을 둔 부분
⚒ 기능 요구사항
추가 기능 구현 사항
📝 앱 구조도
데이터 흐름도
(각 객체간 무슨값을 서로 전달하고 참조하는 지 보기위해서 그려보았습니다.)
📝 컴포넌트 구조
웹 접근성 체크 (WAVE 이용)
📂 디렉토리 구조
1단계 진행 부분에서 달라진 점
❓앱을 구현하면서 궁금했던 점
현재 등수 판단과 관련해서 해당 개수에 따른 등수를 return 해주는 getRankByMatchCount, 와 등수에 따른 상금을 return해주는 getPriceByRank를 lottoUtils로 분리하여 두었습니다. 이렇게 분리한 이유는 해당 로직이 Lotto, LottoApp, LottoView 와 크게 연관성이 없고, 어디에서나 사용 될 수 있으나, 로또 앱에 한정된 유틸성 함수라고 생각했기 때문입니다.
그런데 이렇게 작성하다보니, 현재 앱에서는 getRankByMatchCount는 Lotto 클래스에서만, getPriceByRank는 LottoApp에서만 이용되어서 불필요한 분리가 아니었나 하는 생각이 들었습니다. 이런식으로 조건이나 키에 대한 값을 리턴하는 함수를 만들고, 이용할 때 어느 위치에 있는 것이 더 좋은걸까요?
label을 이용할 때 Navigating 문제가 있어 h 태그는 label tag안에 사용하지 않는 것을 권장한다고, MDN 문서에 작성되어있었습니다. 그래서 현재 index.html에 h4 태그로 되어있는 부분을 스타일 적용을 위해 p 태그로 변경해 둔 상태입니다. 그렇게 변경한 이유는 당첨번호와 보너스번호 부분이 검색과는 크게 연관이 없고, h4 태그가 적용되어 있었기 때문에 그 중요성이 낮다고 판단했기 때문입니다.
이렇게 하는 과정에서 궁금증이 들었습니다. 만약 h태그가 꼭 이용되어야 하는 부분이라면 label로 변경하기 보다, h태그를 이용한 후, 따로 이벤트를 js를 이용하여 걸어주는 것이 맞다는 생각이 들었는데요. 네이버 로고 부분을 확인해 보니 h1 태그내에 img, a 태그 등을 이용하는 것을 확인할 수 있었습니다. 그럼 h 태그 안에 label 태그를 이용할 수 있지 않을까라는 생각도 들었는데, 실제로는 어떻게 이용하는지 궁금합니다.
결과를 계산하는 과정에서, winningRankCounts(당첨 결과에 따른 로또 개수 object)를 메서드 안에서 선언하는 것이 좋은 방법은 아니라고 오늘 수업시간에 피드백을 받았습니다. 그래서 제출 전 수정을 할 까 했으나, 해당 메서드 외부에서 선언하고 이용할 경우 모달에서 취소를 누르고 다시 누르게 되면 해당 object 내의 value가 변경되지 않게 되어서, 변경하지 않고 메서드 내부에 선언하도록 했습니다. 메서드 안에서 선언되지 않으면, 매번 해당 메서드가 실행 될 때마다 object 내부의 value를 초기화해주어야 하는데, 그렇게 해야하는것인지 잘 감이 오지않아서 리뷰어님이라면 어떻게 처리하셨을지 혹은 참고해보면 좋을 키워드 등을 추천해주시면 감사하겠습니다!
감사합니다. 오늘도 좋은 하루 되세요! :)
+) 마크업 에러를 발견하여, 수정하였습니다. (21.2.28 14:05pm)