엑셀 다운로드 핸들러 함수 (handle Excel Download) 리팩터링 #159
Conversation
1) 엑셀 다운로드 e2e 테스트 생성 2) sheetData 지역 변수 제거 - buildSheetData 함수로 추출 3) wb, ws 지역 변수 제거 - workbook 생성 로직 함수 추출 - 함수 인라인 적용 4) excelDownload 구체적 함수명 지정 5) 공용 함수화
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit릴리스 노트
개요테스트 디렉터리를 tests/에서 e2e-tests/로 마이그레이션하고, 엑셀 다운로드 로직을 새로운 유틸리티 함수로 리팩토링합니다. 엑셀 파일 생성 및 다운로드 기능을 공통 유틸리티로 통합하고 E2E 테스트를 추가합니다. 변경사항
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
e2e-tests/excel/excel.download.spec.ts (1)
7-7: paths 상수 사용을 권장합니다.하드코딩된 URL 대신 기존 codebase의
@/const/paths상수를 사용하면 유지보수성이 향상됩니다.다음과 같이 수정할 수 있습니다:
+import { paths } from '@/const/paths'; + test.use({ storageState: 'e2e-tests/auth/admin.json' }); test.describe('엑셀 다운로드 테스트', () => { test('스터디 관리 페이지 (manage-study) 엑셀 다운로드 테스트', async ({ page }) => { - await page.goto('http://localhost:3000/admin/manage-study'); + await page.goto(paths.admin.manageStudy); ... }); test('스터디 신청자 목록 페이지 (manage-student) 엑셀 다운로드 테스트', async ({ page }) => { - await page.goto('http://localhost:3000/admin/manage-student'); + await page.goto(paths.admin.manageStudent); ... }); });Also applies to: 21-21
src/utils/excel.ts (2)
8-8: 시트 이름 파라미터 추가를 고려해보세요.
book_append_sheet호출 시 시트 이름을 명시하지 않으면 기본 이름(예: "Sheet1")이 사용됩니다. 명시적인 시트 이름을 파라미터로 받으면 더 유연하게 사용할 수 있습니다.다음과 같이 개선할 수 있습니다:
-export function buildWorkBook<T>(sheetData: T[]) { +export function buildWorkBook<T>(sheetData: T[], sheetName: string = 'Sheet1') { const wb = xlsx.utils.book_new(); const ws = xlsx.utils.json_to_sheet(sheetData); - xlsx.utils.book_append_sheet(wb, ws); + xlsx.utils.book_append_sheet(wb, ws, sheetName); return wb; }
1-15: JSDoc 주석 추가를 권장합니다.공용 유틸리티 함수이므로 JSDoc 주석을 추가하면 개발자 경험이 향상됩니다.
예시:
/** * 주어진 데이터로부터 Excel 워크북을 생성합니다. * @template T - 시트 데이터의 타입 * @param sheetData - JSON 형식의 시트 데이터 배열 * @param sheetName - 시트 이름 (기본값: 'Sheet1') * @returns 생성된 워크북 객체 */ export function buildWorkBook<T>(sheetData: T[], sheetName: string = 'Sheet1') { // ... } /** * 워크북을 Excel 파일로 다운로드합니다. * @param data - 다운로드할 워크북 객체 * @param filename - 저장할 파일명 (확장자 포함) */ export function downloadExcel(data: xlsx.WorkBook, filename: string) { // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)e2e-tests/excel/excel.download.spec.ts(1 hunks)e2e-tests/reports/report.member.spec.ts(1 hunks)playwright.config.ts(1 hunks)src/pages/Admin/ManageStudent/Page.tsx(2 hunks)src/pages/Admin/ManageStudy/Page.tsx(3 hunks)src/utils/excel.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/Admin/ManageStudent/Page.tsx (1)
src/utils/excel.ts (2)
downloadExcel(13-15)buildWorkBook(3-11)
src/pages/Admin/ManageStudy/Page.tsx (1)
src/utils/excel.ts (2)
downloadExcel(13-15)buildWorkBook(3-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-test
🔇 Additional comments (9)
playwright.config.ts (1)
20-20: 테스트 디렉토리 변경이 올바르게 적용되었습니다.테스트 디렉토리를
./e2e-tests로 변경하여 PR 전반의 구조 개편과 일치합니다..gitignore (1)
37-37: 무시 경로가 새 디렉토리 구조와 일치합니다.
e2e-tests/auth/경로로 업데이트하여 Playwright 설정 변경사항과 일관성을 유지합니다.e2e-tests/reports/report.member.spec.ts (1)
31-31: storageState 경로가 올바르게 업데이트되었습니다.새로운 e2e-tests 디렉토리 구조에 맞춰 인증 상태 파일 경로가 정확하게 변경되었습니다.
e2e-tests/excel/excel.download.spec.ts (1)
1-32: 엑셀 다운로드 e2e 테스트가 잘 작성되었습니다.테스트가 다운로드 이벤트를 올바르게 대기하고 파일명과 다운로드 성공을 검증합니다. 리팩터링된 Excel 유틸리티의 동작을 효과적으로 검증합니다.
src/utils/excel.ts (1)
3-11: 리팩터링 목표를 잘 달성한 유틸리티 함수입니다.단일 책임 원칙에 따라 워크북 생성 로직을 명확하게 분리했습니다. 제네릭 타입을 사용하여 재사용성도 좋습니다.
src/pages/Admin/ManageStudent/Page.tsx (2)
12-12: 공용 Excel 유틸리티 import가 올바릅니다.새로 추출된 유틸리티 함수를 활용하여 코드 재사용성을 높였습니다.
89-106: 리팩터링이 성공적으로 완료되었습니다.단일 책임 원칙에 따라 시트 데이터 생성 로직을
buildApplicantsSheetData함수로 명확하게 분리했습니다.applicants가 없는 경우 빈 배열을 반환하여 안전하게 처리하고 있으며, 워크북 생성과 다운로드 로직도 공용 유틸리티를 사용하여 중복을 제거했습니다.src/pages/Admin/ManageStudy/Page.tsx (2)
8-8: 공용 Excel 유틸리티 import가 적절합니다.ManageStudent 페이지와 동일한 패턴으로 공용 유틸리티를 활용하여 일관성을 유지합니다.
35-57: 리팩터링이 효과적으로 적용되었습니다.
buildStudySheetData함수로 데이터 가공 로직을 분리하고,flatMap을 사용하여 그룹별 멤버 데이터를 효율적으로 변환합니다.activities가 없는 경우 빈 배열을 반환하여 안전성을 보장하며, ManageStudent 페이지와 동일한 구조로 일관성도 우수합니다.
|
@minzziPark 리팩터링 도중에 두 가지 질문이 생겼는데 의견 부탁드려요. 1) sheetData를 inline 하는 것에 대해 어떻게 생각하시나요??Before const sheetData = buildStudySheetData();
downloadExcel(buildWorkBook(sheetData), '스터디그룹활동.xlsx');After downloadExcel(buildWorkBook(buildStudySheetData()), '스터디그룹활동.xlsx'); // sheetData inline 처리2) buildSheetData 함수에서 sheetData를 선언, 대입, 반환을 명시적으로 하는 것에 대해 어떻게 생각하시나요??Before function buildStudySheetData() {
if (!activities) return [];
return activities.flatMap((group) => {
const sheetRow = group.members.map((member) => ({
Group: group.group,
MemberID: member.id,
MemberName: member.name,
MemberNumber: member.sid,
Friends: member.friends.map((friend) => friend.name).join(', '),
Subjects: member.courses.map((subject) => subject.name).join(', '),
Reports: group.reports,
Times: group.times,
}));
return sheetRow;
});
}After function buildStudySheetData() {
if (!activities) return [];
let sheetData; // 명시적으로 선언
sheetData = activities.flatMap((group) => {
const sheetRow = group.members.map((member) => ({
Group: group.group,
MemberID: member.id,
MemberName: member.name,
MemberNumber: member.sid,
Friends: member.friends.map((friend) => friend.name).join(', '),
Subjects: member.courses.map((subject) => subject.name).join(', '),
Reports: group.reports,
Times: group.times,
}));
return sheetRow;
});
return sheetData;
} |
이미 buildWorkBook를 인라인으로 호출하고 있기 때문에 그 안에 함수를 호출하면 저는 동작하는 방식을 이해하기 힘든 것 같습니다.
sheetData를 임시 변수로 두면 사이드 이펙트가 커질 가능성이 높은 것 같습니다. 기존 형태가 좋은 것 같습니다. |
| }, [applicants, searchTerm]); | ||
|
|
||
| const handleExcelDownload = () => { | ||
| if (applicants) { |
There was a problem hiding this comment.
buildApplicantsSheetData 함수에서 applicants의 존재 유무를 확인한다면 기존 로직과 다르게 동작할 것 같습니다.
handleExcelDownload 함수 상단에서 처리해주는 건 어떨까요?
| if (applicants) { | |
| const handleExcelDownload = () => { | |
| if(applicants) return ; |
| downloadExcel(buildWorkBook(sheetData), '스터디신청자목록.xlsx'); | ||
|
|
||
| function buildApplicantsSheetData() { | ||
| if (!applicants) return []; |
There was a problem hiding this comment.
위 제안에 동의하신다면 해당 부분도 삭제해야할 것 같습니다.
src/utils/excel.ts
Outdated
| const wb = xlsx.utils.book_new(); | ||
|
|
||
| const ws = xlsx.utils.json_to_sheet(sheetData); |
There was a problem hiding this comment.
wb, ws가 무엇을 의미하는 변수인지 짐작하기가 어렵습니다. book_new(), json_to_sheet()을 보고 추론할 수는 있겠지만, 그럼에도 w가 의미하는 건 뭔지 잘 모르겠습니다. 변수명을 좀 더 직관적으로 수정해주실 수 있으신가요?
There was a problem hiding this comment.
그렇네요. 변경 할 필요가 있어 보입니다.
wb -> workBook
ws -> workSheet
로 변경하도록 하겠습니다.
src/pages/Admin/ManageStudy/Page.tsx
Outdated
| const sheetData = activities.flatMap((group) => | ||
| group.members.map((member) => ({ | ||
| const sheetData = buildStudySheetData(); | ||
| downloadExcel(buildWorkBook(sheetData), '스터디그룹활동.xlsx'); |
There was a problem hiding this comment.
유틸 함수로 buildWorkBook 로직을 분리하니 함수가 어떻게 동작하는지 이해하기 쉬워졌습니다! 고생하셨습니다 :)
- wb -> workBook - ws -> workSheet
- 엑셀 다운로드명 구체화하고 buildWorkBook을 내부에 숨김으로써 직관적으로 개선
- 해당 데이터 없을 시 return; - buildsheetData 함수 인라인
작업 사항
중복된 엑셀 다운로드 로직을 통합하고, 단일 책임 원칙에 맞게 함수를 분리함.
여러 지역 변수를 제거 하는 방향으로 리팩터링을 진행함.
작업 과정
excel.spec.ts를 작성함AS-IS
TO-BE
AS-IS
TO-BE
AS-IS
TO-BE
관련 이슈
#158