Implement node form data solution#161
Implement node form data solution#161AnnNikiforova wants to merge 4 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
REQUEST_CHANGES
Thank you for your submission! You have the basic server structure in place, but there are several critical issues that must be addressed to meet the assignment requirements:
-
Form Data Parsing
- The POST handler assumes the request body is JSON, but standard HTML forms submit data as
application/x-www-form-urlencoded. As a result, your server will not correctly process form submissions from a regular HTML form. - How to fix: Update your POST handler to parse URL-encoded form data. You can use Node's built-in
querystringmodule or similar logic to parse the body.
- The POST handler assumes the request body is JSON, but standard HTML forms submit data as
-
HTML Response After Saving
- After saving the data, your server responds with raw JSON and sets the content type to
application/json. The requirements specify that you must return an HTML page displaying the saved JSON in a well-formatted way (e.g., inside a<pre>tag). - How to fix: After saving, generate an HTML page that includes the saved JSON inside a
<pre>block, and set the content type totext/html.
- After saving the data, your server responds with raw JSON and sets the content type to
-
File/Directory Existence
- Your code writes to
db/expense.jsonbut does not check if thedbdirectory exists. If it doesn't, the write will fail and the server will throw an error. - How to fix: Before writing, check if the
dbdirectory exists and create it if it doesn't (usingfs.existsSyncandfs.mkdirSync).
- Your code writes to
-
Error Handling for File Operations
- There is no error handling for file operations. If writing fails, the server may crash or the user will not get a helpful response.
- How to fix: Add error handling (try/catch or error callbacks) around file operations, and respond with a user-friendly HTML error message if something goes wrong.
-
Validation of Form Data
- The code only checks that there are 3 keys in the parsed data, but does not check that the required fields (
date,title,amount) are present and valid. - How to fix: Explicitly check for the presence and validity of each required field, and handle missing or invalid data gracefully with a clear error message.
- The code only checks that there are 3 keys in the parsed data, but does not check that the required fields (
Next Steps:
- Address the issues above to ensure your server can handle standard HTML form submissions, save data reliably, and provide the correct user feedback.
- Once you've made these changes, resubmit for review!
You're on the right track—these changes will help you build a more robust and user-friendly application. If you have any questions about how to implement these fixes, feel free to ask!
✨ 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 expensePath = path.resolve(__dirname, '..', 'db/expense.json'); | ||
| const data = Buffer.concat(chunks).toString(); |
There was a problem hiding this comment.
The POST handler assumes the request body is JSON. However, standard HTML forms submit data as application/x-www-form-urlencoded. You need to parse the body accordingly, or ensure the form sends JSON. This is a critical requirement for correct form handling.
src/createServer.js
Outdated
| const expensePath = 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.
The code only checks that there are 3 keys in the parsed data, but does not check that the required fields (date, title, amount) are present and valid. The requirements specify that missing or invalid form data should be handled gracefully.
src/createServer.js
Outdated
| return; | ||
| } | ||
|
|
||
| fs.writeFileSync(expensePath, data); |
There was a problem hiding this comment.
There is no check to ensure the db directory exists before writing the file. If the directory does not exist, this will throw an error. The requirements specify that the app must create the directory/file if they do not exist.
src/createServer.js
Outdated
| return; | ||
| } | ||
|
|
||
| fs.writeFileSync(expensePath, data); |
There was a problem hiding this comment.
There is no error handling for file operations (e.g., if writing fails). The requirements specify that file errors must be handled gracefully, with an appropriate response to the client.
src/createServer.js
Outdated
|
|
||
| fs.writeFileSync(expensePath, data); | ||
| res.statusCode = 200; | ||
| res.setHeader('Content-type', 'application/json'); |
There was a problem hiding this comment.
After saving, the response is sent as raw JSON with content type application/json. The requirements specify that the server must return an HTML page displaying the saved JSON in a well-formatted way (e.g., inside a <pre> tag), not just the JSON itself.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
REQUEST_CHANGES
There are a few critical blockers that must be addressed before this can be approved:
-
Directory Creation Missing
- The code does not check for or create the
dbdirectory before attempting to read or writeexpense.json. This will cause file operations to fail if the directory does not exist, breaking core functionality. You should add logic to check if thedbdirectory exists (usingfs.existsSync) and create it if it doesn't (usingfs.mkdirSync).
- The code does not check for or create the
-
Expense Data Structure Inconsistency
- The logic for updating and saving expenses is inconsistent: sometimes a single object is stored, sometimes an array. The requirements expect all expenses to be stored as an array in
expense.json. Please update your logic so that every new expense is appended to an array, and the file always contains an array of expense objects.
- The logic for updating and saving expenses is inconsistent: sometimes a single object is stored, sometimes an array. The requirements expect all expenses to be stored as an array in
-
Incorrect Response Type After Saving
- After saving an expense, the server currently responds with raw JSON and the content type
application/json. The requirements specify that you must return an HTML page that displays the saved JSON in a well-formatted way (for example, inside a<pre>tag), and set the content type totext/html.
- After saving an expense, the server currently responds with raw JSON and the content type
How to fix:
- Before any file operation, ensure the
dbdirectory exists. - Always read, append, and write expenses as an array.
- After saving, generate an HTML response with the pretty-printed JSON inside a
<pre>tag.
You're on the right track with request handling and validation! Once you address these blockers, your implementation should fully meet the assignment requirements. Let me know if you have any questions or need guidance on these changes.
✨ 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
| return res.end('Invalid data format'); | ||
| } | ||
|
|
||
| const filePath = path.resolve('db', 'expense.json'); |
There was a problem hiding this comment.
You must ensure that the db directory exists before attempting to read or write expense.json. If the directory does not exist, file operations will fail. Add logic to check for and create the directory as needed before this point.
src/createServer.js
Outdated
| let parsedFileData = []; | ||
|
|
||
| if (err) { | ||
| res.writeHead(500, { 'Content-Type': 'text/plain' }); | ||
|
|
||
| return res.end(`Server error: ${err}`); | ||
| } else { | ||
| parsedFileData = JSON.parse(fileData); | ||
|
|
||
| if (typeof parsedFileData === 'object') { | ||
| if (Object.keys(parsedFileData).length) { | ||
| parsedFileData = [parsedFileData]; | ||
| parsedFileData.push(dataObj); | ||
| } else if (!Object.keys(parsedFileData).length) { | ||
| parsedFileData = dataObj; | ||
| } else if (Array.isArray(parsedFileData)) { | ||
| parsedFileData.push(dataObj); | ||
| } |
There was a problem hiding this comment.
The logic for updating parsedFileData is convoluted and may not always result in an array of expenses. The requirements expect the file to contain the latest submitted expense data, and ideally, all expenses should be stored as an array. Simplify this logic to always append new expenses to an array.
src/createServer.js
Outdated
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(newData); |
There was a problem hiding this comment.
After saving, the response is sent as application/json with the raw JSON data. The requirements specify that you must return an HTML page displaying the saved JSON in a well-formatted way (e.g., inside a <pre> tag) and set the content type to text/html. Update this section to generate an HTML response accordingly.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational approval to help you continue progressing in your learning journey! You've done a great job implementing the main server logic, handling form submissions, and managing file operations. Your code now creates the db directory if it doesn't exist and handles basic validation and error cases, which shows solid understanding of Node.js fundamentals.
Key strengths:
- Good use of Node.js core modules for HTTP and filesystem tasks
- Handles missing directories and files gracefully
- Modular and readable structure
Areas to improve for next time:
- Always save an array to
expense.json, even if there's only one expense (lines 87, 90, 93). This ensures data consistency. - Return an HTML page (not just raw JSON) after saving, with the saved data pretty-printed inside a
<pre>tag. Remember to setContent-Type: text/html. - Validate that
amountis numeric (e.g., usingisNaN(Number(amount))) and return a clear error if not. - Consider making error messages more specific, so users know exactly which field is invalid.
These are common challenges when building real-world applications, and addressing them will make your project even stronger. Keep up the great work—you're making excellent progress!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| newDataForFile = dataObj; | ||
| } | ||
| } catch (e) { | ||
| newDataForFile = dataObj; | ||
| } | ||
| } else { | ||
| newDataForFile = dataObj; |
There was a problem hiding this comment.
When the file is empty, invalid, or not an array, the code sets newDataForFile = dataObj; (lines 87, 90, 93). This means a single object will be written to expense.json, but the requirements specify that the file must always contain an array of expense objects. You should always write an array (e.g., [dataObj] or by appending to an existing array).
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(dataObj)); |
There was a problem hiding this comment.
After saving, the response is sent with Content-Type: application/json and the raw JSON data. The requirements specify that you must return an HTML page that displays the saved JSON in a well-formatted way (e.g., inside a <pre> tag), and set the content type to text/html. Please update the response to generate an HTML page with the pretty-printed JSON.
| if (!date || !title || !amount) { | ||
| res.writeHead(400, { 'Content-Type': 'text/plain' }); | ||
|
|
||
| return res.end('Invalid data format'); |
There was a problem hiding this comment.
The code checks for presence of amount, but does not validate that it is numeric. The requirements specify that non-numeric amounts should be handled as invalid input. Consider adding a check like isNaN(Number(amount)) and returning a 400 error if the value is not numeric.
No description provided.