Skip to content

implemented server#136

Open
oleksandr-kovalchuk wants to merge 4 commits into
mate-academy:masterfrom
oleksandr-kovalchuk:develop
Open

implemented server#136
oleksandr-kovalchuk wants to merge 4 commits into
mate-academy:masterfrom
oleksandr-kovalchuk:develop

Conversation

@oleksandr-kovalchuk
Copy link
Copy Markdown

Form data

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request:

  1. Error Handling: The server currently uses a 404 status code for missing data, which is not appropriate. Please update this to use a 400 status code to indicate a "Bad Request" when the data is missing or incorrect.

  2. Data Persistence: The current implementation of fs.writeFile overwrites the expense.json file with each new request. If the goal is to append new expenses, consider using fs.appendFile or read the existing data, append the new expense, and then write it back.

  3. Amount Field: The amount field in expense.json is set to null. Ensure that the server correctly parses and stores the amount value from incoming requests.

Once these issues are resolved, your code will be in great shape! Keep up the good work, and don't hesitate to reach out if you need any help. 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

Comment thread src/createServer.js Outdated
Comment thread src/createServer.js Outdated
Comment thread src/db/expense.json Outdated
Comment thread src/db/expense.json Outdated
Comment thread src/db/expense.json Outdated
Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great progress on the code! 🎉 You've made some significant improvements, especially in error handling by using a 400 status code for missing fields, which aligns well with best practices. However, there are still a few critical issues to address:

  1. Data Persistence: The current use of fs.writeFile overwrites the expense.json file. To ensure new expenses are appended, consider using fs.appendFile or read the existing data, append the new expense, and then write it back.

  2. Parsing Logic: The parseExpenseData function currently attempts to parse the body as JSON first. Ensure the incoming data format is consistent with this approach. If the data is always in querystring format, this JSON parsing might be unnecessary.

  3. Error Handling: Using a 404 status code for invalid URLs is not appropriate for bad requests. Consider using a 400 status code to indicate a 'Bad Request'.

These changes are crucial for maintaining data integrity and ensuring the server handles requests correctly. Keep up the good work, and make sure to address these issues in future iterations to enhance your learning experience. Remember, every challenge is an opportunity to learn and grow! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Comment thread src/createServer.js Outdated
Comment thread src/createServer.js Outdated
Comment thread src/createServer.js Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants