Skip to content

Commit

Permalink
added curly braces around all if/else statements
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeresig
Karl Swedberg authored and jeresig committed Nov 27, 2009
1 parent 1879e8c commit ddb86f8
Show file tree
Hide file tree
Showing 7 changed files with 149 additions and 121 deletions.
20 changes: 10 additions & 10 deletions src/ajax.js
View file
Expand Up @@ -604,19 +604,19 @@ jQuery.extend({

// If an array was passed in, assume that it is an array
// of form elements
if ( jQuery.isArray(a) || a.jquery )
if ( jQuery.isArray(a) || a.jquery ) {
// Serialize the form elements
jQuery.each( a, function() {
add( this.name, this.value );
});

else
} else {
// Encode parameters from object, recursively. If
// jQuery.param.traditional is set, encode the "old" way
// (the way 1.3.2 or older did it)
jQuery.each( a, function buildParams( prefix, obj ) {

if ( jQuery.isArray(obj) )
if ( jQuery.isArray(obj) ) {
jQuery.each( obj, function(i,v){
// Due to rails' limited request param syntax, numeric array
// indices are not supported. To avoid serialization ambiguity
Expand All @@ -626,20 +626,20 @@ jQuery.extend({
add( prefix + ( param_traditional ? "" : "[]" ), v );
});

else if ( typeof obj == "object" )
if ( param_traditional )
} else if ( typeof obj == "object" ) {
if ( param_traditional ) {
add( prefix, obj );

else
} else {
jQuery.each( obj, function(k,v){
buildParams( prefix ? prefix + "[" + k + "]" : k, v );
});

else
}
} else {
add( prefix, obj );

}
});

}
// Return the resulting serialization
return s.join("&").replace(r20, "+");
}
Expand Down
64 changes: 34 additions & 30 deletions src/attributes.js
View file
Expand Up @@ -68,9 +68,9 @@ jQuery.fn.extend({
var elem = this[0];

if ( elem ) {
if( jQuery.nodeName( elem, 'option' ) )
if( jQuery.nodeName( elem, 'option' ) ) {
return (elem.attributes.value || {}).specified ? elem.value : elem.text;

}
// We need to handle select boxes special
if ( jQuery.nodeName( elem, "select" ) ) {
var index = elem.selectedIndex,
Expand All @@ -79,9 +79,9 @@ jQuery.fn.extend({
one = elem.type == "select-one";

// Nothing was selected
if ( index < 0 )
if ( index < 0 ) {
return null;

}
// Loop through all the selected options
for ( var i = one ? index : 0, max = one ? index + 1 : options.length; i < max; i++ ) {
var option = options[ i ];
Expand All @@ -91,9 +91,9 @@ jQuery.fn.extend({
value = jQuery(option).val();

// We don't need an array for one selects
if ( one )
if ( one ) {
return value;

}
// Multi-Selects return an array
values.push( value );
}
Expand All @@ -111,46 +111,50 @@ jQuery.fn.extend({
}

// Typecast once if the value is a number
if ( typeof value === "number" )
if ( typeof value === "number" ) {
value += '';

}
var val = value;

return this.each(function(){
if(jQuery.isFunction(value)) {
val = value.call(this);
// Typecast each time if the value is a Function and the appended
// value is therefore different each time.
if( typeof val === "number" ) val += '';
if( typeof val === "number" ) {
val += '';
}
}

if ( this.nodeType != 1 )
if ( this.nodeType != 1 ) {
return;

if ( jQuery.isArray(val) && /radio|checkbox/.test( this.type ) )
}
if ( jQuery.isArray(val) && /radio|checkbox/.test( this.type ) ) {
this.checked = jQuery.inArray(this.value || this.name, val) >= 0;

}
else if ( jQuery.nodeName( this, "select" ) ) {
var values = jQuery.makeArray(val);

jQuery( "option", this ).each(function(){
this.selected = jQuery.inArray( this.value || this.text, values ) >= 0;
});

if ( !values.length )
if ( !values.length ) {
this.selectedIndex = -1;

} else
}
} else {
this.value = val;
}
});
}
});

jQuery.each({
removeAttr: function( name ) {
jQuery.attr( this, name, "" );
if (this.nodeType == 1)
if (this.nodeType == 1) {
this.removeAttribute( name );
}
},

toggleClass: function( classNames, state ) {
Expand Down Expand Up @@ -182,9 +186,9 @@ jQuery.each({
jQuery.extend({
attr: function( elem, name, value ) {
// don't set attributes on text and comment nodes
if (!elem || elem.nodeType == 3 || elem.nodeType == 8)
if (!elem || elem.nodeType == 3 || elem.nodeType == 8) {
return undefined;

}
if ( name in jQuery.fn && name !== "attr" ) {
return jQuery(elem)[name](value);
}
Expand All @@ -204,23 +208,23 @@ jQuery.extend({

// Safari mis-reports the default selected property of a hidden option
// Accessing the parent's selectedIndex property fixes it
if ( name == "selected" && elem.parentNode )
if ( name == "selected" && elem.parentNode ) {
elem.parentNode.selectedIndex;

}
// If applicable, access the attribute via the DOM 0 way
if ( name in elem && notxml && !special ) {
if ( set ){
if ( set ) {
// We can't allow the type property to be changed (since it causes problems in IE)
if ( name == "type" && /(button|input)/i.test(elem.nodeName) && elem.parentNode )
if ( name == "type" && /(button|input)/i.test(elem.nodeName) && elem.parentNode ) {
throw "type property can't be changed";

}
elem[ name ] = value;
}

// browsers index elements by id/name on forms, give priority to attributes.
if( jQuery.nodeName( elem, "form" ) && elem.getAttributeNode(name) )
if( jQuery.nodeName( elem, "form" ) && elem.getAttributeNode(name) ) {
return elem.getAttributeNode( name ).nodeValue;

}
// elem.tabIndex doesn't always return the correct value when it hasn't been explicitly set
// http://fluidproject.org/blog/2008/01/09/getting-setting-and-removing-tabindex-values-with-javascript/
if ( name == "tabIndex" ) {
Expand All @@ -238,16 +242,16 @@ jQuery.extend({
}

if ( !jQuery.support.style && notxml && name == "style" ) {
if ( set )
if ( set ) {
elem.style.cssText = "" + value;

}
return elem.style.cssText;
}

if ( set )
if ( set ) {
// convert the value to a string (all browsers do this but IE) see #1070
elem.setAttribute( name, "" + value );

}
var attr = !jQuery.support.hrefNormalized && notxml && special
// Some attributes require a special call on IE
? elem.getAttribute( name, 2 )
Expand Down
34 changes: 18 additions & 16 deletions src/data.js
View file
Expand Up @@ -80,19 +80,19 @@ jQuery.extend({
},

queue: function( elem, type, data ) {
if( !elem ) return;
if ( !elem ) { return; }

type = (type || "fx") + "queue";
var q = jQuery.data( elem, type );

// Speed up dequeue by getting out quickly if this is just a lookup
if( !data ) return q || [];
if ( !data ) { return q || []; }

if ( !q || jQuery.isArray(data) )
if ( !q || jQuery.isArray(data) ) {
q = jQuery.data( elem, type, jQuery.makeArray(data) );
else
} else {
q.push( data );

}
return q;
},

Expand All @@ -102,12 +102,12 @@ jQuery.extend({
var queue = jQuery.queue( elem, type ), fn = queue.shift();

// If the fx queue is dequeued, always remove the progress sentinel
if( fn === "inprogress" ) fn = queue.shift();
if ( fn === "inprogress" ) { fn = queue.shift(); }

if( fn ) {
if ( fn ) {
// Add a progress sentinel to prevent the fx queue from being
// automatically dequeued
if( type == "fx" ) queue.unshift("inprogress");
if ( type == "fx" ) { queue.unshift("inprogress"); }

fn.call(elem, function() { jQuery.dequeue(elem, type); });
}
Expand All @@ -126,16 +126,17 @@ jQuery.fn.extend({
if ( value === undefined ) {
var data = this.triggerHandler("getData" + parts[1] + "!", [parts[0]]);

if ( data === undefined && this.length )
if ( data === undefined && this.length ) {
data = jQuery.data( this[0], key );

}
return data === undefined && parts[1] ?
this.data( parts[0] ) :
data;
} else
} else {
return this.trigger("setData" + parts[1] + "!", [parts[0], value]).each(function(){
jQuery.data( this, key, value );
});
}
},

removeData: function( key ){
Expand All @@ -149,19 +150,20 @@ jQuery.fn.extend({
type = "fx";
}

if ( data === undefined )
if ( data === undefined ) {
return jQuery.queue( this[0], type );

}
return this.each(function(i, elem){
var queue = jQuery.queue( this, type, data );

if( type == "fx" && queue[0] !== "inprogress" )
jQuery.dequeue( this, type )
if ( type == "fx" && queue[0] !== "inprogress" ) {
jQuery.dequeue( this, type );
}
});
},
dequeue: function(type){
return this.each(function(){
jQuery.dequeue( this, type );
jQuery.dequeue( this, type )
});
},
clearQueue: function(type){
Expand Down
2 changes: 1 addition & 1 deletion src/dimensions.js
View file
Expand Up @@ -20,7 +20,7 @@ jQuery.each([ "Height", "Width" ], function(i, name){
jQuery.fn[ type ] = function( size ) {
// Get window width or height
var elem = this[0];
if ( !elem ) return null;
if ( !elem ) { return null; }
return ("scrollTo" in elem && elem.document) ? // does it walk and quack like a window?
// Everyone else use document.documentElement or document.body depending on Quirks vs Standards mode
elem.document.compatMode === "CSS1Compat" && elem.document.documentElement[ "client" + name ] ||
Expand Down
44 changes: 24 additions & 20 deletions src/event.js
View file
Expand Up @@ -592,16 +592,18 @@ jQuery.each({

event.special[orig] = {
setup:function() {
if ( this.addEventListener )
if ( this.addEventListener ) {
this.addEventListener( orig, handle, true );
else
} else {
event.add( this, fix, ieHandler );
}
},
teardown:function() {
if ( this.removeEventListener )
if ( this.removeEventListener ) {
this.removeEventListener( orig, handle, true );
else
} else {
event.remove( this, fix, ieHandler );
}
}
};
});
Expand Down Expand Up @@ -820,7 +822,7 @@ jQuery.extend({
var readyBound = false;

function bindReady() {
if ( readyBound ) return;
if ( readyBound ) { return; }
readyBound = true;

// Catch cases where $(document).ready() is called after the
Expand Down Expand Up @@ -857,23 +859,25 @@ function bindReady() {
toplevel = window.frameElement == null;
} catch(e){}

if ( document.documentElement.doScroll && toplevel ) (function() {
if ( jQuery.isReady ) {
return;
}
if ( document.documentElement.doScroll && toplevel ) {
(function() {
if ( jQuery.isReady ) {
return;
}

try {
// If IE is used, use the trick by Diego Perini
// http://javascript.nwbox.com/IEContentLoaded/
document.documentElement.doScroll("left");
} catch( error ) {
setTimeout( arguments.callee, 0 );
return;
}
try {
// If IE is used, use the trick by Diego Perini
// http://javascript.nwbox.com/IEContentLoaded/
document.documentElement.doScroll("left");
} catch( error ) {
setTimeout( arguments.callee, 0 );
return;
}

// and execute any waiting functions
jQuery.ready();
})();
// and execute any waiting functions
jQuery.ready();
})();
}
}

// A fallback to window.onload, that will always work
Expand Down

27 comments on commit ddb86f8

@aheckmann
Copy link

Choose a reason for hiding this comment

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

Kind of surprised at this change. Yes it's better readability but last time I checked, YUICompressor can't remove them all.

@tj
Copy link
Contributor

@tj tj commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

ew no i hate this.. makes it much harder to read, this is why jquery is not readable IMO, way to much cruft

@padolsey
Copy link
Contributor

Choose a reason for hiding this comment

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

@visionmedia, funny you say that; this change was probably made for readability's sake. I for one think it's a good idea. Using IF/ELSE statements without curly blocks is generally considered bad practice. And, frankly, @aheckman, if YUICompressor can't handle these changes, then we should stop using it. Closure Compiler FTW!

@tj
Copy link
Contributor

@tj tj commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

I dont see how people think swarms of braces increase readability. jQuery is a fantastic library but VERY unreadable IMO. This is like saying Ruby's do / end is readable, its just cruft.

@aheckmann
Copy link

Choose a reason for hiding this comment

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

@jamespadolsey - I not sure if Closure Compiler can handle jQuery at all. I've seen some posts around the internets pointing out bugs with it.

http://www.sitepoint.com/blogs/2009/11/12/google-closure-how-not-to-write-javascript/
http://stackoverflow.com/questions/1691861/jquery-compiled-with-google-closure-compiler

It seems the "advanced" setting is what really provides the file size boost, but, that is the mode that causes the most problems.

@padolsey
Copy link
Contributor

Choose a reason for hiding this comment

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

@visionmedia, I guess it's a personal thing. For some projects, where I don't have to worry about other developers working on my code, I will frequently not bother with many best practices like curly braces... but, at the end of the day, for the majority of people, they do enhance readability. Using curly braces allows viewers to break up the code into separate logical pieces more easily. To be honest, using curly braces on single lines like:

if (something) foo();

... is quite pointless, but I guess if you've decided to "go curly", you may as well go all the way...

I found this to be an interesting read: http://stackoverflow.com/questions/359732/why-is-it-considered-a-bad-practice-to-omit-curly-braces

@aheckmann, even using the simple setting shaves off a few kb. I imagine that John will be looking into those bugs prior to the 1.4 release, although, it's not really his problem. Google should fix the closure compiler.

@kswedberg
Copy link
Member

Choose a reason for hiding this comment

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

For the record, I was just trying to align jQuery core with its own Style Guidelines:
http://docs.jquery.com/JQuery_Core_Style_Guidelines#Blocks

@tj
Copy link
Contributor

@tj tj commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

I find this way is much more concise and easy to read.. braces just clutter the code and make it harder to scan. example: http://gist.github.com/255605
Hell if you could use try / catch without braces I would lol but ya its preference I suppose... I used to write like you guys, maybe 3 years ago
but I have more of a functional approach with every language I write in now

@defunkt
Copy link

Choose a reason for hiding this comment

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

I think we should paint it red.

@mislav
Copy link

@mislav mislav commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

Glad we weren't accepting these kind of "patches" to Prototype back when they were all the craze.

@d0ugal
Copy link

@d0ugal d0ugal commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

@visionmedia It sounds like you wish you were writing Python not JavaScript ;)

@tj
Copy link
Contributor

@tj tj commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

how so? the grammar allows this.. lol so why not utilize it? I think its stupid not to. Taking a functional approach with any language is a good thing not a bad thing. But yes Python stuff looks alot better than jQuery :P

@raggi
Copy link

@raggi raggi commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

I have to say, calling "readability" on either side is just BS.

if (condition)
  expression

if (condition) {
  expression
}

Neither of the above is "more readable". The latter may be quicker to "scan" for a "block", but any basic syntax highlighting, even just bold keywords, will recover ease of scan for the prior.

The hated case:

if (condition)
    expression1
    expression2

This is unrealistic. Firstly it is very badly formatted code. Secondly this code stands out like a massively to me, even as a ruby developer where it would be legal. The reason it stands out, is because of the parenthesis. One rule that my mind has generated is that conditional blocks with parenthesis and no curly braces cannot be used in the above fashion, and warning bells go off when I see it.

The point merely is, that the readability of these things is trainable, and to claim that one is more readable than another really shows nothing but a lack of experience. There are areas of language grammars that might be generally considered more or less readable, but frankly, these very, very common constructs should be practically common knowledge by now, and as such should not be subject to any lack of readability for someone with a reasonable portion of experience.

A claim of "I prefer braces" or "I like braces" is perfectly respectable, and acceptable, but calling readability on this, I just find amusing.

@raggi
Copy link

@raggi raggi commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

And when writing code, I generally try to do as little work as possible.....

@ericflo
Copy link

Choose a reason for hiding this comment

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

This is certainly more consistent. Consistency is good.

@tj
Copy link
Contributor

@tj tj commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

consistency is good for sure, im all for that. I find even the strange whitespace within the parens a little weird too.. but hey. Not sure I have ever seen

if (cond)
    expr 
    expr

thats just bad, maybe

 if (cond)
    expr,
    expr
 ...

@raggi
Copy link

@raggi raggi commented on ddb86f8 Dec 13, 2009

Choose a reason for hiding this comment

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

Consistency, lol.

a = a()
if (a) {
 aa()
} else {
 aaa()
}

...

Different code needs different approaches. Having several different forms of definition for conditional blocks is not inconsistent. If you think so, then please use ONLY if statements OR boolean conditionals (&&) in your code, not both. Using two different forms is inconsistent.

@tj
Copy link
Contributor

@tj tj commented on ddb86f8 Dec 14, 2009

Choose a reason for hiding this comment

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

We are talking about consistency with styling ... not the code itself

@jdalton
Copy link
Member

Choose a reason for hiding this comment

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

@mislav Andrew and Tobie have done plenty of code consistency commits. Consistency is a good thing.

@joshuaclayton
Copy link

Choose a reason for hiding this comment

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

+5; I love to see consistency in a framework or library, and the fact that it's the "correct" way to do things is an added bonus.

@mathiasbynens
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency ftw.

@padolsey
Copy link
Contributor

Choose a reason for hiding this comment

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

For those interested, the "jQuery Core Style Guidelines" can be viewed here: http://docs.jquery.com/JQuery_Core_Style_Guidelines#Blocks

@tj
Copy link
Contributor

@tj tj commented on ddb86f8 Dec 14, 2009

Choose a reason for hiding this comment

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

haha ew... i understand there is no turning back now from what has been done but damn those are ugly conventions.

@dsc
Copy link

@dsc dsc commented on ddb86f8 Dec 15, 2009

Choose a reason for hiding this comment

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

On the minus side, I hate curly braces.

On the plus side, I hate painting bikesheds. Style guides, while vaguely creepy and authoritarian, definitely minimize the number of painters we need to keep around.

(And fwiw, consistency could be maintained with mixed braces, so long as they were used in a regular fashion. It's irregularity that creates inconsistency and importantly, confusion.)

@jeresig
Copy link
Member

Choose a reason for hiding this comment

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

I completely missed this brouhaha. There is no possible way in which actually using braces is worse for jQuery. It standardizes the code base and it makes it easier to understand. We're not going back to the dark ages of mixed braces and multi-level deep no-brace statements.

@StevenBlack
Copy link

Choose a reason for hiding this comment

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

Coding standards: use them, no exceptions.

@voodootikigod
Copy link

Choose a reason for hiding this comment

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

+5; painting it red!

Please sign in to comment.