Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

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

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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/articles/_posts/2009-03-06-javascript-best-practices.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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).

for(var i = 0; i < o.members.length; i++) {
var ul = document.createElement('ul');
var li = document.createElement('li');
li.appendChild(document.createTextNode(o.members[i].name));
var nestedul = document.createElement('ul');
Expand All @@ -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 •
edited
Loading

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.

}
out.appendChild(ul);
}
Expand All @@ -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.

for(var i = 0; i < o.members.length; i++) {
var ul = document.createElement('ul');
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].data));
Copy link
Author

@UberKluger UberKluger Aug 7, 2021 •
edited
Loading

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.

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.

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

@UberKluger UberKluger Aug 7, 2021 •
edited
Loading

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.

var ul = document.createElement('ul');
for(var i = 0; i < member.data.length; i++) {
for(var i = 0; i < data.length; i++) {
var li = document.createElement('li');
li.appendChild(
document.createTextNode(
member.data[i].label + ' ' +
member.data[i].value
data[i].label + ' ' +
data[i].value
)
);
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.

}
ul.appendChild(li);
return ul;
}

Expand Down