[PR 코드리뷰] 우아한테크코스 프론트엔드 레벨1 - 나만의 유튜브 강의실 미션

 

 

우아한테크코스 프론트엔드 과정 레벨1 <나만의 유튜브 강의실 미션>을 수행하며 쿠팡의 @wow9144 님께 리뷰를 받을 수 있었다. 리뷰어님께서 실생활의 예시를 비유로 들어주시면서 상세하게 설명해주신 덕분에, 비전공자로서 어렵게 느껴질 수 있는 개념을 보다 쉽게 접근할 수 있었다. 👍👍👍

 

미션 1단계 피드백

👉 PR 바로가기

1. DOM요소에 데이터 저장은 지양

버튼에 모든 videoData를 저장하는 것이 좋은 방법은 아니다.

버튼은 videoId 만 알면 된다. 클릭 시 해당 videoId에 대한 정보를 저장 요청하는 외에, videoData 중 구체적으로 어떤 정보를, 어디에 어떻게 저장할지는 버튼의 관심사가 아니다. 따라서 버튼(DOM)에 저장할 필요가 없다. 검색요청을 통해 받은 데이터를 JS에서 이미 알고 있고, 버튼을 통해 videoId를 알았으니 이 id에 해당하는 정보들을 저장하도록 따로 요청하는 기능을 추가해보자.

게다가, 검색된 시점에 미래의 (일어나지도 않을지 모르는) 저장을 위해 미리 다수의 DOM 조작을 하는 것은 비효율적이다. 이는 페이지 갱신의 속도를 떨어뜨리는 이유가 될 수 있다. 검색된 데이터를 '저장한 시점'에 필요한 정보를 저장하면 된다.

2. 컨트롤러가 '어디에 어떻게' 저장할지 모르게

대부분의 경우, 어디에 저장할지(localStorage, 쿠키, 서버) 또는 어떻게 가져올지(localstorage에서 get, ajax로 fetch)는 요청하는 쪽(컨트롤러)의 관심사는 아니다. 예를 들어, 친구에게 "이거 잠깐 가지고 맡아줘", "다시 돌려줘" 라는 요청을 할 때, 친구가 그 물건을 주머니에 넣어보관하든, 서랍에 보관하든, 가방에 보관하든 그건 나의 관심사가 아니다. 마찬가지로 그저 필요한 데이터를 요청할 뿐, 데이터를 동기적으로 받을지 비동기적으로 받을지만 알고 있으면 된다.. 추후에 localStorage가 아닌 다른 저장 수단으로 바뀌게 될 경우, 지금처럼 요청하는 쪽에서 localStorage 라는 정보가 노출된 객체나 메서드를 사용하도록 한다면 내부 구현 뿐과 이름 모두를 같이 바꿔야 한다. 따라서 사용(요청)하는 쪽에서는 추상적인 이름으로 사용하는 것이 좋다.

videoData를 알지만 어떤 저장소인지 모른 채 저장 요청하는 역할과, videoData는 모르지만 요청이 온 데이터를 어떤 저장소에 어떻게 저장하고 응답해야하는지 아는 2가지 역할은 구분되어야 한다. 지금은 두 역할이 한 데 섞여 책임이 과한 상태이다. 

3. 모델에서 HTTP요청 제거

현재처럼 모델이 request 를 직접 알고 있는 것은 적절하지 않다. request, fetch는 비즈니스로직이 아니기 때문이다. 모델의 관심사가 http는 아니기 때문에 request를 직접 알고 있으면 안된다.

4. 서비스 레이어 추가

SearchService 라는 이름의 Service 레이어를 추가하자. 이로써 Controller가 비대해지는 문제를 해결할 수 있다. SearchService를 Controller에 주입해서, Controller는 검색기능을 Service에 요청하고, Service로부터 받은 데이터를 Controller가 View에 뿌려주는 방향으로 레이어를 구분하자. 참고로 Service는 비디오를 알지만, 로컬스토리지는 알 수도 모를 수도 있다. 모르는게 변경에 대해서는 유연하지만 너무 과할수도 있으니 적절한 수준에서 결정하자.

정리하자면 아래와 같은 의존 방향을 갖는게 좋다. 여기서 Service와 리퀘스트 사이에 몇 개의 레이어가 더 생길 수 있다. View는 DOM을 알고 있고, 주입된 데이터를 화면에 뿌려주는 역할이다. 따라서 VideoLocalStorage를 알고 있으면 안된다.

컨트롤러 -> 모델 -> 리퀘스트 -> fetch (X)
컨트롤러 -> 서비스 -> 리퀘스트 -> fetch (O)

 

미션 2단계 피드백

👉 PR 바로가기

1. 식별자의 네이밍은 '역할'로!

'역할'이 아니라 텍스트(값)에 불과한 기획 명칭을 식별자의 이름에 적용해버리면, 변경에 굉장히 취약해진다. 따라서 '나만의 유튜브 강의실'을 관리하는 Controller를 'ClassroomController'라고 이름지으면 안된다. AppController 또는, App 안에서도 어떤 특정한 역할을 한다면 그 역할에 맞는 이름을 지어주어야 한다.

이런 네이밍 실수는 실무에서도 발생할 수 있다. 처음 기획단계에서 '나만의 유튜브 강의실'로 시작했지만, 유튜브를 검색/저장하는 기능을 살려 타겟 고객층을 확대해서 '강의실'을 빼고 '나만의 유튜브'로 변경될 수 있다. 이런 변경은 언제든지 있을 수 있는데, 그럼 그때마다 'Classroom'과 같은 이름을 가진 모든 코드를 수정해야 한다. 이 말은 처음부터 'Classroom' 이라는 이름이 적절하지 않았다는 뜻이다.

텍스트가 주는 의미에 매몰되어 네이밍하면 안된다. 우는 아기를 안고 달래고 있는 아저씨를 떠올리며 이름을 붙인다고 하면 '아빠'라는 이름이 적절하지, '자프' 라는 구체적인 이름이 적절하지 않은 것과 마찬가지이다. 이 아빠는 '자프' 일수도 있고 '홍길동' 일수도 있고 '철수' 일 수도 있다. 텍스트를 어떻게 추상화해서 네이밍하면 좋을지 생각해보자. 

2. 초기화하지 않은 프로퍼티 접근은 위험

videoToManage 프로퍼티가 Model 내부에서 초기화 되지않고, 외부에서 특정 시점에 set되고 있다. 만약 set이 되기 전에 moveVideo() 를 실행한다면 undefined인 videoToManage의 id 에 접근하려고 해서 런타임 에러가 발생할 것이다. 따라서 videoToManage의 초기화는 이 클래스에서 이뤄져야 한다. 또는 최소한 this.videoToManage 가 있는지 확인하고 안전하게 id를 접근하는 식으로 작성해야 한다.

+ videoToManage 에 warning 이 잡히지 않는다면 에디터 설정 변경이 필요해보인다.

// controller.js
this.model.videoToManage = target.closest('article');

// model.js
moveVideo() {
  const targetId = this.videoToManage.id;
  ...
}

 

미션 3단계 피드백

👉 PR 바로가기

1. NPE 가능성이 있다면 선언부에서 처리

NPE(Null Pointer Exception)을 처리하는 다음 두 가지 방법을 생각해보자. 하나는 선언부에서 if 조건문으로 처리하는 방법이고 다른 하나는 try...catch로 catch에서 처리하는 방법이다. if로 선언부에서 처리하는 것과 달리, try...catch로 처리하는 것은 선언부에서 완성되지 않은 코드를 (문제가 있는 채로) 코드를 사용하는 측에 떠넘기는 것과 같다. 따라서 NPE가 발생할 여지가 있다면 try...catch 보다는 if문으로 객체를 검사하는 것이 옳은 방법이다.

2. messenger가 매개변수 이어야 할까?

현재 showSnackbar()함수에 첫번째 매개변수로 messenger 를 전해주고 있는데 이는 적절하지 않다. messenger에 $snackbar 가 아닌 다른 걸 받을 수도 없고, 받을 일도 없다. 더욱이 messenger에 적용할 CSS 클래스 'show'를 만들어놓지 않았다면 동작하지 않는다. 따라서 messenger가 되는 $snackbar 를 가진 객체를 통해 snackbar.show(message, showtime) 형태로 호출하는 것이 좋겠다.

// 개선 전
// showSnackbar 정의
export const showSnackbar = ({ messenger, message, showtime }) => {
  messenger.innerText = message;
  messenger.classList.add('show');
  setTimeout(() => {
    messenger.classList.remove('show');
  }, showtime);
};
// showSnackbar 호출
showSnackbar({ messenger: this.$snackbar, message, showtime: SNACKBAR_SHOW_TIME });

// 개선 후
class Snackbar {
 constructor(element, time = 0) {
   this.element = element
   this.time = time
 }
 show(message) {
   this.element.innerText = message
   this.element.style.display = 'block'
 }
 hide() {
  setTimout(() => {
    this.element.style.display = 'none'
  }, this.time)
 }
}

const snackbar = new Snackbar($('.js-snackbar'))
snackbar.show("...")

 

다른 크루의 리뷰 메모 👀

피드백 내용 앞의 번호는 순번이 아니라 PR 번호이다. 

 

아키텍쳐/패턴

#12 classList에 특정 클래스를 추가하는 등의 DOM 조작은 컨트롤러가 VIEW의 멤버변수에 접근해서 하기 보다는 VIEW가 직접 하는게 좋겠다. 

#22 비즈니스 로직을 서비스로 분리했다고 하더라도 컨트롤러에서 모델을 직접 접근 해도 괜찮다. 컨트롤러가 모델을 직접 알지 못하도록 레이어를 철저히 구분하면 모델을 변경하는 쪽을 알기 쉬우나 때론 불필요하게 한 단계 서비스를 거쳐서 가야하는 불편함이 있을 수 있다.
트레이드오프가 있는 부분이다.

#22 컨트롤러가 제왕처럼 너무 세세하게 컨트롤하려고 한다면 수정이 일어났을 때 2곳 이상을 수정해야한다. 택시에서 손님이 '인천공항으로 가주세요'만 하면 되는데, '좌회전 해주세요. x번 고속도로 타주세요. 다리 지나 직진해주세요.' 라고 세세히 컨트롤하려는 상황과 같다. 인천공항으로 가는 길이 바뀌면 택시기사 측만 바뀌면 되는데, 손님이 세세하게 컨트롤 하려고 하면 택시기사측과 손님측 모두 바뀌어야 한다.. 고속도로를 탈지 말지는 오롯이 택시기사에게 맡기는 것이 좋고, 손님과 기사의 커뮤니케이션은 '인천공항'으로 정리하는 것이 좋다.

#22 단일 책임 원칙을 위해 메서드를 작게 하다보면 외부에서 그 작은 메서드를 일일이 호출해야 하는 상황이 생긴다. 이럴 경우에 한 책임에 대하여 여러 메서드를 호출하는 중간 메서드를 두고, 그 메서드 하나에만 컨트롤러가 의존하게끔 하면 코드의 가독성을 높일 수 있다.

 

 

함수/클래스

#01 범용적으로 활용가능한 throttle, debounce는 util함수로 분리하자.

#09 가독성을 위해 이벤트핸들러를 bind(attach)하는 순서대로 파일 내 핸들러 정의 순서를 맞추는 것이 좋겠다.

#12 snackbarShowTime과 같이 함수 실행 시 마다 선언해야하는 변수가 아니라면 상수로 빼는게 좋겠다.

#13 클래스 내부에서만 쓰이는 메서드와 외부에 노출될 메서드를 정렬 순서 등으로 구분해주는 것이 좋다.

#15 util 또는 library 내 존재하는 함수에는 특정 도메인, 특정 상황에만 적용될 수 있는 하드코딩은 없어야 한다. 필요한 것은 외부에서 파라미터를 주입해서 내부의 video 와 같은 이름들을 제거하는 방법으로 조정할 수 있다.

#19 공통으로 쓰이는 함수 혹은 추출된 함수의 경우, 파라미터에 대한 예외처리를 해주면 좋다.

#23 지금은 클래스로 만들어도 View, Controller 가 재사용되지 않고 한 객체만 사용하게 되니 함수로 만들어도 되겠다 싶겠지만 서비스가 커지고 여러 객체가 필요할 때는 클래스의 필요성이 느껴질 것이다. 지금 정도의 서비스에서는 페어와 상의하고 결정하면 될 부분이다.

#28 단축평가 대신에 옵셔널 체이닝을 사용하면 더 간결하게 작성할 수 있다.

// 단축평가
action[type] && action[type](target);

// 옵셔널 체이닝
action[type]?.(target);

// 또 다른 예시
const user = { name: "365kim" }
user?.name

// 구형 브라우저를 위한 트랜스파일링
user === null || user === void 0 ? void 0 : user.name;

#33 async-await 문법과 promise를 섞어서 쓰기보다는 한가지만 쓰는게 더 좋겠다.

#36 요구사항이 볼 영상, 본 영상처럼 2개일 경우, 개발할 때도 단 2개의 상태를 갖는 것처럼 코드를 작성해버리는 경우가 있다. 대부분의 입문자들이 볼 영상은 true, 본 영상 false 과 같이 작성한다. 이렇게 되면 나중에 '추천 영상', '인기 영상' 같은 선택지가 하나 더 생길 경우 이 불리언 타입으로 전개되었던 모든 코드를 수정해야하는 상황이 생긴다. WATCHING_TYPE, WATCHED_TYPE처럼 열거형으로 작성하더라도 삼항연산자를 써버리면 결국 true/false로 한 것과 다르지 않다. 열거형으로 작성하되 삼항연산자를 함께 사용하지 않는 것이 확장성에 유리하다.

 

컨벤션/폴더링

#01 네이밍에서 고유명사는 모두 대문자로 쓴다. (ex. ISO, HTML, JWT, JSON)

#05 명시적으로 표현하는 것도 좋지만 코드가 verbose해지는 것도 경계해야 한다. 따라서 생략하려는 default 값이 암묵지(tacit knowledge)이라면 드러내야 하고, 모두가 인지하고 있는 표준 스펙이라면 생략해도 좋다.

#15 의도를 전달하는 이름이 좋은 이름이다. 구현을 전달하는 이름은 좋지 않은 이름이다.

#16 아직 완성된 major 버전이 아니라면 package.json에서 "version"을 "1.0.0"이 아닌 "0.0.1"으로 설정해도 좋겠다.

#16 파일을 너무 잘게 쪼개면 나중에 프로젝트의 규모가 커졌을 때 관리하기 더 어려워질 수 있다.

#22 파일 내부에서 데이터를 저장하고 가져오기 위해 localStorage를 사용하고 있다고 해서 src/storage 밑에 위치해 있어야 하는 파일로 보기는 어렵다. 이는 제이쿼리를 사용하는 파일이 jQuery 폴더에 들어있는 셈이다. 파일의 '역할'과 '책임'을 기준으로 어떤 공통점이 있는지를 기준으로 적절히 추상화하여 폴더를 구성하는 것이 좋겠다.

#22 네이티브 혹은 특정 라이브러리 API를 여기저기서 접근하면, 해당 API가 변경되거나 더 좋은 라이브러리를 도입할 경우 모든 코드를 다 고쳐야 한다. 반대로, API를 랩핑해서 사용하는 포인트를 한 군데로 몰아놓으면 해당 파일의 내부의 구현만 바꾸면 되므로 관리가 쉬워진다.

#24 RegExp도 constant로 분리해서 관리하자.

#24 package.json의 "version"은 아직 메이저 버전이 아니니까 "0.0.1"과 같이 적는 것도 좋겠다. (major.minor.patch)

#26 CSS에서 '!important'는 특수한 상황에서만 쓰인다. 필요해서 사용했다면 주석이나 PR에 왜 사용했는지 설명이 필요하다.

 

DOM

#01 필수로 작성해야할 input이라면 required 속성을 추가하자.

#01 id, class, dataset 셀렉팅 성능 차이는 다음과 같이 실험해 볼 수 있다.

const $body = document.querySelector('body');
$body.innerHTML += '<div id="div" class="div" data-cy="div">test</div>';

console.time('id test');
[...Array(10000000)].forEach(() => document.querySelector('#div'));
console.timeEnd('id test');

console.time('class test');
[...Array(10000000)].forEach(() => document.querySelector('.div'));
console.timeEnd('class test');

console.time('data test');
[...Array(10000000)].forEach(() => document.querySelector('[data-cy="div"]'));
console.timeEnd('data test');

#03 이벤트 콜백함수를 inline으로 넣어주는 방법은 콜백함수가 global scope에 존재해야해서 좋지 않다.

const test = () => alert('test')
$dom.innerHTML = `<div class="test2" onclick="test()">test</div>`

#03 innerHTML도 보안 및 성능 상의 이유로 현업에서는 자주 사용하지 않는다. Vanila JS로 작성한다면 코드가 길어져도 append와 함께 document.createElement를 사용하는것이 안전하다. 이러한 한계로 JSX라는 문법이 나오기도 했고, hyperscript, htm 와 같은 헬퍼함수도 있다.

#07 <img>태그를 넣을 때는 alt 속성을 습관적으로 같이 넣자. 이미지를 직접 볼 수 없고 스크린리더 등 보조도구를 사용하는 사용자들은 alt 속성으로 해당 태그를 파악하게 된다.

#07 날짜포맷 변환은 다음과 같이 할 수 있다.

const date = new Date(date);
const options = { year: 'numeric', month: 'long', day: 'numeric' };
const formattedDate = date.toLocaleDateString('ko-KR', options);
// 2021년 3월 8일

#10 <a>태그에 target="_blank" 속성으로 새 창을 띄운다면 Tabnabbing 피싱 공격을 방지하기 위해 noopenner, noreferrer 옵션을 사용하자. noopenner는 새로 열린 페이지에서 window.opener에 접근을 불가능하게 하고, noopener를 지원하지 않는 브라우저가 있으므로 noreferrer로 window.opener의 제어를 불가능하게 할 수 있다. nofollow는 링크 주스(Link Juice)를 전달하지 않겠다는 표시이다. (참고자료)

#17 data-* 속성과 같이 상태를 DOM 에 저장하는 것은 조금은 올드한 방식이다.

#18 메뉴 선택 <div>태그는 <nav>태그가 더 적절하다.

#18 모달에는 다음과 같은 aria-label을 넣을 수 있다.

<div class="js-search-video-modal modal" role="dialog" aria-labelledby="search-modal-title">

#18 IE7에서 default type을 button을 갖기 때문에 button의 타입은 type="submit"로 항상 명시하는 것이 좋다.

#20 <iframe>에 보안문제가 있는 것은 맞지만, 항상 지양해야하는건 아니다. 웹사이트의 채팅상담 기능 등 써드파티 기능들을 iframe으로 구현할 수 있다. 유튜브 영상 임베딩도 iframe 이다. 보안 문제를 있다는 것을 인지하고, 문제들이 발생하지 않도록 정확히 알고 사용하는 것이 중요하다.

#20 대부분의 경우 CSS만 조작하는 것이 JS로 상태를 관리하고 DOM을 직접 조작하는 것보다 성능이 우수하다. CSS 만으로도 충분히 구현할 수 있는 UI 라면 JS 사용을 최대한 지양하자.

#20 변경되는 데이터 없이 정적인 템플릿은 HTML에 미리 넣어놓고 필요한 경우 class로만 토글하는 것이 좋다.

#20 미리 select 해놓는 DOM 요소들은 이후 동적으로 추가 또는 삭제되지 않는다는 보장이 있어야 한다.

#26 appendChild 대신 append를 사용하면 반복문을 쓰지 않고도 여러 개의 노드를 한번에 붙일 수 있다.

#26 'Not Found' 이미지를 보여주는 경우, DOM에 미리 렌더링 해놓고 class로만 토글하는게 더 효과적이다.

#31 closest의 사용은 HTML구조 변경에 영향을 받는다. closest를 쓰게 된다면 상위 요소에서 event delegation을 시도해보자.

#32 JS 스크립트를 보통 <body> 태그 끝에서 부르는 이유는 DOM Element가 그려지기 전에 JavaScript 에서 DOM에 접근했을 때에 나는 오류를 방지하기 위해서이다. 자바스크립트를 파싱하고 실행하는 동안 다른 작업들이 블로킹상태가 된다. (+ type="module"은 다름!)

#32 트랜지션, 애니메이션 효과를 JS의 setTimeout 등으로 구현하는 것보다 CSS로 작성하는 것이 성능에도 가독성에도 다 좋다.

 

API / fetch

#01 API_KEY는 보통 .env를 통해 정의하거나 secret value로 따로 관리한다. github에 push 하지 않는다.

#03 URLSearchParams 브라우저 API로 쿼리스트링을 만들어줄 수 있다.

#07 callback hell을 해결하기 위해 Promise가, Promise를 더 편하게 쓰기 위해 async - await가 생겼다. async - await 로 작성해보자.

#08 try-catch 문을 이용한 API 에러처리를 추가하자.

#16 비동기 처리 구문은 프로젝트 전반에서 일관적으로 사용하는 것이 좋다. 코드 전반에 async await를 사용하고 있다면 then catch 구문을 추가하기 보다는 async await 구문을 사용하는게 좋아보인다. async await는 ES6 스펙이기도 하다.

try {
   return (
     await fetch(`${YOUTUBE_BASE_URL}/search?${query}`);
   ).json()
} catch (e) {
   console.error(e)
   return e
}

#20 fetch API method의 기본값 'GET'은 명시해줄 필요가 없다. 이를 명시해주는 것은 HTML 문서의 기본스타일을 모르는 사람을 위해 font-size: 16px; background-color: rgba(0,0,0,0);을 명시해주는 것과 비슷하다. fetch는 굉장히 자주 쓰이는 browser 내장 API이므로 기본값은 생략하는 것이 더 맞는 작성법이다.

#36 쿼리스트링이 길어지면 가독성도 떨어지고 실수가 발생할 여지가 커진다. 객체를 쿼리스트링으로 변환하는 유틸 함수를 만들어쓰거나 실무에서는 qs와 같은 라이브러리를 보통 사용한다.

const query = {
  part: 'snippet',
  order: 'viewCont',
  ....
}
const qs = toQueryString(query)

 

로컬스토리지 / DB

#01 localStorage의 키값들을 상수로 관리하자.

#01 localStorage에서 가져온 값이 JSON format이 아닐 경우를 대비해 가드문을 추가하자.

#04 localStroage에 저장하려는 값이 직렬화할 수 없는 경우(undefined, function, Symbol, cyclic object)를 대비해서 가드문을 추가하자.

#03 LocalStorage라는 브라우저의 API를 사용하는 util 라이브러리는 util함수로 빼자.

#19 로컬 스토리지를 읽을때 에러가 난다면, 에러는 나되 사용자는 계속 사용할 수 있어야 한다.

#26 캐싱이 필요한지, 어떤 주기로 캐싱할지는 데이터의 특성마다 다르게 설정해야 한다.

#28 앱의 메모리와 로컬스토리지가 데이터를 각각 들고 있다면, 관리포인트가 늘어나고 두 데이터의 정합성을 관리해야한다. (Single Source of truth) 따라서, 성능적으로 얻는 이득보다 크지 않다면 localStorage에 한 군데에 저장하는 것이 낫겠다.

 

미션 로직 / UX

#01 이미 검색이 된 상태에서 새로 검색할 때, 기존 검색 결과를 없애고 스켈레톤을 띄우자.

#02 빈 문자열로 검색할 경우에 대해 validate check를 하자.

#02 모달을 띄울 때 document는 full height로 전체를 덮어서 스크롤을 없애고, 모달에만 스크롤을 주자.

#03 비활성화된 버튼은 hover효과를 없애고, cursor: pointer가 아니라 cursor: not-allowed 를 주자.

#03 모달에서 영상 재생 후에 팝업을 닫으면 백그라운드에서 소리가 계속 재생되는 문제가 있다.

#04 skeleton표시는 짧을 수록 좋다. iframe이 로드될 때까지 기다리지 않고 thumbnail을 먼저 넣어주자.

#12 최근검색어 사이에 적절한 마진을 넣어주자.

#12 동영상가 저장되면 스낵바 등으로 사용자에게 영상이 정상적으로 저장되었음을 알려주자.

#12 영상로드 요청 시 에러가 나면 skeleton UI를 화면에서 없애고 에러가 발생했음을 사용자에게 알려주자.

#16 검색 후 실수로 저장하는 경우를 대비해서 저장을 취소하는 기능이 있으면 좋겠다.

 

테스트

#03 cypress의 intercept() API를 사용하면 cy.wait()없이도 비동기함수를 테스트할 수 있다.

#24 UI가 없어진 것에 대한 테스트라면 'UI가 사라진다' 라는 표현보다 'UI가 제거된다'는 표현이 더 명확하게 들린다.

 

 

그 외 키워드: (#03)cyclic object value, (#22) 상속과 합성, (#24) N계층 아키텍쳐 스타일 (#28) Single Source of truth