Sandbox 251102 (killerwhale) #307
Conversation
Issue/285/get courses api
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| return 'en' | ||
| }) |
There was a problem hiding this comment.
코드 검토 댓글
이 코드는 @nestjs/common에서 네스트JS 용으로 작성된 파라미터 데코레이터 GetLanguage를 정의하고 있습니다. 아래의 몇 가지 문제점과 개선 사항을 제시합니다:
-
기본 언어 설정: 현재 코드는
accept-language헤더가 비어있거나 문자열이 아닐 경우 기본 언어로 'kr'을 반환합니다. 하지만, 다른 언어 선택이 필요할 수 있습니다. 이 기능에 대한 설정을 외부에서 주입할 수 있도록 고려해보세요. -
헤더 검사:
req.headers['accept-language']에서 직접 헤더를 가져오고 있지만, 'accept-language' 헤더가 아예 존재하지 않는 경우도 발생할 수 있습니다. 따라서 아래와 같은 변형을 고려해보세요:const acceptLanguageHeader = req.headers['accept-language'] || '';
이렇게 하면 기존 코드를 간단히 유지하면서 더 안전한 확인을 할 수 있습니다.
-
확장성 문제: 현재 코드는 한국어(
kr)와 영어(en)만 지원하고 있습니다. 만약 다른 언어를 추가하게 된다면, this may lead to a complex condition structure or significant changes in the code. 이를 해결하기 위해 language mapping 시스템을 도입해보세요. -
타입 안전성: TypeScript의 강점을 활용하여
acceptLanguageHeader의 타입을 더 명확히 해주는 것도 좋습니다.string | undefined와 같은 강력한 타입을 고려해보세요. -
코드 문서화: 기능을 명시적으로 설명하는 주석을 추가하여 코드 유지보수성을 높여야 합니다. 특히 팀원이 이해하는 데 도움을 줄 수 있습니다.
이러한 점들을 개선하면 코드의 가독성과 유지보수성을 높일 수 있을 것입니다.
|
|
||
| export type Response = _Response | ||
|
|
||
| export interface JwtPayload { |
There was a problem hiding this comment.
코드 패치에 대한 몇 가지 우려 사항이 있습니다:
-
행동의 불확실성:
kaist_info및kaist_v2_info에 대한 타입 정의가any로 설정되어 있습니다. 이는 코드의 타입 안정성을 저하시킬 수 있습니다. 이 자리에 더 구체적인 타입을 정의하면 보안성을 높일 수 있습니다. -
필드의 선택적(empty) 상태: 여러 필드(
email,first_name,last_name, 등)가 선택적이며null값도 허용합니다. 이 경우, 비즈니스 로직에서 이 필드들이 반드시 포함되어야 하는 경우를 대비하여 엄격한 검사를 추가하는 것이 좋습니다. -
주석 부족: 코드의 각 타입이나 필드에 대한 자세한 설명이 부족하여 향후 개발자가 이해하는데 어려움을 겪을 수 있습니다. 각 필드에 대한 설명을 주석으로 추가하는 것이 좋습니다.
-
타입 일관성: 여러 타입에서
uid가 빠짐없이 사용되고 있습니다. 하지만 이를 강제하기 위해 공통된 인터페이스를 만들고 이를 확장하는 방식도 고려해볼 수 있습니다. -
유효성 검사: JWT과 같은 인증 관련 데이터를 다룰 때,
iat,exp,iss,aud등의 필드에 대한 유효성 검사를 수행하는 메커니즘을 도입하는 것이 권장됩니다. 유효하지 않은 값이 있을 경우 오류를 반환할 수 있도록 해야 합니다.
이러한 개선 사항을 적용하면 코드의 안정성 및 유지보수성을 높일 수 있습니다.
| @Transform(({ value }) => parseInt(value)) | ||
| limit?: number | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰
이 코드 패치는 기본적으로 나쁜 문제가 없어 보이지만, 몇 가지 잠재적인 버그와 개선 사항이 있습니다.
잠재적인 버그 및 위험
-
배열 변환 문제:
@Transform데코레이터에서value가 문자열일 때 배열로 변환하고 있는데, 이 과정에서 곧바로JSON.parse를 사용합니다. 비정상적인 JSON 문자열이 전달될 경우(예: 잘못된 형식의 문자열)try블록이 예외를 발생시킬 수 있으며, 이 경우에는return [Number(value)]가 실행됩니다. 이 로직이 의도한 대로 작동할지 확실하지 않습니다. 상황에 따라 예외 처리가 부족할 수 있습니다.- 제안:
JSON.parse를 사용하기 전에 문자열의 형식을 미리 검증하는 조건을 추가하는 것이 좋습니다.
- 제안:
-
term필드 타입:term필드는@IsArray()데코레이터가 적용되어 있지만, 실제로는 단일number값으로 설정될 것으로 보입니다.- 제안:
term의 타입을number로 변경하고@IsArray()데코레이터는 제거해야 합니다.
- 제안:
-
department필드와 관련된 변환:department의Transform로직에서 문자열 배열을 숫자 배열로 변환할 때, 입력 값 검증이 부족하여 예기치 않은 입력이 올 경우 오류가 발생할 수 있습니다. 예를 들어,NaN변환이 가능할 수 있습니다.- 제안: 변환 후 숫자가 유효한지 확인하는 로직을 추가하세요.
개선 사항
-
주석 및 문서화: 각 프로퍼티에 대한 설명이 추가되어 있으나, 모든 프로퍼티에 대한 설명이 충분히 명확하지 않을 수 있습니다. 더 많은 세부 정보를 제공하면 코드의 가독성이 높아질 수 있습니다.
-
데코레이터 정리: 현재 코드에서 datatypes (예:
@IsArray(),@IsOptional())와@Transform은 밀접하게 연결되어 있습니다. 데코레이터의 순서를 정리하여 가독성을 높이는 것이 좋습니다. -
검사 및 오류 메시지 개선: 각
@IsIn()의 오류 메시지는 당신의 코드가 아닌 하드코딩된 문자열입니다.- 제안: 사용자 정의 오류 메시지를 넣어 더 직관적인 피드백을 제공하도록 합시다.
이러한 개선 사항을 고려할 때까지는 이 코드를 병합하는 것이 적절하지 않을 수 있습니다.
| @Transform(({ value }) => parseInt(value)) | ||
| offset?: number | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰 코멘트
-
버그 가능성:
@Transform이 JSON 문자열을 문법적으로 올바른 형태로 다룰 수 있는지 확인하는 예외 처리가 미흡합니다. 예를 들어, 잘못된 JSON 문자열이 들어오면JSON.parse에서 오류가 발생하여 전체 프로세스가 중단될 수 있습니다. 이 부분에 대한 더 나은 예외 처리가 필요합니다. -
타입 안전성:
department배열의 경우,@IsInt로 요소가 정수인지 체크하고 있지만, JSON 파싱 후 유효성 검사가 필요합니다. 이 배열에 잘못된 데이터가 들어올 수 있으므로 추가적인 검증이 필요할 수 있습니다. -
중복 코드:
@Transform(({ value }) => parseInt(value))이 여러 필드에서 반복되고 있습니다. 이 로직을 공통 함수로 빼내어 중복을 줄이고 코드의 가독성을 높일 수 있습니다. -
주석 및 문서화: 코드에 대한 설명이나 주석이 불충분합니다. 특히 API의 파라미터에 대한 설명이 JSON 형식에서 어떤 형태로 들어오는지에 대한 명확한 설명이 필요합니다. 이는 코드 사용자가 이해하는데 도움이 됩니다.
-
변수 명명 규칙:
courseWrapped대신CourseWrapped와 같이 클래스 이름을 대문자로 시작하는 변수를 사용하는 것이 관례에 더 부합합니다. -
의존성 관리: 현재 사용하고 있는
class-transformer와class-validator의 버전이 어떤지도 명시하면 좋습니다. 또한, 코드의 변화가 의존성에 따라 어떤 영향을 미칠지 고려해보는 것이 좋습니다.
이러한 업데이트를 통해 코드의 안정성을 높이고 유지보수성을 증가시키는 것이 좋겠습니다.
| accessToken: string | ||
| refreshToken: string | ||
| } | ||
| } |
There was a problem hiding this comment.
-
중복 코드 제거:
TokenDto와TokenResponse인터페이스가 두 번 정의되었습니다. 이는 코드의 가독성을 떨어뜨리고 유지보수에 어려움을 줄 수 있습니다. 중복된 정의를 제거해야 합니다. -
주석 추가: 각 새로운 인터페이스와 클래스에 대한 설명 주석이 없어서 코드의 의도를 이해하기 어렵습니다. 각 클래스와 인터페이스에 대한 설명을 추가하면 코드를 사용하는 개발자에게 큰 도움이 될 것입니다.
-
타입 안전성:
sso_info속성에서 사용될 수 있는 문자열의 종류가 명확하지 않습니다. 가능하다면 더 구체적인 타입을 지정하거나, 타입 가드와 함께 문서화하는 것이 좋습니다. -
테스트 케이스: 새로 추가된 클래스와 인터페이스에 대한 유닛 테스트가 포함되어 있는지 확인할 필요가 있습니다. 최소한의 테스트가 없다면, 향후 리팩토링 시 regressions(회귀 버그)의 위험이 있습니다.
| const lectureIdsInCourse = lecturesInCourse.map((lec) => lec.id) | ||
| return takenLectureIds.map((lt) => lt.lecture_id).filter((id) => lectureIdsInCourse.includes(id)) | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 검토
-
타입 안정성:
filterList가object[]로 타입이 지정되어 있지만, 이 배열은 필터링 후null을 포함할 수 있습니다.null값이 포함될 수 있는 경우에 대한 처리를 적절히 해야 합니다. 추천하는 타입은Prisma.subject_courseWhereInput[]일 수 있습니다. -
변수 이름:
term_filter변수는 일관성을 위해termFilter로 이름을 변경하는 것이 좋습니다. -
쿼리 성능:
OR조건을 사용할 때, 해당 객체에서 취급하는 데이터가 많아질 수 있으므로 성능 저하를 초래할 수 있습니다. 인덱스 도메인이 가장 효과적으로 처리될 수 있도록 유의해야 합니다. -
에러 처리: 데이터베이스 쿼리를 실행할 때 발생할 수 있는 예외를 처리하는 코드를 추가하는 것이 좋습니다. 예를 들면,
try-catch구문을 활용할 수 있습니다. -
매직 넘버: 일반적인 상수는 클래스 내 상단에서
private readonly로 선언하여 관리하는 것이 좋습니다. 예를 들어,DEFAULT_LIMIT는 상수 목록에 추가하여 데이터를 더 명확히 할 수 있습니다. -
중복 코드:
getTakenLectureIdsByUser메서드에서 사용되는findMany구문이getTakenCourseIdsByUser와 유사하므로, 중복 로직을 별도의 헬퍼 함수로 분리하여 재사용성을 높일 수 있습니다. -
코드 주석: 코드 주석이 전반적으로 잘 작성되어 있습니다. 그러나 떼어 놓을 수 있는 부분이 많으니, 주석을 더욱 유용하게 하도록 정리할 수 있습니다.
-
의존성 주입: 객체 타입을 주입하는 곳에서 입력된 타입을 명시적으로 정의하는 것이 좋습니다. 타입을 추론하도록 하여 예측 가능한 유지 보수를 도울 수 있습니다.
| return [id, ctByLecture.get(id) ?? [], exam as ELectureV2.ExamTime | null, profByLecture.get(id) ?? []] | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 패치에 대한 리뷰는 다음과 같습니다:
-
세민터 필터 오류:
semesterFilter메서드에서years라는 속성이 사용되고 있습니다. 이는 아마year라는 속성이 잘못된 것으로 보입니다. 이를 수정해야 합니다. -
Nullable 처리: 메서드들이
null반환 가능성에 대해 잘 처리하고 있으나, 호출하는 부분에서도 이를 적절히 처리해야 합니다. 예를 들어,timeFilter가null을 반환할 수 있을 때 그것을 호출하는 코드에서 적절히 처리되고 있는지 확인해야 합니다. -
기본값 설정:
limit이나offset이 정의되지 않았을 경우를 처리하는 부분이 있으나, 이러한 매개변수들에 대해 청크 처리를 어떻게 구현할지에 대한 논의가 필요합니다. -
코드 가독성: 필터 설정 과정에서
filters배열을 만들 때 사용된 방식이 다소 복잡하여 유지보수 시 가독성이 떨어질 수 있습니다. 코드에 주석을 더 추가하거나, 슬라이스(예: 쪼개서 각각의 개별 필터에 이름 붙이기)를 고려하는 것이 좋습니다. -
ESLint 규칙: 전체적으로 ESLint 설정에 따라 코드 스타일을 맞추는 것이 좋습니다. 예를 들어, 변수 선언 방식이나 'any' 타입 사용을 피해야 할 필요성이 있습니다.
-
성능: 다수의 async 작업이
Promise.all을 사용하여 병렬적으로 수행되고 있는데, 상황에 따라 이로 인해 데이터베이스 쿼리가 지연될 수 있는 가능성도 고려해야 할 것입니다. -
테스트 케이스: 이 코드가 추가된 뒤 해당 기능에 대한 테스트 케이스가 있는지 확인이 필요합니다. 특히, 경계 조건이나 예외적인 상황에 대한 테스트를 수행해야 합니다.
| }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰 코멘트
-
버그 가능성:
getProfessorById메서드는professorId가 유효하지 않거나 존재하지 않을 경우null을 반환합니다. 하지만 이러한 경우에 대한 처리가 호출하는 코드에서 명확하게 이루어질지 확인해야 합니다. 이를 명시적으로 문서화하거나, 에러 핸들링 로직을 추가할 것을 권장합니다. -
프라미스 핸들링: 현재 구조에서
await를 사용하는 것은 적절하지만,return await대신return this.prismaRead.subject_professor.findUnique(...)와 같이 반환할 수 있습니다. 이는 조금 더 간결한 코드가 됩니다. -
종속성 주입:
PrismaService와PrismaReadService를 함께 사용하고 있는데, 두 서비스 간의 의존성을 명확히 하고, 필요할 경우 이를 분리하는 것을 고려할 수 있습니다. 예를 들어, 서비스의 역할을 더 명확히 해 주는 것이 좋습니다. -
타입 정의:
EProfessorV2.Basic이 어떤 구조를 가지는지 명확하지 않습니다. 이 타입의 정의를 문서화함으로써 코드를 사용하는 다른 개발자들이 이해하기 쉽게 할 수 있습니다. -
테스트 부족: 이 코드에 대한 단위 테스트가 없는 경우, 추가적인 테스트 케이스를 작성하여 코드의 신뢰성을 높이는 것이 중요합니다. 특히 데이터베이스 쿼리에 대한 테스트는 매우 중요합니다.
종합적으로, 코드가 기본적인 동작을 수행하는 것처럼 보이지만 위의 대안들을 고려하여 개선할 여지가 있습니다.
| const map = await this.prisma.session_userprofile.findFirst({ where: { uid } }) | ||
| return map?.sid ?? null | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 검토 결과, 몇 가지 주의해야 할 사항과 개선점이 있습니다.
-
에러 핸들링 부족:
findByUid와findSidByUid메서드에서 데이터베이스 호출 시 에러가 발생할 경우 적절한 에러 핸들링이 없습니다. 비동기 함수에서 발생할 수 있는 예외를 처리하는 로직을 추가하는 것이 좋습니다. 예를 들어, try-catch 블록을 사용해 오류를 캐치하고 로그를 남기거나 사용자에게 적절한 오류 메시지를 반환하는 방안을 고려해 보세요. -
uid 유효성 검사:
findByUid와findSidByUid메서드에서uid가 올바른 형식인지 검사하는 기능이 없습니다. 예를 들어, 형식 검사를 추가하여 예상치 못한 입력으로부터 데이터베이스를 보호할 수 있습니다. -
return 타입 명시:
findByUid와findSidByUid메서드의 반환 타입이 명확하게 정의되어 있지 않습니다. TypeScript의 장점을 활용해 반환 타입을 명시하는 것이 좋습니다. 이는 이후 유지보수와 가독성에 도움이 됩니다. -
null 사용:
findSidByUid메서드의 인수가string | null인 것처럼 nullable한 타입을 다루고 있습니다. 이 경우uid를 사용하기 전에 명확히 null 체크를 하고 있는데,uid가 null일 경우 호출 자체를 방지해주는 것도 좋습니다. 추가 안전성에 기여할 수 있습니다. -
Mapping 기대값 명확화:
findSidByUid의 반환 값인map?.sid에서map이 null일 경우에 대한 올바른 처리 및 개발 문서화가 필요합니다. 만약sid가 존재하지 않을 경우에 대한 비즈니스 로직이 필요할 수 있습니다.
이러한 개선점을 고려하여 코드를 수정한 후에 다시 검토할 것을 권장합니다.
libs/prisma-client/src/schema.prisma
Outdated
There was a problem hiding this comment.
코드 패치를 검토한 결과, 다음과 같은 우려 사항 및 개선 제안이 있습니다.
-
모델 구조의 명확성:
sid_uid_map모델이 추가되었는데, 이 모델의 역할이나 사용 목적이 코드의 다른 부분에서 명확히 드러나지 않습니다. 추가적인 주석이나 문서화를 통해 이 모델의 목적과 사용 방법을 설명하는 것이 좋습니다. -
데이터 유효성 검사:
sid와uid모두String으로 선언되어 있지만, 입력 데이터의 유효성을 검사하는 로직이 필요할 수 있습니다. 예를 들어, 특정 형식을 요구하는 경우 이를 보장하는 유효성 검사를 추가하는 것이 좋습니다. -
효율성 및 인덱스:
sid_uid_map모델의 인덱스를 고려해 볼 필요가 있습니다. 이 모델에서 어떤 쿼리가 빈번히 발생할지에 따라 적절한 인덱스를 추가하여 쿼리 성능을 개선할 수 있습니다. -
새로운 모델 추가 시 테스트 필요: 새로운 모델을 추가할 때는 항상 관련 테스트 케이스를 추가하여 기존 기능과의 호환성을 검증해야 합니다. 새로운 모델이 다른 부분과의 상호작용에서 문제가 생길 수 있으니, 이에 대한 테스트를 작성하는 것이 필수적입니다.
-
파일 끝의 개행 누락: 마지막 줄에 개행문자가 없어 가독성이 떨어질 수 있습니다. 이는 코드 스타일의 일관성을 유지하는 데 방해가 될 수 있으므로 주의가 필요합니다.
이러한 점들을 검토하여 필요한 수정 및 개선 작업을 진행하는 것을 권장합니다.
| } | ||
|
|
||
| return 'en' | ||
| }) |
There was a problem hiding this comment.
코드 검토 댓글
이 코드는 @nestjs/common에서 네스트JS 용으로 작성된 파라미터 데코레이터 GetLanguage를 정의하고 있습니다. 아래의 몇 가지 문제점과 개선 사항을 제시합니다:
-
기본 언어 설정: 현재 코드는
accept-language헤더가 비어있거나 문자열이 아닐 경우 기본 언어로 'kr'을 반환합니다. 하지만, 다른 언어 선택이 필요할 수 있습니다. 이 기능에 대한 설정을 외부에서 주입할 수 있도록 고려해보세요. -
헤더 검사:
req.headers['accept-language']에서 직접 헤더를 가져오고 있지만, 'accept-language' 헤더가 아예 존재하지 않는 경우도 발생할 수 있습니다. 따라서 아래와 같은 변형을 고려해보세요:const acceptLanguageHeader = req.headers['accept-language'] || '';
이렇게 하면 기존 코드를 간단히 유지하면서 더 안전한 확인을 할 수 있습니다.
-
확장성 문제: 현재 코드는 한국어(
kr)와 영어(en)만 지원하고 있습니다. 만약 다른 언어를 추가하게 된다면, this may lead to a complex condition structure or significant changes in the code. 이를 해결하기 위해 language mapping 시스템을 도입해보세요. -
타입 안전성: TypeScript의 강점을 활용하여
acceptLanguageHeader의 타입을 더 명확히 해주는 것도 좋습니다.string | undefined와 같은 강력한 타입을 고려해보세요. -
코드 문서화: 기능을 명시적으로 설명하는 주석을 추가하여 코드 유지보수성을 높여야 합니다. 특히 팀원이 이해하는 데 도움을 줄 수 있습니다.
이러한 점들을 개선하면 코드의 가독성과 유지보수성을 높일 수 있을 것입니다.
|
|
||
| export type Response = _Response | ||
|
|
||
| export interface JwtPayload { |
There was a problem hiding this comment.
코드 패치에 대한 몇 가지 우려 사항이 있습니다:
-
행동의 불확실성:
kaist_info및kaist_v2_info에 대한 타입 정의가any로 설정되어 있습니다. 이는 코드의 타입 안정성을 저하시킬 수 있습니다. 이 자리에 더 구체적인 타입을 정의하면 보안성을 높일 수 있습니다. -
필드의 선택적(empty) 상태: 여러 필드(
email,first_name,last_name, 등)가 선택적이며null값도 허용합니다. 이 경우, 비즈니스 로직에서 이 필드들이 반드시 포함되어야 하는 경우를 대비하여 엄격한 검사를 추가하는 것이 좋습니다. -
주석 부족: 코드의 각 타입이나 필드에 대한 자세한 설명이 부족하여 향후 개발자가 이해하는데 어려움을 겪을 수 있습니다. 각 필드에 대한 설명을 주석으로 추가하는 것이 좋습니다.
-
타입 일관성: 여러 타입에서
uid가 빠짐없이 사용되고 있습니다. 하지만 이를 강제하기 위해 공통된 인터페이스를 만들고 이를 확장하는 방식도 고려해볼 수 있습니다. -
유효성 검사: JWT과 같은 인증 관련 데이터를 다룰 때,
iat,exp,iss,aud등의 필드에 대한 유효성 검사를 수행하는 메커니즘을 도입하는 것이 권장됩니다. 유효하지 않은 값이 있을 경우 오류를 반환할 수 있도록 해야 합니다.
이러한 개선 사항을 적용하면 코드의 안정성 및 유지보수성을 높일 수 있습니다.
| @Transform(({ value }) => parseInt(value)) | ||
| limit?: number | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰
이 코드 패치는 기본적으로 나쁜 문제가 없어 보이지만, 몇 가지 잠재적인 버그와 개선 사항이 있습니다.
잠재적인 버그 및 위험
-
배열 변환 문제:
@Transform데코레이터에서value가 문자열일 때 배열로 변환하고 있는데, 이 과정에서 곧바로JSON.parse를 사용합니다. 비정상적인 JSON 문자열이 전달될 경우(예: 잘못된 형식의 문자열)try블록이 예외를 발생시킬 수 있으며, 이 경우에는return [Number(value)]가 실행됩니다. 이 로직이 의도한 대로 작동할지 확실하지 않습니다. 상황에 따라 예외 처리가 부족할 수 있습니다.- 제안:
JSON.parse를 사용하기 전에 문자열의 형식을 미리 검증하는 조건을 추가하는 것이 좋습니다.
- 제안:
-
term필드 타입:term필드는@IsArray()데코레이터가 적용되어 있지만, 실제로는 단일number값으로 설정될 것으로 보입니다.- 제안:
term의 타입을number로 변경하고@IsArray()데코레이터는 제거해야 합니다.
- 제안:
-
department필드와 관련된 변환:department의Transform로직에서 문자열 배열을 숫자 배열로 변환할 때, 입력 값 검증이 부족하여 예기치 않은 입력이 올 경우 오류가 발생할 수 있습니다. 예를 들어,NaN변환이 가능할 수 있습니다.- 제안: 변환 후 숫자가 유효한지 확인하는 로직을 추가하세요.
개선 사항
-
주석 및 문서화: 각 프로퍼티에 대한 설명이 추가되어 있으나, 모든 프로퍼티에 대한 설명이 충분히 명확하지 않을 수 있습니다. 더 많은 세부 정보를 제공하면 코드의 가독성이 높아질 수 있습니다.
-
데코레이터 정리: 현재 코드에서 datatypes (예:
@IsArray(),@IsOptional())와@Transform은 밀접하게 연결되어 있습니다. 데코레이터의 순서를 정리하여 가독성을 높이는 것이 좋습니다. -
검사 및 오류 메시지 개선: 각
@IsIn()의 오류 메시지는 당신의 코드가 아닌 하드코딩된 문자열입니다.- 제안: 사용자 정의 오류 메시지를 넣어 더 직관적인 피드백을 제공하도록 합시다.
이러한 개선 사항을 고려할 때까지는 이 코드를 병합하는 것이 적절하지 않을 수 있습니다.
| @Transform(({ value }) => parseInt(value)) | ||
| offset?: number | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰 코멘트
-
버그 가능성:
@Transform이 JSON 문자열을 문법적으로 올바른 형태로 다룰 수 있는지 확인하는 예외 처리가 미흡합니다. 예를 들어, 잘못된 JSON 문자열이 들어오면JSON.parse에서 오류가 발생하여 전체 프로세스가 중단될 수 있습니다. 이 부분에 대한 더 나은 예외 처리가 필요합니다. -
타입 안전성:
department배열의 경우,@IsInt로 요소가 정수인지 체크하고 있지만, JSON 파싱 후 유효성 검사가 필요합니다. 이 배열에 잘못된 데이터가 들어올 수 있으므로 추가적인 검증이 필요할 수 있습니다. -
중복 코드:
@Transform(({ value }) => parseInt(value))이 여러 필드에서 반복되고 있습니다. 이 로직을 공통 함수로 빼내어 중복을 줄이고 코드의 가독성을 높일 수 있습니다. -
주석 및 문서화: 코드에 대한 설명이나 주석이 불충분합니다. 특히 API의 파라미터에 대한 설명이 JSON 형식에서 어떤 형태로 들어오는지에 대한 명확한 설명이 필요합니다. 이는 코드 사용자가 이해하는데 도움이 됩니다.
-
변수 명명 규칙:
courseWrapped대신CourseWrapped와 같이 클래스 이름을 대문자로 시작하는 변수를 사용하는 것이 관례에 더 부합합니다. -
의존성 관리: 현재 사용하고 있는
class-transformer와class-validator의 버전이 어떤지도 명시하면 좋습니다. 또한, 코드의 변화가 의존성에 따라 어떤 영향을 미칠지 고려해보는 것이 좋습니다.
이러한 업데이트를 통해 코드의 안정성을 높이고 유지보수성을 증가시키는 것이 좋겠습니다.
| accessToken: string | ||
| refreshToken: string | ||
| } | ||
| } |
There was a problem hiding this comment.
-
중복 코드 제거:
TokenDto와TokenResponse인터페이스가 두 번 정의되었습니다. 이는 코드의 가독성을 떨어뜨리고 유지보수에 어려움을 줄 수 있습니다. 중복된 정의를 제거해야 합니다. -
주석 추가: 각 새로운 인터페이스와 클래스에 대한 설명 주석이 없어서 코드의 의도를 이해하기 어렵습니다. 각 클래스와 인터페이스에 대한 설명을 추가하면 코드를 사용하는 개발자에게 큰 도움이 될 것입니다.
-
타입 안전성:
sso_info속성에서 사용될 수 있는 문자열의 종류가 명확하지 않습니다. 가능하다면 더 구체적인 타입을 지정하거나, 타입 가드와 함께 문서화하는 것이 좋습니다. -
테스트 케이스: 새로 추가된 클래스와 인터페이스에 대한 유닛 테스트가 포함되어 있는지 확인할 필요가 있습니다. 최소한의 테스트가 없다면, 향후 리팩토링 시 regressions(회귀 버그)의 위험이 있습니다.
| const lectureIdsInCourse = lecturesInCourse.map((lec) => lec.id) | ||
| return takenLectureIds.map((lt) => lt.lecture_id).filter((id) => lectureIdsInCourse.includes(id)) | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 검토
-
타입 안정성:
filterList가object[]로 타입이 지정되어 있지만, 이 배열은 필터링 후null을 포함할 수 있습니다.null값이 포함될 수 있는 경우에 대한 처리를 적절히 해야 합니다. 추천하는 타입은Prisma.subject_courseWhereInput[]일 수 있습니다. -
변수 이름:
term_filter변수는 일관성을 위해termFilter로 이름을 변경하는 것이 좋습니다. -
쿼리 성능:
OR조건을 사용할 때, 해당 객체에서 취급하는 데이터가 많아질 수 있으므로 성능 저하를 초래할 수 있습니다. 인덱스 도메인이 가장 효과적으로 처리될 수 있도록 유의해야 합니다. -
에러 처리: 데이터베이스 쿼리를 실행할 때 발생할 수 있는 예외를 처리하는 코드를 추가하는 것이 좋습니다. 예를 들면,
try-catch구문을 활용할 수 있습니다. -
매직 넘버: 일반적인 상수는 클래스 내 상단에서
private readonly로 선언하여 관리하는 것이 좋습니다. 예를 들어,DEFAULT_LIMIT는 상수 목록에 추가하여 데이터를 더 명확히 할 수 있습니다. -
중복 코드:
getTakenLectureIdsByUser메서드에서 사용되는findMany구문이getTakenCourseIdsByUser와 유사하므로, 중복 로직을 별도의 헬퍼 함수로 분리하여 재사용성을 높일 수 있습니다. -
코드 주석: 코드 주석이 전반적으로 잘 작성되어 있습니다. 그러나 떼어 놓을 수 있는 부분이 많으니, 주석을 더욱 유용하게 하도록 정리할 수 있습니다.
-
의존성 주입: 객체 타입을 주입하는 곳에서 입력된 타입을 명시적으로 정의하는 것이 좋습니다. 타입을 추론하도록 하여 예측 가능한 유지 보수를 도울 수 있습니다.
| return [id, ctByLecture.get(id) ?? [], exam as ELectureV2.ExamTime | null, profByLecture.get(id) ?? []] | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 패치에 대한 리뷰는 다음과 같습니다:
-
세민터 필터 오류:
semesterFilter메서드에서years라는 속성이 사용되고 있습니다. 이는 아마year라는 속성이 잘못된 것으로 보입니다. 이를 수정해야 합니다. -
Nullable 처리: 메서드들이
null반환 가능성에 대해 잘 처리하고 있으나, 호출하는 부분에서도 이를 적절히 처리해야 합니다. 예를 들어,timeFilter가null을 반환할 수 있을 때 그것을 호출하는 코드에서 적절히 처리되고 있는지 확인해야 합니다. -
기본값 설정:
limit이나offset이 정의되지 않았을 경우를 처리하는 부분이 있으나, 이러한 매개변수들에 대해 청크 처리를 어떻게 구현할지에 대한 논의가 필요합니다. -
코드 가독성: 필터 설정 과정에서
filters배열을 만들 때 사용된 방식이 다소 복잡하여 유지보수 시 가독성이 떨어질 수 있습니다. 코드에 주석을 더 추가하거나, 슬라이스(예: 쪼개서 각각의 개별 필터에 이름 붙이기)를 고려하는 것이 좋습니다. -
ESLint 규칙: 전체적으로 ESLint 설정에 따라 코드 스타일을 맞추는 것이 좋습니다. 예를 들어, 변수 선언 방식이나 'any' 타입 사용을 피해야 할 필요성이 있습니다.
-
성능: 다수의 async 작업이
Promise.all을 사용하여 병렬적으로 수행되고 있는데, 상황에 따라 이로 인해 데이터베이스 쿼리가 지연될 수 있는 가능성도 고려해야 할 것입니다. -
테스트 케이스: 이 코드가 추가된 뒤 해당 기능에 대한 테스트 케이스가 있는지 확인이 필요합니다. 특히, 경계 조건이나 예외적인 상황에 대한 테스트를 수행해야 합니다.
| }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰 코멘트
-
버그 가능성:
getProfessorById메서드는professorId가 유효하지 않거나 존재하지 않을 경우null을 반환합니다. 하지만 이러한 경우에 대한 처리가 호출하는 코드에서 명확하게 이루어질지 확인해야 합니다. 이를 명시적으로 문서화하거나, 에러 핸들링 로직을 추가할 것을 권장합니다. -
프라미스 핸들링: 현재 구조에서
await를 사용하는 것은 적절하지만,return await대신return this.prismaRead.subject_professor.findUnique(...)와 같이 반환할 수 있습니다. 이는 조금 더 간결한 코드가 됩니다. -
종속성 주입:
PrismaService와PrismaReadService를 함께 사용하고 있는데, 두 서비스 간의 의존성을 명확히 하고, 필요할 경우 이를 분리하는 것을 고려할 수 있습니다. 예를 들어, 서비스의 역할을 더 명확히 해 주는 것이 좋습니다. -
타입 정의:
EProfessorV2.Basic이 어떤 구조를 가지는지 명확하지 않습니다. 이 타입의 정의를 문서화함으로써 코드를 사용하는 다른 개발자들이 이해하기 쉽게 할 수 있습니다. -
테스트 부족: 이 코드에 대한 단위 테스트가 없는 경우, 추가적인 테스트 케이스를 작성하여 코드의 신뢰성을 높이는 것이 중요합니다. 특히 데이터베이스 쿼리에 대한 테스트는 매우 중요합니다.
종합적으로, 코드가 기본적인 동작을 수행하는 것처럼 보이지만 위의 대안들을 고려하여 개선할 여지가 있습니다.
| const map = await this.prisma.session_userprofile.findFirst({ where: { uid } }) | ||
| return map?.sid ?? null | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 검토 결과, 몇 가지 주의해야 할 사항과 개선점이 있습니다.
-
에러 핸들링 부족:
findByUid와findSidByUid메서드에서 데이터베이스 호출 시 에러가 발생할 경우 적절한 에러 핸들링이 없습니다. 비동기 함수에서 발생할 수 있는 예외를 처리하는 로직을 추가하는 것이 좋습니다. 예를 들어, try-catch 블록을 사용해 오류를 캐치하고 로그를 남기거나 사용자에게 적절한 오류 메시지를 반환하는 방안을 고려해 보세요. -
uid 유효성 검사:
findByUid와findSidByUid메서드에서uid가 올바른 형식인지 검사하는 기능이 없습니다. 예를 들어, 형식 검사를 추가하여 예상치 못한 입력으로부터 데이터베이스를 보호할 수 있습니다. -
return 타입 명시:
findByUid와findSidByUid메서드의 반환 타입이 명확하게 정의되어 있지 않습니다. TypeScript의 장점을 활용해 반환 타입을 명시하는 것이 좋습니다. 이는 이후 유지보수와 가독성에 도움이 됩니다. -
null 사용:
findSidByUid메서드의 인수가string | null인 것처럼 nullable한 타입을 다루고 있습니다. 이 경우uid를 사용하기 전에 명확히 null 체크를 하고 있는데,uid가 null일 경우 호출 자체를 방지해주는 것도 좋습니다. 추가 안전성에 기여할 수 있습니다. -
Mapping 기대값 명확화:
findSidByUid의 반환 값인map?.sid에서map이 null일 경우에 대한 올바른 처리 및 개발 문서화가 필요합니다. 만약sid가 존재하지 않을 경우에 대한 비즈니스 로직이 필요할 수 있습니다.
이러한 개선점을 고려하여 코드를 수정한 후에 다시 검토할 것을 권장합니다.
libs/prisma-client/src/schema.prisma
Outdated
There was a problem hiding this comment.
코드 패치를 검토한 결과, 다음과 같은 우려 사항 및 개선 제안이 있습니다.
-
모델 구조의 명확성:
sid_uid_map모델이 추가되었는데, 이 모델의 역할이나 사용 목적이 코드의 다른 부분에서 명확히 드러나지 않습니다. 추가적인 주석이나 문서화를 통해 이 모델의 목적과 사용 방법을 설명하는 것이 좋습니다. -
데이터 유효성 검사:
sid와uid모두String으로 선언되어 있지만, 입력 데이터의 유효성을 검사하는 로직이 필요할 수 있습니다. 예를 들어, 특정 형식을 요구하는 경우 이를 보장하는 유효성 검사를 추가하는 것이 좋습니다. -
효율성 및 인덱스:
sid_uid_map모델의 인덱스를 고려해 볼 필요가 있습니다. 이 모델에서 어떤 쿼리가 빈번히 발생할지에 따라 적절한 인덱스를 추가하여 쿼리 성능을 개선할 수 있습니다. -
새로운 모델 추가 시 테스트 필요: 새로운 모델을 추가할 때는 항상 관련 테스트 케이스를 추가하여 기존 기능과의 호환성을 검증해야 합니다. 새로운 모델이 다른 부분과의 상호작용에서 문제가 생길 수 있으니, 이에 대한 테스트를 작성하는 것이 필수적입니다.
-
파일 끝의 개행 누락: 마지막 줄에 개행문자가 없어 가독성이 떨어질 수 있습니다. 이는 코드 스타일의 일관성을 유지하는 데 방해가 될 수 있으므로 주의가 필요합니다.
이러한 점들을 검토하여 필요한 수정 및 개선 작업을 진행하는 것을 권장합니다.
merge sandbox to dev