Closures within a switch: cool or stupid?
Page 1 of 2 12 LastLast
Results 1 to 15 of 18

Thread: Closures within a switch: cool or stupid?

  1. #1
    High Energy Magic Dept. NogDog's Avatar
    Join Date
    Aug 2006
    Location
    Ankh-Morpork
    Posts
    13,943

    Closures within a switch: cool or stupid?

    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?

    PHP Code:
        /**
         * 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;
        } 
    Last edited by NogDog; 11-13-2012 at 11:53 AM.
    Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be." ~ from Nation, by Terry Pratchett

    "But the main reason that any programmer learning any new language thinks the new language is SO much better than the old one is because hes a better programmer now!" ~ http://www.oreillynet.com/ruby/blog/...ck_to_p_1.html


    eBookworm.us

  2. #2
    Senior Member traq's Avatar
    Join Date
    Jun 2011
    Location
    so.Cal
    Posts
    949
    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:
    PHP Code:
    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):
    Code:
    [
        'likes'             => '5/10/12'
       ,'dislikes'          => '1/3/12'
       ,'review_timestamp'  => '10/1/12'
    ]
    BTW, why are 'likes' and 'dislikes' time values?
    Last edited by traq; 11-13-2012 at 04:39 PM.

  3. #3
    High Energy Magic Dept. NogDog's Avatar
    Join Date
    Aug 2006
    Location
    Ankh-Morpork
    Posts
    13,943
    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. )
    Last edited by NogDog; 11-13-2012 at 05:03 PM.
    Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be." ~ from Nation, by Terry Pratchett

    "But the main reason that any programmer learning any new language thinks the new language is SO much better than the old one is because hes a better programmer now!" ~ http://www.oreillynet.com/ruby/blog/...ck_to_p_1.html


    eBookworm.us

  4. #4
    High Energy Magic Dept. NogDog's Avatar
    Join Date
    Aug 2006
    Location
    Ankh-Morpork
    Posts
    13,943
    PS: I do like the the idea of new-to-old being defined once and used as needed. Hmm....
    Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be." ~ from Nation, by Terry Pratchett

    "But the main reason that any programmer learning any new language thinks the new language is SO much better than the old one is because hes a better programmer now!" ~ http://www.oreillynet.com/ruby/blog/...ck_to_p_1.html


    eBookworm.us

  5. #5
    Senior Member traq's Avatar
    Join Date
    Jun 2011
    Location
    so.Cal
    Posts
    949
    Quote Originally Posted by NogDog View Post
    Likes and dislikes would be simple integer values ...
    That's what I assumed, but then why
    PHP Code:
    strtotime($a['likes']) // et.all. 
    ?

    Quote Originally Posted by NogDog
    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.
    Last edited by traq; 11-13-2012 at 05:46 PM.

  6. #6
    High Energy Magic Dept. NogDog's Avatar
    Join Date
    Aug 2006
    Location
    Ankh-Morpork
    Posts
    13,943
    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).
    Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be." ~ from Nation, by Terry Pratchett

    "But the main reason that any programmer learning any new language thinks the new language is SO much better than the old one is because hes a better programmer now!" ~ http://www.oreillynet.com/ruby/blog/...ck_to_p_1.html


    eBookworm.us

  7. #7
    Senior Member traq's Avatar
    Join Date
    Jun 2011
    Location
    so.Cal
    Posts
    949
    Quote Originally Posted by NogDog View Post
    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.,
    PHP Code:
    ##  untested  ##
        
    $mosthelpful = function( $a,$b )use( $leasthelpful,$newtoold ){ 
            
    $diff $b['likes'] - $a['likes']; 
            if( 
    $diff == ){ // tied for likes
                // negate result: we want the *least* disliked, not the *most* disliked
                
    $diff = -( $leasthelpful$a,$b ) );
            }
            if( 
    $diff == ){ // tied for dislikes also
                
    $diff $newtoold$a,$b );
            }
            return 
    $diff;
        }; 
    Quote Originally Posted by NogDog View Post
    ...stupid copy-and-paste errors...
    that explains that, then.
    Last edited by traq; 11-13-2012 at 07:15 PM.

  8. #8
    High Energy Magic Dept. NogDog's Avatar
    Join Date
    Aug 2006
    Location
    Ankh-Morpork
    Posts
    13,943
    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 ).
    Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be." ~ from Nation, by Terry Pratchett

    "But the main reason that any programmer learning any new language thinks the new language is SO much better than the old one is because hes a better programmer now!" ~ http://www.oreillynet.com/ruby/blog/...ck_to_p_1.html


    eBookworm.us

  9. #9
    Pedantic Curmudgeon Weedpacket's Avatar
    Join Date
    Aug 2002
    Location
    General Systems Vehicle "Thrilled To Be Here"
    Posts
    21,885
    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.
    PHP Code:
    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;

    THERE IS AS YET INSUFFICIENT DATA FOR A MEANINGFUL ANSWER
    FAQs! FAQs! FAQs! Most forums have them!
    Search - Debugging 101 - Collected Solutions - General Guidelines - Getting help at all

  10. #10
    High Energy Magic Dept. NogDog's Avatar
    Join Date
    Aug 2006
    Location
    Ankh-Morpork
    Posts
    13,943
    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." 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.
    Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be." ~ from Nation, by Terry Pratchett

    "But the main reason that any programmer learning any new language thinks the new language is SO much better than the old one is because hes a better programmer now!" ~ http://www.oreillynet.com/ruby/blog/...ck_to_p_1.html


    eBookworm.us

  11. #11
    Senior Member traq's Avatar
    Join Date
    Jun 2011
    Location
    so.Cal
    Posts
    949
    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 Code:
    <?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 Code:
    <?php

    function closureTest(){
        static 
    $closure;
        
    $closure = function(){ /* do stuff */ };
        
    $closure();
        
    /* do more stuff */ 
    }
    closureTest();
    but PHP didn't like it
    Last edited by traq; 11-14-2012 at 11:15 PM.

  12. #12
    High Energy Magic Dept. NogDog's Avatar
    Join Date
    Aug 2006
    Location
    Ankh-Morpork
    Posts
    13,943
    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.
    Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be." ~ from Nation, by Terry Pratchett

    "But the main reason that any programmer learning any new language thinks the new language is SO much better than the old one is because hes a better programmer now!" ~ http://www.oreillynet.com/ruby/blog/...ck_to_p_1.html


    eBookworm.us

  13. #13
    Senior Member traq's Avatar
    Join Date
    Jun 2011
    Location
    so.Cal
    Posts
    949
    Strategy pattern (passing the closure as an arg).

  14. #14
    Pedantic Curmudgeon Weedpacket's Avatar
    Join Date
    Aug 2002
    Location
    General Systems Vehicle "Thrilled To Be Here"
    Posts
    21,885
    Quote Originally Posted by traq
    I was trying to do something like:
    PHP Code:
    <?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.
    THERE IS AS YET INSUFFICIENT DATA FOR A MEANINGFUL ANSWER
    FAQs! FAQs! FAQs! Most forums have them!
    Search - Debugging 101 - Collected Solutions - General Guidelines - Getting help at all

  15. #15
    Senior Member traq's Avatar
    Join Date
    Jun 2011
    Location
    so.Cal
    Posts
    949
    ha...!

    I must have been typing something wrong last night. Yeah, it seems to work!
    PHP Code:
    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.

    ... static (or possibly is_callable()) 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!

Thread Information

Users Browsing this Thread

There are currently 1 users browsing this thread. (0 members and 1 guests)

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •