Update 2009-03-06-javascript-best-practices.md #635
Conversation
| | ||
| function renderProfiles(o) { | ||
| var out = document.getElementById('profiles'); | ||
| var ul = document.createElement('ul'); |
There was a problem hiding this comment.
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).
| nestedul.appendChild(datali); | ||
| } | ||
| li.appendChild(nestedul); | ||
| ul.appendChild(li); |
There was a problem hiding this comment.
Previously had no attachment of the generated li node to the 'enclosing' ul node.
| | ||
| function renderProfiles(o) { | ||
| var out = document.getElementById('profiles'); | ||
| var ul = document.createElement('ul'); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
| data[i].value | ||
| ) | ||
| ); | ||
| ul.appendChild(li); |
There was a problem hiding this comment.
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.
UberKluger left a comment
UberKluger left a comment There was a problem hiding this comment.
Change specific comments (explanations) added.
nowaktz force-pushed the master branch 2 times, most recently from c460a25 to c89a560 Compare In the "Avoid nested loops" section: Fixed one typo Fixed several semantic errors in the code (see Pull comments)
1 participant
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)