[1단계 - 나만의 유튜브 강의실] 병민(윤병인) 미션 제출합니다. #111
Conversation
commitlint + commitizen + git-commit-emoji를 추가한다
eslint, prettier, webpack을 설정한다
babel webpack loaders를 설치한다
|
API 횟수제한이 초과되어 있네요. 데모페이지 테스트는 미뤄두고 코드만 먼저 보도록 하겠습니다. |
| // skip any questions you want | ||
| skipQuestions: ['ticketNumber', 'breaking', 'issues', 'issuesClosed'], | ||
| subjectLimit: 100, | ||
| }; |
There was a problem hiding this comment.
src/js/__test__/searchModal.test.js
Outdated
| const dummy = { ...DUMMY_VIDEO_LIST }; | ||
| dummy.items = dummy.items.slice(0, MAX_RENDER_VIDEOS_COUNT); | ||
| const videos = searchModal.checkSearchResult(dummy); | ||
| expect(videos).toHaveLength(MAX_RENDER_VIDEOS_COUNT); |
There was a problem hiding this comment.
이 테스트는 이 자체로는 별 의미가 없는 것 같은데, 15번줄의 테스트와 병합하는게 어떨까요?
There was a problem hiding this comment.
네 동감입니다. 첫번째 테스트 케이스는 테스트로서의 효용성은 없어보이고, 두번째 테스트 코드 같은경우에는 Youtube Data API정책이 바뀌지 않았는데 실수로 checkSearchResult함수나 VideoItem클래스를 바꾸었을때 이를 알려주는 용도로는 필요할듯 싶습니다.
병합 보다는, 그냥 첫번째 케이스만 삭제하는건 어떻게 생각 하시나요? 첫번째 테스트 케이스에서는 그냥 10개로 잘라서 그 길이가 10인지 테스트 하고 있기 때문에, 실상 a = 1으로 하고 a가 1인지 테스트하는것과 비슷하다고 느껴집니다.
src/js/app.js
Outdated
| this.render(); | ||
| this.addEvent(); |
There was a problem hiding this comment.
제 리뷰 단골메뉴입니다만, 이 메소드들을 분리하는 실익이 있나요? 기능별로 분리한 취지는 이해하지만, 실제로는 '초기화' 프로세스 중 일부에 불과하고 달리 따로 호출될 일이 없는데 말이에요. 불필요한 prototype 메모리만 차지하는 것이 아닐까요? 가독성을 위해 희생할 가치가 있다고 보시나요?
There was a problem hiding this comment.
네, 짚어주신 부분에 대해서 다시 깊이 고민해보았습니다. 아래와 같은 기준이 결정에 영향을 끼칠것 같습니다.
- 함수 코드의 길이
render함수 안에서 addEvent함수 안에 있는 일을 해도, 아래와 같이 리팩토링 하면 함수 길이가 별로 늘어나지 않습니다.
searchResultTemplate() {
return `
<section class="search-result">
<ul class="video-list">
</ul>
<div class="no-result">
<img src="${NotFoundImage}" alt="no result image" class="no-result__image">
<p class="no-result__description">
검색 결과가 없습니다<br />
다른 키워드로 검색해보세요
</p>
</div>
</section>`;
}
render() {
$('.search-modal').insertAdjacentHTML('beforeend', this.searchResultTemplate());
$('#search-modal-button', this.$nav).addEventListener('click', this.openModal);
$('.dimmer').addEventListener('click', this.closeModal.bind(this));
}
-
역할
화면에 View를 그리는것과, 내부 요소에 이벤트를 거는것은 다른 역할이라고 생각합니다. 그리고 항상 같이 세트로 다니리라는 보장도 하기는 어렵지 않나 싶습니다. 왜냐하면, 화면에 그리고 -> 어떤 일을 한다음에 그 일이 잘 되었을때 -> 이벤트 리스너를 걸어서 Interactive하게 만든다. 이런 프로세스가 있을 수도 있기 때문입니다. 혹은 이런 경우도 생각이 납니다.
Event Delegation패턴을 사용하고, render함수가 자주 호출되는 경우에는 render함수에 (Event Delegation을 위한) 이벤트 리스너를 계속 거는것은 버그를 발생시킬 수 있다고 생각합니다(핸들러의 중복 호출).
다만, 지금의 경우에는 위 두가지 항목이 해당되지 않는다고 생각합니다. -
가독성
1, 2번과 연결되는 부분인데, 엄연히 다른 역할을 가지고 있고 코드길이가 늘어나면 가독성도 떨어질것이라 생각합니다. 하지만, 지금의 경우에는 가독성에 영향을 주지 않아 보입니다.
그래서 지금의 경우에는 리뷰어님이 말씀하신 대로 둘을 합치는게 좋아보입니다.
다만, 위에 말씀드린것처럼 분리하는 경우가 더 좋을때도 있을것 같은데, 이에 대한 리뷰어님의 생각이 궁금합니다.
추가적으로, 2번에 말씀드린 render함수가 자주 호출되는 경우는, Component형식으로 View를 만들어서, 리액트 처럼 state가 바뀔때마다 다시 그려주는(replace)방식을 의미한것입니다.
There was a problem hiding this comment.
- 함수길이가 늘어나면 문제가 있나요? 요구사항에 함수길이에 대한 제한이 있었나요?
- '역할이 다르다'는 건 해석하기 나름인데, '화면에 그린다'는 의미의 render와, 화면에서 일어나는 사용자의 행동을 감시하여 명령을 수행한다는 의미의 event listener 둘을 합쳐서 View로 해석하는 것이 일반적입니다. render와 event listener는 상호 결합도가 높기 때문입니다.
말씀하신 render시마다 event listener를 등록하는 것은 당연히 해서는 안되는 일이죠. 그런 의미에서 분리한 취지는 이해한다고 말씀드린 거였고, 현재의 코드만을 바탕으로 의견을 드린 겁니다. - 반드시 코드 길이와 가독성이 연관되지는 않는다고 봅니다. 필요에 따라 적절한 구획을 나누어 줄바꿈 처리만 해주어도 가독성은 월등히 높아지게 되죠. 리액트를 예로 들면, 함수형 컴포넌트는 실제로는 '하나의 함수' 입니다. return구문의 위쪽에 매우 많은 로직이 들어가게 되는데, 단지 이것만 가지고 가독성이 떨어진다고 하지는 않습니다.
- render를 여러번 호출할 필요가 있는 경우에는, render함수와 무관하게 delegation 처리한 event listener를 한 번만 등록해두고, 이후에는 render만 호출하면 되겠죠. 그런 경우에는 위 코드 중에서
this.render()는 남겨두는 가치가 있을 것이고, 그런 경우에도 여전히this.addEvent()는 constructor로 옮기는 것이 맞다고 봅니다.
만약 addEvent 메소드를 여러번 호출할 필요가 있는 경우라면, 코드를 리팩토링할 좋은 기회로 삼아야 하겠습니다.
There was a problem hiding this comment.
- 함수길이가 늘어나면 문제가 있나요? 요구사항에 함수길이에 대한 제한이 있었나요?
- 함수 길이가 늘어나도 사실 잘 읽히기만 한다면 문제는 없다고 생각합니다.
함수 길이의 제한은 요구사항에 없었지만, 15줄까지만 작성해보도록 권고를 받은적이 있어서 가능하면 나누려 했습니다.
- '역할이 다르다'는 건 해석하기 나름인데, '화면에 그린다'는 의미의 render와, 화면에서 일어나는 사용자의 행동을 감시하여 명령을 수행한다는 의미의 event listener 둘을 합쳐서 View로 해석하는 것이 일반적입니다. render와 event listener는 상호 결합도가 높기 때문입니다.
말씀하신 render시마다 event listener를 등록하는 것은 당연히 해서는 안되는 일이죠. 그런 의미에서 분리한 취지는 이해한다고 말씀드린 거였고, 현재의 코드만을 바탕으로 의견을 드린 겁니다.
- 네, 이해했습니다. 감사합니다 :D
- 반드시 코드 길이와 가독성이 연관되지는 않는다고 봅니다. 필요에 따라 적절한 구획을 나누어 줄바꿈 처리만 해주어도 가독성은 월등히 높아지게 되죠. 리액트를 예로 들면, 함수형 컴포넌트는 실제로는 '하나의 함수' 입니다. return구문의 위쪽에 매우 많은 로직이 들어가게 되는데, 단지 이것만 가지고 가독성이 떨어진다고 하지는 않습니다.
- 네, 맞습니다.
- render를 여러번 호출할 필요가 있는 경우에는, render함수와 무관하게 delegation 처리한 event listener를 한 번만 등록해두고, 이후에는 render만 호출하면 되겠죠. 그런 경우에는 위 코드 중에서 this.render()는 남겨두는 가치가 있을 것이고, 그런 경우에도 여전히 this.addEvent()는 constructor로 옮기는 것이 맞다고 봅니다.
만약 addEvent 메소드를 여러번 호출할 필요가 있는 경우라면, 코드를 리팩토링할 좋은 기회로 삼아야 하겠습니다.
- 아, 아마 제가 리뷰어님의 말을 잘못 이해한듯 싶습니다. this.render()와 this.addEvent()를 분리 하는 실익이 있냐는 말을 저는 this.render()함수 안에 this.addEvent()함수의 내용물을 넣어도 괜찮다는 말로 이해했습니다. 그런데 지금 답변해 주신 내용과 리뷰를 다시 읽어보니, render()와 addEvent()안에 있는 코드를 모두 초기화 단계에 그냥 적어도 괜찮다는 의미인것 같습니다.
비교를 위해서 아래 코드를 작성해 보았습니다.
함수를 분리하는 경우
class MyView1 {
constructor() {
this.render();
this.bindElements();
this.addEvents();
}
templateList() {
return `
<li>A</li>
<li>B</li>
<li>C</li>
<li>D</li>
`;
}
template() {
return `
<button class="my-btn">BUTTON</button>
<ul class="my-list">
${templateInner()}
</ul>
`;
}
render() {
document.querySelector('.container').innerHTML = this.template();
}
bindElements() {
this.$container = document.querySelector('.container');
this.$button = this.$container.querySelector('.my-btn');
this.$list = this.$container.querySelector('.my-list');
}
addEvents() {
this.$button.addEventListener('click', this.clickHandler);
this.$list.addEventListener('scroll', this.scrollHandler);
}
}
초기화 단계에서 전부 처리해 주는 경우
class MyView2 {
constructor() {
this.$container = document.querySelector('.container');
this.$container.innerHTML = this.template();
this.$button = this.$container.querySelector('.my-btn');
this.$list = this.$container.querySelector('.my-list');
this.$button.addEventListener('click', this.clickHandler);
this.$list.addEventListener('scroll', this.scrollHandler);
}
templateList() {
return `
<li>A</li>
<li>B</li>
<li>C</li>
<li>D</li>
...
`;
}
template() {
return `
<button class="my-btn">BUTTON</button>
<ul class="my-list">
${templateInner()}
</ul>
...
`;
}
}
이렇게 두개를 놓고 보니, 대략적으로 이 Class가 뭘 하려는 건지는 전자가 조금 더 잘 표현한것 같은데('그리고 -> 바인딩하고 -> 이벤트를 거는구나'), addEvent()와 bindElements()의 위치가 바뀌면 에러가 발생하기 때문에 좀 아슬아슬 해보입니다.
구체적으로 뭘 하려는지는 후자가 더 잘 읽힙니다.
말씀하신 대로 하는게 더 좋아보입니다. 알려주셔서 감사합니다 :D
src/js/app.js
Outdated
| $('#search-modal-button', this.$nav).addEventListener('click', this.openModal); | ||
| $('.dimmer').addEventListener('click', this.closeModal.bind(this)); |
There was a problem hiding this comment.
bind(this) 대신 handler로 등록할 메소드들을 class field (arrow function)으로 전환하시는걸 추천합니다.
There was a problem hiding this comment.
감사합니다 :D
혹시 해당 방식을 추천해주시는 이유가 있는지 궁금합니다.
There was a problem hiding this comment.
메소드 선언 방식에 의하면 prototype에 원본메소드가 남아있는 채로, 실제로 호출되는건 새로 만들어진 bound 메소드입니다. 사용하지 않을 불필요한 메소드가 남아있다는 뜻이죠. 반면 class field를 이용하면 prototype에는 남아있지 않고, 인스턴스 자신이 사용할 함수만 들고 있게 됩니다.
There was a problem hiding this comment.
병민님 안녕하세요, 리뷰어 로이입니다. 필요한 코드만 간결하게 작성하셔서 읽기에 좋았습니다. 코멘트 남겼고, 추가로 몇가지 피드백 드릴게요.
- 화면 레이아웃은 한 줄에 4개씩이니 한 번에 로드하는 컨텐츠 개수도 4의 배수로 맞추면 좋겠어요.
- 역할이 모호한 변수나 여러가지 역할을 수행하는 함수 등은 주의가 필요해요.
- 다음 스텝 때는 intersectionObserver를 활용해보시면 어떨까 합니다.
로드하는 컨텐츠의 개수는 요구사항에 10개로 지정되어 있어서, 수정하지 않은점 참고 바랍니다 :D
src/js/app.js
Outdated
| } | ||
| } | ||
|
|
||
| new App(); |
There was a problem hiding this comment.
EOF 처리가 필요합니다.
prettier를 사용하신걸로 보이는데 왜 이럴까요...
src/js/searchModal.js
Outdated
| if (this.saveVideo(videoId, videoList)) { | ||
| target.setAttribute('hidden', true); | ||
| } | ||
| } |
There was a problem hiding this comment.
테스트해보지 못해서 확신할 순 없지만, 코드상으로 보기엔 초기 로딩시에 localStorage에 저장된 값과 비교하는 과정이 빠져있어서,
- 같은 검색어로 다시 검색하면,
- 앞서 말씀드린 '무한! 로딩'으로 인해 스크롤을 내리면,
이미 저장된 비디오를 중복하여 저장할 수 있을것으로 추측되네요.
| while (parentNode.firstChild) { | ||
| parentNode.removeChild(parentNode.firstChild); | ||
| } | ||
| }; |
There was a problem hiding this comment.
src/js/videoItem.js
Outdated
| conventPublishedAtFormat(publishedAt) { | ||
| const [year, month, day] = publishedAt.split('T')[0].trim().split('-'); | ||
| return `${year}년 ${month}월 ${day}일`; | ||
| } |
There was a problem hiding this comment.
처음에 말씀하신 대로 하려 했는데, 함수명 짓기가 어려웠습니다.
그래서 videoItem안에 있으면 해당 함수를 한정적으로 사용하는것이기 때문에 함수명을 general하게 지을 수 있다고 생각했습니다.
밖으로 뺀다면 'ISO8601형식의 날짜를 년월일로 변환한다'라는 의미를 가진 함수명을 짓고 싶은데,, 혹시 추천해주실 만한 함수명이 있을까요? 아니면 해당 함수가 밖에 있어도 함수명을 좀 general하게 지어도 될까요?
함수로 안하고 초기화 단계에서 그냥 바로 변환을 해보았습니다. 리뷰어님의 생각은 어떠신지 궁금합니다!
bd6c675
src/js/app.js
Outdated
| } | ||
|
|
||
| openModal() { | ||
| const $modalContainer = $('.modal-container'); |
There was a problem hiding this comment.
$modalContainer는 몇군데서 중복 호출되니 class의 멤버로 미리 지정해두는 편이 어떨까요?
src/js/searchModal.js
Outdated
| saveVideo(videoId, videoList) { | ||
| if (videoList.length >= MAX_SAVABLE_VIDEOS_COUNT) { | ||
| alert(`비디오는 ${MAX_SAVABLE_VIDEOS_COUNT}개 이상 저장할 수 없습니다`); | ||
| return false; | ||
| } | ||
| localStorage.setItem(LOCAL_STORAGE_VIDEO_LIST_KEY, JSON.stringify([...videoList, videoId])); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
saveVideo의 역할이 너무 많습니다.
validation 체크는 별도로 분리해두는게 좋을 것 같네요.
There was a problem hiding this comment.
한번 try - catch문으로 작성해 보았습니다. 확인 부탁드립니다 :D
44c435c
그렇군요, 그러면 혹시 제가 뭔가 해야하는건가요? |
리뷰 진행방식에 대해서는 이전 미션들에서 많이 경험해보시지 않았나요? 혹시 그간 리뷰 진행이 제대로 안되었나요? 코멘트 확인해보시고 수정하고 싶은 부분이 있다면 수정하신 다음 다시 리뷰요청 해주시면 됩니다. |
|
아 저는 뭔가 잘못 골랐다고 하셔서 제가 추가적으로 뭔가를 해야하나 싶어서 질문 드렸습니다. 제가 용어를 잘 몰라서요. 아 그리고 ㅋㅋ,, 저는 콤피가 아니라 병민입니다! |
에고 죄송합니다 ㅠ |
렌더 함수를 따로 분리하는것보다 초기화 단계에 몰아 넣는게 더 효율적이고, 가독성도 좋아진다
DOM에서 엘리먼트를 탐색하는 일은 적잖은 비용이 발생하기 때문에, 자주 사용한다면 미리 찝어놓는게 효율적이다.
아래 테스트가 checkSearchResult함수의 유효성을 검사해 주기 떄문에, 이 테스트는 필요없다.
… 처리한다 굳이 분리할만큼 복잡하지도 않고, 초기화 단계에 적어놓는것이 더 가독성에 좋기 때문이다
…메서드를 사용한다 bind는 새로운 함수를 리턴한다. class field에 화살표 메서드를 만들고 이것을 핸들러로 사용하면 bind로 새로운 함수를 리턴하는건 메모리 낭비다
다만 새로 검색을 하는 경우에는 예외적으로 다시 생성한다. 그 이유는 스켈레톤이 아닌 요소들만 선택해서 지우는것보다는 더 비용이 적게 들기 때문이다.
불러올 동영상이 없을때 첫페이지부터 다시 로드하는 로직이었는데, 이것을 수정했다. 무한히 계속 동영상이 로드될 수 있기 때문이다.
검색 결과가 없을 시 checkSearchResult가 빈배열을 리턴하는데 !로 검사해서 항상 통과한다. searchResult가 null이면 checkSearchResult까지 갈 필요가 없기 때문에, 미리 검사를 해준다.
null로 들어가면 올바르지 않은 request로 판정되어 에러가 난다
nextPageToken을 안비워주면 같은 검색어로 검색해도 다음 페이지가 보이게 된다
1. 날짜 변환 함수의 내용물이 간단하다 2. 날짜 변환은 VideoItem에서만 이루어지고 있다 3. VideoItem은 자주 initialized되는 클래스이다 그리고 함수가 자주 호출되면 성능에 안좋기 때문에 지금 같은 경우에는 초기화 단계에서 바로 날짜 변환을 해도 된다
replaceChildren이 있기 때문에 removeChildren 유틸 함수를 굳이 만들어서 쓸 필요가 없다
|
데모페이지는 다시 배포되지 않은것일까요? |
재배포 되었습니다! 확인 바랍니다 :D |
나만의 유튜브 강의실 데모 페이지
안녕하십니까 리뷰어님! 이번 리뷰 잘 부탁드립니다 :D