Skip to content

final code for review#140

Open
nikalaiii wants to merge 4 commits intomate-academy:masterfrom
nikalaiii:develop
Open

final code for review#140
nikalaiii wants to merge 4 commits intomate-academy:masterfrom
nikalaiii:develop

Conversation

@nikalaiii
Copy link
Copy Markdown

No description provided.

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

Hi! 👋 Thanks for your submission! The server is serving the HTML form and handling the POST request correctly. However, the current implementation overwrites the expense.json file with each new expense, which means only the last expense is stored. To fix this, please modify the code to read the existing expense data, append the new expense to it, and then write the updated array back to the file. Also, src/request.js is not required for the task. You've got this! 👍


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/createServer.js Outdated
Comment on lines +54 to +55

expenseData = { ...parsedData };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The current implementation overwrites the entire expenseData object with the new parsedData. To store multiple expenses, you should read the existing data from expense.json (if any), append the new expense to it, and then write the updated array back to the file. This is essential for accumulating expense records instead of just keeping the last one.

Consider changing this line to something like:
expenseData = Array.isArray(expenseData) ? [...expenseData, parsedData] : [parsedData];
to ensure you're appending to an array (or creating one if it doesn't exist).

Comment thread src/createServer.js

expenseData = { ...parsedData };

fs.writeFileSync(expensePath, JSON.stringify(expenseData, null, 2));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ensure that you are writing an array to the expense.json file. If expenseData is not an array, the data might not be stored as expected.

Consider changing this line to:
fs.writeFileSync(expensePath, JSON.stringify(Array.isArray(expenseData) ? expenseData : [expenseData], null, 2));

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 work on this pull request! 🎉 The code is well-structured and aligns with the task description. The server logic handles GET and POST requests correctly, with error handling in place. The HTML form is also well-structured with necessary fields and the JavaScript correctly sends the form data to the server. src/request.js is not needed, as expected.

Here's a summary of the review:

  • The code now correctly reads existing expenses from expense.json, appends the new expense, and writes the updated data back to the file.
  • The HTML form includes the necessary fields (date, title, and amount).
  • JavaScript code correctly handles form submission, prevents the default action, and sends data via POST. It handles the server response with an alert.
  • src/request.js is not required and can be removed.

Since the solution aligns with the task description and addresses all key functionalities, I'm approving this pull request to help you progress in your learning. Keep up the good work! 👍 Please do review the comments to understand where you can improve your code.


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

Footnotes

  1. Rate AI review example

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