Fixed JavaScript code to follow the coding style

[ariya]

[email protected] commented:

We should use 4-space indentation.

Also, the code should pass JSLint checks.

Disclaimer:
This issue was migrated on 2013-03-15 from the project's former issue tracker on Google Code, Issue #612.
🌟   4 people had starred this issue at the time of migration.

[detro]

[email protected] commented:

So +1.

[jgonera]

[email protected] commented:

How about JSHint? JSLint sometimes seems too pedantic (though I know this is controversial).

[detro]

[email protected] commented:

Optimisation like "putting var declarations at the top of the function" are
some of the diff between JSLint and JSHint.

Crockford is right not because anything changes in the execution.
He is right because it's better to put the variable where the interpreter
will create it anyway: in the long run, when code grows, it will pay.
AND it makes execution flow clear EVEN to people that have no idea what
variable hoisting is.

But I'm well aware that doing this I might just have started a flame...

[jgonera]

[email protected] commented:

I have yet to see a real case when variable hoisting got someone into trouble. I tend to think that functions should be short enough so that no matter where you declare the variable, you still know how it'll work. I tried to find some examples of dangerous hoisting, but the most popular I found was about someone mistakingly blaming hoisting for an error that was a result of a missing closure.

I also think that

var i;
for (i=0; i < sth; i += 1) {}

is uglier (unnecessary long) than

for (var i=0; i < sth; ++i) {}

And I can't understand why

var a = someObject.getSomething();
var b = someObject.getSomethingElse();

is worse than

var a = someObject.getSomething(),
b = someObject.getSomethingElse();

(assuming that in both cases those lines are at the beginning of the function).

Yet this is what JSLint is trying to force. I do agree that most things that JSLint checks should be checked, but I do not think that only Douglas Crockford knows how to write JS.

As an interesting example, look at npm's (Node's package manager) code:
https://github.com/isaacs/npm

It's author uses the dreaded semicolon insertion feature of JS (Twitter's Bootstrap is another popular example of such code).
JSLint stops scanning npm.js at 5% claiming that there are too many errors, but still npm's author doesn't seem to drown in bugs (at least I've never had problems with npm).

I'm not saying that PhantomJS should not use semicolons in its JS code, I'm just trying to point out that there are many ways of writing good JS code ;)

That's all from me, I also don't want any flamewars.

[jgonera]

[email protected] commented:

BTW, even pasting current https://raw.github.com/ariya/phantomjs/master/src/bootstrap.js in JSLint already returns lots of errors.

[detro]

[email protected] commented:

Yep, and I think that should be fixed.
Been linking that multiple times. Will do again.

[detro]

[email protected] commented:

As I see it, we have 2 ways to go:

• either we define a style and apply it everywhere (JSHint is configurable as far as I know, and we could set it up as we like)
• or we accept a "common and tested" standard and stick with it

JSLint is already there. We don't need to spend time arguing and just adopt that.
If we were working full time on PhantomJS, maybe I'd vote for properly define a standard. But given we struggle even to find the time to make the binaries (for which people cry about), Crockford is a very good "compromise" to settle for.

No?

[jgonera]

[email protected] commented:

I'd still prefer JSHint, it's just more flexible and I like the fact that it's community-driven. But it's obviously not up to me to decide.

No matter if we choose JSHint or JSLint, we can't run them with default options (e.g. we should permit ECMAScript 5 because we know the JS engine we're writing for and shouldn't limit ourselves only to old JS features).

[detro]

[email protected] commented:

I have had a very good 12h time to think about where this project is going.
And I believe I need to re-think my position: given the wide spread of JSHint in the industry (mainly because it's maintained project), I think we should focus on JSHint.

But this needs someone to work on creating the "configuration" for JSHint.

I'm sure I can control myself and have a constructive discussion about this here :)

[ariya]

[email protected] commented:

 

 
Metadata Updates

• Milestone updated: Release1.8 (was: Release1.7)

[JamesMGreene]

[email protected] commented:

To throw my opinion into the mix: JSHint is actively maintained, substantially more configurable, and is gaining few features all the time (e.g. max depth, max complexity, max parameter count, etc.).

[detro]

[email protected] commented:

I agree.
Since last time I was here I changed my opinion.
JSHint is the way to go forward.

If anyone (James?) wants to lead the activity to define the config file for JSHint... ;)

[ariya]

[email protected] commented:

Rescheduled to 1.9.

 
Metadata Updates

• Milestone updated: Release1.9 (was: Release1.8)

[stale]

Due to our very limited maintenance capacity, we need to prioritize our development focus on other tasks. Therefore, this issue will be automatically closed (see #15395 for more details). In the future, if we see the need to attend to this issue again, then it will be reopened. Thank you for your contribution!