[2단계 - 웹 기반 로또게임] 치코(이재민) 미션 제출합니다.#309
Conversation
Lotto클래스로 이동
askWinNumbers 외부에서 split 사용
LottoProcess 클래스에서 Lotto로 이동
클래스에서 객체 리터럴로 변경 getRateOfRevenue, calculateResult 함수 controller에서 이동
jaeml06
changed the title
liswktjs
left a comment
liswktjs
left a comment
There was a problem hiding this comment.
안녕하세요 치코
2단계 미션도 수고하셨습니다 🤗
코멘트에 남긴 질문들에 대한 답입니다!
innerHTML사용에 대한 고민
저라면 createElement를 사용하여 개발을 할 것 같습니다!
innerHTML은 말씀하신 대로 XSS공격에 취약한 면이 있기 때문에 실제 프로덕트 에서는 사용을 주의해야합니다 그래서 되도록이면 createElement를 사용하는 것이 좋습니다
추가로 createElement관련해서 태그 생성에 많은 코드가 생성되는 것이 사용하지 않은 이유로 꼽으아 주셨는데, 자주 사용되는 동일한 디자인 또는 역할을가지는 Element를 생성하는 로직의 경우 함수로 분리해서 사용해주면 그 수고스러움이 어느 정도 덜 수 있을 것 같아 이런 방향으로 구현해보셔도 좋을 것 같습니다:)
태그 class 이름에 대한 고민
중복되는 css를 asset으로 따로 분리하신 점은 좋다고 생각합니다
추가로 저는 어떻게 작성해주시는지도 물어봐주셨는데, 일단 이건 팀의 컨벤션 마다 또 어떤 스타일 라이브러리를 쓰느냐에 따라서도 다르기 때문에 답을 하긴 애매한 것 같습니다
일례로 스타일 라이브러리 들 중 tailwind를 사용하면 classname이 엄청 길어지고 emotion을 사용하면 classname을 안 붙여주어도 됩니다 이렇게 어떤 스타일로 개발을 하냐에 따라 classname의 길이가 많이 달라집니다
추가로 배포해주신 링크로 테스트 해보면서 문제점을 발견하여 제보드립니다!
- 보너스 번호가 로또 번호와 일치해도 에러가 발생하지 않는 것 같은데 확인 부탁 드려요!
| <input class="purchase-input-text" type="text" placeholder="금액"> | ||
| <input class="purchase-button button" type="submit" value="구입"> |
There was a problem hiding this comment.
button tag대신 input tag를 사용하신 이유가 무엇인가요?
There was a problem hiding this comment.
딱히 별 이유는 없었습니다. 두 태그 차이가 button태그 안에는 새로운 태그 선언이 가능하다는 것 이외에는 차이가 없다는 것으로 알고 있어서요
button 태그를 사용하는 것이 더 나은가요?
There was a problem hiding this comment.
아하 그런 건 아니였고 button보다 input을 선택하신 이유가 궁금해서 질문했습니다!
liswktjs
left a comment
liswktjs
left a comment
There was a problem hiding this comment.
안녕하세요 치코
2단계 미션 피드백 반영 수고하셨습니다!
createElement로 바꿔서 구현하셔서 좋았습니다 👍
다만, 저의 리뷰에 치코의 코멘트나 반응들이 나와있지 않아서
저의 리뷰가 잘 치코에게 전달이 된 건지 알 수 없어서 아쉬웠습니다 🥲
(혹시 pending만 되고 제출이 안 될 걸까요? 🥹)
의견 남겨주시고, 추가로 코멘트들도 남겼으니깐 확인해보시고 리퀘스트 체인지 날려주세요 👍
| while (ramdomSection.firstChild) { | ||
| ramdomSection.removeChild(ramdomSection.firstChild); | ||
| } |
There was a problem hiding this comment.
혹시 while문을 사용한 이유를 알 수 있을까요?
혹여 모든 자식들을 제거 하고 싶다고 한 거라면, 자식 요소들을 하나의 부모 요소로 묶어서 해당 묶은 Element만 지우는 방법도 있습니다! While문은 무한 루프에 빠질 위험이 있기 때문에 비교적 사용을 안 하는 방향이 좋습니다!
There was a problem hiding this comment.
전체를 한번에 지우고자 해서 while을 사용했습니다. 자식요소를 부모 요소로 묶기 위해서 또 새로운 div로 감싸고 클래스이름을 지정하는게 부자연스럽다고 생각되어 잠깐 생각하고 넘겼습니다. while문도 자식 노드가 없을 경우 while문을 탈출하기 때문에 무한 루프의 위험에 빠질 위험이 없다고 생각했습니다. 하지만 사람은 언제나 실수를 할 수 있으니 무한루프에 빠질 위험보다 차라리 div 태그를 추가하는게 현명한 판단이네요
다만 이번에는 replaceChildren()함수를 사용했습니다
There was a problem hiding this comment.
아 넵 좋습니다 replaceChildren도 자식 컴포넌트를 모두 지우는데 좋은 방법이라고 생각합니다
| createModalContainer(result = [], rateOfRevenue = 0) { | ||
| const modalContainer = document.createElement('div'); | ||
| modalContainer.className = 'modal-container'; | ||
|
|
||
| const modalContents = document.createElement('div'); | ||
| modalContents.className = 'modal-contents'; | ||
| modalContainer.appendChild(modalContents); | ||
|
|
||
| const modalTitle = document.createElement('p'); | ||
| modalTitle.className = 'modal-title'; | ||
| modalTitle.textContent = '🏆 당첨 통계 🏆'; | ||
| modalContents.appendChild(modalTitle); | ||
|
|
||
| modalContents.appendChild(this.createModalTable(result)); | ||
|
|
||
| const totalPrice = document.createElement('div'); | ||
| totalPrice.className = 'total-price'; | ||
| totalPrice.textContent = `당신의 총 수익률은 ${rateOfRevenue}%입니다`; | ||
| modalContents.appendChild(totalPrice); | ||
|
|
||
| const restartButton = document.createElement('button'); | ||
| restartButton.className = 'restart-button button'; | ||
| restartButton.textContent = '다시 시작하기'; | ||
| modalContents.appendChild(restartButton); | ||
|
|
||
| return modalContainer; |
There was a problem hiding this comment.
함수의 길이가 길다고 생각합니다. 다만 결국 이것들이 모여 같은 템플릿을 구성하는 것이고 재사용성이 없어 따로 함수로 구분하지 않았습니다. 전 오히려 함수로 따로 나누는 것 가독성이 떨어질 것 같습니다
차라리 이번에 수정한 분할 방식은 어떠신가요?
modalContainer를 createElement함수로 분할하여 작성했습니다
There was a problem hiding this comment.
create로 함수명을 통일해서 element를 나눠주셨군요 element 끼리 적절히 잘 나누신것 같습니다 👍
| const tbody = document.createElement('tbody'); | ||
| result.forEach((value) => { | ||
| const [matchCount, isBonus, price, winCount] = value; | ||
| const tr = document.createElement('tr'); | ||
|
|
||
| const tdMatchCount = document.createElement('td'); | ||
| tdMatchCount.textContent = `${matchCount}개${isBonus ? ' + 보너스 볼' : ''}`; | ||
| tr.appendChild(tdMatchCount); | ||
|
|
||
| const tdPrice = document.createElement('td'); | ||
| tdPrice.textContent = price.toLocaleString(); | ||
| tr.appendChild(tdPrice); | ||
|
|
||
| const tdWinCount = document.createElement('td'); | ||
| tdWinCount.textContent = `${winCount}개`; | ||
| tr.appendChild(tdWinCount); | ||
|
|
||
| tbody.appendChild(tr); | ||
| }); |
There was a problem hiding this comment.
modal tabel도 함수 길이가 긴데 forEach쪽을 분리해주는게 어떨까요?
There was a problem hiding this comment.
확실히 이 부분은 따로 빼야 할까 고민 했는데 저는 전체 테이블을 만드는 로직을 한 번에 모아두는 것이 더 가독성 측면에서 좋을 것 같아 분할하지 않았습니다. 다만 forEach문은 역시 따로 빼는 것이 가독성이 더 좋을 것 같네요
|
|
||
| createRandomLottoList(lottos = []) { | ||
| const ul = document.createElement('ul'); | ||
| const fragment = document.createDocumentFragment(); |
There was a problem hiding this comment.
DOM 조작은 코스트가 비싸다고 알고 있습니다. 위의 코드에서 매번 li태그를 생성에 appendChild를 해주면 매번 reflow가 일어나 성능에 악영향을 끼칩니다. 이때 DocumentFragment 메인 DOM에 존재하지 않고 가상메모리 상에 존재하는 노드 트리이므로 reflow를 발생시키지 않습니다. 실제로 이용자가 몇개의 로또를 구매할지 모르기 때문에 성능 향상을 위해 fragment를 이용했습니다
| </head> | ||
|
|
||
| <body> | ||
| <div id="app"> |
There was a problem hiding this comment.
저는 아래의 UI가 기본으로 공개되는 태그들이여서 동적으로 구현하지 않았습니다. 통일성을 위해 동적으로 구현하는 것이 더 좋았을지 생각이 듭니다
There was a problem hiding this comment.
이번 미션에서는 이렇게 구현해보신다음에 다음 미션인 spa에서는 동적으로 구현할 예정이니
그곳에서 한 번 적용해보시면 좋을 것 같아요~
| resultButton.addEventListener('click', () => this.restart(modalPosition).bind(this)) | ||
| }, | ||
|
|
||
| restart(){ |
There was a problem hiding this comment.
저는 다시 시작하는 부분을 새로고침으로 구현했습니다. 하지만 크루중에 새로고침보다 태그와 이벤트 리스너를 제거하고 input안에 있는 값을 제거하는 것이 더 맞지 않냐? 라는 의견이 있었습니다. 저는 그러면 결국 innerHTML로 수정을 해야하고 다시 셀렉터를 이용하여 input value를 제거해야 하는데 결국 이것 코스트가 많이 들어 새로고침과 차이가 있나? 라는 생각이 듭니다. 샐리의 의견은 어떠신가요?
There was a problem hiding this comment.
새로고침을 하게 되면, 화면 깜빡임이 있을 수 있습니다
이건 유저의 사용성을 저하시킬 수 있어서 최대한 지양하는 편입니다
또 지금은 화면 내에 많은 데이터를 그리고 있지 않지만, 화면에 정보가 많을 경우
dom을 직접 접근하여 초기화하는것이 새로고침시 처음부터 다시 렌더링 하는 것 보다 더 짧게 걸리는 경우도 있습니다
| import './web/css/modal.css' | ||
| import './web/css/asset.css' | ||
|
|
||
| PurchaseSection.initPurchaseEvent(); No newline at end of file |
There was a problem hiding this comment.
함수가 함수를 호출하는 구조, 저는 PurchaseSection.initPurchaseEvent();가 결국 제가 구현한 여러 함수들을 호출하게 됩니다. 함수가 함수를 호출하고 또 그 함수가 또 함수를 호출하는 구조가 구현하면서 '이래도 괜찮나'라는 생각이 듭니다. 그렇다고 이게 정말 느낌대로 문제가 되는지, 그렇다면 다르게 어떻게 구현할지가 떠오르지 않아 이대로 구현했습니다. 과연 이러한 구조에 문제가 있을까요?
There was a problem hiding this comment.
함수로 구현했을 때 구조가 고민이시군요
함수형과 관련해서 이해를 도울 수 있는 책이 있어서
https://product.kyobobook.co.kr/detail/S000001952246
한 번 괜찮으시다면 읽어보시는 것도 추천드립니다
|
|
||
| createRandomLottoList(lottos = []) { | ||
| const ul = document.createElement('ul'); | ||
| const fragment = document.createDocumentFragment(); |
There was a problem hiding this comment.
DOM 조작은 코스트가 비싸다고 알고 있습니다. 위의 코드에서 매번 li태그를 생성에 appendChild를 해주면 매번 reflow가 일어나 성능에 악영향을 끼칩니다. 이때 DocumentFragment 메인 DOM에 존재하지 않고 가상메모리 상에 존재하는 노드 트리이므로 reflow를 발생시키지 않습니다. 실제로 이용자가 몇개의 로또를 구매할지 모르기 때문에 성능 향상을 위해 fragment를 이용했습니다
| const tbody = document.createElement('tbody'); | ||
| result.forEach((value) => { | ||
| const [matchCount, isBonus, price, winCount] = value; | ||
| const tr = document.createElement('tr'); | ||
|
|
||
| const tdMatchCount = document.createElement('td'); | ||
| tdMatchCount.textContent = `${matchCount}개${isBonus ? ' + 보너스 볼' : ''}`; | ||
| tr.appendChild(tdMatchCount); | ||
|
|
||
| const tdPrice = document.createElement('td'); | ||
| tdPrice.textContent = price.toLocaleString(); | ||
| tr.appendChild(tdPrice); | ||
|
|
||
| const tdWinCount = document.createElement('td'); | ||
| tdWinCount.textContent = `${winCount}개`; | ||
| tr.appendChild(tdWinCount); | ||
|
|
||
| tbody.appendChild(tr); | ||
| }); |
There was a problem hiding this comment.
확실히 이 부분은 따로 빼야 할까 고민 했는데 저는 전체 테이블을 만드는 로직을 한 번에 모아두는 것이 더 가독성 측면에서 좋을 것 같아 분할하지 않았습니다. 다만 forEach문은 역시 따로 빼는 것이 가독성이 더 좋을 것 같네요
| createModalContainer(result = [], rateOfRevenue = 0) { | ||
| const modalContainer = document.createElement('div'); | ||
| modalContainer.className = 'modal-container'; | ||
|
|
||
| const modalContents = document.createElement('div'); | ||
| modalContents.className = 'modal-contents'; | ||
| modalContainer.appendChild(modalContents); | ||
|
|
||
| const modalTitle = document.createElement('p'); | ||
| modalTitle.className = 'modal-title'; | ||
| modalTitle.textContent = '🏆 당첨 통계 🏆'; | ||
| modalContents.appendChild(modalTitle); | ||
|
|
||
| modalContents.appendChild(this.createModalTable(result)); | ||
|
|
||
| const totalPrice = document.createElement('div'); | ||
| totalPrice.className = 'total-price'; | ||
| totalPrice.textContent = `당신의 총 수익률은 ${rateOfRevenue}%입니다`; | ||
| modalContents.appendChild(totalPrice); | ||
|
|
||
| const restartButton = document.createElement('button'); | ||
| restartButton.className = 'restart-button button'; | ||
| restartButton.textContent = '다시 시작하기'; | ||
| modalContents.appendChild(restartButton); | ||
|
|
||
| return modalContainer; |
There was a problem hiding this comment.
함수의 길이가 길다고 생각합니다. 다만 결국 이것들이 모여 같은 템플릿을 구성하는 것이고 재사용성이 없어 따로 함수로 구분하지 않았습니다. 전 오히려 함수로 따로 나누는 것 가독성이 떨어질 것 같습니다
차라리 이번에 수정한 분할 방식은 어떠신가요?
modalContainer를 createElement함수로 분할하여 작성했습니다
|
저의 부족함 때문에 리뷰에 차질이 생겨 정말 죄송합니다. 샐리말대로 pending된 채 그대로 있었네요. 제가 더 신경을 들였어야하는데 너무 아쉽고 죄송합니다 |
| return modalContainer; | ||
| }, | ||
|
|
||
| createModalElement(rateOfRevenue = 0) { |
liswktjs
left a comment
liswktjs
left a comment
There was a problem hiding this comment.
안녕하세요 치코
이번에는 열정적으로 코멘트를 달아주셨군요 🔥
저도 답할 수 있을 만큼 답변을 달아보았습니다 한 번 읽어봐주세요!
다만, 반영해주셨다고 하신 css관련 부분들에서 제가 코드에서 변경점을 못 찾아서요!
(혹시 push가 안 된 걸까요? )
그래도 css 관련 리뷰들을 치코도 인지하고 있는 것을 이번 코멘트들을 통해서
알게 되어서 merge하겠습니다!
다음 단계 미션도 파이팅입니다 💪
2 participants
안녕하세요! 샐리
이번에는 우테코에서 처음으로 콘솔로 작동하는 기능을 웹페이지로 구현하는 작업을 진행했습니다.
html과 css는 학교에서 간단한 과제 정도로만 배웠던 것이 전부라 처음에 구현하는데 고생을 했네요.
그래도 부족한 부분 있으면 빠르게 수정해 볼테니 다양한 의견 주시면 감사하겠습니다!
이번 미션을 진행하기 전
이번 미션을 진행하기 전에 저번에 클래스에 있을 필요가 없는 함수들을 밖으로 뺴는 것이 좋지 않냐는 의견을 주셨습니다.
step1에서는 수정하지 못한 상태로 merge가 되었는데요. 이번에는 다른 크루들의 코드를 확인해 보면서 그게 무슨 말씀이 었는지 조금이나마 느낄 수 있었습니다. 그래서 step2를 진행하기 전에 그 부분을 수정했습니다.
이번 미션을 진행하며 생긴 고민들
innerHTML사용에 대한 고민
처음에는 index.html에 모든 태크를 정적으로 생성되어 있는 상태에서 css의 visibility속성을 사용하여 구현을 해야하나 생각했습니다. 하지만 index.html에 너무 많은 태크가 존재하는 것 같았고, 로또 규칙 변경 (ex: 6개수 입력에서 8개의 수 입력 받음)이 생길 경우, 수정이 어렵다고 생각하여 동적으로 생성하는 방향으로 잡았습니다.
이러한 과정에서 innerHTML로의 구현과 createElement사용한 생성 중 무엇을 사용할지 고민 했습니다.
innerHTML의 경우 ``을 사용하여 태그 생성이 쉽다고 가독성이 높다는 장점이 있지만 성능과 XSS공격에 취약하다는 단점이 있고, createElement()의 경우 성능이 우수하다는 장점이 있지만 태그 생성에 많은 코드가 필요하여 가독성과 개발 속도가 저하 된다는 단점이 있었습니다.
저는 구현에 용이한 innerHTML을 선택했습니다.
샐리라면 이러한 상황에서 무엇을 선택했을까요? 이미 구현을 한 상태에서 다른 크루의 코드를 보니 웹컴포넌트라는 것도 있더군요. 다음 미션의 목표는 웹 컴포넌트를 사용해 보는 것 입니다.
태그 class 이름에 대한 고민
태그에 css와 셀렉터를 사용하기 위해 class이름 작명을 해야 했습니다. 처음에는 당연히도 기능을 중심으로 작명을 했습니다. 그러다 css 파일상에 중복되는 css가 생겨 보기 좋지 않다고 생각이 들었습니다. 그래서 겹치는 Button과 에러 메세지의 css를 asset파일에 모아 두었습니다. 그렇게 쪼개다 보는 css가 겹치는 문제는 줄어들었지만, 태그에 class = ""가 너무 길어져 가독성이 문제가 생긴 것 같았습니다. 그래서 절충안으로 완전히 겹칠 수 있는 폰트나 버튼의 양식은 asset으로 만들고 display: flex와 같은 레이아웃은 각각의 기능에 따라 css를 구현하였습니다.
저의 방식에 대한 캘리의 의견과 캘리는 어떻게 작성하시는지 궁금합니다!