ProgerXP / FileDrop

Self-contained cross-browser pure JavaScript class for Drag & Drop and AJAX (multi) file upload.
filedropjs.org
The Unlicense
264 stars 61 forks source link

Assign the correct XHR method to NewXHR rather than re-testing each time it's invoked #1

Closed barneycarroll closed 11 years ago

barneycarroll commented 11 years ago

NewXHR's initial assignment is now to a self executing function which invokes the same tests, but assigns the result to a variable instead of immediately returning it. Instead it, returns a function which calls that variable. This way the test runs immediately and doesn't need to be executed multiple times.

ProgerXP commented 11 years ago

This won't work: NewXHR not just determines available method but creates a new XHR object. Your version, however, creates a singleton XHR object and returns it on each NewXHR call. In other words the instance is created once and then shared among the calls. This will break things in more ways than one.

Moreover, I don't see a point in optimizing this function in any way because in modern browsers (actually, starting with IE 6, i.e. for the last decade) its execution is already as quick as possible running a single instruction: return new XMLHttpRequest;

I'm closing this pull unless you have more ideas.

barneycarroll commented 11 years ago

Woops. Yeah, talk about premature optimisation ;)

I fixed the fallacious constructor invocation (with a commit --amend — @9d2ad3bed583cde1a0f9acd65ea6725a25c3a196 — don't understand why that hasn't pulled through to this pull request) … still saves iteration in old old old IE but yeah, you're right: kind of the point was to avoid repeatedly invoking a try/catch (I thought there was a notable performance hit there; turns out I was wrong).

Still, new version avoids repeated query… Good practice but not that significant an improvement I suppose.

ProgerXP commented 11 years ago

Still, new version avoids repeated query… Good practice but not that significant an improvement I suppose.

You're right. It makes the code more complex and in this particular case it's optimizing for IE 6 - so I will leave current version.