Currently our coding conventions have major prohibitions on the use of isset() and empty(). I can understand isset(), because sometimes it makes more sense logically to have boolean casting or strict type checking, but in the case of empty(), it's significantly faster than boolean casting and is more readable.
If you're checking for *whether an array is empty*, which is more intuitive:
if( !$array ) // If the boolean value of the array is false if( empty( $array ) ) // If the array is empty
Functionally, they're almost equivalent, as any value that evaluates as false will also be treated as empty. The sole difference is 1) empty() first checks if the variable is set and 2) empty() is faster.
I understand that there is a slight risk here due to empty()'s error suppression, but in many cases it's obvious that a variable exists and it's not necessary to sacrifice the logical readability of the code just because a developer might make a typo and not realize it even though the same variable is on the line above.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
---------- Forwarded message ---------- From: Brad Jorsch bjorsch@wikimedia.org Date: Tue, Feb 5, 2013 at 5:43 PM Subject: Re: [Gerrit] API PageSet allows generator for non-query modules - change (mediawiki/core[master]) To: tylerromeo@gmail.com
On Tue, Feb 5, 2013 at 5:03 PM, Parent5446 (Code Review) gerrit@wikimedia.org wrote:
Yeah, I see the page, but it doesn't tell me anything interesting. The
only disadvantage of using empty() is for the off chance that you mispell a variable name and the error is hidden, but it's obviously not the case here. And in terms of readability, it makes a lot more sense to have if( empty())
Then perhaps you should raise the question of changing the coding standards on https://www.mediawiki.org/wiki/Manual_talk:Coding_conventions/PHP
Until then, let's not use empty() unless we explicitly want the "isset" check.
On 06/02/13 15:18, Tyler Romeo wrote:
If you're checking for *whether an array is empty*, which is more intuitive:
if( !$array ) // If the boolean value of the array is false if( empty( $array ) ) // If the array is empty
The second is more intuitive. The first is more readable, since it doesn't look like an error made by someone who is unfamiliar with PHP, and who is thus following their intuition rather than their experience.
If you really want something that is readable by users who are unfamiliar with PHP, you can always use !count( $array ).
Functionally, they're almost equivalent, as any value that evaluates as false will also be treated as empty. The sole difference is 1) empty() first checks if the variable is set and 2) empty() is faster.
It doesn't seem faster to me.
$a = array('x'); $t = microtime(true); for ($i=0; $i < 10000000;
$i++ ) { empty($a); } print microtime(true) - $t; 0.57716178894043
$a = array('x'); $t = microtime(true); for ($i=0; $i < 10000000;
$i++ ) { !$a; } print microtime(true) - $t; 0.57688283920288
$a = array(); $t = microtime(true); for ($i=0; $i < 10000000; $i++ )
{ empty($a); } print microtime(true) - $t; 0.58778500556946
$a = array(); $t = microtime(true); for ($i=0; $i < 10000000; $i++ )
{ !$a; } print microtime(true) - $t; 0.57663702964783
The implementations are pretty much identical, so it's hard to see why one would be faster than the other.
I understand that there is a slight risk here due to empty()'s error suppression, but in many cases it's obvious that a variable exists and it's not necessary to sacrifice the logical readability of the code just because a developer might make a typo and not realize it even though the same variable is on the line above.
Developers often make typos without realising it, that's why we have the rule.
-- Tim Starling
Hey,
Why is this thread not titled "return of the bikeshed"? I've seen this one so many times over the past few days. Wonder how many man-days have been wasted on this :)
It doesn't seem faster to me.
Those benchmarks are only testing some rather small arrays no?
I did some stuff a while back as well: https://github.com/JeroenDeDauw/php/blob/master/profile/array-empty.php
If you really want something that is readable by users who are unfamiliar with PHP, you can always use !count( $array ).
I am very dubious that this is actually clearer. In any case, this one IS less efficient.
Currently our coding conventions have major prohibitions on the use of
isset() ...
Oh, would not think so if you did a grep on core.
... and empty()
I used to use this for array empty checks, and though some people complained, just as many thought it was perfectly fine.
Anyway, if you are thinking of reaching a consensus on the list here and enforce everyone to use the form you like, good luck. Such a thing might happen of course if we wait long enough, though I wonder if protons will decay before that or not. Anyone want to place a bet? I'm going for proton decay winning.
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --
On Wed, Feb 6, 2013 at 8:36 PM, Jeroen De Dauw jeroendedauw@gmail.comwrote:
Currently our coding conventions have major prohibitions on the use of
isset() ...
Oh, would not think so if you did a grep on core.
There are valid uses for isset, and as far as I am aware, those are not
prohibited and are actively being used. That does not mean that we do or should allow isset in every case where it can be used.
Bryan
So basically here's my goal (it's the same as if ( v. if( ). I don't care about reaching consensus on either side, because such an attempt is futile. The only thing I want to at least get some support on is that a given patchset should not be blocked from merging just because it uses empty() in a place where it's obviously OK to use it (for example, when you're checking if a function argument is empty one line below the beginning of the function).
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Feb 6, 2013 at 2:40 PM, Bryan Tong Minh bryan.tongminh@gmail.comwrote:
On Wed, Feb 6, 2013 at 8:36 PM, Jeroen De Dauw <jeroendedauw@gmail.com
wrote:
Currently our coding conventions have major prohibitions on the use of
isset() ...
Oh, would not think so if you did a grep on core.
There are valid uses for isset, and as far as I am aware, those are not
prohibited and are actively being used. That does not mean that we do or should allow isset in every case where it can be used.
Bryan _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Le 06/02/13 22:22, Tyler Romeo a écrit :
So basically here's my goal (it's the same as if ( v. if( ). I don't care about reaching consensus on either side, because such an attempt is futile.
I prefer unless( !$foo ) but that is just me.
The only thing I want to at least get some support on is that a given patchset should not be blocked from merging just because it uses empty() in a place where it's obviously OK to use it (for example, when you're checking if a function argument is empty one line below the beginning of the function).
I discourage the use of empty(), it can leads to so many potential mistakes that is better to simply never use it.
If I wanted to check an array is empty I would probably:
count( $array ) === 0
cheers,
I discourage the use of empty(), it can leads to so many potential mistakes that is better to simply never use it.
The only mistake it can lead to is if there's a typo in a variable name, but in cases like this:
function myfunc( array $var ) { if( empty( $var ) )
I highly doubt there's a chance of running into a typo or mistake.
If I wanted to check an array is empty I would probably:
count( $array ) === 0
cheers,
count() requires determining the size of the array, which is significantly slower than any of the solutions.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Wed, Feb 6, 2013 at 5:06 PM, Antoine Musso hashar+wmf@free.fr wrote:
Le 06/02/13 22:22, Tyler Romeo a écrit :
So basically here's my goal (it's the same as if ( v. if( ). I don't care about reaching consensus on either side, because such an attempt is
futile.
I prefer unless( !$foo ) but that is just me.
The only thing I want to at least get some support on is that a given patchset should not be blocked from merging just because it uses empty()
in
a place where it's obviously OK to use it (for example, when you're checking if a function argument is empty one line below the beginning of the function).
I discourage the use of empty(), it can leads to so many potential mistakes that is better to simply never use it.
If I wanted to check an array is empty I would probably:
count( $array ) === 0
cheers,
-- Antoine "hashar" Musso
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Le 06/02/13 23:48, Tyler Romeo wrote: <snip>
The only mistake it can lead to is if there's a typo in a variable name, but in cases like this:
There are a few more possible such as 0 or "0" being considered empty. And as you said, that hide the fact a variable is not defined, that is sometime useful, lot of the time hiding an error.
If I wanted to check an array is empty I would probably:
count( $array ) === 0
count() requires determining the size of the array, which is significantly slower than any of the solutions.
Your assumption is probably based on the meaning of the word "count" but does not reflect the PHP implementation for an array. That is actually a hashtable which keep track of the number of elements it contains, counting is just about returning that value.
In ext/standard/array.c:
// The count() function definition: PHP_FUNCTION(count) ... case IS_ARRAY: RETURN_LONG (php_count_recursive (array, mode TSRMLS_CC));
// Counting the element in an array: static int php_count_recursive(zval *array, long mode TSRMLS_DC) .. cnt = zend_hash_num_elements(Z_ARRVAL_P(array));
That zend_hash_num_elements is in Zend/zend_hash.c so small I am pasting it fully:
ZEND_API int zend_hash_num_elements(const HashTable *ht) { IS_CONSISTENT(ht); return ht->nNumOfElements; }
As we can see, the array is ultimately a HashTable which has a property to give out the number of elements in it. It is incremented whenever one add an object to the HashTable :-]
On 07/02/13 06:36, Jeroen De Dauw wrote:
Those benchmarks are only testing some rather small arrays no?
I did some stuff a while back as well: https://github.com/JeroenDeDauw/php/blob/master/profile/array-empty.php
That is an interesting way to do benchmarks. XHProf has a bug in it which caused the times it reported to be off by a factor of 100 or so for about half the runs. I fixed it, and also unrolled the size loop to allow the xhprof_enable() call to be done only once, reducing the effects of inaccurate RDTSC calibration. I increased the iteration count and switched to a normal loop counter instead of range() to avoid thrashing the L2 cache.
The XHProf overhead was distorting the count() case, since it adds a profiling hook to internal function calls. Specifying the XHPROF_FLAGS_NO_BUILTINS flag to xhprof_enable() reduced the time for counting small arrays by a factor of 2.
I added the "!" operator to the list of things to benchmark.
The results were:
* ! and empty() within an error bar of each other, ~700ns per iteration * Triple-equals took 1300ns per iteration * count() took around 3us plus 370ns per array element (determined by linear regression).
The modified script: http://paste.tstarling.com/p/JYnLPi.html The XHProf patch: http://paste.tstarling.com/p/tgCAyC.html
The reason count() is O(N) in this case is because $array is a reference. See zend_send_by_var_helper in zend_vm_def.h:
} else if (PZVAL_IS_REF(varptr)) { zval *original_var = varptr;
ALLOC_ZVAL(varptr); ZVAL_COPY_VALUE(varptr, original_var); Z_UNSET_ISREF_P(varptr); Z_SET_REFCOUNT_P(varptr, 0); zval_copy_ctor(varptr);
If you pass the array as a non-reference parameter, you get O(1) performance of count():
function a( $array ) { for ( $i = 0; $i < 10000000; $i++ ) { $a = count( $array ) == 0; } }
It takes around 2us per iteration that way, regardless of array size.
-- Tim Starling
wikitech-l@lists.wikimedia.org