[이태경] Sprint5 #180
[이태경] Sprint5 #180
Conversation
…ithub-actions [Fix] delete merged branch github action
addiescode-sj
left a comment
addiescode-sj
left a comment
There was a problem hiding this comment.
수고하셨습니다!
전체적으로 잘 하셨는데,
컴포넌트를 분리하는 기준, 메모이제이션을 적용하는 기준 관련해서 리팩토링 진행해보시면 좋을것같네요 :)
주요 리뷰 포인트
- 폴더 구조
- 컴포넌트 분리 피드백
- 메모이제이션 적용 피드백
- 상태 관리 관련 피드백
There was a problem hiding this comment.
src 바로 밑에 있는 components 폴더는 공용 폴더로 쓰는게 일반적입니다.
AllProductArea 컴포넌트가 프로그램 내에서 여기저기 쓰이는게 아니라면 사용하는 입장에서 가까운 (찾기 편한) 위치에 두는게 좋겠죠?
src > pages > ItemsPage > components 경로안으로 파일 위치를 바꿔볼까요?
| const ITEM_COUNT = { | ||
| WEB: 10, | ||
| TABLET: 6, | ||
| MOBILE: 4, | ||
| }; | ||
|
|
||
| const INIT_PAGE_SIZE = getItemCount(ITEM_COUNT); |
There was a problem hiding this comment.
만약 이런 상수들도 여러 페이지에서 공유된다면 src > constants 폴더를 만들고 그 안에 위치하는 방법으로 관리하셔도 됩니다!
| const [pageSize, setPageSize] = useState(INIT_PAGE_SIZE); | ||
| const [productList, setProductList] = useState([]); | ||
| const [totalCount, setTotalCount] = useState(0); |
There was a problem hiding this comment.
재사용 가능한 로직이 있다면 -> 커스텀 훅으로, UI는 -> 컴포넌트로 중복을 제거해주실수있습니다. 또, 컴포넌트는 재사용 목적이 아니더라도 한 컴포넌트가 너무 많은 역할을 담당하고있을때, 컴포넌트 단위로 고유의 데이터 관리가 필요할때 등등 여러 상황에서 분리가 필요합니다.
AllProductsArea 컴포넌트는 data fetching의 결과로 전체 상품을 조회하고 렌더링하는 역할을 담당하도록 유지하는게 맞아요.
이중에 페이지네이션 로직이 해당 컴포넌트에 결합되어있으면 다른 페이지에서 페이지네이션이 필요할때마다 새로 코드를 작성해야할테니 코드 중복이 늘어나겠죠?
처음 작성할때부터 재사용 가능한 페이지네이션 로직은 커스텀훅으로 분리해보면 어떨까요? :)
| <> | ||
| <h2 className={styles.bestProductArea__title}>베스트 상품</h2> | ||
| <div className={styles.bestProductArea__content}> | ||
| <ProductList list={bestList.slice(0, pageSize)} type="best" /> |
There was a problem hiding this comment.
list prop은 페이지네이션 처리된 데이터를 조합해 넘긴다기보단 초기 데이터가 넘어가는 느낌이네요! 이 용도라면 initialData가 더 명확한 네이밍인것같아요.
하지만 이 방법은 지금으로써는 복잡도만 올라가고 불필요한 처리라고 생각됩니다.
원본 배열 그대로 넘기고 ProductList 자체에서 페이지네이션 처리를 하는 방법으로 개선해보면 어떨까요?
There was a problem hiding this comment.
해당 파일에서 useMemo, useCallback을 사용해 메모이제이션을 적용하셨는데 불필요한 최적화라고 생각됩니다.
일반적으로 memoization이 유용한 경우는 이런 경우들입니다:
- 자식 컴포넌트에 prop으로 전달되는 콜백 함수
- 의존성 배열에 포함되어 다른 훅에서 사용되는 함수
- 복잡한 로직이 포함되어 연산에 드는 비용이 비싼 함수
- 계산 비용이 큰 값
메모이제이션을 사용하면, 필연적으로 메모이제이션 처리를 위해 추가적인 비용이 발생합니다 (메모리 공간 할당, 메모이제이션 처리 연산 등).
따라서 성급한 최적화는 피해주시는것도 좋은 개발 습관입니다.
이 경우 단순한 연산만을 수행하고있기때문에 전혀 계산 비용이 크지 않은 상황이죠? :)
| const INIT_VALUE = { email: "", password: "" }; | ||
| const INIT_VALID = { | ||
| email: { | ||
| isValid: null, | ||
| msg: "", | ||
| }, | ||
| password: { | ||
| isValid: null, | ||
| msg: "", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
상태를 객체로 관리하게되면 값 하나만 바뀌어도 전체 객체가 새로운 참조로 변경되기때문에 불필요한 리렌더링이 발생해요. 개별적으로 state를 만들어서 관리하시는게 좋습니다 :)
| const [userValues, setUserValues] = useState(INIT_VALUE); | ||
| const [valueValids, setValueValids] = useState(INIT_VALID); |
There was a problem hiding this comment.
여기서도 개별적으로 상태관리할수있도록 바꿔주세요! :)
|
|
||
| // 비밀번호 확인 필드 함수 재정의 | ||
| // 렌더링마다 함수 재생성 방지를 위해 useCallback 사용 | ||
| const redefinePasswordConfirm = useCallback( |
There was a problem hiding this comment.
여기서도 useCallback과 useMemo를 쓰셨는데, 성급한 최적화는 아닐지 위 코멘트 보시면서 다시 한번 생각해볼까요? :)
| 이메일 | ||
| </label> | ||
| <div className="auth-form__input-box"> | ||
| <input |
There was a problem hiding this comment.
form과 input은 디자인시스템에도 있고 프로그램 내 여기저기서 쓰일텐데,
컴포넌트별로 코드를 중복해서 사용하는것보다 컴포넌트화해주면 좋겠죠? :)
| {!valueValids.email.isValid && ( | ||
| <p className="auth-form__error-msg">{valueValids.email.msg}</p> |
There was a problem hiding this comment.
리턴문은 항상 깔끔하게 유지하는 습관을 드려보시면 좋습니다 :)
const emailIsValid = !valueValids.email.isValid;
조건식을 상수로 정리해주면 복잡해보이는 리턴문 일부를 더 읽기 쉽고 이해하기 쉽게 바꿔줄수있겠죠? :)
4 participants
요구사항
기본
중고마켓
중고마켓 반응형
심화
주요 변경사항
스크린샷
멘토에게