Warning fix, and normalization

[Itee]

Hello there,

This is a little contrib for fixing lot of minor warning, and normalize some code part.

I add some semi-colons at line end, remove some comma that can be dangerous, or even create undefined behaviors. It toke the bet to refactor (mean normalize) some if/else statement accordingly to what i saw in other code part.

That mean, i turn:

if(foo){ return true; } return false;

to

return (foo);

Finally, i fix some implicit variable declaration.

I hope this help, best regards.
Tristan

[mrdoob]

I would prefer...

return ( point.x < this.min.x || point.x > this.max.x || point.y < this.min.y || point.y > this.max.y || point.z < this.min.z || point.z > this.max.z ) === false;

[Itee]

Thanks for your reactivity !
I will implement it in few minutes, but just before do this, a little question.

Why would you keep === false at the end of the line ?
For readability ? Because with:

return !( point.x < this.min.x || point.x > this.max.x || point.y < this.min.y || point.y > this.max.y || point.z < this.min.z || point.z > this.max.z );

You know immediately that the statement must be the inverse to be true. And finally, the negate statement (!) is really alot faster than using the comparison with false... Almost 50-60% faster.

You could run this bench:

var pointA     = {
    x: 5,
    y: 7,
    z: 1
};
var pointB     = {
    x: 98,
    y: 15,
    z: 7
};
var iterations = 10000000;

function comparisonFunction() {
    return ( pointA.x < pointB.x || pointA.x > pointB.x || pointA.y < pointB.y || pointA.y > pointB.y || pointA.z < pointB.z || pointA.z > pointB.z ) === false;
}

function negationFunction() {
    return !( pointA.x < pointB.x || pointA.x > pointB.x || pointA.y < pointB.y || pointA.y > pointB.y || pointA.z < pointB.z || pointA.z > pointB.z );
}

for (var a = 0, numberOfBench = 10; a < numberOfBench; ++a) {
    var t0 = performance.now();
    for (var i = 0; i < iterations; ++i) {
        comparisonFunction()
    }
    var t1 = performance.now();
    console.log("Call to comparisonFunction took " + (t1 - t0) + " milliseconds.");

    var t2 = performance.now();
    for (var j = 0; j < iterations; ++j) {
        negationFunction()
    }
    var t3 = performance.now();
    console.log("Call to negationFunction took " + (t3 - t2) + " milliseconds.");

    console.log(" ");
}

But the goal of this question is more to be sure that in future you REALLY WANT this type of statement with comparison operator, or if you prefer performance while keeping the readability with negate statement ?

Best regards,
Tristan

[Itee]

The question is still pending, but it is done with fcf6239.

[mrdoob]

@tschw looks good?

[Itee]

Just a little bit explanation.

This for loop could be writtin like:

for(var _ in bindingByName){  // Iterate over object properties
    break;                    // Break loop immediatly... so does nothing
}
remove_empty_map;             // Does nothing too

but instead of comment this line, i could totally remove it ! Or maybe it's an obscure voodoo javascript trick that i don't know yet.. ?

[mrdoob]

@tschw knows a lot of obscure voodoo javascript tricks 😁

[Itee]

Ok i understand what's happen in this pretty piece of code, now !
I had never eared about javascript labeled statement...

So i will amend this commit as soon as possible !
And i make some bench on it versus this conditional code that do the same (notion):

    if(Object.keys(obj).length === 0 && obj.constructor === Object) {
        // delete
    }

using labeled is 50% faster due to break on first iteration ! Really powerfull stuff !!!

[Itee]

Reverted at 9b43b7f.

[mrdoob]

@tschw looks good?

[Itee]

More explanation about context of this.

There is a private variable call _loopCount at line 51:

this._loopCount = -1;

and a local loopCount variable inside _updateTime method that take the _loopCount value at initialization. This could be provoc undefined behavior, or to contrary fix a not detected bug. (I don't analyse in deep this code yet). But the private _loopCount variable is used only 5 times, and just in the _updateTime and reset (to his original value of -1) methods... So...

[tschw]

Yes, looks like a bugfix. Currently, in case of LoopOnce, the actual loop count will not be updated to 0 as I originally intended.

[WestLangley]

Nice improvements. Have a look at http://zz85.github.io/mrdoobapproves/ so you get familiar with the formatting style.

@mrdoob BTW, Is formatting not a big deal anymore? Do you autocorrect the code yourself?

[mrdoob]

@WestLangley I never stressed too much about it 😅

[Itee]

This is exactly what i'm looking for !

@mrdoob What about JSLint and JSCS implementation in three.js compilation chain ???

Nota: care about JSCS 3.0 will be the last version of JSCS. See here

[Itee]

I have just to see the "How to contribute to three.js", and read about:

Once done with a patch / feature do not add more commits to a feature branch (pull requests are not repository state snapshots, any change you do in that branch will be included in the pull request).

So... @mrdoob this PR is still ok (for this first and last time) ? Or would you a new PR including all commit ? And what about the bench of Box3 ???

See you soon,
Tristan

[Itee]

@mrdoob What is missing, need to be improve, to be pull ?

[Itee]

I will resend a PR due to naming collision... The new name will be src_warning_fix !
Sorry for disagreement...