코드 리뷰 (1)

[cadenzah]

Branch Name

review/27-code-review

Type of Change

review

Priority

high

Summary

서교필 매니저님의 코드 리뷰 내용 반영

Content of Review

lib/index.tsx

기존 라이브러리 매핑 관련: namespace 모듈 패턴을 이용해 정의한 인터페이스(namespace Player) 중 기존 video.js 라이브러리 인터페이스를 완전히 매핑하지 못하는 형태로 제한된 점이 의도가 있는 것인지. 예를 들어 VideoJsPlayerOptions 인터페이스를 매핑하여 타입으로 정의한 IPlayerOptions 인터페이스의 경우 VideoJsPlayerOptions에서 선택적으로 적용가능한 값을 제한적으로 담아내고 있는 점.(videojs를 편리하게 사용하게 하기 위한 의도라는 점에서 모든 옵션을 지원하도록 매핑하고 bypass할 부분과 custom하여 편의를 도모할 부분을 고민하여 제공하면 좋을 것 같습니다.)

lib/utils/initiallizeEventListener.ts

오탈자 관련(line36): player.on 메서드 오탈자(player.one)

lib/utils/initializePlayer.ts

• 기존 라이브러리 매핑 관련: lib/index.tsx에서 언급한 내용과 같은 맥락입니다. initialPlayer 함수의 첫번째 매개변수의 타입으로 HTMLVideoElement | null 유니온으로 정의하였는데, video.js 라이브러리에 정의 상에는 <audio />, <video-js>, element 노드의 id 값 등도 포함할 수 있는 것 같습니다. 이후에 확장할 수 있으면 좋을 것 같습니다.
• 함수 매개변수 타입 안정성 관련: initialPlayer 함수의 두번째 매개변수의 타입이 videojs.PlayerOptions 인데, lib/index.tsx에서 정의한 namespace Player의 IPlayerOptions 인터페이스로 정의하는 것이 안전하지 않을 까 하는 생각을 했습니다.

lib/utils/filterPlugins.ts

• 반복문 관련(line: 22): Array.map 함수는 배열의 모든 요소에 대하여 전달 인자로 주어진 함수로 새로운 배열을 반환합니다. 모든 요소에 접근하여 조작할 수 있다는 점에서 반복문 대신 사용할 수 있지만, 각 요소를 조작해 새로운 배열을 반환한다는 용도로 사용하는 것이 좋을 것 같습니다. 대신 forEach 문이나 for 문을 사용하는 것이 어떨까 싶습니다.
• 간결한 코드를 위해서(line23 ~ 27): if-else 문을 사용하는 대신 하나의 if 문을 이용하여 좀 더 클린한 코드를 작성할 수 있을 것 같습니다.

if (element.plugin !== undefined) {
    manualPlugins.push(element);
    //forEach문의 return false 혹은 for문의 continue 등을 이용해 다음 반복이 시작되도록 합니다.
}
autoPlugins[element.name] = element.options;

Estimated Story Point

5

Reference (If exists)

작업 수행시 참고한 자료, 외부 링크 등을 기록해주세요.