[최창환] sprint6 #181
[최창환] sprint6 #181
Conversation
addiescode-sj
left a comment
addiescode-sj
left a comment
There was a problem hiding this comment.
수고하셨습니다!
저번보다 코드 퀄리티가 훨씬 좋아지셨네요 ㅎㅎㅎ
이번엔 useEffect 훅을 사용할때 잘못 들이면 안좋은 습관에 대해 피드백 드려봤는데,
제일 우선적인 개선사항이니 한번 리팩토링 시도해보세요! :)
주요 리뷰 포인트
- useEffect 훅의 사용 용도를 정확히 알기
- 단일 책임 원칙에 의거해 페이지네이션 로직 분리하기
- state 객체 형태로 사용하지않기
| const INITIAL_VALUES = { | ||
| name: '', | ||
| description: '', | ||
| price: '', | ||
| tags:[], | ||
| tagInput:'', | ||
| images: null, | ||
| }; | ||
|
|
||
|
|
||
| function AddItem() { | ||
| const [values, setValues] = useState(INITIAL_VALUES) |
There was a problem hiding this comment.
상태를 객체로 관리하게되면 값 하나만 바뀌어도 전체 객체가 새로운 참조로 변경되기때문에 불필요한 리렌더링이 발생해요. 개별적으로 state를 만들어서 관리하시는게 좋습니다 :)
| import { getProducts } from "../../api/ProductApi.jsx"; | ||
| import { useScreenSize } from "../../utils/useScreenSize.jsx"; | ||
| import { useState, useEffect, useCallback } from "react"; | ||
| import { debounce } from "lodash"; |
There was a problem hiding this comment.
디바운스의 경우 로대시를 쓰는것보다 직접 구현하시는게 좋을거예요.
패키지 사이즈가 워낙 크기도하고, 정말 필요한 몇개 유틸성 기능을 제외하고는 성능상 큰 차이가 나지 않습니다.
만약 로대시를 계속 사용하실거라면 lodash-es를 사용하시는게 트리 셰이킹이 제공되어 좀더 번들사이즈가 작아질거예요!
| const PAGE_SIZES = { | ||
| lg: { best: 4, all: 10 }, | ||
| md: { best: 2, all: 6 }, | ||
| sm: { best: 1, all: 4 }, | ||
| }; |
There was a problem hiding this comment.
이런 상수들이 프로그램 내에서 여기저기 재사용된다면 따로 src > constants 폴더 만들어서 공용으로 빼두는게 좋겠죠? :)
| useEffect(() => { | ||
| const { best, all } = PAGE_SIZES[screenSize]; | ||
| setBestProductCount(best); | ||
| setAllProductCount(all); | ||
| }, [screenSize]); |
There was a problem hiding this comment.
useEffect를 state setter 용도로 사용하지 않으시는게 좋아요.
그 이유는 useEffect를 사용하면 컴포넌트가 먼저 렌더링되고, 그 다음에 useEffect가 실행되어 상태를 업데이트하고, 또다시 렌더링이 발생하기때문이예요.
불필요한 렌더링 사이클을 만들어 이런 패턴이 누적되면 결과적으로 전체적인 성능에 영향을 줄 수 있습니다.
There was a problem hiding this comment.
useEffect는 일반적으로, 외부 시스템과 컴포넌트를 동기화하는 목적으로만 사용하시는것이 좋아요 (API 호출, 이벤트 리스너 등록/해제, DOM 조작 등)
useEffect를 state setter 용도로 사용하지않고 이렇게 바꿔보면 어떨까요?
// 1. 상수로 분리
const getPageSizes = (size: ScreenSize) => {
const { best, all } = PAGE_SIZES[size];
return { best, all };
};
// 2. 컴포넌트 내에서 직접 사용
const { best, all } = getPageSizes(screenSize);
setBestProductCount(best);
setAllProductCount(all);| const { all } = PAGE_SIZES[screenSize]; | ||
| if (all !== allProductCount) { | ||
| const firstItemOfCurrentPage = Math.max( | ||
| 0, | ||
| (currPage - 1) * allProductCount | ||
| ); | ||
| const newPage = Math.max( | ||
| 1, | ||
| Math.floor(firstItemOfCurrentPage / all) + 1 | ||
| ); | ||
| const totalPages = Math.ceil(totalCount / all); | ||
| const validPage = Math.min(newPage, totalPages || 1); | ||
|
|
||
| setCurrPage(validPage); | ||
| setAllProductCount(all); | ||
| } else if (currPage > Math.ceil(totalCount / allProductCount)) { | ||
| setCurrPage(1); | ||
| } |
There was a problem hiding this comment.
이런 페이지네이셔 관련 로직이 Items 페이지에 있게되면 재사용성도 떨어지고 단일 책임 원칙을 지키지 못하게되니까 재사용을 위해 커스텀 훅으로 분리해주시는게 좋을것같아요!
| return ( | ||
| <> | ||
| <Nav /> | ||
| <Content> | ||
| <Header type='h1' text={"베스트 상품"} /> | ||
| <ProductList | ||
| orderBy={"favorite"} | ||
| pageSize={bestProductCount} | ||
| type='large' | ||
| /> | ||
| <div className={styles.headers}> | ||
| <Header type='h1' text={"전체 상품"} /> | ||
| <SearchItem value={search} onChange={handleSearch} /> | ||
| <Button href={"additem"} buttonText={"상품등록하기"} /> | ||
| <DropDown onChangeOrder={handleOrder} /> | ||
| </div> | ||
| <ProductList | ||
| orderBy={order} | ||
| pageSize={allProductCount} | ||
| keyword={search} | ||
| page={currPage} | ||
| type='small' | ||
| /> | ||
| <Pagination | ||
| currPage={currPage} | ||
| totalProducts={totalProducts} | ||
| pageSize={allProductCount} | ||
| handlePage={handlePage} | ||
| /> | ||
| </Content> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
굳굳 👍 리턴문은 이래야죠! 깔끔하네요 ㅎㅎ
제가 이전에 코드 리뷰 채널에서 공유드린 |
2 participants
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게