프로젝트 전반 코드베이스를 리뷰해요 #4
Conversation
perceptron 초기화 시 스크롤 상태가 반영되지 않던 문제 해결
| "compilerOptions": { | ||
| "target": "ES2020", | ||
| "module": "ES2022", | ||
| "moduleResolution": "Node", |
There was a problem hiding this comment.
moduleResolution을 Node로 설정해준 이유가 있나요?
| <meta http-equiv="X-UA-Compatible" content="ie=edge"> | ||
| <title>Neural Network Visualization</title> | ||
| <script type="module" src="src/main.ts"></script> | ||
| <script src="modernizr-custom.js"></script> |
There was a problem hiding this comment.
이런 외부 코드들은 public 폴더에 넣어주세요.
배포 시점에 함께 번들링 되지 않아요.
실제로 배포 사이트 들어가면 404가 뜨고 있습니다.
| <canvas id="perceptron"></canvas> | ||
| </div> | ||
| <button id="clear"> | ||
| <img src="assets/eraser.png" alt="eraser_icon"> |
There was a problem hiding this comment.
프로젝트에 assets/, public/assets/ 이 중복되어있어요. 최대한 public 하에서 관리하도록 변경해주세요.
| @@ -0,0 +1,35 @@ | |||
| class CanvasComponentBase { | |||
| const edgeRenderer = new EdgeRenderer(perceptronBaseCanvas.getCtx()); | ||
| const edgeHandler = new EdgeHandler({ edgeRenderer, perceptronBaseCanvas }); | ||
| const gridRenderer = new GridRenderer(perceptronBaseCanvas.getCtx()); | ||
| const gridHandler = new GridHandler({ gridRenderer, perceptronBaseCanvas }); | ||
| const $PC = new PerceptronController({ | ||
| NodeRenderer: nodeRenderer, | ||
| EdgeRenderer: edgeRenderer, | ||
| GridRenderer: gridRenderer, | ||
| NodeHandler: nodeHandler, | ||
| EdgeHandler: edgeHandler, | ||
| GridHandler: gridHandler, | ||
| BaseCanvas: perceptronBaseCanvas, | ||
| ScrollEventHandler: new ScrollEventHandler(perceptronBaseCanvas), | ||
| }); |
There was a problem hiding this comment.
프로젝트 전반적으로 추상화 레벨이 굉장히 낮은 부분들이 보여요.
ex) AppController가 EdgeRender/Handler, GridRender/Handler 등등의 정보를 알 필요가 있나요?
굳이 의존성을 외부에서 관리/주입할게 아니라면 이런 부분들은
도메인에 맞는 맥락에 격리 및 추상화시키는 편이 좋다고 생각해요.
| import { value } from '@/view/components/perceptron/types/Node.ts'; | ||
|
|
||
| export class Node { | ||
| private value: Number; | ||
| constructor(value: number) { | ||
| this.value = value; | ||
| } | ||
| setValue(value: number): void { | ||
| this.value = value; | ||
| } | ||
| getValue(): number { | ||
| return this.value; | ||
| } | ||
| } |
There was a problem hiding this comment.
프로젝트 전반적으로 이런 모델들을 뷰 계층에 넣는 대신,
더 상위 레이어로 빼서 더 많은 일을 위임시키고, 모델을 받아서 처리하는 로직들을 구성하면 더 맥락이 간단해질 거 같네요.
ex) nodeState 인터페이스를 굳이 하나 더 만들 필요 없이 Node 클래스에서 state까지 관리해주면 좋지 않을까요?
| @@ -0,0 +1,19 @@ | |||
| export interface IinitialCoords { | |||
There was a problem hiding this comment.
IInitialCoords
요런 오타들 신경써주면 더 좋을 것 같습니다!
참고) eslint 규칙을 빡세게 잡으면 이런 식별자 오타들도 잡을 수 있어요.
| maxY: number; | ||
| } | ||
|
|
||
| export interface originalObject { |
There was a problem hiding this comment.
타입 명명 규칙 컨벤션을 통일해주세요.
어떤데는 시작이 소문자고, 어떤데는 시작이 대문자고, 어떤데는 시작이 대문자 I고,
이런 것들은 개발에 많은 혼란을 줄 수 있는 부분이기 때문에 통일해주는게 좋습니다.
| } | ||
|
|
||
| clear(): void { | ||
| this.canvas.width = this.canvas.height = 1; |
There was a problem hiding this comment.
협업 관점에서 이런 것들은 왜 1로 초기화하는지 알려주면 더 좋아요.
협업자가 이 코드를 봤을 때 생각할 맥락을 줄여줄 수 있어요.
방법들
- 이런 류의 magic number를 변수로 만들고, 변수명을 맥락을 포함하여 짓는다.
- 주석을 단다.
| on<K extends keyof eventPayloads>( | ||
| eventName: K, | ||
| subscriber: (payload: eventPayloads[K]) => any, | ||
| ): void { | ||
| if (!this.events[eventName]) this.events[eventName] = []; | ||
| this.events[eventName].push(subscriber); | ||
| }, |
There was a problem hiding this comment.
의도치 않게 같은 subscriber를 중복해서 구독할 수 있는 경우가 생길듯해요.
| async getWeights(): Promise<weights> { | ||
| if (this._cache) return this._cache; | ||
| try { | ||
| const json = await loadPretrainedWeights(this.path); |
There was a problem hiding this comment.
pretrainedWeights 파일 크기가 굉장히 큰데,
동기적으로 불러오면 앱을 렌더링하는데 지연이 생길 수 있어요.
실제로 AppController 상에서 getWeights가 트리거되는 시점이 빠른편이라
실제 렌더링이 돔 로딩 시점 대비 많이 지연되더라구요.
큰 문제는 없겠지만, 어떻게 개선할 수 있을지 고민해보면 좋을 거 같아요.
만약 제가 이 문제를 해결해야했다면
- 우선 다른 콘텐츠부터 렌더링 시키고, 로딩 UI 같은거 띄우고
- 동시에 pretrainedWeights를 백그라운드로 불러온 다음(non-awaited promise)
- resolved 되었다면 이벤트 버스를 통해 view 레이어에서 렌더링하도록 구성할듯하네요.
| const App = new AppController(); | ||
| await App.initialize(); | ||
| console.log('App initialized!'); |
There was a problem hiding this comment.
| const App = new AppController(); | |
| await App.initialize(); | |
| console.log('App initialized!'); | |
| window.onload = () => { | |
| const App = new AppController(); | |
| await App.initialize(); | |
| console.log('App initialized!'); | |
| } |
| content="width=device-width, user-scalable=no, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0"> | ||
| <meta http-equiv="X-UA-Compatible" content="ie=edge"> | ||
| <title>Neural Network Visualization</title> | ||
| <script type="module" src="src/main.ts"></script> |
There was a problem hiding this comment.
-
script 태그의 위치에 따라 브라우저가 어느 시점에 파싱하는지 알아보면 좋을 거 같아요.
-> 왜 대부분의 script는 body 태그 하단에 선언하는 게 이상적일까요? -
async, defer attribute에 대해 알아보면 좋아요.
아래 의도를 가지고 리뷰를 진행했어요.
생각보다 코드 베이스가 크기도하고 내부 모듈끼리 엮여있는 부분이 많더라고요.
최대한 리뷰의 남겨보려고 했지만 남지기 못한 부분들이 있어요.
이런 류의 리뷰들은 대부분 파일 구조적 아키텍처적 리뷰들이고,
우선적으로 처리해야할 아키텍처 리뷰들을 처리하고, 더 나은 구조를 고민하다보면 자연스럽게 해결되는 부분들이 많아서 생략했어요.