-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[회비 생성] - 회비 생성 기능 리팩토링 #198
base: main
Are you sure you want to change the base?
Conversation
- dues로 전부 페이지가 이동되는 것을 routeParam 인자값으로 이동 되도록 수정
- 엑셀 파일을 읽는 프로미스를 리턴함 - 엑셀 파일을 읽을 수 있도록 배열 형태로 변경하는 makeWorksheetToArray 함수 추가
- 회비 업데이트는 다음 4단계를 통해 업데이트 된다. - 미납된 회비를 납부한 경우, 미납된 회비를 납부 처리한다. - 미납된 회비가 없는 경우, 납부한 달의 회비를 납부 처리한다. - 면제 인원의 경우 면제 처리한다. - 납부한 회비가 없는 경우 미납 처리한다.
- 엑셀 읽기 - 엑셀 읽은 후 엑셀 정보를 화면에 출력 - 정보에 맞게 회비 업데이트하기
- lint 에러 수정
- api 요청 성공 시 dues를 새롭게 불러오도록 dues 쿼리 무효화 기능 추가
- 모달 호출 후 모달을 닫도록 로직 추가
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review by ChatGPT
@@ -14,7 +17,7 @@ | |||
export default function YearPagination({ pageName, duesYear, setDuesYear }: YearPaginationProps) { | |||
const navigate = useNavigate(); | |||
const currentYear = new Date().getFullYear(); | |||
const param = useQueryParam('page'); | |||
const param = useQueryParam("page"); | |||
const page = Number(param); | |||
|
|||
const goToPrevYear = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주요 사항:
- 코드에서 큰 오류는 없으나, 코드 스타일 일관성을 개선할 수 있는 부분이 있습니다.
- 문자열을 정의할 때 일관성 있게 작은 따옴표나 큰 따옴표 중 하나를 사용하면 코드 가독성이 좋아집니다.
개선 제안 사항:
- 문자열 사용 시 큰 따옴표로 통일하였고, 추가적으로 코드 스타일에 대한 개선을 제안합니다.
const param = useQueryParam('page');
+ const param = useQueryParam("page");
이 외에도, 코드 라인의 일관성을 위해 모든 문자열을 큰 따옴표로 통일하는 것을 추천합니다. 또는 팀의 코드 스타일 가이드에 따라 따옴표 사용을 통일하는 것이 좋습니다.
@@ -42,6 +42,7 @@ export interface Member { | |||
updatedAt: string; | |||
isAuthed: boolean; | |||
isDeleted: boolean; | |||
isFeeExempt: boolean; | |||
deleteReason: string; | |||
birthday: string; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주어진 코드에서 Member
인터페이스에 isFeeExempt
필드가 추가되었습니다. 이 필드는 어떤 기능적 요구사항을 기반으로 추가된 것인지 명확하지 않은데, 이는 독자의 이해를 돕기 위해 주석으로 설명하는 것이 좋습니다.
아래는 개선이 필요한 부분을 diff 형식으로 제안합니다.
@@ -42,6 +42,8 @@ export interface Member {
isDeleted: boolean;
+ /** 사용자가 수수료 면제 상태인지 여부 */
isFeeExempt: boolean;
deleteReason: string;
birthday: string;
}
주요 사항 및 개선 제안:
isFeeExempt
필드에 대해 어떤 역할을 하는지 주석을 추가하여 코드의 가독성을 높였습니다. 이렇게 함으로써 다른 개발자가 이 필드의 목적을 쉽게 이해할 수 있도록 하였습니다.
return duesInfo; | ||
}; | ||
return { parseExcelData }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주어진 코드에서 몇 가지 개선할 부분과 오류를 발견했습니다. 주의할 점은 코드 가독성과 오류 처리를 좀 더 명확히 하는 것입니다.
주요 사항
- 불필요한 타입 단언:
row[NAME_INDEX]
의 타입을string
으로 단언하는 것은 위험할 수 있습니다. 값이undefined
일 경우 에러가 발생할 수 있습니다. - 비동기 처리:
useGetMembers
가 비동기 호출을 하는 것으로 보이지만, 해당 데이터가 준비되지 않았을 경우의 처리 로직이 빠져 있습니다. - 입력 데이터 유효성 검사: 불완전한 엑셀 데이터에 대한 추가 검사가 필요합니다.
개선 제안 사항
+ const name = row[NAME_INDEX] as string || ''; // 기본값 제공
+ if (!name) throw new Error(`이름이 없습니다: ${JSON.stringify(row)}`);
+
+ const { content } = members ?? { content: [] }; // members가 undefined일 경우 처리
+ const memberId = content.find((member) => member.name === name)?.id;
설명
- 첫 번째 수정에서는
row[NAME_INDEX]
의 값이undefined
일 경우를 대비해 기본값을 제공하고, 에러 메시지를 추가하여 어떤 행에서 문제가 발생했는지 추적 가능한 형태로 개선했습니다. - 두 번째 수정에서는
members
가undefined
인 경우를 처리하여 안전성을 높였습니다.
}; | ||
|
||
return readFile; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰를 수행한 결과, 몇 가지 주요 사항과 개선 사항이 발견되었습니다. 아래에 설명하겠습니다.
worksheet
가 없을 때의 처리: 현재reject
뒤에return;
이 없어서 이어서resolve
가 호출될 수 있습니다. 이 부분을 유의해주세요.onload
의nullable
처리:e.target
이null
일 수 있기 때문에 해당 부분을 체크하는 것이 필요합니다.
아래는 개선이 필요한 부분에 대한 제안입니다.
@@ -22,8 +22,10 @@
if (!worksheet) {
reject(new Error('worksheet is not found'));
+ return; // worksheet가 없으면 더 이상 진행하지 않도록 추가
}
- resolve(worksheet ? makeWorksheetToArray(worksheet) : null);
+ resolve(makeWorksheetToArray(worksheet)); // worksheet가 없는 경우는 위에서 처리하므로 단순 호출 가능
} catch (error) {
reject(error);
}
@@ -10,6 +12,7 @@
const data = new Uint8Array(e.target?.result as ArrayBuffer);
+ if (!e.target) throw new Error('FileReader target is null'); // e.target 체크 추가
await workbook.xlsx.load(data);
위의 개선 사항을 통해 코드의 안정성을 높이고, 예외 상황에 대한 처리를 보완할 수 있습니다.
}; | ||
|
||
return { runWorkflow }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
검토한 코드에서 몇 가지 개선 사항이 있습니다. 주로 비동기 함수에서 예외 처리를 추가하거나, 상태 업데이트를 더 명확하게 하기 위한 설명을 추가하는 것을 권장합니다. 다음은 제안하는 변경 사항입니다.
@@ -12,6 +12,8 @@ export function useWorkflowPipeline({ excelFileRef }: WorkflowPipelineProps) {
+ try {
+ const worksheet = await readExcelFile();
+ if (worksheet === null) return;
+ const parsedData = parseExcelData(worksheet);
+ applyFeeStatus(parsedData);
+ markExempt();
+ updateUnpaidStatus();
+ } catch (error) {
+ console.error('Error during workflow execution:', error);
+ }
주요 사항 및 개선 제안 사항
- 예외 처리 추가:
runWorkflow
함수 안에서await
구문으로 인해 비동기 작업이 실패할 경우를 대비하여try-catch
블록을 추가했습니다. 이는 런타임 에러를 포착하고, 오류 메시지를 출력하는 데 유용합니다. - 코드 가독성 향상: 예외가 발생했을 때의 처리 방식을 명확하게 하기 위해 로깅을 추가하여 개발자가 문제를 추적할 수 있도록 했습니다.
이러한 개선 사항은 코드의 안정성과 유지보수성을 높이는 데 도움을 줄 것입니다.
회비 생성 기능 리팩토링
기능
동아리 계좌가 변경되어 회비 엑셀 시트에 맞게 테이블 형식을 변경하였습니다.
일종의 워크 플로우 형식을 가지고 있어
useWorkflowPipeline
훅을 통해 회비 생성 기능을 만들어 비즈니스 로직을 분리하였습니다.워크 플로우
useReadExcelFile
useParseExcelFile
useApplyParsedData
useReadExcelFile
엑셀 파일을 읽고 해당 파일을 프로미스 객체로 받고, 해당 프로미스 객체는 읽은 엑셀 파일을 배열 형태로 리턴하도록 하였습니다.
useParseExcelFile
엑셀 파일을 읽고 DB에 적용가능한 형식으로 파싱하도록 합니다.
useApplyParsedData
applyFeeStatus
파싱한 값을 통해 회비를 낸 사람의 회비 내용을 적용합니다
markExempt
isFeeExempt
를 통해 회비 면제자를 파악하여 회비 면제를 적용합니다.updateUnpaidStatus
회비의 값이
PAID
SKIP
즉 회비 납부를 하지 않은 인원은 미납으로 적용합니다.Please check if the PR fulfills these requirements
main
branch?Screenshot
Precautions (main files for this PR ...)