Skip to content

Commit

Permalink
Cleanup the code a wee bit. Thanks CPojer
Browse files Browse the repository at this point in the history
  • Loading branch information
@subtleGradient @cpojer
subtleGradient authored and cpojer committed Sep 9, 2010
1 parent d1d503c commit 2b12d56
Showing 1 changed file with 6 additions and 10 deletions.
16 changes: 6 additions & 10 deletions Source/Element/Element.js
View file
Expand Up @@ -150,7 +150,7 @@ Array.mirror(Elements);
var createElementAcceptsHTML;
try {
var x = document.createElement('<input name=x>');
createElementAcceptsHTML = x.name == 'x';
createElementAcceptsHTML = (x.name == 'x');
} catch(e){}

var escapeQuotes = function(html){
Expand All @@ -163,17 +163,13 @@ Document.implement({
newElement: function(tag, props){
if (props && props.checked != null) props.defaultChecked = props.checked;
/*<ltIE8>*/// Fix for readonly name and type properties in IE < 8
if (props && createElementAcceptsHTML){
if (createElementAcceptsHTML && props){
tag = '<' + tag;
if (props.name){
tag += ' name="' + escapeQuotes(props.name) + '"';
delete props.name;
}
if (props.type){
tag += ' type="' + escapeQuotes(props.type) + '"';
delete props.type;
}
if (props.name) tag += ' name="' + escapeQuotes(props.name) + '"';
if (props.type) tag += ' type="' + escapeQuotes(props.type) + '"';
tag += '>';
delete props.name;
delete props.type;

This comment has been minimized.

Sign in to view

Copy link
@kassens

kassens Sep 10, 2010

Member

No need to delete the property if it wasn't there. That's just more unnecessary code executed. Why the change?

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Sep 10, 2010

The if condition checks for the truthiness of the property, not its existence. The properties may still exist, even if they're falsey.

This comment has been minimized.

Sign in to view

Copy link
@cpojer

cpojer Sep 10, 2010

Member

The idea was to remove 4 lines of code by moving them out of the if statements.

This comment has been minimized.

Sign in to view

Copy link
@kassens

kassens Sep 10, 2010

Member

Trading LOC for performance is stupid. (Note, that this is pure LOC, not readability)

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Sep 10, 2010

@kassens Remember, there is actually a change in functionality here, too, even if it may not have been intended

This comment has been minimized.

Sign in to view

Copy link
@cpojer

cpojer Sep 10, 2010

Member

I would have just gone without the delete. It doesn't matter at all really. Consider that if the property is readonly in IE, setAttribute would not do anything anyway.

@michaelficarra what is the use case in passing a falsy value as name/type and actually have it set via name="falsyValue" ?

This comment has been minimized.

Sign in to view

Copy link
@michaelficarra

michaelficarra Sep 10, 2010

I'm not saying one exists. It is likely that there is no use case. But I was just reminding kassens that it deletes the name and type properties of props regardless of whether they are truthy, which strays from the original functionality. I'm not commenting on whether or not this was a good design decision.

I do believe it enhances readability, and if the functional changes are acceptable/desirable, then it was a good change.

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton Sep 10, 2010

@kassens The deletes have no real performance impact, however I am sure that calling the DOM methods for those properties if not deleted (setAttribute n what not) would have a bigger impact (DOM operations are generally slower than language operations). Keep in mind this is snippet of code is only for IE6/7 too.

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton Sep 10, 2010

@cpojer to solve the falsey issue you could do if ("name" in prop) ....
}
/*</ltIE8>*/
return this.id(this.createElement(tag)).set(props);
Expand Down

0 comments on commit 2b12d56

Please sign in to comment.