Cache array length in ol.control.Attribution #636

Merged
merged 1 commit into from Apr 22, 2013

Conversation

Projects
None yet
3 participants
@twpayne
Contributor

twpayne commented Apr 22, 2013

No description provided.

@fredj

This comment has been minimized.

Show comment
Hide comment
@fredj

fredj Apr 22, 2013

Member

LGTM

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

This comment has been minimized.

@elemoine

elemoine Apr 22, 2013

Member

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.

@elemoine

elemoine Apr 22, 2013

Member

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.

This comment has been minimized.

@twpayne

twpayne Apr 22, 2013

Contributor

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.

@twpayne

twpayne Apr 22, 2013

Contributor

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.

This comment has been minimized.

@elemoine

elemoine Apr 22, 2013

Member

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.

@elemoine

elemoine Apr 22, 2013

Member

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.

This comment has been minimized.

@twpayne

twpayne Apr 22, 2013

Contributor

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

@twpayne

twpayne Apr 22, 2013

Contributor

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

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Apr 22, 2013

Member

+1 from me. Thanks Tom.

Member

elemoine commented Apr 22, 2013

+1 from me. Thanks Tom.

twpayne added a commit that referenced this pull request Apr 22, 2013

Merge pull request #636 from twpayne/cache-length
Cache array length in ol.control.Attribution

@twpayne twpayne merged commit bc3dc0f into openlayers:master Apr 22, 2013

1 check passed

default The Travis build passed
Details

@twpayne twpayne deleted the twpayne:cache-length branch Apr 22, 2013

afabiani pushed a commit to geosolutions-it/openlayers that referenced this pull request May 22, 2017

Bart van den Eijnden
Merge pull request #636 from bartvde/autoresize
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