Skip to content

solution#170

Open
mForkosh wants to merge 3 commits intomate-academy:masterfrom
mForkosh:develop
Open

solution#170
mForkosh wants to merge 3 commits intomate-academy:masterfrom
mForkosh:develop

Conversation

@mForkosh
Copy link

No description provided.

Copy link

@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 job setting up the server and handling the form submission! However, this solution requires a couple of changes to meet the core requirements.

First, the current implementation overwrites db/expense.json on every submission, losing all previous data. The goal is to store a list of expenses, so you'll need to read the existing file content, add the new expense to an array, and then write the entire updated array back to the file.

Second, the server responds to the POST request with raw JSON. The requirement is to return an HTML page that displays the well-formatted JSON. To fix this, please change the Content-Type header to text/html and wrap the JSON string within HTML tags, like <pre>, to render it correctly in the browser.


✨ 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

const jsonData = JSON.stringify(dataObject);

try {
await fs.writeFile(pathToDb, jsonData);

Choose a reason for hiding this comment

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

This implementation overwrites the expense.json file with every new submission. The task implies storing expenses, which usually means maintaining a list. Consider reading the file first, adding the new expense to an array, and then writing the updated array back. You'll also need to handle the case where the file doesn't exist or is empty.

Comment on lines 135 to 136
res.setHeader('Content-Type', 'application/json');
res.end(jsonData);

Choose a reason for hiding this comment

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

The task requires returning an 'HTML page with well formatted JSON'. Currently, the response Content-Type is set to application/json. To meet the requirement, you should set the Content-Type to text/html and wrap the JSON string in HTML tags (e.g., inside a <pre> tag) to display it on a page.

Copy link

@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 job addressing the previous feedback by appending expenses and returning an HTML page! This is a big step forward.

However, I am requesting changes because of a critical issue: the server fails to save the very first expense if the db/expense.json file is empty or doesn't exist. Your try...catch block should handle this specific scenario gracefully by initializing the expenses list as an empty array ([]) instead of throwing a generic server error.

As a smaller improvement, to meet the "well-formatted JSON" requirement, please add indentation to the JSON output. You can do this easily with JSON.stringify(yourData, null, 2). You're very close to the finish line!


✨ 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


let expensesList;

try {

Choose a reason for hiding this comment

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

This try...catch block needs to differentiate between error types. If fs.readFile fails because the file doesn't exist yet or is empty (which causes JSON.parse to fail), it shouldn't be treated as a generic server error. In these expected cases, you should initialize expensesList as an empty array ([]) to allow the first expense to be saved.

expensesList = JSON.parse(await fs.readFile(pathToDb));

if (!Array.isArray(expensesList)) {
expensesList = JSON.stringify([dataObject]);

Choose a reason for hiding this comment

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

To meet the "well formatted JSON" requirement, the JSON string should be indented for readability. You can achieve this by providing additional arguments to JSON.stringify, for example: JSON.stringify([dataObject], null, 2).

} else {
const copyList = [...expensesList, dataObject];

expensesList = JSON.stringify(copyList);

Choose a reason for hiding this comment

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

To produce "well-formatted JSON" as required, consider adding indentation when stringifying the data. You can do this with JSON.stringify(copyList, null, 2) to make the output more readable.

Copy link

@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

Excellent work on this submission! I am approving it so you can continue to the next stage, but I want to point out one important detail for you to review.

You did a great job handling cases where expense.json might be missing and ensuring the output is well-formatted. There is, however, a subtle logical issue in your error handling in src/createServer.js. When fs.readFile fails, your .catch() block assigns a value but doesn't return it, causing the readedData variable to become undefined. The code only works by chance because a later try...catch handles the resulting error.

A more robust implementation would be to return the default value from the catch block directly:

let readedData = await fs.readFile(pathToDb).catch(() => '[]');

This ensures your asynchronous code is more predictable. You're very close to a perfect solution, keep up the great effort!


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

Footnotes

  1. Rate AI review example

Comment on lines +137 to +139
let readedData = await fs.readFile(pathToDb).catch((_e) => {
readedData = `[]`;
});

Choose a reason for hiding this comment

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

There's a subtle issue with how this .catch() is used. The arrow function (_e) => { readedData = '[]'; } doesn't return a value, so if fs.readFile fails, readedData will be assigned undefined.

The code only works by chance because the subsequent JSON.parse(undefined) throws an error that is caught later. To make the code work as intended, you should return the default value from the catch block.

A cleaner implementation would be:

let readedData = await fs.readFile(pathToDb).catch(() => '[]');

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