Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update 2009-03-06-javascript-best-practices.md #635

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

UberKluger
Copy link

In the "Avoid nested loops" section:
Fixed one typo
Fixed several semantic errors in the code
(see specific line Pull comments, this might not work, first time)

@@ -522,8 +522,8 @@ The other problem of nesting is variable names and loops. As you normally start

function renderProfiles(o) {
var out = document.getElementById('profiles');
var ul = document.createElement('ul');
Copy link
Author

@UberKluger UberKluger Aug 7, 2021

Choose a reason for hiding this comment

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

Moved assignment of ul out of loop so not reassigned for every element of o.members accessed (thereby throwing away all but the last into the garbage collector).

@@ -538,6 +538,7 @@ The other problem of nesting is variable names and loops. As you normally start
nestedul.appendChild(datali);
}
li.appendChild(nestedul);
ul.appendChild(li);
Copy link
Author

@UberKluger UberKluger Aug 7, 2021

Choose a reason for hiding this comment

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

Previously had no attachment of the generated li node to the 'enclosing' ul node.

@@ -546,26 +547,27 @@ As I am using the generic — really throw-away — variable names `ul` and `li`

function renderProfiles(o) {
var out = document.getElementById('profiles');
var ul = document.createElement('ul');
Copy link
Author

Choose a reason for hiding this comment

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

Same as line 525. ul node only created once.

var li = document.createElement('li');
li.appendChild(document.createTextNode(data.members[i].name));
li.appendChild(addMemberData(o.members[i]));
li.appendChild(document.createTextNode(o.members[i].name));
Copy link
Author

Choose a reason for hiding this comment

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

Typo: data.members -> o.members

li.appendChild(addMemberData(o.members[i]));
li.appendChild(document.createTextNode(o.members[i].name));
li.appendChild(addMemberData(o.members[i].data));
ul.appendChild(li);
Copy link
Author

Choose a reason for hiding this comment

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

Same as 541, attach new li node to enclosing ul node.

li.appendChild(document.createTextNode(data.members[i].name));
li.appendChild(addMemberData(o.members[i]));
li.appendChild(document.createTextNode(o.members[i].name));
li.appendChild(addMemberData(o.members[i].data));
Copy link
Author

@UberKluger UberKluger Aug 7, 2021

Choose a reason for hiding this comment

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

Information hiding and access optimisation (q.v. avoid excessive member access implied by Optimize loops).
Only the data member is used within addMemberData so don't supply whole member object and repeatedly access the data member, just supply the data member itself.

}
out.appendChild(ul);
}
function addMemberData(member) {
function addMemberData(data) {
Copy link
Author

@UberKluger UberKluger Aug 7, 2021

Choose a reason for hiding this comment

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

Changes to reflect that only the data member is being passed now.
All uses of member.data changed to just data.

Note: this data is not the "data" member of o.members[i] but a function parameter named "data" that just happens to have been assigned the "data" member of o.members[i] in the function call. This is a subtle but important distinction that could confuse novices. Maybe I should have chosen a completely different name rather than trying to minimise the variations from the original, e.g. "theData" or "dataToAdd".

Actually, on the subject of better names, perhaps the function name "addMemberData" is a bit vague. Add to what? It isn't a member function so there is no invoking object to give context. Perhaps a better name would be "buildMemberDataList" since this is what it actually does. Again, left it as is to minimise changes.

)
);
ul.appendChild(li);
Copy link
Author

Choose a reason for hiding this comment

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

Having ul.appendChild(li); outside the loop was similar to reassigning ul for every iteration. Only the last generated li node was being connected, all others ending up in the garbage collector.

Copy link
Author

@UberKluger UberKluger left a comment

Choose a reason for hiding this comment

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

Change specific comments (explanations) added.

@UberKluger UberKluger marked this pull request as ready for review August 7, 2021 18:39
@nowaktz nowaktz force-pushed the master branch 2 times, most recently from c460a25 to c89a560 Compare February 7, 2022 15:43
In the "Avoid nested loops" section:
Fixed one typo
Fixed several semantic errors in the code (see Pull comments)
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.

None yet

1 participant