Solution#148
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! Before your solution can be approved, there are a few critical issues that need to be addressed to meet the core requirements of the task:
- The form submits data as
application/x-www-form-urlencoded, but your server only handles JSON. This means expense submissions from the form will not be processed correctly. Please update either the server to handle form-encoded data or the form to submit JSON via JavaScript (e.g., usingfetch). - The server currently overwrites the entire
expense.jsonfile with each new expense, so only the latest expense is saved. To track multiple expenses, you should read the existing file as an array, append the new expense, and write the updated array back. - There is no error handling for invalid JSON in the request body, which could crash your server. Please add a try-catch block and return a 400 error if parsing fails.
Once you address these issues, your solution will be much closer to meeting all requirements! 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(filePath, JSON.stringify(expense, null, 2)); |
There was a problem hiding this comment.
Issue: The current implementation overwrites the entire 'expense.json' file with each new expense. This means only the most recent expense will be saved. If the intention is to keep a list of expenses, you should read the existing file, parse it as an array, push the new expense, and then write the updated array back to the file.
| }); | ||
|
|
||
| req.on('end', () => { | ||
| const expense = JSON.parse(body); |
There was a problem hiding this comment.
Error: There is no error handling for invalid JSON in the request body. If the client sends malformed JSON, this line will throw and crash the server. Consider wrapping this in a try-catch block and returning a 400 error if parsing fails.
| <title>Document</title> | ||
| </head> | ||
| <body> | ||
| <form action="/add-expense" method="POST"> |
There was a problem hiding this comment.
Issue: The form uses the default submission method, which sends data as 'application/x-www-form-urlencoded'. Your server expects JSON in the POST body, so this will not work. You need to either update the server to handle form-encoded data or use JavaScript to intercept the form submission and send the data as JSON using fetch or XMLHttpRequest.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! There are still two critical issues that need to be addressed before your solution meets the core requirements: (1) The server does not handle form submissions correctly because it expects JSON, but the form sends data as application/x-www-form-urlencoded. (2) The server overwrites the expense.json file with each new submission, so only the latest expense is saved instead of keeping a list of all expenses. Please update your code to fix these issues so that multiple expenses are preserved and form submissions are processed correctly. You're almost there—keep going! 🚀
✨ 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(filePath, JSON.stringify(expense, null, 2)); |
There was a problem hiding this comment.
Issue: This line overwrites the entire expense.json file with only the latest expense object. According to the requirements, you should store all expenses as an array. To fix this, read the existing file (if it exists), parse it as an array (or initialize an empty array if the file is empty or missing), append the new expense, and then write the updated array back to the file. This is a critical issue that prevents your server from tracking multiple expenses.
| <title>Document</title> | ||
| </head> | ||
| <body> | ||
| <form action="/add-expense" method="POST"> |
There was a problem hiding this comment.
Issue: The form submits data as application/x-www-form-urlencoded by default, but your server expects JSON in the request body. To fix this, you should either:
- Use JavaScript to handle the form submission and send the data as JSON using
fetch, or - Update your server to parse form-encoded data.
Without this change, expense submissions from this form will not be processed correctly by your server.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your submission! 🎉 Your implementation covers most of the requirements: the HTML form is correctly set up, form submission and response formatting are handled well, and the overall structure is solid. However, there is a critical issue that needs your attention: currently, each new expense submission overwrites the entire db/expense.json file, so only the latest expense is saved. According to the requirements, you must preserve all submitted expenses by storing them in an array within the JSON file.
To fix this, update your server logic so that it reads the existing array of expenses from db/expense.json, appends the new expense to this array, and then writes the updated array back to the file. This ensures that all expenses are retained, not just the most recent one. You can refer to the Node.js fs module documentation for help with reading and writing files.
I'm approving your pull request to help you move forward with your learning process, but please make sure to address this critical issue in your next iteration. Fixing this will ensure your solution fully meets the requirements and will help you avoid similar problems in the future. Keep up the good work, and don't hesitate to ask questions or seek feedback as you continue improving your code! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return; | ||
| } | ||
|
|
||
| fs.writeFileSync(filePath, JSON.stringify(expense, null, 2)); |
There was a problem hiding this comment.
Issue: The current logic overwrites the entire expense.json file with only the latest expense, instead of appending the new expense to a list. According to the requirements, all submitted expenses should be preserved in an array. Please modify this section to read the existing expenses, append the new one, and then write the updated array back to the file.
2 participants
No description provided.