GNOME Bugzilla – Bug 612941
More style issues pointed out by jslint
Last modified: 2011-05-19 16:05:38 UTC
jslint is insistent that you should always do: stuff stuff stuff binary-operator stuff stuff stuff not stuff stuff stuff binary-operator stuff stuff stuff in theory this is because of the threat of semicolon insertion, but semicolon insertion doesn't work that way. (A semicolon can only be inserted in the middle of an expression if there would be a syntax error without it.) I guess there's a threat that you could come up with some (syntactically incorrect) statement such that in the latter case semicolon-insertion would turn it into two syntactically-correct statements (neither of which does what you meant) whereas in the former case it wouldn't... Anyway, I like the operator-at-end-of-line form better than the operator-at-start-of-line form anyway. (Especially for cases where the expression as a whole is not parenthesized.) So I'm in favor of this patch. I think the other changes (parseInt usage, and adding {}s around multi-line bodies) are non-controversial.
Created attachment 156178 [details] [review] More style issues pointed out by jslint Taken from a patch from Lex Hider on the mailing list.
Review of attachment 156178 [details] [review]: For complicated expressions, I do find having the operator on the new line is more readable because it makes the overall structure of the expression clear. I wouldn't say: if (focusedApp == this._focusedApp && ((lastSequence == null && this._activeSequence == null) || (lastSequence != null && this._activeSequence != null && lastSequence.get_id() == this._activeSequence.get_id()))) Is *clear*, but it's more clear to me than: if (focusedApp == this._focusedApp && ((lastSequence == null && this._activeSequence == null) || (lastSequence != null && this._activeSequence != null && lastSequence.get_id() == this._activeSequence.get_id()))) This used to be an area where I deviated from the GNU coding standards ("When you split an expression into multiple lines, split it before an operator, not after one."), but at some point I switched my opinion. However, if we really cared about clarity, we'd probably want to break that up anyways, and it's certainly not a strong opinion on my part, so I'm fine with the change. Rest of the changes look good. The commit message should really list the style points being addressed.
this patch is really out of date now