Conversation
Summary of ChangesHello @withJihyuk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 공간 사용 내역 관리 시스템의 핵심 기능을 확장하고 개선하는 것을 목표로 합니다. 주요 변경 사항은 사용 내역에 대한 추가 인원 정보를 기록하는 기능 도입과, 이를 활용하여 공간별 월별 이용 현황을 연령 및 성별로 상세하게 집계하여 엑셀 파일로 내보내는 새로운 보고서 기능 추가입니다. 또한, 기존 엑셀 내보내기 로직은 더 유연하고 확장 가능한 구조로 전면 리팩토링되었습니다. 이로써 관리자는 공간 사용 데이터를 더욱 세밀하게 분석하고 활용할 수 있게 됩니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 월별 이용 통계를 템플릿 기반의 Excel 파일로 내보내는 중요한 기능과, 공간 이용 시 추가 인원을 기록하는 기능을 구현했습니다.
전반적인 코드 품질이 높습니다. 특히 Excel 생성 로직을 단일 책임 원칙에 따라 작은 컴포넌트로 리팩토링한 점은 유지보수성 측면에서 매우 훌륭한 개선입니다. 또한 @ConfigurationProperties를 활용한 설정 관리나 성능 최적화를 위한 JOIN FETCH 사용도 좋은 선택입니다.
코드의 안정성과 유지보수성을 더욱 향상시키기 위해 몇 가지 제안 사항을 댓글로 남겼습니다. 파일명 오타 수정, 수식 파싱 로직 유연성 확보, 하드코딩된 값 제거 등에 대한 내용이니 검토 부탁드립니다.
| import org.aing.danurirest.domain.admin.usecase.GetUsageHistoriesUsecase | ||
| import org.aing.danurirest.domain.admin.usecase.GetUsageHistoryUsecase | ||
| import org.aing.danurirest.domain.admin.usecase.UpdateUsageHistoryUsecase | ||
| import org.aing.danurirest.domain.admin.usecase.* |
There was a problem hiding this comment.
와일드카드(*) import는 간결해 보일 수 있지만, 어떤 클래스가 사용되는지 명확하지 않아 코드 가독성과 유지보수성을 저해할 수 있습니다. 특히 협업 환경에서는 명시적으로 클래스를 import하는 것이 충돌을 피하고 코드를 이해하는 데 도움이 됩니다. 개별 클래스를 import하는 방식으로 되돌리는 것을 고려해 보세요.
| import org.springframework.web.bind.annotation.RequestMapping | ||
| import org.springframework.web.bind.annotation.RestController | ||
| import org.springframework.validation.annotation.Validated | ||
| import org.springframework.web.bind.annotation.* |
There was a problem hiding this comment.
와일드카드(*) import는 코드에서 사용하는 클래스의 출처를 모호하게 만들 수 있습니다. 가독성과 유지보수성을 위해 필요한 어노테이션들을 명시적으로 import하는 것이 좋습니다.
| .ok() | ||
| .header("Content-Type", "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet") | ||
| .header("Content-Disposition", "attachment; filename=usage_history.xlsx") | ||
| .header("Content-Disposition", "attachment; filename=usage_history_.xlsx") |
There was a problem hiding this comment.
엑셀 파일명에 불필요한 밑줄(_)이 포함되어 있습니다. usage_history_.xlsx 대신 usage_history.xlsx로 수정하는 것이 자연스러워 보입니다.
| .header("Content-Disposition", "attachment; filename=usage_history_.xlsx") | |
| .header("Content-Disposition", "attachment; filename=usage_history.xlsx") |
| fun updateFormula( | ||
| formula: String, | ||
| startRow: Int, | ||
| endRow: Int, | ||
| ): String = | ||
| formula.replace(Regex("SUM\\(([A-Z]+)\\d+:([A-Z]+)\\d+\\)")) { match -> | ||
| val column1 = match.groupValues[1] | ||
| val column2 = match.groupValues[2] | ||
| "SUM(${column1}$startRow:${column2}$endRow)" | ||
| } |
There was a problem hiding this comment.
현재 updateFormula의 정규식은 SUM(A1:A10)과 같은 간단한 형태의 수식만 처리할 수 있습니다. 만약 템플릿의 합계 수식이 SUM(A1, B1) 또는 SUM(A:A)와 같이 다른 형태로 변경될 경우, 이 로직이 깨질 수 있습니다. 보다 유연한 수식 파싱 및 업데이트 방법을 고려해보거나, 템플릿의 수식 형태에 대한 제약을 문서화하는 것이 좋겠습니다.
| try { | ||
| // 템플릿 로드 | ||
| val workbook = templateLoader.loadMonthlyUsageTemplate() | ||
| val sheet = workbook.getSheetAt(0) ?: throw CustomException(CustomErrorCode.UNKNOWN_SERVER_ERROR) |
There was a problem hiding this comment.
workbook.getSheetAt(0)을 사용하여 시트를 인덱스로 가져오고 있습니다. 이는 템플릿 파일의 첫 번째 시트가 항상 우리가 원하는 시트라는 것을 가정합니다. 템플릿의 구조가 변경될 경우(예: 시트 순서 변경) 버그가 발생할 수 있습니다. workbook.getSheet("시트이름")과 같이 시트 이름으로 조회하는 것이 더 안정적입니다. 시트 이름 또한 ExcelProperties에서 관리하는 것을 고려해볼 수 있습니다.
| for (day in (daysInMonth + 1)..31) { | ||
| val rowIndex = properties.data.startRow + day - 1 | ||
| val row = sheet.getRow(rowIndex) | ||
| row?.let { clearRow(it) } | ||
| } | ||
|
|
||
| if (sheet.lastRowNum > properties.data.startRow + daysInMonth) { | ||
| sheet.shiftRows( | ||
| properties.data.startRow + 31, | ||
| sheet.lastRowNum, | ||
| -(31 - daysInMonth), | ||
| ) | ||
| } |
There was a problem hiding this comment.
handleExtraDays 메서드에서 숫자 31이 여러 곳에 하드코딩되어 있습니다. 이는 템플릿이 항상 31일 기준으로 작성되었다는 강한 가정을 내포하고 있습니다. 이 값을 ExcelProperties를 통해 설정 파일로 옮기거나, 클래스 상수로 정의하여 코드의 의도를 명확히 하고 유지보수성을 높이는 것이 좋습니다.
| private fun mapAgeOption(ageOption: String?): Age? = | ||
| when (ageOption) { | ||
| "초등학교" -> Age.ELEMENTARY | ||
| "중학교" -> Age.MIDDLE | ||
| "고등학교" -> Age.HIGH | ||
| "대학교" -> Age.COLLEGE | ||
| "학교 밖 청소년" -> Age.OUT_OF_SCHOOL_YOUTH | ||
| "성인/유아" -> Age.ADULT | ||
| else -> null | ||
| } |
There was a problem hiding this comment.
mapAgeOption 함수 내에서 "초등학교", "중학교" 등 UI에 표시되는 문자열을 사용하여 Age enum으로 변환하고 있습니다. 이는 UI의 문구가 변경될 경우 서버 코드를 수정해야 하는 의존성을 만듭니다.
이러한 문제를 해결하기 위해, 폼 정의에 value와 같은 내부 식별자를 추가하고, 서버에서는 이 식별자를 사용하여 매핑하는 것을 고려해볼 수 있습니다.
예시:
{
"label": "초등학교",
"value": "ELEMENTARY"
}이렇게 하면 UI 변경에 더 유연하게 대처할 수 있습니다.
1 participant
💡 JIRA TICKET
📃 작업내용
✅ PR 체크리스트
.env,노션,README)"API 개발 완료됐어요","환경값 추가되었어요")