Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up the server to handle POST requests! However, I'm requesting a few changes to meet all the project requirements.
Here are the key areas to focus on:
- Missing HTML Form: The server needs to handle
GETrequests to serve the HTML form for submitting expenses. - Data Overwriting: Currently,
expense.jsonis overwritten with each submission. You'll need to read the existing data, append the new expense, and then write the updated array back to the file. - Incorrect Response: The server should return an HTML page containing the JSON, not raw JSON. Please set the
Content-Typetotext/html. - Error Handling: The server crashes on invalid JSON. Please add a
try...catchblock aroundJSON.parse()to handle this and return a400error.
Addressing these points will make your application fully functional. Keep up the great work!
✨ 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
src/createServer.js
Outdated
|
|
||
| req.on('end', () => { | ||
| const json = Buffer.concat(bodyChunks); | ||
| const expense = JSON.parse(json); |
There was a problem hiding this comment.
This line will throw an error and crash the application if the request body contains invalid JSON. You should wrap JSON.parse() in a try...catch block to handle this case and send a 400 Bad Request response.
src/createServer.js
Outdated
| const fileStream = fs.createWriteStream( | ||
| path.normalize(path.join(__dirname, '../db/expense.json')), |
There was a problem hiding this comment.
fs.createWriteStream overwrites the file on each request, so only the last expense will be saved. To store all expenses, you should first read the existing content of expense.json, parse it into an array, add the new expense, and then write the entire updated array back to the file.
src/createServer.js
Outdated
| res.setHeader('Content-Type', 'application/json'); | ||
| res.end(JSON.stringify(expense, null, 2)); |
There was a problem hiding this comment.
The task requires returning an 'HTML page with well formatted JSON'. This implementation returns a response with Content-Type: application/json. You should set the Content-Type to text/html and wrap the JSON string in HTML, for example, using <pre> tags.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job adding the GET route to serve the HTML form! That's a solid step forward. However, this requires changes as there are a few critical issues that prevent the application from meeting all the requirements.
Here’s what needs to be fixed:
- Data is being overwritten: Currently, each new expense overwrites the
expense.jsonfile. You need to first read the existing content of the file, parse it into an array, push the new expense object into that array, and then write the entire updated array back to the file. - Incorrect POST response: The server should respond to a successful POST request with an HTML page that displays the saved JSON. Right now, it's sending raw JSON. Please set the
Content-Typeheader totext/htmland wrap the JSON string in an HTML tag like<pre>for proper formatting. - Server crashes on invalid JSON: The
JSON.parse()call is not handled for errors. If the request body contains malformed JSON, the server will crash. Please wrap this call in atry...catchblock and send a400 Bad Requestresponse if parsing fails.
You're very close to a complete solution. Keep up the great work!
✨ 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
src/createServer.js
Outdated
|
|
||
| req.on('end', () => { | ||
| const json = Buffer.concat(bodyChunks); | ||
| const expense = JSON.parse(json); |
There was a problem hiding this comment.
This line can crash the server if the request body contains invalid JSON. It's important to wrap this in a try...catch block to handle potential parsing errors and return a 400 Bad Request response, as mentioned in the previous review.
| return; | ||
| } | ||
|
|
||
| fs.writeFile(dbPath, JSON.stringify(expense, null, 2), (error) => { |
There was a problem hiding this comment.
This line is overwriting the contents of expense.json with only the new expense. The requirement is to append the new data.
To fix this, you should first parse the data (from line 58) which contains the existing expenses, push the new expense into that array, and then write the entire updated array back to the file.
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
The response here should be an HTML page that displays the saved JSON, not raw JSON. The requirements state to "return an HTML page with well formatted JSON".
You'll need to set the Content-Type header to text/html and create an HTML string for the response body, similar to how you did for the GET request. You can use a <pre> tag to display the JSON nicely formatted.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job adding the try...catch block to handle invalid JSON! That's a fantastic step towards making the server more robust. I am approving this submission as an educational override to help you progress, but there are two key requirements to address. First, new expenses are overwriting expense.json instead of being added. You need to parse the existing file content, add the new expense to the array, and then write the updated array back. Second, the response after a successful POST should be an HTML page; please set the Content-Type to text/html and wrap the JSON output in <pre> tags. You're doing great, keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return; | ||
| } | ||
|
|
||
| fs.writeFile(dbPath, JSON.stringify(expense, null, 2), (error) => { |
There was a problem hiding this comment.
You've correctly read the existing file content into the data variable. The next step is to parse this data into an array, add the new expense to it, and then write the entire updated array back to the file. Currently, you are only writing the new expense, which overwrites all existing data.
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
|
|
||
| res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
The response for a successful POST should be an HTML page that displays all the saved expenses. The requirements specify setting the Content-Type to text/html and wrapping the complete JSON data (the entire array of expenses) in <pre> tags.
2 participants
No description provided.