done#147
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your work! Before this can be approved, there are a few critical issues that need to be addressed to meet the core requirements of the task. Currently, the /add-expense endpoint overwrites the entire expense.json file instead of appending new expenses, which means only the latest expense is saved—this breaks the intended functionality of an expense tracker. Additionally, there is no error handling for invalid JSON input or file operations, so the server could crash if it receives bad data or if the file/directory does not exist. Please update your code to: (1) append new expenses to the JSON file, (2) add error handling for JSON parsing and file operations, and (3) ensure the db/expense.json file and its directory exist before writing. Once these are fixed, your solution will be much more robust! Keep going—you’re almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| return; | ||
| } | ||
|
|
||
| fs.writeFileSync(dataPath, data); |
There was a problem hiding this comment.
Issue: The use of fs.writeFileSync(dataPath, data) will overwrite the entire expense.json file with the new data. If the intention is to store multiple expenses, you should read the existing file, parse it as an array, append the new expense, and then write the updated array back to the file. This is a logic error related to how expenses are stored.
| const dataPath = path.resolve(__dirname, '..', 'db/expense.json'); | ||
| const data = Buffer.concat(chunks).toString(); | ||
|
|
||
| if (Object.keys(JSON.parse(data)).length !== 3) { |
There was a problem hiding this comment.
Potential issue: JSON.parse(data) may throw an error if the incoming data is not valid JSON. You should wrap this in a try-catch block to handle invalid JSON and respond with a 400 status code and an appropriate error message.
| }); | ||
|
|
||
| req.on('end', () => { | ||
| const dataPath = path.resolve(__dirname, '..', 'db/expense.json'); |
There was a problem hiding this comment.
Potential issue: The code assumes that the db/expense.json file and its parent directory exist. If they do not, fs.writeFileSync will throw an error. Consider checking for their existence and creating them if necessary before writing.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! However, there are still some critical issues that need to be addressed before your solution can be approved. The /add-expense endpoint currently overwrites the entire expense.json file with each new expense, instead of appending to an array, which breaks the core functionality of the expense tracker. Additionally, there is no error handling for invalid JSON input or file operations—this means the server could crash if it receives malformed data or if the file/directory does not exist. Please update your code to append expenses correctly and add robust error handling for both JSON parsing and file operations. Once these core requirements are met, your solution will be much stronger! Keep going—you’re almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| return; | ||
| } | ||
|
|
||
| fs.writeFileSync(dataPath, data); |
There was a problem hiding this comment.
Issue: You are using fs.writeFileSync(dataPath, data); to write the incoming expense data directly to the file. This overwrites the entire expense.json file with only the latest expense, instead of appending it to a list of expenses. According to the task requirements, you should append the new expense to an array in the JSON file, not replace the file contents. Please update this logic to read the existing expenses, append the new one, and then write the updated array back to the file.
| const dataPath = path.resolve(__dirname, '..', 'db/expense.json'); | ||
| const data = Buffer.concat(chunks).toString(); | ||
|
|
||
| if (Object.keys(JSON.parse(data)).length !== 3) { |
There was a problem hiding this comment.
Issue: There is no error handling for invalid JSON input. If the request body is not valid JSON, JSON.parse(data) will throw and crash the server. You should wrap this in a try-catch block and respond with a 400 error if parsing fails.
| return; | ||
| } | ||
|
|
||
| fs.writeFileSync(dataPath, data); |
There was a problem hiding this comment.
Issue: There is no error handling for file operations. If the file or directory does not exist, or if there is a write error, the server will throw and crash. You should handle file system errors gracefully and respond with an appropriate error message.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your submission! 🎉 While your code meets the basic requirements in terms of file structure and relevant logic, there are several critical issues that need your attention to ensure your expense tracker works reliably and robustly:
- Expense Overwriting: Currently, your
/add-expenseendpoint overwrites the entireexpense.jsonfile with each new expense. According to the requirements, you should be appending each new expense to an array withinexpense.json, not replacing the file contents. This is a critical logic error that breaks the core functionality of tracking multiple expenses. - JSON Parsing & Error Handling: You are parsing the JSON body outside of a try-catch block. If the input is malformed, this will throw an exception and potentially crash your server. Move the JSON parsing inside a try-catch block to handle errors gracefully and prevent server crashes.
- Redundant Parsing: You are parsing the JSON data more than once. Parse it only once, inside the try-catch, and reuse the parsed object for validation and further processing.
- File Operation Errors: There is no robust error handling for file read/write operations. If the file does not exist or there is a file system error, the server could crash or behave unpredictably. Make sure to handle these errors appropriately.
Despite these issues, I am approving your pull request to help you move forward with your learning. Please review the comments and address these critical issues in your next iteration. Fixing these problems will help you build more reliable and maintainable code. Keep up the effort, and don’t hesitate to consult the Node.js documentation on file operations and Express error handling for more guidance. You’re making good progress—just remember to focus on robust error handling and correct data management in your future work! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const dataPath = path.resolve(__dirname, '..', 'db/expense.json'); | ||
| const data = Buffer.concat(chunks).toString(); | ||
|
|
||
| if (Object.keys(JSON.parse(data)).length !== 3) { |
There was a problem hiding this comment.
You are parsing JSON data before the try-catch block. If the input is not valid JSON, this will throw an exception and crash the server. Move this check inside the try-catch block to handle errors gracefully. This is a critical issue related to error handling.
| } | ||
|
|
||
| try { | ||
| JSON.parse(data); |
There was a problem hiding this comment.
Parsing the JSON here is redundant since you already did it above (and incorrectly, outside the try-catch). You should parse the JSON only once, inside the try-catch block, and reuse the parsed object for validation and further processing.
| res.end('All params must be completed'); | ||
| } | ||
|
|
||
| fs.writeFile(dataPath, data, (err) => { |
There was a problem hiding this comment.
You are overwriting the entire expense.json file with the new data. According to the requirements, you should append the new expense to an array of expenses, not replace the file contents. This is a critical logic error that breaks the core functionality of the expense tracker.
No description provided.