Skip to content

Commit

Permalink
Consolidate Array.isArray calls in MutableEvent setters
Browse files Browse the repository at this point in the history
  • Loading branch information
@tejaede
tejaede committed Jan 5, 2019
1 parent 0c38e12 commit 5711bf1
Showing 1 changed file with 6 additions and 4 deletions.
10 changes: 6 additions & 4 deletions core/event/mutable-event.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,10 @@ if (typeof window !== "undefined") {
return this._event.touches;
},
set: function (value) {
if (Array.isArray(value) && this._event.touches) {
var isArray = Array.isArray(value);
if (isArray && this._event.touches) {
this._event.touches.splice.apply(this._event.touches, [0, Infinity].concat(value));
} else if (Array.isArray(value)) {
} else if (isArray) {
this._event.touches = value;
}
}
Expand All @@ -254,9 +255,10 @@ if (typeof window !== "undefined") {
return this._event.changedTouches;
},
set: function (value) {
if (Array.isArray(value) && this._event.changedTouches) {
var isArray = Array.isArray(value);
if (isArray && this._event.changedTouches) {
this._event.changedTouches.splice.apply(this._event.changedTouches, [0, Infinity].concat(value));
} else if (Array.isArray(value)) {
} else if (isArray) {
this._event.changedTouches = value;
}
}
Expand Down

4 comments on commit 5711bf1

@marchant
Copy link
Member

@marchant marchant commented on 5711bf1 Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@tejaede why not:
if(Array.isArray(value)) {
if(this._event.touches) {
this._event.touches.splice.apply(this._event.touches, [0, Infinity].concat(value));
}
else {
this._event.touches = value;
}

}

@tejaede
Copy link
Collaborator Author

@tejaede tejaede commented on 5711bf1 Jan 5, 2019

Choose a reason for hiding this comment

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

Although the blocks in this case are small enough that readability is not a concern, I find chained if/else statements are more readable than nested ifs as the statement(s) grows. So all things being equal, I'll go with if/else over nested ifs as a standard practice.

@marchant
Copy link
Member

Choose a reason for hiding this comment

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

All things are not equal here, events, especially touch, can be high frequency, and it's not worth creating a local var to be looked up only twice, especially in such a simple case. Even creating a new [0, Infinity] every time for the sake of calling concat on it should be avoided, it all adds to garbage collection's duty, and related performance consequences

@johnnykahalawai
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose there is nothing in Montage code that will use the setter? So perhaps we should remove the array check and splice apply altogether?

I do not see anything in the leaflet code that expects this.

Please sign in to comment.