-
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단계 - 웹 기반 로또 게임] - 버건디(전태헌) 미션 제출합니다. #296
Conversation
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com> fix Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com> fix
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com> fix
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com정
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
Co-authored-by: JEON TAEHEON <brgndyy@users.noreply.github.com>
보너스 숫자 setBonusNumber 추가 Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
Co-authored-by: brgndyy <brgndyy@gmail.com>
src/step2-index.js
Outdated
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.
이번에 사실 바닐라자바스크립트로는 처음으로 이렇게 모듈 분리를 해보는데요..! (원래는 한곳에 다 모아서 작성을 했었습니다.)
해당하는 각 레이아웃마다 해당 하는 부분별로 파일을 나눠주어서 함수를 작성해보았는데, 이러한 구분 기준이 괜찮은것인지, 아니면
eventHandlers // 이벤트 핸들러 함수
renderFunctions // 렌더링 관련 함수
이런식으로 각레이아웃 기준이 아니라, 핸들러와 렌더링 함수, 즉 함수 기준으로 나누는것이 더 효율적일지 카일의 의견 한번 여쭙고 싶어요!
저는 일단 각각 레이아웃의 역할에 따라서 직관적으로 나누어주어야겠다고 생각은 했지만, 이런 관점에서 놓치는 부분이 있을까 싶어서 한번 여쭙습니다 🙇
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.
이 부분에 대해서는 제가 아닌 버건디가 답을 주셔야하는 부분이 아닐까 생각되네요! 다음의 질문을 확인해주시고 고민한 부분에 대해 나눠주시면 좋겠어요~
- 지금 구조에서 각 폴더의 역할은 어디까지인가요?
- 각 폴더의 의존 관계는 어떻게 형성 되어있나요? (어떤 모듈에서 어떤 모듈을 import해야 하는지)
- 현재 구조에서 취할 수 있는 장단점은 무엇일까요?
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.
- 현재 각 구조에서 핸들러를 나눈 기준은 레이아웃 기준입니다.
handlers
┣ boughtLottoHandlers.js // 구매한 로또를 렌더링 하는 핸들러
┣ inputMoneyHandler.js // 구입 금액을 입력하는 것과 관련된 핸들러
┣ inputWinningLottoNumbersHandler.js // 당첨 번호를 입력하는 핸들러
┣ statisticsResultHandler.js // 당첨 통계 모달창을 띄워주는 핸들러
┗ uiUtils.js // ui 관련된 유틸함수 핸들러
┗ instances.js // 도메인 인스턴스를 모아두는 곳
입니다.
일단 레이아웃 별로 핸들러들을 나눈 것이라서 직관적이라는 장점은 있다고 판단했는데요..!
가령 boughtLottoHandler.js 에서 쓰이는 함수들은
단순히 innerHTML 과 textContent 를 사용해서 타겟 태그에 내용을 추가해주는 식의 함수들이 정의가 되어있습니다!
그렇다면 차라리 레이아웃 별로 나누는 것이 기준이 아니라,
단순히 UI에 보여주고싶은 결과물들을 렌더링해주는 renderFunctions
와 각 도메인에서 필요한 값들을 계산해주는 eventFunctions
의 기준으로 나누어 보는것이 더 유지보수의 관점에서 나은거 아닐까? 라는 생각도 들었던것 같습니다.
근데 또 이렇게 된다면 각 도메인 로직들이 흩어져 버리게 되는 문제가 발생하는것 같기도하여서 어떤것이 더 나을지 아직도 확신이 서질 않습니다 ..🥲
관심사의 기준을 어디로 잡아야하는 것일지 그 부분이 계속 헷갈리는것 같습니다..!
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.
넵 특정 프레임워크 없이 vanilla js를 사용하여 프로젝트를 구현할 때의 한계를 잘 느끼고 계시는 것 같네요 ㅎㅎ 우선 이러한 기준을 조금씩 구체화 하기 위해서는,
- 현재 운영 중인 꽤 규모 있는 프로젝트의 레포를 살펴보면서 왜 폴더 구조를 이렇게 나눌까 고민해보기 (e.g. react repo)
- vanilla js를 사용한 프로젝트 아키텍처 관련 아티클을 읽어보기 (추천 키워드 -
frontend architecture using vanilla js
)
정도가 있을 것 같습니다~
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/Constants/lottoReward.js
Outdated
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.
.js
suffix를 붙여주게 된 이유가 있을까요? 이거 때문에 file diff가 더 생긴 거 같아서 여쭤봅니다~
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.
현재 웹팩에서 resolve
속성에 .js
가 포함이 되어있어서 step1 때는 확장자에 js파일을 붙이지 않더라도 에러가 발생하지 않았었는데요.
이번 step2에서는 왜인지 모르게 js 확장자가 없으면 에러가 발생해서, 임의로 전부 다 붙여주게 되었습니다..🥲
- step2.config.js
resolve: {
extensions: ['.js', '.mjs', '.css'],
},
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.
넵 이해했습니다~ 현재 PR에서 다룰 영역은 아니라 이에 대한 자세한 피드백은 스킵하겠지만, 추후 webpack같은 번들러를 학습하실 때 이 문제에 대한 원인이나 해결방법도 고민해보시면 좋겠네요!
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.config.js
Outdated
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.
무언가 prettier rule이 변경된 듯 한데, 이게 왜 step 2에 올라오는지 궁금하네요 🤔
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.
기존에 step1에서 아예 prettier 룰을 config 파일들에 다 적용을 안해놨더라구요..!
일단 변경이 적용 된 것이라 커밋을 했는데, 혼란끼쳐드린것 같아 죄송합니다.
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.
�아하 그랬군요~ 죄송할 건 아닌 것 같습니다! 다만 PR에 올라올 컨텍스트를 벗어나는 diff가 발생했고 부득이하게 함께 올라와야 하는 것이라면 미리 공유하는 것도 좋은 방법일 듯해요 ㅎㅎ
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.
맞는 말씀이에요! 짚어주셔서 감사합니다!
index.html
Outdated
<input type="text" class="winningLottoInput" /> | ||
<input type="text" class="winningLottoInput" /> | ||
<input type="text" class="winningLottoInput" /> | ||
<input type="text" class="winningLottoInput" /> | ||
<input type="text" class="winningLottoInput" /> | ||
<input type="text" class="winningLottoInput" /> |
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.
이렇게 동일한 요소가 반복되는 경우 js를 이용해서 동적으로 처리할 수 있는 방법은 없을까요~?
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.
제가 현재 알고 있는 바로는, html에서는 필요한 부분을 일단 다 작성을 해주고 js 에서 기능적인 부분 (에를 들어서 값을 따로따로 받아야할때)에 관해서는 querySelectorAll 이나 각각 input에 다른 id 값들을 주어서 처리하는 방식인데요..!
혹시 카일이 말씀하시는 부분은 기능적인 부분이 아니라, 아예 저 input 태그를 js를 통해 반복적으로 렌더링해주는걸 말씀하시는 걸까요 ??
만약 그렇다면 createElement
메서드를 통해서 아예 js 코드가 실행될때 바로 input 태그를 생성해줄수 있을거라고 예상하는데,
근데 만약 이렇다면 직접적으로 DOM에 접근을 하게 되는거라서 비용이 많이 들지 않을까? 라는 생각도 듭니다..!
만약 100개가 넘을정도로 많은 동일 요소가 필요하다면 js 내에서 동적으로 처리해주는것이 맞겠으나, 현재는 필요한 동일 요소가 6개이기 때문에 직접 적어주어도 괜찮지 않을까 ?라는 생각도 동시에 들긴 드는데 카일의 의견도 한번 말씀 주시면 감사하겠습니다!
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을 조작하는 방식은 적극적으로 사용하되, 성능을 고려한 코드 패턴을 찾아보시면서 (이벤트 위임, reflow 최소화 등) 본인만의 타협점을 찾아보면 어떨까 싶네요!
+ 성급한 성능 최적화를 지양해야 하는 이유에 대해서도 같이 찾아보시면 좋을 것 같습니다~
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.
id와 class를 작성하는 기준이 있으실까요?
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.
class 는 단순히 styling을 위한 명,
id는 js에서 직접 하나의 값을 불러와서 이벤트를 처리해주어야할때 라는 기준으로 작성했습니다!
하지만 당첨 번호 관련된 인풋들 같은 경우에, 동일한 도메인이지만 하나의 인풋만 처리해주는것이 아니다보니 class로 가져와서 여러 인풋을 한번에 처리해주려고 했습니다.
정리하자면 id => js내에서 동적으로 이벤트를 핸들링 하기 위해서 고유한 값을 부여
class => js 내에서 여러개의 동일 속성을 핸들링 하기 위해서 부여할수도 있지만, 스타일링을 먼저 고려해서 부여
라고 말씀드려볼 수 있을것 같습니다.
혹여나 제가 잘못생각하고 있는 부분이 있다면 짚어주신다면 감사하겠습니다! 🙇
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.
잘못 생각하신 건 아니고 버건디의 기준이 궁금해서 여쭤봤습니다 :) 다만 저라면, js와 css로 구분 두기보다는 후반에 설명 주신 것처럼 '유일한 요소'와 '다중 요소'로 기준을 두어 css, js 내에서 각 선택자를 유연하게 사용하는 편이 DX 상 적합한 방식이지 않을까 싶어요!
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.
다음 미션에서 한번 더 유연하게 적용해보도록 하겠습니다🙇
index.html
Outdated
<label for="lottoInput" class="inputLabel">구입할 금액을 입력해주세요</label> | ||
<input type="text" placeholder="금액" name="moneyInput" id="moneyInput" class="moneyInput" /> |
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.
(질문) label
의 for
는 어떤 목적으로 사용하는 걸까요?
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.
input의 name 값과 label의 for 값을 매칭시켜주어서 label을 클릭하면 input에 포커싱이 가도록 사용자의 편의성을 고양시켜주는 것으로 알고 있는데요..!
지금 살펴보니 label for 속성 값과 input의 name 값이 다르네요..
짚어주셔서 감사합니다..!
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.
넵 의도에 맞게 답변 주신 듯 하네요 👍
index.html
Outdated
<p class="statisticsText">3개</p> | ||
</div> | ||
<div class="row"> | ||
<p class="statisticsText">5,000</p> | ||
</div> | ||
<div class="row"> | ||
<p class="statisticsText winningCount"></p> | ||
</div> | ||
</div> | ||
<div class="tableTitle"> | ||
<div class="row"> | ||
<p class="statisticsText">4개</p> | ||
</div> | ||
<div class="row"> | ||
<p class="statisticsText">50,000</p> | ||
</div> | ||
<div class="row"> | ||
<p class="statisticsText winningCount"></p> | ||
</div> | ||
</div> | ||
<div class="tableTitle"> | ||
<div class="row"> | ||
<p class="statisticsText">5개</p> | ||
</div> | ||
<div class="row"> | ||
<p class="statisticsText">1,500,000</p> | ||
</div> | ||
<div class="row"> | ||
<p class="statisticsText winningCount"></p> | ||
</div> | ||
</div> | ||
<div class="tableTitle"> | ||
<div class="row"> | ||
<p class="statisticsText">5개+보너스볼</p> | ||
</div> | ||
<div class="row"> | ||
<p class="statisticsText">30,000,000</p> | ||
</div> | ||
<div class="row"> | ||
<p class="statisticsText winningCount"></p> | ||
</div> |
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.
이 또한 현재 상수 객체로 분리해서 js 파일 내에서 createElement
를 통해 직접 만들어주는 방법이 생각납니다..!
카일의 리뷰를 보다보니, 제가 js를 통해 직접 DOM에 내용물을 추가하는 것을 비용적으로 많이 드는 것이라고 생각하여 무의식적으로 계속 지양했던것 같다는 생각이 듭니다.
예를 들어서 아예 텍스트가 없다가, 어떠한 기능이 실행 된후에 텍스트가 생겨야만 하는 상황이라면 (직접 DOM에 내용물을 삽입해야하는경우) js를 사용 할수밖에 없지! 라고 생각했었는데요.
지금 현재 이미 하드코딩으로 레이아웃을 잡아줄수 있는 부분들이 있다면, 굳이 js로 렌더링을 안해주려고 했던것 같습니다.
갯수가 수십, 수백개여서 제가 직접 수동으로 일일히 넣어주기 힘든 경우가 아니라고 판단해서 직접 html 내에서 작성해주었던것 같아요..!
이런 관점에 대해서 카일은 어떻게 생각하실지 여쭈어 보고 싶습니다!
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.
동일 코멘트일 듯 하여 이미 작성한 코멘트 첨부할게요!
index.html
Outdated
</div> | ||
<script type="module" src="./src/js/index.js"></script> | ||
<script type="module" src="./dist/step2-bundle.js"></script> |
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.
script
태그에 type="module"
을 선언하면 어떤 이점이 있을까요?
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.
이번에 자바스크립트의 module
과 관련하여 pr을 올린후 정리해보았는데요..!
요약하자면, module
의 선언이 없다면 각각의 스크립트는 분리되어 작성 되더라도 같은 스코프를 공유하기때문에 변수 오염이나 에러가 발생할 가능성이 크다.
하지만 module
의 선언을 통해서 각각의 독립된 스코프를 보장해줄수 있다. 라고 말씀드려볼수 있을것 같습니다!
한번 읽어봐주시고 혹여나 잘못된 부분이 있으시다면 지적해주시면 새겨 듣겠습니다..!
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.
오.. 모르는 지식에 대해 블로그로 바로 정리하시다니 바람직한 습관을 갖고 계시는 군요 ㅎㅎ 현재 정리하신 부분에 추가로 말씀 드리고 싶은 부분은 type="module"
을 선언한다는 것은 defer
속성을 내포하고 있기 때문에 굳이 body
하단에 스크립트 태그를 명시하지 않아도 된다는 것이었습니다!
- 추천 키워드 - script 태그 내의 async, defer / rendering blocking resources
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.
안녕하세요 버건디~ 2단계 미션 구현하시느라 고생 많으셨습니다! 전반적인 코드에 대한 코멘트는 하나만 남기고 질문에 대한 답변드리고 마무리하겠습니다.
- PR에 올라와야할 내용과 올라오면 안되는 내용엔 무엇이 있을지 고민해보기
UI => HTML에 관련된 부분도 feat으로 컨벤션을 적는게 맞을까요 ?
conventional commit 문서에서 힌트를 얻을 수 있지 않을까 싶네요~
uiUtils.js라는 파일을 하나 만들어서, 이 안에서 input태그의 값을 초기화 해주거나, 타겟 태그에 클래스명을 추가 or 삭제해주는 핸들러들을 만들었는데 이러한 방향성에 대해서 어떻게 생각하시는지 여쭙고 싶어요!
어떤 방향성을 얘기하시는 걸까요? 모든 부분에 일맥상통하는 부분이지만, 코드 작성이나 모듈 관리 시 본인이 선택한 방식에서 고려한 장단점이나 비교군을 먼저 제시해주시면 좋겠습니다. 그럼 거기에 맞게 제 의견도 드릴 수 있을 것 같아요~
요구사항에 클래스(또는 객체)를 사용하는 경우, 프로퍼티를 외부에서 직접 꺼내지 않는다. 객체에 메시지를 보내도록 한다. 이러한 문장이 있었는데요! 저같은 경우 Money 클래스에서 getMoney() 라는 get 함수를 만들어서 필요한 도메인에서 해당 값을 가져와서 사용하도록 해주었습니다.
단순히 특정 객체의 프로퍼티를 외부에서 접근해서 쓰고 안쓰고라기 보다는 어떤 로직을 연산하는 책임을 어디에 두어야 하느냐가 주된 관점이 아닐까 싶습니다. 아래의 예시처럼 사용처에서 호출하는 객체의 프로퍼티를 불러와서 직접 연산을 처리하는 것과 호출하는 객체에 연산을 위임하는 차이가 아닐까요?
// 1.
const instance = new Class()
return instance.a + instance.b
// 2.
const instance = new Class()
return instance.getTotal() // instance.a + instance.b를 반환
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.
안녕하세요 카일!
피드백 주신 부분 반영해서 수정해서 다시 올려 보았습니다.
css 컨벤션에 대해서 무지했어서, 카일이 보시기에도 좀 지저분하다고 느끼셨을것 같은데 양해해주셔서 감사합니다!
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.
이번에 1차 pr을 한후에 제가 시멘틱 태그와 기본적인 css에 관해서 너무 무지했었다는걸 깨닫게 되었습니다..!
이번에 pr을 준비하면서는 최대한 시멘틱 태그를 지키고, css 컨벤션을 지켜보려고 해보았습니다..!
하지만 아직 BEM 방식에 익숙치 않아, 네이밍이 매끄럽게 된건지는 확실치 않은것 같습니다..!
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.
스킴하면서 보긴 했는데, 나쁘진 않은 듯 합니다. 만일 네이밍에 대한 기준이 어렵다면 아래의 방식대로 해보면 어떨까 싶어요.
- 본인이 할 수 있는 한 '최대한' 구체적인 이름을 명시하기 (저는 이름의 길이보다는 구체성이 훨씬 중요하다고 생각해요)
- 리뷰를 받으면서 네이밍을 하면서 꼭 없어도 되는 부분(e.g. 전치사 등)이라고 생각이 든다면 조금씩 줄여나가보기
저도 요새 많이 느끼는 부분이긴 하지만, 상대방에게 나의 의도를 명확하게 관철하기 위해서는 본인이 생각하는 범위 그 이상으로 구체적으로 작성하도록 노력하는 부분이 중요하다고 봅니다. (단, 장황한 것과 구체적인 것은 구분 필요)
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.
1차 pr에서 사용자가 잘못된 값을 입력했거나, 유효성 검증에 실패했을때 사용자에게 알림을 띄워주는 ux 고려를 하지 못했었습니다..!
그래서 이번에는 예외 처리시, alert 경고창을 띄워주도록 했는데 이 과정에서 try-catch-finally 구문이 반복되는것 같아서 유틸 함수로 한번 빼보았습니다.
하지만 try - catch - finally 구문 자체가 3단계로 나뉘어지다보니 결국 사용하는 곳에서 바로 직관적으로 이해하기 쉽지 않은것 같다는 생각도 들었습니다.
safeEventHandlerWithAlertError(
() => {
winningLotto.setWinLottoNumbers(winningNumbers);
},
(error) => {
alert(error.message);
winningLottoInputs.forEach((input) => {
initializeInputValue(input);
});
},
);
차라리 try-catch-finally 구문으로 직관성을 더해주는것이 더 나은 방법일지, 이런식으로 유틸함수로 묶어주어서 재사용성을 높이는게 더 나은 방법일지 아직 고민이 됩니다!
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.
그냥 있는 걸 사용하는 게 좋을 것 같네요! 그 이유는 아래와 같습니다.
- 기본으로 제공되는 내장 메서드를 wrapping한 것 이상의 사용성이 없어보임
try
,catch
,finally
에 비해 명시도가 떨어짐 (사용처를 보았을 때 각 인자가 어떤 걸 의미하는지 알기 힘듦)
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.
안녕하세요 버건디~ 리뷰 반영하시느라 고생 많으셨습니다. 전반적으로 이번 PR에서는 블로그 정리를 통한 학습과 본인의 의도를 리뷰어에게 공격적이지 않게 드러내시면서 논의를 이어나가려고 하시는 모습이 보기 좋았습니다 😀 현재 드린 코멘트의 컨텍스트가 꽤 많아서 한번에 학습하시긴 힘들 수도 있겠지만, 우선 키워드라도 익숙해지자는 마음으로 접근하셔서 다음 미션에서 점진적으로 적용해보면 좋을 것 같아요!
해당 미션은 이만 머지하도록 할게요 👍
안녕하세요 카일! 다시 뵙겠습니다.
이번 step2에서는 웹으로 구현을 하게 되었는데요!
잘부탁드립니다 🙇
- 여쭤보고 싶은 부분들
UI => HTML에 관련된 부분도 feat으로 컨벤션을 적는게 맞을까요 ?
feat은 기능 추가 관련된 컨벤션으로 알고 있는데, html 코드 작성은 단순히 ui 레이아웃을 잡는 것이다보니 어떻게 컨벤션을 하는게 더 맞는 방법일지 여쭈어 보고 싶습니다. 일단 feat이 제일 근접 한것 같아서, feat으로 했습니다!
uiUtils.js
라는 파일을 하나 만들어서, 이 안에서 input태그의 값을 초기화 해주거나, 타겟 태그에 클래스명을 추가 or 삭제해주는 핸들러들을 만들었는데 이러한 방향성에 대해서 어떻게 생각하시는지 여쭙고 싶어요!요구사항에
클래스(또는 객체)를 사용하는 경우, 프로퍼티를 외부에서 직접 꺼내지 않는다. 객체에 메시지를 보내도록 한다.
이러한 문장이 있었는데요! 저같은 경우 Money 클래스에서getMoney()
라는 get 함수를 만들어서 필요한 도메인에서 해당 값을 가져와서 사용하도록 해주었습니다.하지만 이부분이
객체를 객체스럽게 사용한다
와는 목적과 조금 벗어난 부분이 아닌가? 라는 생각이 들었습니다.이와 관해서 글을 정리해서 올려보았었는데, 혹시 카일은 어떻게 생각하시는지 한번 여쭙고 싶습니다!
그 외에 부분들 코멘트로 남겨보겠습니다.
감사합니다~
ps. 원래 레이아웃 구조도를 STEP2README.md 에 남겼었는데, 현재 직접 파일 자체를 들어가지 않는 이상 링크만 보이는것 같아서 직접 여기에 이미지를 첨부합니다..! 양해 부탁드립니다. 감사합니다 : )