Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Below is my current code for sorting an array, "$jobs_list".

It's not very DRY and it's fugly. And there are several more similar case statements still to go in. How could I write it so my brain doesn't twitch to look at it?

switch ($sort) {
    case 'number':
        usort($jobs_list, function($a, $b){
            return ( (int) $a->number > (int) $b->number );
        });
        break;
    case 'client':
        usort($jobs_list, function($a, $b){
            return strcmp($a->client_contact_last_name, $b->client_contact_last_name);
        });
        break;
    case 'name':
        usort($jobs_list, function($a, $b){
            return strcmp($a->name, $b->name);
        });
        break;
    case 'status':
        usort($jobs_list, function($a, $b){
            return ( (int) $a->status > (int) $b->status );
        });
        break;
    case 'revenue':
        usort($jobs_list, function($a, $b){
            return ( (float) $a->base_revenue > (float) $b->base_revenue );
        });
        break;
    case 'profit':
        usort($jobs_list, function($a, $b){
            return ( (float) $a->profit > (float) $b->profit );
        });
        break;
    case 'margin':
        usort($jobs_list, function($a, $b){
            return ( (float) $a->margin > (float) $b->margin );
        });
        break;

    default:
        # do nothing...
}
share|improve this question

migrated from stackoverflow.com Dec 4 '12 at 16:07

This question came from our site for professional and enthusiast programmers.

4 Answers 4

up vote 6 down vote accepted

First, it may make sense to make sure that the field you're checking in $sort, is the same as the propertyname of the object in $jobs_list.

Also, it looks like there's a lot of identical sort functions; the only difference being that the property you're checking is different.

You could just combine the switch statements that have an identical sorting strategy (revenue, status, profit, margin, etc..) and turn the field into a variable, such as:

switch ($sort) {
    case 'number':
    case 'status':
    case 'profit':
    case 'margin':
        usort($jobs_list, function($a, $b) use ($sort) {
            return ( (float) $a->$sort > (float) $b->$sort );
        });
        break;

}
share|improve this answer
    
Of course. I was looking at my code thinking "this is... wrong" - combining the case statements never occurred to me. Doh. Thank you - your point about checking the property name also taken on board :) –  Wintermute Dec 4 '12 at 15:37
    
Bad thing is: $sort is not always stating the property that should be compared. And I like it so - a mapping is a good idea in this spot, because requested ordering and existing object property need not correlate. –  Sven Dec 4 '12 at 23:16

I tried to minimise the obvious copy/paste repetition as much as possible, without sacrificing performance (which moving the switch into the comparator would do):

usort($jobs_list, get_sort_function($sort));

function get_sort_function($sort) {
    switch ($sort) {
        case 'number':
            return function($a, $b) {
                return ( (int) $a->number > (int) $b->number );
            }
        case 'client':
            return function($a, $b) {
                return strcmp($a->client_contact_last_name, $b->client_contact_last_name);
            }
    }
}

I don't have a PHP interpreter to hand so I apologise in advance if this isn't quite the right syntax, but hopefully you get the idea from the code anyway.

share|improve this answer

I came across this solution, avoiding the switch statement alltogether:

$sort_functions = array(
    'number' => function($a, $b){ return ( (int) $a->number > (int) $b->number ); },
    'client' => function($a, $b){ return strcmp($a->client_contact_last_name, $b->client_contact_last_name); },
    'name' => function($a, $b){ return strcmp($a->name, $b->name); },
    'status' => function($a, $b){ return ( (int) $a->status > (int) $b->status ); },
    'revenue' => function($a, $b){ return ( (float) $a->base_revenue > (float) $b->base_revenue ); },
    'profit' => function($a, $b){ return ( (float) $a->profit > (float) $b->profit ); },
    'margin' => function($a, $b){ return ( (float) $a->margin > (float) $b->margin ); },
);

if (isset($sort_functions[$sort])) {
    usort($jobs_list, $sort_functions[$sort]);
}

Bonus: It is easily extensible should new fields appear. Negative: Still a lot of code duplication for three basic comparisons... But we will address it:

Extracting all basic functions into separate variables (an array would work here, too), and adding the field that should be compared to this function, you get an almost universal arsenal of comparison of object properties.

$intcompare = function($a, $b, $field){ return ( (int) $a->$field > (int) $b->$field ); };
$stringcompare = function($a, $b, $field){ return strcmp($a->$field, $b->$field); };
$floatcompare = function($a, $b, $field){ return ( (float) $a->$field > (float) $b->$field); };

Call these functions by passing them into the anonymous functions for each field.

$sort_functions = array(
    'number' => function($a, $b) use ($intcompare) { return $intcompare($a, $b, 'number'); },
    'client' => function($a, $b) use ($stringcompare) { return $stringcompare($a, $b, 'client_contact_last_name'); },
    'name' => function($a, $b) use ($stringcompare) { return $stringcompare($a, $b, 'name'); },
    'status' => function($a, $b) use ($intcompare) { return $intcompare($a, $b, 'status' ); },
    'revenue' => function($a, $b) use ($floatcompare) { return $floatcompare($a, $b, 'base_revenue' ); },
    'profit' => function($a, $b) use ($floatcompare) { return $floatcompare($a, $b, 'profit' ); },
    'margin' => function($a, $b) use ($floatcompare) { return $floatcompare($a, $b, 'margin' ); },
);

if (isset($sort_functions[$sort])) {
    usort($jobs_list, $sort_functions[$sort]);
}

Testing if it works:

$jobs_list = array(
    (object) array('number' => 1),
    (object) array('number' => 0),
 );
$sort = 'number';

var_dump($jobs_list);

if (isset($sort_functions[$sort])) {
    usort($jobs_list, $sort_functions[$sort]);
}

var_dump($jobs_list);

Result:

array(2) {
  [0] =>
  class stdClass#11 (1) {
    public $number =>
    int(1)
  }
  [1] =>
  class stdClass#12 (1) {
    public $number =>
    int(0)
  }
}

array(2) {
  [0] =>
  class stdClass#12 (1) {
    public $number =>
    int(0)
  }
  [1] =>
  class stdClass#11 (1) {
    public $number =>
    int(1)
  }
}
share|improve this answer

I'd start by moving the switch statement into the callback function:

usort($jobs_list, function($a, $b){
 switch ($sort):
      case 'number':return ( (int) $a->number > (int) $b->number ); break;
      case 'client':return strcmp($a->client_contact_last_name, $b->client_contact_last_name); break;
      //etc...
 endswitch;
 });
share|improve this answer
2  
This would be a lot worse performance wise –  Evert Dec 4 '12 at 15:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.