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

Cache array length in ol.control.Attribution #636

Merged
merged 1 commit into from
Apr 22, 2013

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Apr 22, 2013

No description provided.

@fredj
Copy link
Member

fredj commented Apr 22, 2013

LGTM

@@ -84,7 +84,7 @@ ol.control.Attribution.prototype.getTileSourceAttributions =
continue;
}
tileRanges = usedTiles[tileSourceKey];
for (i = 0; i < tileSourceAttributions.length; ++i) {
for (i = 0, ii = tileSourceAttributions.length; i < ii; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

I've been wondering about this construct, i.e. separating variable assignments with comas without a heading var keyword. It turns out that this is using the JavaScript's Comma Operator, which is fine. But it is to be noted that:

var i = 1, ii = 10;

is not the same as:

var i, ii;
i = 1, i = 10;

In the second code block, i = 1, i = 10 is an actual expression that returns the value of the second operand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! Thanks.

For me, jslint complains about for (var i = 1, ii = 10; ...) so I've moved the var declarations out of the for statement.

Copy link
Member

Choose a reason for hiding this comment

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

Same for me. So I also do like that var declarations are out of for statements.

Also, on a related note. I like

var someVarName0 = 0;
var someVarName1 = 1;
var someVarName2 = 2;
var someVarName3 = 3;
var someVarName4 = 4;

I find this ok:

var a, b, c, d;

But I dislike:

var someVarName0 = 0,
      someVarName1 = 1,
      someVarName2 = 2,
      someVarName3 = 3,
      someVarName4 = 4;

Just a matter of taste, but I find the last construct less readable, especially when combining declarations of initialized and uninitialized variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding var declarations, I have exactly the same tastes as you :)

@elemoine
Copy link
Member

+1 from me. Thanks Tom.

twpayne added a commit that referenced this pull request Apr 22, 2013
Cache array length in ol.control.Attribution
@twpayne twpayne merged commit bc3dc0f into openlayers:master Apr 22, 2013
@twpayne twpayne deleted the cache-length branch April 22, 2013 08:34
afabiani pushed a commit to geosolutions-it/openlayers that referenced this pull request May 22, 2017
introduce new property on the Map called autoUpdateSize (r=@ahocevar)
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.

3 participants