[wp-hackers] We already know you are awesome, you don't have
to prove it
wordpress at santosj.name
Sun Jan 6 06:40:18 GMT 2008
Perhaps. I do agree that having someone else look over your code is a
great way to find obvious mistakes.
There aren't too many places that I've seen where there is a "WTF?"
moment. Digging through patches for me, is whether said patch would make
my life as a plugin writer easier or harder. If it is the latter, then
you'll know I'll be kicking and screaming about its inclusion.
I usually write overly complex code while I'm not tired and usually
don't realize it until I go to fix a mistake. The problem with the
referenced logic is that it is confusing based on the precedence.
However, I first read the '=' as '==' as that is what I expect to see in
an IF expression. Looking at it, I immediately questioned, "WTF? Does
this check to see if the variable is an array? BY TYPECASTING!!!!
Nooooo!" If you can write code like that at 2am, then I bow to you oh
great one! Seriously, it is l33t code, but it is not needed because
is_array() exists for the purpose of checking if something is an array
and you won't confuse people who don't know about type casting.
Still working on the patch however. There are a lot of foreach arrays
and not all of them need to be type casted to an array.
Peer reviews would be good for already existing code, well for any code.
I do think that peer reviews are more for obvious bug issues and "WTF?"
moments. For myself personally, I think the foreach issue should be
fixed by first evaluating whether the variable is an array using
is_array() before starting the array, but that is just me. I'm unsure if
you type cast if the foreach will silently fail if it sees that the
count is 0 as it would be if you type cast something that isn't an
array, "What, there aren't any elements in this array. Failure!" I've
never had a chance to test my theory however. I do assume that if you
type cast something that it will at least run the loop once, but again
I've never had the opportunity to test my theory. Sanity checks are
always good however.
Oh yeah. The first reference was a for loop and not a foreach (had
foreach on my mind). Did you know that in C#, you are not allowed to
manipulate the array you are traversing? Wild. Did you know that in PHP
you are technically not supposed to manipulate the array you are
traversing in a foreach loop, but PHP will not crash and burn like C#
will? Good stuff to keep in mind.
Robin Adrianse wrote:
> I'm pretty sure (well, I hope this is the case) that this was all part of
> the "2 am syndrome" -- looking at jumbled code so long it starts to make
> sense. It may be a good idea to enforce peer review for big patches, much
> like how the Webkit (and others) project does it, so the chance of these
> weird pieces of code can be caught and ironed out. I often can't find
> mistakes in my code, but it takes only a few seconds for others to find
> simple mistakes.
> On Jan 5, 2008 9:43 PM, Jacob Santos <wordpress at santosj.name> wrote:
>> Can we keep overly complex code out of the source when it does not need
>> to be?
>> I've noticed while loops that should be foreach loops (fixed). I've just
>> seen code that tests whether a variable is an array without using
>> is_array() (will be fixed in patch when finished).
>> Yes, you have proven that you are awesome, but you don't need to prove
>> it. Now for the love of all that is holy please stop. Overly complex
>> code when it does not need to be does not help people decipher your code.
>> Thank You.
>> Jacob Santos
>> http://www.santosj.name - blog
>> Also known as darkdragon and santosj on WP trac.
>> wp-hackers mailing list
>> wp-hackers at lists.automattic.com
> wp-hackers mailing list
> wp-hackers at lists.automattic.com
http://www.santosj.name - blog
Also known as darkdragon and santosj on WP trac.
More information about the wp-hackers