Closures within a switch: cool or stupid?

NogDog · Nov 13, 2012

Came up with the following function to perform an array sort, making use of closures to define the callback function that will be used by usort(). Trying to decide if it's a decent way to accomplish this, or if there might be a cleaner way?

	/**
	 * Sort the array of reviews
	 * @return array
	 * @param array $data
	 * @param string $orderBy
	 */
	private function applySort(array $data, $orderBy)
	{
		switch($orderBy) {
			case 'mosthelpful':
				$sortFunc = function($a, $b) {
					$diff = strtotime($b['likes']) - strtotime($a['likes']);
					return ($diff != 0) ? $diff :
						strtotime($b['review_timestamp']) - strtotime($a['review_timestamp']);
				};
				break;
			case 'leasthelpful':
				$sortFunc = function($a, $b) {
					$diff = strtotime($b['dislikes']) - strtotime($a['dislikes']);
					return ($diff != 0) ? $diff :
						strtotime($b['review_timestamp']) - strtotime($a['review_timestamp']);
				};
				break;
			case 'oldtonew':
				$sortFunc = function($a, $b) {
					return strtotime($a['review_timestamp']) - strtotime($b['review_timestamp']);
				};
				break;
			case 'newtoold':
			default:
				$sortFunc = function($a, $b) {
					return strtotime($b['review_timestamp']) - strtotime($a['review_timestamp']);
				};
		}
		usort($data, $sortFunc);
		return $data;
	}

traq · Nov 13, 2012

I like it. My first thought was to declare all of the closures (allowing you to use the 'newtoold' closure inside your 'mosthelpful'/'leasthelpful' closures), and do away with the switch statement:

function applySort_revised( array $data,$orderBy='newtoold' ){

$oldtonew = function($a, $b) {
    return strtotime($a['review_timestamp']) - strtotime($b['review_timestamp']);
};
$newtoold = function($a, $b) {
    return strtotime($b['review_timestamp']) - strtotime($a['review_timestamp']);
};
$mosthelpful = function( $a,$b )use( $newtoold ){
    $diff = strtotime($b['likes']) - strtotime($a['likes']);
    return ($diff != 0) ? 
        $diff :
        $newtoold( $a,$b );
};
$leasthelpful = function($a, $b)use( $newtoold ){
    $diff = strtotime($b['dislikes']) - strtotime($a['dislikes']);
    return ($diff != 0) ? 
        $diff :
        $newtoold( $a,$b );
};

if( empty( $$orderBy ) ){ $orderBy = 'newtoold'; }

usort($data, $$orderBy);
return $data;
}

This offers similar performance, so there's no clear advantage.

However, neither method seems to scale well (testing 1,000 iterations each, I hit max execution time with only 50 elements* in the $data array).

*which I assumed looked something like this (using arbitrary values):

[
    'likes'             => '5/10/12'
   ,'dislikes'          => '1/3/12'
   ,'review_timestamp'  => '10/1/12'
]

BTW, why are 'likes' and 'dislikes' time values?[/i]

NogDog · Nov 13, 2012

Likes and dislikes would be simple integer values (how many users liked or disliked that review), and for those with the same number, they should then be sorted new-to-old. (And if any of that logic bothers you, that's not negotiable. )

NogDog · Nov 13, 2012

PS: I do like the the idea of new-to-old being defined once and used as needed. Hmm....

traq · Nov 13, 2012

NogDog;11017363 wrote:
Likes and dislikes would be simple integer values ...

That's what I assumed, but then why

strtotime($a['likes']) // et.all.

?

NogDog wrote:
PS: I do like the the idea of new-to-old being defined once and used as needed. Hmm....

That's what I was originally thinking of, but it didn't give the benefit I expected.

[edit]

removing those strtotime() calls and treating likes/dislikes as integers almost halves your execution time.

NogDog · Nov 13, 2012

Heh...stupid copy-and-paste errors...now I have to send myself email to check tomorrow whether that bug is still in the actual code (applying strtotime() to the likes/dislikes). Thanks for catching that (hasn't gone to QA yet).

traq · Nov 13, 2012

NogDog;11017363 wrote:
Likes and dislikes would be simple integer values (how many users liked or disliked that review), and for those with the same number, they should then be sorted new-to-old. (And if any of that logic bothers you, that's not negotiable. )

Doesn't bother me per se (and I certainly understand if it's simply "non negotiable"), but have you considered using "dislikes" to break a "likes" tie (and vice-versa)? e.g.,

##  untested  ##
    $mosthelpful = function( $a,$b )use( $leasthelpful,$newtoold ){ 
        $diff = $b['likes'] - $a['likes']; 
        if( $diff == 0 ){ // tied for likes
            // negate result: we want the *least* disliked, not the *most* disliked
            $diff = -( $leasthelpful( $a,$b ) );
        }
        if( $diff == 0 ){ // tied for dislikes also
            $diff = $newtoold( $a,$b );
        }
        return $diff;
    };

NogDog;11017369 wrote:
...stupid copy-and-paste errors...

that explains that, then.

NogDog · Nov 14, 2012

I've actually considered looking at different formulas for figuring out some sort of total "likeability" rating taking both counts into consideration, with weighting factors based on total number of responses and so forth, but the powers that be want to keep it simple (for now). Obviously it's not exactly perfect right now, since one review could, in theory, show up at the top of both the most helpful and least helpful sorts, which at least seems illogical at first glance (and second glance?).

Oh, and if anybody is wondering why I'm not sorting it in the DB query, it's because part of the review data comes from a DB query and the other part comes from a separate API call to another data source, and for now this is the easiest way to get the two data sets merged and sorted (subject to change without notice ).

Weedpacket · Nov 15, 2012

I'm late here, but I'm stepping in to say I like it. I know there are lots of people who want OO with everything, but in cases like this a functional approach is much cleaner than a whole class and a bunch of instances all for the sake of implementing one method.

One idea that comes to me when I see this is the idea of having the caller function provide the sort criterion function, rather than have applySort construct it. Then the criterion can make use of additional values drawn from the scope of the caller. (You can still have a lookup of really common criteria, and have applySort look to see if it was passed a callable thing or the name of something in the lookup table.) You could also refactor the common parts of those functions into a more generic version.

function sortByField($field)
{
	// Determine appropriate comparison function to use for this type of field
	// (strnatcasecmp() might be enough depending on the format used for timestamps)

// Or have a distinct lookup function that maintains an array of fieldname => comparator pairs.

$comparator = 'strnatcasecmp';

return function($a,$b)use($field, $comparator)
{
	return $comparator($b[$field], $a[$field]);
};
};


function breakTiesWithReviewTimestamp($criterion)
{
	return function($a, $b)use($criterion)
	{
		return $criterion($a, $b) ?: strtotime($b['review_timestamp']) - strtotime($a['review_timestamp']);
	}
}



function applySort(array $data, $orderBy)
{
	switch($orderBy) {
		case 'mosthelpful':
		$sortFunc = breakTiesWithReviewTimestamp(sortByField('likes'));
		break;
	case 'leasthelpful':
		$sortFunc = breakTiesWithReviewTimestamp(sortByField('dislikes'));
		break;
	case 'oldtonew':
		$sortFunc = breakTiesWithReviewTimestamp(function($a, $b) { return 0;});
		$sortFunc = function($a, $b)use($sortFunc)
		{
			return -$sortFunc($b, $a);
		}; // A "reverseSortOrder" wrapper function?
		break;
	case 'newtoold':
	default:
		$sortFunc = breakTiesWithReviewTimestamp(function($a, $b) { return 0;});
		break;
	}
	usort($data, $sortFunc);
	return $data;
}

NogDog · Nov 15, 2012

Thanks for the feedback, guys. This sort of discussion is always a lot more "fun" than, say, "Why do I get this 'not a resource' error?"

Unfortunately (or fortunately), after what I'd cobbled together in a day-and-a-half for what was supposedly an urgent requirement has suddenly turned into an "Oops, sorry for the miscommunication, but we don't want/need that feature." :rolleyes: So now I have to pull it out of the main git branch for that project and stick it away in its own branch for if/when it becomes a requirement again -- at which point we'll probably decide to do it in a completely different way.

traq · Nov 15, 2012

that got me thinking. in terms of performance, the best approach is to declare the closure outside of the function, and pass it in as a argument (or use via global), rather than declaring it each time inside the function:

<?php

## passing closure - 1,000 iterations: avg.0008080s

function closureTest( $closure ){
    $closure();
    /* do more stuff */ 
}
$closure = function(){ /* do stuff */ };
closureTest( $closure );

## global closure - 1,00 iterations: avg.0008709s

function closureTest(){
    global $closure;
    $closure();
    /* do more stuff */ 
}
$closure = function(){ /* do stuff */ };
closureTest();

## local closure - 1,000 iterations: avg.0012829s

function closureTest(){
    $closure = function(){ /* do stuff */ };
    $closure();
    /* do more stuff */ 
}
closureTest();

makes sense, but defeats encapsulation.

I was trying to do something like:

<?php

function closureTest(){
    static $closure;
    $closure = function(){ /* do stuff */ };
    $closure();
    /* do more stuff */ 
}
closureTest();

but PHP didn't like it

NogDog · Nov 15, 2012

In this particular use case, it would only get defined once on any given page request, but from a scalability issue it's definitely worth thinking about. I suppose the OOP alternative might be to use a decorator pattern, though the framework we're using makes that a bit clunkier than I'd like. Hmm...now I'm thinking as I type (a dangerous thing): we have a sort of Registry-like class in which I could create/store the selected closure-style sort function, create an interceptor class for that particular action that would populate the registry with the applicable closure based on the url criteria, then use that in the method where I have to do the sort....

Now I have to send myself another email -- just in case this rises from the grave at some point.

traq · Nov 15, 2012

Strategy pattern (passing the closure as an arg).

Weedpacket · Nov 15, 2012

traq wrote:

I was trying to do something like:

<?php

function closureTest(){
    static $closure;
    $closure = function(){ /* do stuff */ };
    $closure();
    /* do more stuff */ 
}
closureTest();

but PHP didn't like it

How do you mean? It works for me without trouble.

traq · Nov 15, 2012

ha...!

I must have been typing something wrong last night. Yeah, it seems to work!

function closureTest(){
    static $closure;
    if( !is_callable( $closure ) ){
        print "defining closure\n";
        $closure = function(){ return "something\n"; };
    }
    print $closure();
}

closureTest();
closureTest();
closureTest();
/* output:
defining closure
something
something
something
*/

only defined once

Last night I kept getting parse errors and/or it always redefined the closure. But I was really tired.

... [font=monospace]static[/font] (or possibly [font=monospace]is_callable()[/font]) seems to kill performance, not help it. 1,000 iterations (same test as before) took .01s; it's better to just define the closure each time!

Weedpacket · Nov 15, 2012

How about

static $closure = null;
if(!isset($closure)) {

?

traq · Nov 16, 2012

tried that earlier; no significant effect on execution time. I guess [font=monospace]static[/font] is the expensive part.

Weedpacket · Nov 16, 2012

I think both that and the test hold things up.

<?php
function closureTest(){
//	static $closure = null;
//	if(!isset($closure))
	$closure = function(){ return "something\n"; };
 	return $closure();
}

From my own timings (static vs. nonstatic, checked vs. nonchecked) enabling both or neither are roughly the same, and are significantly better than doing only one.

Then again, how does

function closure()
{
    return "something\n";
}

function closureTest(){
    print closure();
}

closureTest();
closureTest();
closureTest();

perform? My point being, that a main strength of closures is that they "close over" variables in their defining scope, so that you can customise the function on the fly. If it has the same body every time you create it you might as well just make it an ordinary function - one with a name that you can refer to it by.

Closures within a switch: cool or stupid?

NogDog

Ttraq

NogDog

NogDog

Ttraq

NogDog

Ttraq

NogDog

Weedpacket

NogDog

Ttraq

NogDog

Ttraq

Weedpacket

Ttraq

Weedpacket

Ttraq

Weedpacket