프로젝트 리뷰

[danbileee]

안녕하세요 팀원분들, 프로젝트 완성하시느라 정말 수고 많으셨습니다.
늦게나마 리뷰하고 내용 정리하여 전달드립니다.
부득이하게 이슈 탭을 사용하게 되어 양해 부탁드리고, 파일 또는 라인별로 매칭하여 확인하실 수 있는 부분들은 명시해두었으니 참고해주시면 감사하겠습니다!
제가 미처 리뷰하지 못한 부분들이 있거나 추가로 확인이 필요한 내용들은 코멘트로 편하게 말씀 부탁드립니다!

관심사 분리

스타일

스타일 적용을 위해 className과 inline style이 함께 사용되고 있는데요, 한 방향으로 통일이 되면 스타일 관리가 보다 용이할 것 같습니다. 그중에서도 관심사 분리 측면에서 보면 className 활용이 보다 내용과 스타일 코드를 분리할 수 있고, 스타일을 모듈화할 수 있다는 점에서 나을 것 같습니다. 이메일 환경같은 경우처럼 className이 지원되지 않는 등의 명확한 이유가 있다면 inline style을 사용하셔도 문제는 없습니다.

prop과 state

prop으로 받는 것과 state로 관리해야할 것의 기준이 명확해지면 좋을 것 같습니다.

• 각 라우트 엔트리 컴포넌트에서 prop으로 전달받은 데이터를 보정하여 setPost하는 로직같은 경우, 부모에서 전달할 때 보정하여 전달함으로써 불필요한 useState, useEffect 구문을 없앨 수 있습니다.
  • 보정하는 로직을 컴포넌트 내부에 유지하고 싶다고 해도 한번 필터링한 후 값이 업데이트될 필요가 없는 데이터이므로 useState, useEffect 사용 없이 filter로직만 작성해주셔도 목적에 충실한 구현이 될 것 같습니다.
  • Main.jsx, DetailFestival.jsx 경우처럼 보정이 필요하지 않은 경우에는 더더욱 useState, useEffect 구문 없이 전달받은 prop만 필요한 로직에 사용하면 됩니다.
• App.jsx의 category state 또한 Festival.jsx에서만 사용되고 있는데 굳이 prop으로 전달할 필요 없이 Festival.jsx내부의 state로 관리해도 무방해 보입니다.

인라인 함수 분리

이벤트 핸들러같은 경우 인라인 함수를 컴포넌트 내 별도로 분리하셔서 jsx 코드가 비대해지는 것을 막을 수 있습니다. 보통 이런 경우 handle- 프리픽스를 붙이는데 잘 해주신 곳도 있는 반면, 인라인 함수로 되어 있는 곳들도 있어 개선되면 좋을 듯합니다.

• 인라인 삼항연산자로 서로 다른 함수를 적용하거나 null처리하는 경우도 마찬가지로 분리된 함수 내에서 조건 처리를 할 수 있습니다. (예: Running.jsx:280)
• 만약 리렌더링에 의해 함수가 재생성되는 것을 막고자 한다면 useCallback으로 감싸셔도 됩니다. 모든 경우에 필요한 것은 아니며 비용을 따져서 필요한 곳에 제한적으로 사용하시면 되는데, 공통 모듈로서 여러곳에 재사용되는 함수인 경우에는 가급적 적용되면 좋습니다.

컴포넌트 분리

하나의 컴포넌트 내에서 많은 역할들을 담당하고 있는 경우, 역할에 따라 적절히 분리해주면 좋습니다. 내용으로 봤을 때 서로 분리되어도 상관 없거나, 배열을 순환해서 표시하는 목록 성격의 컴포넌트의 경우 개별 item들을 별도 컴포넌트로 분리할 수 있습니다. 이것도 잘 해주신 컴포넌트가 있는 반면, 개선이 필요한 컴포넌트도 있어 보였습니다.

• DetailFestival.jsx의 180라인 이하로 post 관련된 코드는 분리될 필요가 있어 보입니다.
  • 관련된 조건문의 경우 삼항 연산자 + if문이 섞여 복잡도가 높아 보입니다. if문은 DetailFestival 이 꼭 알아야 하는 로직은 아니므로 분리된 컴포넌트 내로 감출 수 있습니다.
  • 또한 false케이스에 null을 반환해야 한다면 삼항연산자 대신 && 연산자로 불필요한 null처리를 제외할 수 있습니다. 다만 배열의 length의 경우 0이 falsy case인데 0 && (...) 처리되어서 결과적으로 0이 화면에 렌더링되는 이슈가 있어, Boolean(post.length) && (...) 로 타입변환이 필요합니다.

html

시멘틱 태그

대체적으로 잘 작성해 주셨는데, 시멘틱을 적용할 수 있는데도 div가 사용된 경우가 종종 보여서 적어두었습니다.

• 레이아웃별 해당되는 태그를 활용해 보세요. (main, article, aside, section...)
• 목록이 나열되는 곳이라면 ul, li를 활용해 보세요.

img 최적화

브라우저가 보다 최적화된 이미지 렌더링을 진행할 수 있도록 img태그 사용시에 몇가지 보완되면 좋을 부분들이 있습니다.

• width, height을 명시해 주세요.
• 초기 페이지 랜딩시 뷰포트 내에 들어오는 엘리먼트가 아니라면 loading="lazy" 속성을 적용해 보세요.
• 이미지가 로드되지 않았을 경우 또는 스크린 리더 사용자를 위하여 alt 속성은 꼭 명시해 주세요.

가독성 개선

린트/포맷터 도입

협업을 통해 구현되는 프로젝트이다 보니, 린트나 포맷터를 도입하여 코드 컨벤션을 맞추면 보다 가독성에 도움이 될 것 같습니다. eslint, prettier 등 설정을 추가해 보시면 어떨까요? 현재는 indent, line break 등 사소한 부분부터 파일별로 다른 것이 보입니다.

네이밍 컨벤션

보다 컨벤션에 맞춘 네이밍을 통해서 해당 코드가 어떤 역할을 하는지 짐작하는 시간을 아낄 수 있습니다.

• 함수는 선언형으로 작성해 주세요. 어떤 동작을 수행하는지를 동사 prefix로 명시해 주세요.
  • handle-, change-, process-, create-, update-, render-...
• boolean 변수임을 알 수 있는 몇가지 자주 쓰이는 prefix를 팀내 use case에 맞춰 통일해주세요.
  • is-, has-, should-, need- ...
• case 통일해주세요.
  • 컴포넌트: PascalCase
  • 변수, 함수명: camelCase
  • 파일명: camelCase, snake_case, kebab-case...

용도에 맞는 변수 선언

• 재할당이 이루어지지 않는 변수라면 let 대신 const 를 사용해 주세요.
  • 현재 프로젝트에서 쓰인 let은 대부분 const로 사용해야 하는 경우로 보입니다.
• 만약 컴포넌트 라이프 사이클에 영향받지 않는 상수라면 컴포넌트 외부에 선언해주세요.
  • 상수의 경우 대문자 snake case를 사용하는 일반적인 컨벤션이 있습니다. (예: UPPER_CASE)

css 관리

• css에서 사용되는 속성들을 유형별로 순서를 통일하면 좋습니다. 유형을 나눌 때 참고할 수 있는 링크 몇가지 공유드립니다.
  • https://9elements.com/css-rule-order/
  • https://css-tricks.com/poll-results-how-do-you-order-your-css-properties/
• css 모듈, BEM, saas
  • 세 가지 키워드에 대해서 검토해보시고, 필요한 경우에 도입해 보시면 더욱 css 관리가 편리해질 것 같습니다.

성능 개선

많은 양의 데이터를 순환하여 특정 형식으로 변환한 후 API를 요청해야하는 경우 이런 방법들을 고민해볼 수 있을 것 같습니다.

• 마이그레이션이 불가능하고 데이터 변환 작업이 불가피하다면 캐시를 활용하면 좋겠습니다. 직접 구현하는 경우 useMemo 등으로 memoization을 활용하거나, react-query등의 라이브러리를 통하여 지원되는 캐시 기능을 활용하실 수 있을 것 같습니다.
• 데이터 변환 작업을 서버사이드에서 하여 내려받을 수 있도록 SSR을 도입해 봐도 좋겠습니다. next.js 등 쉽게 SSR을 구현할 수 있는 프레임워크도 검토해보실 수 있을 것 같습니다.