Skip to content

Comments

add task solution#1693

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

add task solution#1693
IvanovvvIhor wants to merge 3 commits intomate-academy:masterfrom
IvanovvvIhor:develop

Conversation

@IvanovvvIhor
Copy link

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

This requires a few changes before it can be approved. You have the right idea with the recursive structure, but there are a few critical logic errors that prevent the code from running and producing the correct output.

Here are the main points to fix:

  • Incorrect Loop Type: The for...of loop is for iterables like arrays, not objects, and will cause a TypeError. To iterate over an object's keys, you should use a for...in loop.
  • Flawed Recursive Condition: Your if statement checks if the key is an object, but the key is always a string. The check for recursion should be on the value, data[key], to see if it's a nested object.
  • Elements Not Appended: The <li> elements are created in the loop but are never attached to the <ul> element. You need to append each li to the ul inside the loop.

✨ 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

// WRITE YOUR CODE HERE
const ul = document.createElement('ul');

for (const key of data) {

Choose a reason for hiding this comment

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

The for...of statement is designed for iterating over iterable objects like Arrays or Strings. A plain Object like data is not iterable by default, and this will cause a runtime error. To loop through an object's keys, you should consider using a for...in loop or Object.keys().


li.textContent = key;

if (key !== null && key.constructor === Object) {

Choose a reason for hiding this comment

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

This condition is checking if key is an object. However, key is a string representing the property name. The check for recursion should be on the value of the property, which is data[key]. You need to verify if data[key] is an object that contains other properties to decide whether to make a recursive call.

Comment on lines 28 to 35
const li = document.createElement('li');

li.textContent = key;

if (key !== null && key.constructor === Object) {
createTree(li, data[key]);
}
}

Choose a reason for hiding this comment

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

You are creating li elements inside the loop, but they are not being added to the ul element. Each li needs to be appended to the ul for it to appear in the list. This should happen within the loop.

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 iteration! You've successfully addressed the previous issues, and the core recursive logic is now correct. However, some changes are needed as there's one remaining edge case: the function creates an empty <ul> for items that don't have any sub-items (like "Wine"). This results in incorrect HTML markup. A <ul> should only be created if the object it represents is not empty. You can fix this by adding a check at the start of your function to see if the data object has any keys before creating the list.


✨ 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


function createTree(element, data) {
// WRITE YOUR CODE HERE
const ul = document.createElement('ul');

Choose a reason for hiding this comment

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

This is a good start, but there's a small issue with this approach. When the function is called with an empty data object (which happens for leaf nodes like "Wine"), it still creates and appends an empty <ul> element. This results in unnecessary <ul></ul> tags in your final HTML.

To fix this, consider adding a base case at the start of your function. If the data object has no keys, the function should simply return without creating any elements.

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 iteration! I am approving your solution. You've successfully addressed the previous feedback by adding a check to prevent the creation of empty <ul> elements. The recursive logic is solid, and the code now correctly handles all cases specified in the requirements. Keep up the great work!


✨ 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