PHP Coding Practices

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

PHP Coding Practices

Mark Jaquith
I'd like to get some group feedback on this:

Issue #1

WP currently uses strstr() in many places just to establish basic  
"this string exists in that string" functionality.  According to  
http://php.net/strstr, "Note:  If you only want to determine if a  
particular needle  occurs within haystack, use the faster and less  
memory intensive function strpos() instead."

And, of course, when using strpos(), you have to be careful to check  
it with === or !== against boolean false.  if ( strpos($haystack,  
$needle) !== false ) { // $needle exists in $haystack }

I made this change in one of my recent "Massive Code Cleanups" but  
would like to know if it would be worth changing all other instances  
for speed/consistency/code-is-poetry.


Issue #2

We've gotten into trouble several times with running foreach() on  
something that might not be an array.  We currently are using a lot  
of "if ( !empty($things) )" checks to prevent that.  I'm rather a fan  
of casting to array when using foreach:

foreach ( (array) $things as $thing ) {

If $things is null or false or whatever, it just becomes an empty  
array, and the foreach block gets skipped... the desired behavior.  
It's less code, and it brings all foreach() loop up a level in terms  
of code indentation, which increases readability.

--
Mark Jaquith
http://txfx.net/


_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Evan Broder
Mark Jaquith wrote:

> I'd like to get some group feedback on this:
>
> Issue #1
>
> WP currently uses strstr() in many places just to establish basic
> "this string exists in that string" functionality.  According to
> http://php.net/strstr, "Note:  If you only want to determine if a
> particular needle  occurs within haystack, use the faster and less
> memory intensive function strpos() instead."
>
> And, of course, when using strpos(), you have to be careful to check
> it with === or !== against boolean false.  if ( strpos($haystack,
> $needle) !== false ) { // $needle exists in $haystack }
>
> I made this change in one of my recent "Massive Code Cleanups" but
> would like to know if it would be worth changing all other instances
> for speed/consistency/code-is-poetry.
>
>
> Issue #2
>
> We've gotten into trouble several times with running foreach() on
> something that might not be an array.  We currently are using a lot of
> "if ( !empty($things) )" checks to prevent that.  I'm rather a fan of
> casting to array when using foreach:
>
> foreach ( (array) $things as $thing ) {
>
> If $things is null or false or whatever, it just becomes an empty
> array, and the foreach block gets skipped... the desired behavior.  
> It's less code, and it brings all foreach() loop up a level in terms
> of code indentation, which increases readability.
>
> --
> Mark Jaquith
> http://txfx.net/
+1 on both counts. Adding those recommendations to the Codex coding
practices page would be nice as well, at least the second one---I had
never thought of casting the variables. My code has always used the if
method.

- Evan
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Andy Skelton
In reply to this post by Mark Jaquith
On 2/14/06, Mark Jaquith <[hidden email]> wrote:
> I'd like to get some group feedback on this:
>
> Issue #1

Worthwhile to use strpos !== false in all new and updated code (I
recently started doing that, too) but I wouldn't sweat the old stuff
unless you're really bored.

> Issue #2
> foreach ( (array) $things as $thing ) {

That's awesome except you can also foreach an object. Be careful about that.

Andy
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Mark Jaquith
On Feb 14, 2006, at 7:46 PM, Andy Skelton wrote:

> Worthwhile to use strpos !== false in all new and updated code (I
> recently started doing that, too) but I wouldn't sweat the old stuff
> unless you're really bored.

I am, from time to time (bored, that is).  I've submitted patches  
that purely fix spaces => tabs and fix indentation issues. :-)

>> Issue #2
>> foreach ( (array) $things as $thing ) {
>
> That's awesome except you can also foreach an object. Be careful  
> about that.

You can only foreach() objects in PHP5.  I don't see WordPress  
dropping PHP4 support anytime soon, so I think we're safe there.

--
Mark Jaquith
http://txfx.net/


_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Dave Grijalva
That's not really true.  I'm pretty sure you can foreach a PHP4 array.  It's
been a while since I coded in 4, but I think it returns the objects
properties.  In either case, it seems foolish to include code that will
become a problem in the future, no matter how remote.  Plus, people who are
running on 5 and want to take advantage of it should be able to.  Backwards
compatibility is great, but forwards compatibility is a no brainer.

On 2/14/06, Mark Jaquith <[hidden email]> wrote:

>
> On Feb 14, 2006, at 7:46 PM, Andy Skelton wrote:
>
> > Worthwhile to use strpos !== false in all new and updated code (I
> > recently started doing that, too) but I wouldn't sweat the old stuff
> > unless you're really bored.
>
> I am, from time to time (bored, that is).  I've submitted patches
> that purely fix spaces => tabs and fix indentation issues. :-)
>
> >> Issue #2
> >> foreach ( (array) $things as $thing ) {
> >
> > That's awesome except you can also foreach an object. Be careful
> > about that.
>
> You can only foreach() objects in PHP5.  I don't see WordPress
> dropping PHP4 support anytime soon, so I think we're safe there.
>
> --
> Mark Jaquith
> http://txfx.net/
>
>
> _______________________________________________
> wp-hackers mailing list
> [hidden email]
> http://lists.automattic.com/mailman/listinfo/wp-hackers
>
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Robert Deaton
On 2/15/06, Dave Grijalva <[hidden email]> wrote:
> That's not really true.  I'm pretty sure you can foreach a PHP4 array.

Where did this come from? Nobody was debating that. In the event that
you really meant object, you are wrong, as per http://php.net/foreach,
object iteration was added in PHP5.

> <snip>  In either case, it seems foolish to include code that will
> become a problem in the future, no matter how remote.  Plus, people who are
> running on 5 and want to take advantage of it should be able to.  Backwards
> compatibility is great, but forwards compatibility is a no brainer.

I am in no way seeing how this would break forwards compatability.
Anyone who wants to submit a patch that iterates over an object can
easily do so, leaving out the typecast (maybe even making a comment
that it is an object so that nobody else comes along and tries to
typecast it), and continue along with their life.

We cannot change the behavior of foreach to force the typecast, but we
can however typecast wherever we know we are expecting an array.

--
--Robert Deaton
http://somethingunpredictable.com

_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Dave Grijalva
actually, it is in php4:

:~ dave$ php4
<?php
class test {
var $a;
var $b;
}

$test = new test();
$test->a = 'abc';
$test->b = 'hey, you';

foreach($test as $key => $val){
 echo "$key: $val";
echo "\n";
}
?>
------  output ------
a: abc
b: hey, you

it may not be documented, but it's there.

yes, this doesn't break iterating over an object, but it does break creating
an object that can be iterated elsewhere.  in PHP5, an iteratable object
should behave properly when used anywhere an array is expected.  typecasting
would break this compatibility all over the wordpress core

-dave

On 2/14/06, Robert Deaton <[hidden email]> wrote:

>
> On 2/15/06, Dave Grijalva <[hidden email]> wrote:
> > That's not really true.  I'm pretty sure you can foreach a PHP4 array.
>
> Where did this come from? Nobody was debating that. In the event that
> you really meant object, you are wrong, as per http://php.net/foreach,
> object iteration was added in PHP5.
>
> > <snip>  In either case, it seems foolish to include code that will
> > become a problem in the future, no matter how remote.  Plus, people who
> are
> > running on 5 and want to take advantage of it should be able
> to.  Backwards
> > compatibility is great, but forwards compatibility is a no brainer.
>
> I am in no way seeing how this would break forwards compatability.
> Anyone who wants to submit a patch that iterates over an object can
> easily do so, leaving out the typecast (maybe even making a comment
> that it is an object so that nobody else comes along and tries to
> typecast it), and continue along with their life.
>
> We cannot change the behavior of foreach to force the typecast, but we
> can however typecast wherever we know we are expecting an array.
>
> --
> --Robert Deaton
> http://somethingunpredictable.com
>
> _______________________________________________
> wp-hackers mailing list
> [hidden email]
> http://lists.automattic.com/mailman/listinfo/wp-hackers
>
>
>
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Mark Jaquith
On Feb 14, 2006, at 9:14 PM, Dave Grijalva wrote:

> it may not be documented, but it's there.

I stand corrected.  Interesting that it's incorrectly documented.

> yes, this doesn't break iterating over an object, but it does break  
> creating
> an object that can be iterated elsewhere.  in PHP5, an iteratable  
> object
> should behave properly when used anywhere an array is expected.  
> typecasting
> would break this compatibility all over the wordpress core

We're not going to cast objects to array.  So there's no problem.  
We're not running foreach() loops on objects... only on arrays or  
queries that normally return arrays, but may return false (no  
results) instead.

ezSQL isn't going to start returning objects where it used to return  
arrays, so there is no issue of compatibility.
--
Mark Jaquith
http://txfx.net/


_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Robert Deaton
In reply to this post by Dave Grijalva
On 2/15/06, Dave Grijalva <[hidden email]> wrote:

> actually, it is in php4:
>
> :~ dave$ php4
> <?php
> class test {
> var $a;
> var $b;
> }
>
> $test = new test();
> $test->a = 'abc';
> $test->b = 'hey, you';
>
> foreach($test as $key => $val){
>  echo "$key: $val";
> echo "\n";
> }
> ?>
> ------  output ------
> a: abc
> b: hey, you
>
> it may not be documented, but it's there.
Actually, that's a typecast. Try using an explicit one in the code,
you'll notice that it runs the same.

lappy ~ # php
<?php
class test {
var $a;
var $b;
}

$test = new test();
$test->a = 'abc';
$test->b = 'hey, you';

var_dump((array) $test);
?>

array(2) {
  ["a"]=>
  string(3) "abc"
  ["b"]=>
  string(8) "hey, you"
}

Mark, you were right, and the documentation is correct.

--
--Robert Deaton
http://somethingunpredictable.com

_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Andy Skelton
On 2/15/06, Robert Deaton <[hidden email]> wrote:
> Actually, that's a typecast. Try using an explicit one in the code,
> you'll notice that it runs the same.

So... it doesn't hurt the object to cast it as an array in a foreach.
I shoulda known better because there's no assignment operator. My bad,
I retract my statement.

Andy
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

David House
In reply to this post by Robert Deaton
On 15/02/06, Robert Deaton <[hidden email]> wrote:
> Actually, that's a typecast. Try using an explicit one in the code,
> you'll notice that it runs the same.

So for all extents and purposes you can iterate an object in PHP4.

I do agree however that doing so is harmless.

--
-David House, [hidden email], http://xmouse.ithium.net
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Mark Jaquith
In reply to this post by Mark Jaquith
On Feb 14, 2006, at 7:19 PM, Mark Jaquith wrote:

> WP currently uses strstr() in many places just to establish basic  
> "this string exists in that string" functionality.  According to  
> http://php.net/strstr, "Note:  If you only want to determine if a  
> particular needle  occurs within haystack, use the faster and less  
> memory intensive function strpos() instead."

You know what?  I just ran a bunch of speed tests.  I used small  
haystacks and big haystacks.  I ran strstr($haystack, $needle)  
against strpos($haystack, $needle) !== false (as well as the opposite  
versions with a million iterations each.  strstr() is faster by  
anywhere from 5 to 20%.  Plus, it looks nicer in the code.

So now this is just about casting to array for foreach() rather than  
testing with !empty().  Any objections?
--
Mark Jaquith
http://txfx.net/


_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Robert Deaton
In reply to this post by David House
On 2/15/06, David House <[hidden email]> wrote:
> On 15/02/06, Robert Deaton <[hidden email]> wrote:
> > Actually, that's a typecast. Try using an explicit one in the code,
> > you'll notice that it runs the same.
>
> So for all extents and purposes you can iterate an object in PHP4.

You could say it, but its not really true, what you can do is typecast
an object to an array and iterate the array, and it will (most likely)
have what you expect, assuming that you're not trying to set iterator
properties like you could in PHP5. The point here being that, yes, we
can safely use the (array) typecast right now anywhere we are doing a
foreach(), even on objects, since we are supporrting PHP4. In the
future, if support for PHP4 is ever dropped (this is like...a few
years off I'd imagine, but since someone mentioned it), at the point
where someone wants to iterate an object with new code, they simply do
not add the typecast, it is not being forced upon them. Make a comment
saying that its an object, not an array, and move along.

--
--Robert Deaton
http://somethingunpredictable.com

_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Chris Lott
In reply to this post by Mark Jaquith
On 2/15/06, Mark Jaquith <[hidden email]> wrote:
> You know what?  I just ran a bunch of speed tests.  I used small
> haystacks and big haystacks.  I ran strstr($haystack, $needle)
> against strpos($haystack, $needle) !== false (as well as the opposite
> versions with a million iterations each.  strstr() is faster by
> anywhere from 5 to 20%.  Plus, it looks nicer in the code.

This doesn't accord with my experience or others (see, for example,
http://schogini.us/wordpress/index.php/2005/11/21/tips-to-speed-up-php-code/)
-- wonder if something else is going on here?

It's only nominally faster sans real pattern matching, but still...

c
_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Robert Deaton
On 2/15/06, Chris Lott <[hidden email]> wrote:
> On 2/15/06, Mark Jaquith <[hidden email]> wrote:
http://schogini.us/wordpress/index.php/2005/11/21/tips-to-speed-up-php-code/)
> -- wonder if something else is going on here?

Your tests are not near detailed or run enough times to be a true
indication. Here's something I threw together that might be a more
accurate test, that tries not only real time, but cpu time as well,
because both are very important to us, we need to keep server load
down.

Here's the results with testa.php, which does simple timing. I've run
it three times so you get a good image of the results, and I've
attached the script as testa.txt (I hope txts don't get filtered out)

lappy ~ # php test.php > out
lappy ~ # php test.php >> out
lappy ~ # php test.php >> out
lappy ~ # cat out
strstr at starting point run time: 0.00860
strpos at starting point run time: 0.00824
strstr at mid point run time: 0.00948
strpos at mid point run time: 0.00859
strstr difficult match: 0.01135
strpos difficult match: 0.01045
strstr fail: 0.01075
strpos fail: 0.01057

strstr at starting point run time: 0.00873
strpos at starting point run time: 0.00868
strstr at mid point run time: 0.00907
strpos at mid point run time: 0.00862
strstr difficult match: 0.01142
strpos difficult match: 0.01079
strstr fail: 0.01090
strpos fail: 0.01070

strstr at starting point run time: 0.00866
strpos at starting point run time: 0.00914
strstr at mid point run time: 0.00896
strpos at mid point run time: 0.00884
strstr difficult match: 0.01115
strpos difficult match: 0.01062
strstr fail: 0.01085
strpos fail: 0.01059

As you can see, strstr is slower in all three tests. Next, I did a
test with `time` to see which is faster in system time, run 5 times
each, here are their averages:

strstr (testb.txt):
sys     0m0.006s

strpos (testc.txt):
sys     0m0.003s

So, as you can see, strpos is clearly the faster of the two, digging
into the source of PHP will show you some obvious reasons why. Also,
if anyone is interested on the memory benchmarks of these tests, let
me know.

--
--Robert Deaton
http://somethingunpredictable.com

_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers

testa.txt (2K) Download Attachment
testb.txt (562 bytes) Download Attachment
testc.txt (562 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Robert Deaton
Erm, that last email was meant as a reply to Mark.

--
--Robert Deaton
http://somethingunpredictable.com

_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: PHP Coding Practices

Mark Jaquith
In reply to this post by Robert Deaton
On Feb 15, 2006, at 5:07 PM, Robert Deaton wrote:

> Your tests are not near detailed or run enough times to be a true
> indication. Here's something I threw together that might be a more
> accurate test, that tries not only real time, but cpu time as well,
> because both are very important to us, we need to keep server load
> down.

You're right, my tests were simplistic.

Replacing the existing strstr() instances with strpos() would have to  
be done manually... or with several regexes (that might mess things  
up).  But what would be easy is to replace all instances of "strstr("  
with "wp_strstr(".  We could then make something like:

function wp_strstr($haystack, $needle) {
        if ( strpos($haystack, $needle) !== false )
                return true;
        return false;
}

Would passing it through a function like that erode too much of the  
benefit that strpos() offers?  I like this approach, because strpos
($haystack, $needle) !== false is ugly... and is prone to error (if  
someone accidentally uses !=, or just checks boolean true/false).  
wp_strstr($haystack, $needle) is much  easier to read and to code.  
And I easily made that change project wide with a simple search/replace.

Can you run your test between strstr() and wp_strstr() and see if it  
is still worth doing?
--
Mark Jaquith
http://txfx.net/


_______________________________________________
wp-hackers mailing list
[hidden email]
http://lists.automattic.com/mailman/listinfo/wp-hackers
Loading...