Rewrite a confusing condition in default-widgets.php

[alexvorn2]

default-widgets.php file:

line 679:

 if ( empty( $instance['number'] ) || ! $number = absint( $instance['number'] ) )
 $number = 5;

shouldn't be ?:

 if ( empty( $instance['number'] ) || ! $number == absint( $instance['number'] ) )
 $number = 5;

[helen]

No. It's checking the result of absint() while assigning it to the variable for use later if not 0. Assignment in an if can be purposeful.

[scribu]

It can also be confusing, especially when negating the result.

[scribu]

A more readable version would be:

$number = false;

if ( !empty( $instance['number'] ) )
  $number = absint( $instance['number'] );

if ( !$number )
  $number = 5;

[alexvorn2]

Replying to scribu:

A more readable version would be:

$number = false;

if ( !empty( $instance['number'] ) )
  $number = absint( $instance['number'] );

if ( !$number )
  $number = 5;

agree! +1

[SergeyBiryukov]

23089.patch​ uses the pattern we already have in a couple of other places:
​http://core.trac.wordpress.org/browser/tags/3.5/wp-includes/default-widgets.php#L612

[azaozz]

Replying to scribu:

A more readable version...

+1. I think we had something about not doing assignments inside of if ( ... ) in the ​coding standards, not seeing it now.

Replying to SergeyBiryukov:

Yeah, that is far better:

$number = isset( $instance['number'] ) ? absint( $instance['number'] ) : 5; 

Perhaps the isset() has to be !empty() so it doesn't allow $number to be 0 (zero).

[MikeHansenMe]

+1 to readable code

[SergeyBiryukov]

Replying to azaozz:

Perhaps the isset() has to be !empty() so it doesn't allow $number to be 0 (zero).

Good point, 23089.2.patch​ uses !empty().

[kovshenin]

I rarely use the ternary operator, but when I do, I always put brackets around the condition, so that it's more obvious, easier to read and modify, just like an if statement.

$number = ( ! empty( $instance['number'] ) ) ? absint( $instance['number'] ) : 10;

[SergeyBiryukov]

Replying to kovshenin:

I rarely use the ternary operator, but when I do, I always put brackets around the condition

Yes, I prefer that form too. However, the form without the brackets appears to be prevailing throughout the core.

[nacin]

The problem with something like ! empty( $instance['number'] ) ? absint( $instance['number'] ) : 10 is if 'number' is malformed, it gets cast to 0, and that 0 never becomes a 5.

Something like this seems like it would be the most straightforward:

$title = ! empty( $instance['title'] ) ? $instance['title'] : __( 'Recent Posts' );
$title = apply_filters( 'widget_title', $title, $instance, $this->id_base );
$number = ! empty( $instance['number'] ) ? absint( $instance['number'] ) : 10;
if ( ! $number )
    $number = 10;

I also don't think this is too hard to read:

if ( empty( $instance['number'] ) || ! $number = absint( $instance['number'] ) )
   $number = 10;

Which happens to be exactly what we have now.

[SergeyBiryukov]

23089.3.patch​ simplifies the conditions without changing the functionality.

Also contains a bit of cleanup for WP_Widget_Recent_Comments::form() for consistency with WP_Widget_Recent_Posts::form() cleanup done in [21935].

[SergeyBiryukov]

In 23597:

Simplify logic in WP_Widget_Recent_Posts and WP_Widget_Recent_Comments. fixes #23089.