🙌 페이지네이션 코드 리뷰 받은 부분 리팩토링
지난 번 페이지네이션을 구현한 부분에 대해 FE팀원 분께서 코드리뷰를 해주셨다.
12월은 여러 일정 때문에 바빠서 늦었지만 지금이라도 복습 겸 정리해보려 한다.
페이지네이션 기능 구현 부분에서 몇가지 🐛가 있었고, 언급해 주신 부분을 참고하여 리팩토링 하였다.
리팩토링 후에 FE팀원 분이 더 간결한 코드로 리팩토링 해주신 부분도 추가하였다.
지난 번에 구현한 페이지네이션 코드는 다음과 같다.
import React, { useMemo } from 'react';
import styles from './Pagenation.module.scss';
interface IPagenationProps {
maxPage: number;
currentPage: number;
setCurrentPage: React.Dispatch<React.SetStateAction<number>>;
visiblePageCount: number;
}
export const Pagenation: React.FC<IPagenationProps> = ({
maxPage,
currentPage,
setCurrentPage,
visiblePageCount,
}) => {
const beforePageBase = Math.floor(currentPage / visiblePageCount - 1) * visiblePageCount;
const currentPageBase = Math.floor(currentPage / visiblePageCount) * visiblePageCount;
const nextPageBase = Math.ceil(currentPage / visiblePageCount) * visiblePageCount;
const lastPageCount = maxPage % visiblePageCount;
const pageList = useMemo(() => { // 리팩토링 1
if (1 === currentPage || 1 > currentPage) {
return Array.from({ length: visiblePageCount }, (_, index) => index + 1);
}
if (0 === currentPage % visiblePageCount) {
return Array.from({ length: visiblePageCount }, (_, index) => beforePageBase + index + 1);
}
if (maxPage - visiblePageCount < currentPage) {
return Array.from({ length: lastPageCount }, (_, index) => currentPageBase + index + 1);
}
return Array.from({ length: visiblePageCount }, (_, index) => currentPageBase + index + 1);
}, [currentPage]);
return (
<div className={styles.flex}>
<button className={styles.button} onClick={() => setCurrentPage(1)}>
First
</button>
<button
className={styles.button}
onClick={() => { // 리팩토링 2
setCurrentPage((prev) => {
if (1 < prev) {
return beforePageBase + 1;
}
return prev;
});
}}>
◀
</button>
<ul className={styles.flex}>
{pageList.map((item) => {
return (
<li
key={item}
onClick={() => setCurrentPage(item)}
className={currentPage === item ? styles.accord : styles.disaccord}>
{item}
</li>
);
})}
</ul>
<button
className={styles.button}
onClick={() => // 리팩토링 3
setCurrentPage((prev) => {
if (maxPage - visiblePageCount > prev) {
return nextPageBase + 1;
}
return prev;
})
}>
▶
</button>
<button className={styles.button} onClick={() => setCurrentPage(maxPage)}>
Last
</button>
<span>{`${currentPage}/${maxPage}`}</span>
</div>
);
};
♻️ 코드리뷰 / 리팩토링
1. ❗PageList if문 조건 처리 순서 / 후의 조건이 반영되지 않아 버그 발생 확인 불가
기존코드
const beforePageBase = Math.floor(currentPage / visiblePageCount - 1) * visiblePageCount;
const currentPageBase = Math.floor(currentPage / visiblePageCount) * visiblePageCount;
const nextPageBase = Math.ceil(currentPage / visiblePageCount) * visiblePageCount;
const lastPageCount = maxPage % visiblePageCount;
const pageList = useMemo(() => { // 리팩토링 1
if (1 === currentPage || 1 > currentPage) {
return Array.from({ length: visiblePageCount }, (_, index) => index + 1);
}
if (0 === currentPage % visiblePageCount) {
return Array.from({ length: visiblePageCount }, (_, index) => beforePageBase + index + 1);
}
if (maxPage - visiblePageCount < currentPage) {
return Array.from({ length: lastPageCount }, (_, index) => currentPageBase + index + 1);
}
return Array.from({ length: visiblePageCount }, (_, index) => currentPageBase + index + 1);
}, [currentPage]);
우선 기존 조건문 순서에서 맨 위 조건과 두번째, 세번째 조건이 겹쳐서
가장 상위에 있는 (1 === currentPage || 1 > currentPage) 조건이 최우선으로 적용되고 두번째, 세번째 조건이 적용되지 않았다.
때문에 두번째, 세번째 조건에서 버그가 발생하여도 조건이 적용되지 않아 확인할 수 없었다.
우선적으로 조건문 순서 먼저 수정하였다.
✅ PageList if문 조건 처리 순서 변경
const pageList = useMemo(() => { // 리팩토링 1
if (maxPage - visiblePageCount < currentPage) {
return Array.from({ length: lastPageCount }, (_, index) => currentPageBase + index + 1);
}
if (0 === currentPage % visiblePageCount) {
return Array.from({ length: visiblePageCount }, (_, index) => beforePageBase + index + 1);
}
if (1 === currentPage || 1 > currentPage) {
return Array.from({ length: visiblePageCount }, (_, index) => index + 1);
}
return Array.from({ length: visiblePageCount }, (_, index) => currentPageBase + index + 1);
}, [currentPage]);
이렇게 수정하니 다음 조건들이 반영되어 다음 조건에 대한 버그를 확인할 수 있었다.
1-1) ❗조건문 / 마지막 페이지 리스트 버그 => if (maxPage - visiblePageCount < currentPage)
const pageList = useMemo(() => { // 리팩토링 1
if (maxPage - visiblePageCount < currentPage) {
return Array.from({ length: lastPageCount }, (_, index) => currentPageBase + index + 1);
}
마지막 페이지 리스트에서는 maxPage(49)에서 - lastPageCount(9) = 40을 초과할 경우 남은 페이지 수(lastPageCount)만큼 보여줘야 한다.
visible PageCount(10)가 아니라 lastPageCount(9)를 입력했어야 했는데 도중에 헷갈렸는지 잘못입력하였으나 위의 조건문 순서 때문에 적용이 안되어 버그가 발생했는지 조차 몰랐다..
(maxPage(49) - visiblePageCount(10) < currentPage) => (39 < currentPage)이기 때문에 현재페이지가 40일 때 부터 41~49페이지를 보여주는 버그가 발생했다.
✅ visiblePageCount => lastPageCount 로 수정
const pageList = useMemo(() => { // 리팩토링 1
if (maxPage - lastPageCount < currentPage) {
return Array.from({ length: lastPageCount }, (_, index) => currentPageBase + index + 1);
}
41페이지 부터 41~49페이지를 보여줘야 하기 때문에 원래의 의도대로
(maxPage(49) - lastPageCount(9) < currentPage) => (40 < currentPage) 로 변경하여 41페이지부터 41~49페이지를 보여주도록 수정하였다.
✖️♻️ 추후 리팩토링 받은 부분
lastPageFirst 변수를 할당하여 사용하도록 리팩토링 해주셨다.
const lastPageFirst = maxPage - lastPageCount +1
...
if (lastPageFirst <= currentPage) {
return Array.from({ length: lastPageCount }, (_, index) => lastPageFirst + index);
}
후에 다른 곳에서 lastPageFirst 사용이 필요할 경우 더 용이하기 때문에
1-2) ❗조건문 / 10, 20, 30 ... 등 값이 페이지가 딱 떨어지는 숫자일 경우, (0 === currentPage % visiblePageCount)
if (0 === currentPage % visiblePageCount) {
return Array.from({ length: visiblePageCount }, (_, index) => beforePageBase + index + 1);
✖️♻️ 추후 리팩토링 받은 부분
위 조건에서 beforePageBase를 사용할 경우 -1된 base를 제공하고,
이는 visiblePageCount가 변경됨에 따라 예외가 발생할 수 있기 때문에
beforePageBase를 사용하지 않고 currrentPage - visiblePageCount 로 base를 구하도록 리팩토링 해주셨다.
if (0 === currentPage % visiblePageCount) {
return Array.from(
{ length: visiblePageCount },
(_, index) => currentPage - visiblePageCount + index + 1,
);
}
그 외에도 불필요한 currentPage, beforePage, nextPageBase들을 삭제하였다.
const pageList = useMemo(() => {
if (lastPageFirst <= currentPage) {
return Array.from({ length: lastPageCount }, (_, index) => lastPageFirst + index);
}
if (0 === currentPage % visiblePageCount) {
return Array.from(
{ length: visiblePageCount },
(_, index) => currentPage - visiblePageCount + index + 1,
);
}
return Array.from(
{ length: visiblePageCount },
(_, index) => Math.floor(currentPage / visiblePageCount) * visiblePageCount + index + 1,
);
}, [currentPage]);
코드가 훨씬 간결해졌다.
1-3) ❗조건문 / 불필요한 조건 축약 => (1 === currentPage || 1 > currentPage)
✅ (1 === currentPage || 1 > currentPage) 조건 축약 => (1 >= currentPage)
if (1 >= currentPage) {
return Array.from({ length: visiblePageCount }, (_, index) => index + 1);
}
✖️♻️ 추후 리팩토링 받은 부분
if (1 === currentPage || 1 > currentPage) {
return Array.from({ length: visiblePageCount }, (_, index) => index + 1);
}
return Array.from({ length: visiblePageCount }, (_, index) => currentPageBase + index + 1);
(1 === currentPage || 1 > currentPage) 조건의 경우,
아래의 return Array.from({ length: visiblePageCount }, (_, index) => currentPageBase + index + 1) 으로 예외처리에 포함되므로 생략한다.
return Array.from({ length: visiblePageCount }, (_, index) => currentPageBase + index + 1);
2. ❗ ◀ 버튼 클릭 시 2 ~ 5 페이지 (1페이지가 아닌 경우)에서 마이너스 페이지로 이동됨
기존코드
onClick={() => {
setCurrentPage((prev) => {
if (1 < prev) {
return beforePageBase + 1;
}
return prev;
});
}}>
◀
</button>
2~5페이지 정도에서 ◀버튼 클릭시 마이너스 페이지로 이동되는 버그가 발생했다.
마이너스 페이지를 방어하는 기능이 필요하다.
onClick={() => {
if (currentPage <= visiblePageCount) {
return;
}
setCurrentPage(
(prev) =>
(Math.floor((prev) / visiblePageCount) - 1) *
visiblePageCount +
1
);
}}
>
✅ currentPage가 visiblePageCount보다 같거나 작을경우(마이너스) 함수를 끝내고,
그 외 10 초과일 경우 에만 beforeBase + 1 한다.
리팩토링에서 beforeBase를 삭제했으니
Math.floor((prev) / visiblePageCount) - 1) *visiblePageCount + 1 로 풀어서 작성해 준다.
3. ❗현재 페이지가 10, 20, 30 ... 등 값이 페이지가 딱 떨어지는 숫자일 경우,
=> (0 === currentPage % visiblePageCount) 일 경우,
기존코드
const beforePageBase = Math.floor(currentPage / visiblePageCount - 1) * visiblePageCount;
onClick={() => { // 리팩토링 2
setCurrentPage((prev) => {
if (1 < prev) {
return beforePageBase + 1;
}
return prev;
});
}}>
◀
</button>
◀ 버튼 클릭 시 그 전 페이지리스트의 첫 번째 페이지가 아닌, 해당 리스트의 첫번째 페이지로 이동된다.
ex ) ◀ 버튼클릭 시 21~ 30 페이지일 경우, 21~29페이지 까지는 base가 1로 계산되어 1페이지로 이동하지만, 30일 경우 2로 계산되어 21페이지로 이동한다.
✅이를 해결하기 위해 위 코드에서( prev-1)을 해준다.
ex) 현재 페이지가 30일 경우,
기존(prev) => Math.floor((30)/10) - 1 하면 3-1로 base가 2로 산출되나,
변경(prev-1) => Math.floor((30 -1)/10) - 1 하면 앞자리가 (2.9)-1로 변경되어 base가 1.9 => 1로 산출된다.
따라서 21~29의 base가 1인 것과 같이 30도 1로 산출되어 현재 페이지가 30일 때 ◀버튼을 눌러도 21이 아닌 11로 이동 가능하다.
onClick={() => {
if (currentPage <= visiblePageCount) {
return;
}
setCurrentPage(
(prev) =>
(Math.floor((prev-1) / visiblePageCount) - 1) *
visiblePageCount +
1
);
}}
>
4. 마지막 페이지 리스트에서 ▶버튼 클릭 시, maxpage를 초과하지 않도록 방어
기존코드
onClick={() =>
setCurrentPage((prev) => {
if (maxPage - visiblePageCount > prev) {
return nextPageBase + 1;
}
return prev;
})
}>
▶
</button>
maxPage를 초과하지 않도록 방어하는 기능이 필요하다. 같은 방식으로
onClick={() => {
if (lastPageFirst <= currentPage) {
return;
}
setCurrentPage(
(prev) => Math.ceil(prev / visiblePageCount) * visiblePageCount + 1
);
}}
>
▶
</button>
✅ currentPage가 lastPageFirst(41)보다 같거나 클경우 함수를 끝내고,
그 외 lastPageFirst 미만일 경우 에만 Math.ceil(올림)하여 base를 구한다.
리팩토링에서 nextBase를 삭제했으니
Math.ceil(prev / visiblePageCount) * visiblePageCount + 1 로 풀어서 작성해 준다.
▶의 경우, 21~30까지는 모두 올림 처리하면 base가 3이기 때문에 따로 10,20 ... 단위를 고려하지 않아도 된다.
리팩토링 최종 코드
import React, { useMemo } from 'react';
import styles from './Pagenation.module.scss';
interface IPagenationProps {
maxPage: number;
currentPage: number;
setCurrentPage: React.Dispatch<React.SetStateAction<number>>;
visiblePageCount: number;
}
export const Pagenation: React.FC<IPagenationProps> = ({
maxPage,
currentPage,
setCurrentPage,
visiblePageCount,
}) => {
const lastPageCount = maxPage % visiblePageCount || visiblePageCount;
const lastPageFirst = maxPage - lastPageCount + 1;
const pageList = useMemo(() => {
if (lastPageFirst <= currentPage) {
return Array.from({ length: lastPageCount }, (_, index) => lastPageFirst + index);
}
if (0 === currentPage % visiblePageCount) {
return Array.from(
{ length: visiblePageCount },
(_, index) => currentPage - visiblePageCount + index + 1,
);
}
return Array.from(
{ length: visiblePageCount },
(_, index) => Math.floor(currentPage / visiblePageCount) * visiblePageCount + index + 1,
);
}, [currentPage]);
return (
<div className={styles.flex}>
{currentPage <= visiblePageCount ? (
''
) : (
<button className={styles.button} onClick={() => setCurrentPage(1)}>
First
</button>
)}
<button
className={styles.button}
onClick={() => {
if (currentPage <= visiblePageCount) {
return;
}
setCurrentPage(
(prev) => (Math.floor((prev - 1) / visiblePageCount) - 1) * visiblePageCount + 1,
);
}}>
◀
</button>
<ul className={styles.flex}>
{pageList.map((item) => {
return (
<li
key={item}
onClick={() => setCurrentPage(item)}
className={currentPage === item ? styles.accord : styles.disaccord}>
{item}
</li>
);
})}
</ul>
<button
className={styles.button}
onClick={() => {
if (lastPageFirst <= currentPage) {
return;
}
setCurrentPage((prev) => Math.ceil(prev / visiblePageCount) * visiblePageCount + 1);
}}>
▶
</button>
{currentPage < lastPageFirst ? (
<button className={styles.button} onClick={() => setCurrentPage(maxPage)}>
Last
</button>
) : (
''
)}
<span>{`${currentPage}/${maxPage}`}</span>
</div>
);
};
.
.
.
이상으로 페이지네이션과 관련해 코드 리뷰 받은 부분에 대해 정리해 보았다.
이렇게나 많은 버그가 있을 줄 몰랐는데, 항상 모든 버그의 발생 가능성을 고려하고 계속해서 코드를 테스트 해봐야 할 필요성을 크게 느꼈다.
아무래도 하루만에 급하게 쥐어짜낸 코드라 더 버그가 많았던 것 같다.. 그래도 처음인데 목표한 기능을 완성한 것에 대해 너무 칭찬한다!😆
그저 기능만 작동하도록 코드를 작성하면 만족한다는 안일한 생각에서 위험성과 긴장감을 크게 느꼈다.
다음부턴 버그 발생 가능성을 좀 더 줄일 수 있도록 신중하게 코드를 작성하도록 하자😂
코드 리뷰 받은 부분이 생각보다 엄청난 도움이 되어 항상 팀원분께 너무 감사한 마음으로 배우고 있다.
'Project > 향수프로젝트' 카테고리의 다른 글
[React] 페이지네이션(Pagenation) 구현하기 (0) | 2022.12.03 |
---|