Code review comment for lp:~edwin-grubbs/launchpad/bug-399554-timeline-improvements

Revision history for this message
Gavin Panella (allenap) wrote :

> Line 133 character 9: The body of a for in should be wrapped in an if
> statement to filter unwanted properties from the prototype.
> for (i in this.series.landmarks) {

The error above is not very helpful, but the MDC does recommend against using for...in loops on Arrays:

  "Although it may be tempting to use this as a way to iterate over an
   Array, this is a bad idea. The for...in statement iterates over
   user-defined properties in addition to the array elements"

    -- https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Statements/for...in#Description

I don't think you've added any properties to these arrays, but I think it's better to code defensively here. It'll also shut jslint up ;)

What do you think?

« Back to merge proposal