1
\$\begingroup\$

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...
}
\$\endgroup\$
0

5 Answers 5

7
\$\begingroup\$

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;

}
\$\endgroup\$
2
  • \$\begingroup\$ 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 :) \$\endgroup\$
    – Wintermute
    Dec 4, 2012 at 15:37
  • \$\begingroup\$ 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. \$\endgroup\$
    – Sven
    Dec 4, 2012 at 23:16
2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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)
  }
}
\$\endgroup\$
1
\$\begingroup\$

So far I don't see anything indicating that you need something other than ASC sorting. The spaceship operator auto-magically compares numeric values as numeric values and non-numeric strings as strings, so it seems to me that there is currently no benefit to creating conditional sorting calls.

Just pass the target column into usort() using use() (if under php7.4).

$column = 'name';
usort(
    $jobs_list,
    function($a, $b) use($column) {
        return $a->$column <=> $b->$column;
    }
);
var_export($job_list);

Above PHP7.4, you can use arrow function syntax.

$column = 'name';
usort($jobs_list, fn($a, $b) => $a->$column <=> $b->$column);
var_export($job_list);
\$\endgroup\$
-1
\$\begingroup\$

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;
 });
\$\endgroup\$
1
  • 2
    \$\begingroup\$ This would be a lot worse performance wise \$\endgroup\$
    – Evert
    Dec 4, 2012 at 15:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.