[1단계 - 나만의 유튜브 강의실] 하루(김하루) 미션 제출합니다 #11
Conversation
src/js/models/searchModel.js
Outdated
| setVideoData(target, videoData) { | ||
| target.dataset.videoId = videoData.videoId; | ||
| target.dataset.videoTitle = videoData.videoTitle; | ||
| target.dataset.channelId = videoData.channelId; | ||
| target.dataset.channelTitle = videoData.channelTitle; | ||
| target.dataset.publishedAt = videoData.publishedAt; | ||
| } | ||
|
|
||
| getVideoData(target) { | ||
| return { | ||
| videoId: target.dataset.videoId, | ||
| videoTitle: target.dataset.videoTitle, | ||
| channelId: target.dataset.channelId, | ||
| channelTitle: target.dataset.channelTitle, | ||
| publishedAt: target.dataset.publishedAt, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
질문드립니다! 🙋🏻♀️ - 개별 영상데이터 보관 방법
DOM요소에 'dataset 속성'으로 필요한 정보를 임시로 담아두는 것이 적절한지 궁금합니다.
유튜브 검색결과에서 '저장 버튼'을 클릭하면 해당 영상정보를 로컬스토리지에 저장하는 기능을 구현했습니다. 저장해야 할 videoData는 총 다섯가지 입니다(영상id, 영상제목, 채널id, 채널제목, 만든 날짜)
사용자가 특정 영상의 '저장버튼'을 클릭하면 해당 영상의 정보만 넘겨서 저장해주어야 하고, 사용자가 어떤 영상을 저장할지 모르기 때문에 조회된 모든 영상의 videoData를 계속 가지고 있어야 했습니다. '이 videoData를 누가 들고있는게 좋을까'가 저희의 고민이었습니다. '저장버튼'의 클릭이벤트가 트리거이니까 각 '저장버튼'에 각 영상의 정보를 담아두면, 클릭이벤트의 핸들러에서 videoData를 target에서 바로 받아올 수 있겠다고 생각했습니다. 그래서 현재 각 저장버튼에 각각의 videoData를 저장버튼에 넣어두게 되었습니다.
이렇게 DOM요소에 많은(?) 정보를 담아본게 처음이기도 하고, 조회를 할때마다 모든 저장버튼에 이렇게 videoData를 넣어주는 방법이 반복되고 있어서 괜찮은 방법인지 확신이 서지않아 질문드려봅니다. DOM요소에 요정도 규모가 되는 데이터를 dataset으로 저장하는 방식에 대해서 어떻게 생각하시는지 의견주시면 감사하겠습니다. 🙏
질문을 작성하던 중, 검색결과 전체의 videoData를 통째로 상태값으로 들고있다가 클릭이벤트가 발생한 버튼이 검색결과 중 몇 번째 검색결과인지 index만 가져와서 n번째 영상을 저장해주는 방법이 생각났습니다...! 이 방법에 대해서도 어떻게 생각하시는지 궁금합니다...! 리뷰어님의 의견부탁드리겠습니다...!! 🙏
There was a problem hiding this comment.
버튼에 정보를 저장하는 것이 좋은 방법은 아닙니다.
-
우선 버튼은 videoId 만 알면 됩니다.
videoId에 대한 정보를 저장해달라는 이벤트 처리 정도가 버튼 클릭이 일어났을 때 하는일이고 videoId 에 해당 하는 정보중에 어떤 정보를 저장할지, 어디에 어떻게 저장할지 등등은 버튼의 관심사가 아니므로 버튼(특히 DOM)에 저장할 필요는 없습니다. 검색된 데이터를 js 에서 이미 알고 있고, 버튼을 통해 id를 알았으니 이 id에 해당하는 정보들을 저장 할 수 있는 형태로 잘 말아서 저장하도록 요청하는 그런 기능이 필요할 것 같습니다. -
검색된 데이터를 '저장' 했을때 필요한 정보를 저장하면 되는데, 저장 하지도 않았는데 검색된 시점에 저장을 위한 다수의 DOM 조작이 미리 일어나는 것은 비효율적이기도 합니다. 이런것들이 페이지 갱신의 속도를 떨어뜨리는 이유가 될 수도 있습니다.
There was a problem hiding this comment.
저장버튼에서 video의 id만 남기고 videoData를 덜어냈습니다. 이후 저장버튼 클릭 이벤트 발생 시점에 Model에서 알고있는 검색결과에서 id로 조회하도록 변경하였습니다! 앞으로 JS에서 관리해야할 상태값을 DOM 요소에 무리하게 담지 않도록 주의하겠습니다. 상세한 조언 감사합니다
src/js/models/localStorage.js
Outdated
| { | ||
| */ | ||
|
|
||
| export default class VideoLocalStorage { |
There was a problem hiding this comment.
- 비디오를 알고 있고 저장 해달라고 요청하는 역할(localStorage는 모름)과
- 구체적인것(비디오 등)은 모르고 요청이 온 데이터를 저장하고 응답하는 역할(localStorage는 알고 있음)
2개로 역할을 나누면 좋을 것 같습니다.
지금은 2개의 역할이 한 데 뒤섞여서 책임이 과한 것 같습니다.
파일 이름은 localStorage 이고 class 이름은 VideoLocalStorage 로 되어 있는데, 역할을 나누면 이 부분도 깔끔해질 것 같습니다.
** 참고로 localStorage에 저장하는지 쿠키에 저장하는지는 서버에 저장하고 Ajax를 통해 데이터를 가져오는지 등은 데이터를 저장하고 가져는것은 대부분의 경우 요청하는 쪽의 관심사는 아닙니다. (관심사 일 때도 있지만)
비유하자면 내가 친구에게 물건을 맡겨두면서 이거 잠깐 가지고 있어 라는 요청과 다시 줘 라는 요청을 할 수 있다고 가정할 때 맡아두는 친구가 물건을 주머니에 넣어두든지 서랍에 두든지 가방에 두든지 그건 내 관심사가 아니라는것과 비슷합니다.
그저 필요한 데이터를 요청할뿐이고 데이터를 동기적으로 받을지 비동기적으로 받을지만 알고 있으면 되는 것 입니다. 요청하는쪽에서 localStorage 라는 정보가 노출된 객체나 메서드를 사용하도록 하면, 추후에 localStorage가 아닌 다른 수단으로 바뀌게 되었을때 내부 구현만 바꾸면 되는게 아니라 이름도 같이 바꿔야 하기 때문에 사용하는쪽의 코드도 바뀌어야 합니다.
따라서 사용하는쪽에서는 한층 더 추상적인 이름으로 사용 할 수 있도록 네이밍 하는것이 좋습니다.
역할을 어떻게 나누느냐에 따라 사용하는 쪽에서 어디에 저장하는지 알 수도 있고 모를 수도 있습니다. 따라서 이 부분은 구현 하실 때 어떤 관점으로 역할을 나눌 것인지 생각하는 포인트 정도로만 이해하시면 될 것 같습니다.
There was a problem hiding this comment.
Controller에서는 videoData를 알지만 어떤 저장소인지 모른 채 필요한 내용을 요청하고(e.g. getRecentKeywords, saveVideo) Model 에서 CRUD를 수행하도록 변경하였습니다. 로컬스토리지를 직접적으로 사용하는 함수는 범용적으로 사용할 수 있을 것이라고 판단해서 유틸함수로 분리했습니다!
src/js/models/searchModel.js
Outdated
| } | ||
|
|
||
| requestSearchResult() { | ||
| return request(this.getSearchApiURI(), 'GET').then((response) => { |
There was a problem hiding this comment.
모델이 request 를 직접 알고 있는것은 적절하지 않은 것 같습니다.
컨트롤러 -> 모델 -> 리퀘스트 -> fetch (X)
컨트롤러 -> 서비스 -> 리퀘스트 -> fetch (O)
위와 같은 의존 방향을 갖는게 좋습니다.
서비스와 리퀘스트 사이에 몇 개의 레이어가 더 생길 수도 있습니다.
There was a problem hiding this comment.
비즈니스 로직이 아닌 request, fetch 관련 작업을 모두 SearchService에 담았습니다. 현재 API요청을 보내고 받은 데이터를 가공하는 작업을 모두 Service가 수행하고, 컨트롤러에서 this.service.getSearchResultAsync()과 같이 서비스를 사용하도록 변경하였습니다! 👍
src/js/models/searchModel.js
Outdated
| setVideoData(target, videoData) { | ||
| target.dataset.videoId = videoData.videoId; | ||
| target.dataset.videoTitle = videoData.videoTitle; | ||
| target.dataset.channelId = videoData.channelId; | ||
| target.dataset.channelTitle = videoData.channelTitle; | ||
| target.dataset.publishedAt = videoData.publishedAt; | ||
| } | ||
|
|
||
| getVideoData(target) { | ||
| return { | ||
| videoId: target.dataset.videoId, | ||
| videoTitle: target.dataset.videoTitle, | ||
| channelId: target.dataset.channelId, | ||
| channelTitle: target.dataset.channelTitle, | ||
| publishedAt: target.dataset.publishedAt, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
버튼에 정보를 저장하는 것이 좋은 방법은 아닙니다.
-
우선 버튼은 videoId 만 알면 됩니다.
videoId에 대한 정보를 저장해달라는 이벤트 처리 정도가 버튼 클릭이 일어났을 때 하는일이고 videoId 에 해당 하는 정보중에 어떤 정보를 저장할지, 어디에 어떻게 저장할지 등등은 버튼의 관심사가 아니므로 버튼(특히 DOM)에 저장할 필요는 없습니다. 검색된 데이터를 js 에서 이미 알고 있고, 버튼을 통해 id를 알았으니 이 id에 해당하는 정보들을 저장 할 수 있는 형태로 잘 말아서 저장하도록 요청하는 그런 기능이 필요할 것 같습니다. -
검색된 데이터를 '저장' 했을때 필요한 정보를 저장하면 되는데, 저장 하지도 않았는데 검색된 시점에 저장을 위한 다수의 DOM 조작이 미리 일어나는 것은 비효율적이기도 합니다. 이런것들이 페이지 갱신의 속도를 떨어뜨리는 이유가 될 수도 있습니다.
src/js/views/searchView.js
Outdated
|
|
||
| export default class SearchView { | ||
| constructor() { | ||
| this.videoStorage = new VideoLocalStorage(); |
There was a problem hiding this comment.
View는 DOM을 알고 있고, 주입된 데이터를 화면에 뿌려주는 역할입니다.
따라서 VideoLocalStorage를 알고 있으면 안됩니다.
사용되는곳은 없는것 같긴한데.. 그냥 지우면 될 것 같습니다.
There was a problem hiding this comment.
삭제하려고 했는데 미처 확인 못하고 PR 올렸습니다 ㅜㅜ 죄송합니다! 🙇🏻♀️
| import { VIDEOS_TO_WATCH, STORAGE_CAPACITY_FULL, MAX_VIDEO_STORAGE_CAPACITY, RECENT_KEYWORDS } from '../constants.js'; | ||
| import { isEndOfPage } from '../utils/DOM.js'; | ||
|
|
||
| export default class SearchController { |
There was a problem hiding this comment.
단계가 진행됨에 따라 Controller가 비대 해질 것 같습니다.
SearchService 라는 이름의 역할을 하나 더 만들어서 컨트롤러에 주입하고 검색과 관련된 기능을 그쪽에 두면 어떨까 싶습니다.
컨트롤러는 서비스에 요청하고 서비스로 부터 받은 데이터를 뷰에 뿌려준다는 느낌으로 레이어를 구분 하면 좋을것 같습니다.
** 이 부분은 VideoLocalStorage 역할을 나누면서 이번에 같이 하셔도 되고, 다음 단계로 가서 하셔도 될 것 같습니다.
There was a problem hiding this comment.
Controller에서 최대한 추상적으로 요청하고 기능을 덜어낼 수 있도록 했습니다...! Service를 추가하면서 Controller의 기능을 덜어내는 동시에 모델의 구조를 맞물려 변경하면서 커밋을 각각 구분해서 올리지 못했습니다,,,😢😢
📌 리뷰반영 요약표페어와 함께 다음과 같은 고민하며 코드를 수정했습니다...!
|
| this.nextPageToken = ''; | ||
| this.totalSearchResult = []; | ||
| this.keyword = keyword; | ||
| this.saveKeyword(); |
There was a problem hiding this comment.
SearchModel이 생성되면 keyword를 저장한다.
라고 저는 읽힙니다.
SearModel의 사용 부분을 가보니 SearchModel은 컨트롤러가 생성될 때 생성되고 있고요.
다시 말하면 어플리케이션의 구동 단계에서 유저가 아무 검색도 하지 않았는데 키워드를 세이브 하는 로직이 도는 셈이됩니다.
saveKeyword 내부 구현 로직도 save 를 한다고 보기 어렵습니다.
어떤 조건일 때는 delete 하는 로직이 숨어있거든요.
프로세스, 로직, 네이밍 전반적으로 다시 검토가 필요한 부분 같습니다.
There was a problem hiding this comment.
앗... 페어와 네이밍으로 고민을 많이 했던 부분인데 결국 더 나은 이름을 찾지 못하고 그대로 제출했었습니다...! 말씀해주신대로 이름만 고민할 것이 아니라 프로세스 전반에 대해서 다시 한 번 고민해보면 더 좋은 네이밍이 자연스럽게 나올 것 같네요...! 조언 감사드립니다 👍👍👍
| import { httpRequest } from '../utils/httpRequest.js'; | ||
| import { isSavedVideo } from '../controllers/elementValidator.js'; | ||
|
|
||
| export default class SearchService { |
There was a problem hiding this comment.
service가 getSearchApi 를 호출하고 getSearchApi 에서 uri 및 쿼리스트링 조작 등의 요청/응답 관련 책임이 있도록 레이어를 좀 더 나누면 좋을듯 한데 일단 이 정도로 해놓고 추후에 아 이래서 나눠야 되는구나하는 생각이 들면 그 때 바꿔보셔도 될 것 같습니다.
There was a problem hiding this comment.
아하 레이어를 더 추가할 수 있다고 말씀해 주셨던 부분이 요롷게 적용되는 거군용! 추가 설명 감사드립니다 👍👍👍
자프님, 안녕하세요!
처음 인사드리는 🙋🏻♀️ 하루(@365kim) 입니다. 앞으로 2주간 잘 부탁드리겠습니당!!! 👍👍👍
저는 디토(@dudtjr913)와 함께 페어프로그래밍을 진행했습니다.
요약
유튜브 강의실 미션
1단계 - 구현기능 목록
src/images/status/not_found.png경로에 있다.저장할 수 있다. (실제 저장이 아닌 영상 id를 Web Storage에 저장). 단 이미 저장된 경우는 저장 버튼이 보이지 않게 한다.바쁘신 와중에 시간내서 리뷰해주셔서 정말 감사드립니다!!! 🙇🏻♀️
크고 작은 개선사항 코멘트로 전해주시면 열심히 개선해보겠습니다!! 🙏
감사합니다 😆