Restores oldest google favicon. Motty Katan(c) 05-09-2015 last updated 15-09-2015
< Feedback on Google old favicon
I consider the word "break" as deprecated, personally it reminds me the days of GOTO. This is a classic WHILE since WHILE denotes a special condition other than enumeration. While a FOR loop normally implies N times something.
About the children[1], this was my first reaction, but I quickly voted against that since I desire scripts that works for life not months or years.
Regarding break statements, it is considered good practice to not rely on them to exit from loops, in case the condition which it should break is not met. But if you already have a main exit condition (eg end of the array), then break could be useful to exit from the loop early, if it is not necessary to continue iterating through the rest of the array (eg if you only want to find one particular element, and you have actually found it).
The for loop would still work fine without the break statement, but it would just keep iterating unnecessarily to the end of the array (it would also iterate to the end of the array if it doesn't find the element, and then exit normally without making any changes)
I definitely wouldn't place break or continue in the same league as goto; break and continue are at least sometimes appropriate. But goto should always be considered depracated.
Compared to a while loop, I think a for loop is basically the same thing (loop until condition is met), except it also allows declaring temporary variables.
So basically it just means you have to use the temporary variable inside the loop itself, which means for (var i = 0; document.head.children[i].getAttribute('rel') != "shortcut icon" && i++ < document.head.children.length;) wouldn't work, since the variable i would no-longer exist when you want to use it.
But it also means you don't have to check if document.head.children[i].getAttribute('rel') != "shortcut icon" twice.
I also did consider writing the for loop like this: for (var i = document.head.children.length; i--;)
But I decided that since the matching element is closer to zero than the end of the array, it's better to start from zero, so it will iterate less times.
And I think I agree that it's probably safer to not hardcode it to use children[1], but to actually check which element it should use.
Well surely Google will always have a shortcut icon. But I hates scripts that can through uncaught exception. This if is there just for "good manners" motive.
Maybe w/o the if my script looks more elegent (no notFound variable, no break), but I rather keep it rather than adding a variable or using break.
Agree about the direction. My toughts exactly.
In the for loop I made, the break statement is basically just there for CPU efficiency. Without it, the favicon still gets replaced, but it keeps iterating until the end of the array anyway - it doesn't become an infinite loop.
And if the favicon is not found, it just iterates to the end of the array regardless and doesn't change anything.
So basically, this works:
for (var i = 0; i++ < document.head.children.length;)
if (document.head.children[i].getAttribute('rel') == "shortcut icon") {
document.head.children[i].href = icon;
}
It just means that it iterates through the loop more times than it needs to.
Works perfectly, though method is a bit strange.
It looks a bit strange to have a while loop just to iterate i. Personally I would write it like this:
Because you can always just break out of the for loop after you find the correct element.
And I think it's not necessary to check whether
oChildren[i].tagName == 'LINK'I also made a one-liner that works:
document.head.querySelector('link[rel="shortcut icon"]').href = icon;The only issue being that the querySelector function seems to be a bit slow.
I also checked what element was actually being replaced (the value of i), and it seemed to be value 1 - Assuming the value is always going to be 1, this should work:
document.head.children[1].href = icon;