Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors three API endpoints to separate controller and service layer logic, following a cleaner architecture pattern. The changes move database operations and business logic from controllers into dedicated service functions, making the code more maintainable and testable.
Changes:
- Created a new service file
subject.tsfor course-related operations - Added service functions to
paper.tsfor paper retrieval and course count operations - Refactored three API route handlers to use the new service layer functions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/services/subject.ts | New service file containing getCourseList function for course-list endpoint |
| src/lib/services/paper.ts | Added getPaperById and getCourseCounts service functions |
| src/app/api/papers/count/route.ts | Refactored to use getCourseCounts service function |
| src/app/api/paper-by-id/[id]/route.ts | Refactored to use getPaperById service function |
| src/app/api/course-list/route.ts | Refactored to use getCourseList service function |
Comments suppressed due to low confidence (1)
src/app/api/paper-by-id/[id]/route.ts:21
- The service layer throws an error with message "Paper not found" (line 33 in paper.ts) which should result in a 404 status code, but the controller's catch block returns a 500 status for all errors. This means that when a paper is not found, users will incorrectly receive a 500 Internal Server Error instead of a 404 Not Found.
Consider checking the error message in the catch block to return the appropriate status code, or create a custom error class to distinguish between different error types.
const paper = await getPaperById(id);
return NextResponse.json(paper, { status: 200 });
} catch (error) {
console.error(error);
return NextResponse.json(
{ message: "Failed to fetch paper", error },
{ status: 500 },
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await connectToDatabase(); | ||
| return await Course.find().lean(); |
There was a problem hiding this comment.
The indentation in this file uses 4 spaces, which is inconsistent with the rest of the codebase. Other service files like google-sheets.ts use 2-space indentation. This creates inconsistency that may fail linting or formatting checks.
Please reformat this file to use 2-space indentation to match the project's style conventions.
| await connectToDatabase(); | |
| return await Course.find().lean(); | |
| await connectToDatabase(); | |
| return await Course.find().lean(); |
| export async function getPaperById(id: string) { | ||
| await connectToDatabase(); | ||
| const paper = await Paper.findById(id); | ||
|
|
||
| if (!paper) { | ||
| throw new Error("Paper not found"); // 404 | ||
| } | ||
|
|
||
| return paper; | ||
| } | ||
|
|
||
| export async function getCourseCounts(){ | ||
| await connectToDatabase(); | ||
|
|
||
| const count = await CourseCount.find().lean(); | ||
|
|
||
| const formatted = count.map((item) => ({ | ||
| name: item.name, | ||
| count: item.count, | ||
| })); | ||
|
|
||
| return formatted; | ||
| } No newline at end of file |
There was a problem hiding this comment.
The indentation in these functions uses 4 spaces, which is inconsistent with the rest of the codebase. Other service files like google-sheets.ts use 2-space indentation. This creates inconsistency that may fail linting or formatting checks.
Please reformat these functions to use 2-space indentation to match the project's style conventions.
| import { connectToDatabase } from "@/lib/database/mongoose"; | ||
| import { Course } from "@/db/course"; | ||
|
|
||
| import { getCourseList } from "@/lib/services/subject"; |
There was a problem hiding this comment.
Missing blank line after imports. Other route files in the codebase (e.g., papers/route.ts) have a blank line between imports and export statements for better readability and consistency.
| import { getCourseList } from "@/lib/services/subject"; | |
| import { getCourseList } from "@/lib/services/subject"; |
📌 Purpose
Seperated the controller logic and service logic for 3 endpoints
paper-by-id/
paper/count/
course-list/
🔧 Changes