2
\$\begingroup\$

This is what I have:

function get_create returns a link with new filtered/sorted properties:

Code on screenshot

function get_create ($new_query = array(), $script = null, $ext = true, $amp = true) {  
   $pr_url = $GLOBALS['pref_url'].$GLOBALS['host_name'];
   $arr_query = array();

if (!empty($new_query)):

   $new_query = array_filter($new_query, function($element) {
        return !empty($element);}); //delete empty elements

 if ($ext) {
   $cur_url = str_replace("&", "&",$GLOBALS['cur_url'])
   $str_query = parse_url($cur_url, PHP_URL_QUERY); // get string of queries 
   parse_str($str_query,$arr_query); // create an array with queries

   $arr_query = array_replace_recursive($arr_query,$new_query)
   $arr_query = get_sort_by_key($arr_query); 
 }
 else {
   $arr_query = $new_query; // if $new_query empty
 }

 //make a new link with new parameters
  $fn_query = (!empty($arr_query) ? '?'.http_build_query($arr_query,'',($amp ? '&' : '')) : null);
  $fn_script = (!empty($script) ? $script : $GLOBALS['script_name']);

 return $pr_url.$fn_script.$fn_query; // return the link
else : 
 return $GLOBALS['home_url']; 
endif; 

} // end func

In parameters we have:

  1. $new_query (which contains the new properties for $_get part)
  2. $script (contain a new way to script, where data)
  3. $ext (boolean given parameter which make the logic different)
  4. $amp (if true then formats and again)

function get_sort_by_key helps with sorting array (the keys in array must be in this order: lng-topic-page-sort-search).

Code 2 on screenshot

function get_sort_by_key($array = array()) {
$new_array = array();
 foreach ($array as $key => $value) {
    $key = strtolower(trim($key));
    switch ($key) {
        case $key == 'lng': $order = 0; 
            break;
        case $key == 'topic': $order = 1; 
            break;
        case $key == 'page': $order = 2; 
            break;
        case $key == 'sort': $order = 3; 
            break;
        case $key == 'search': $order = 4; 
            break;
        default: $order = 5; break;     
    }
    $new_array[$order][$key] = $value;
 }

ksort($new_array);
   return $fn_array = call_user_func_array('array_merge',$new_array);
}

I need to create the function which gets an array full of new parameters and adds or replaces the current $_GET by them in a specific order.

An example:

I had the URL like this: http://localhost/test.php. After initialization func: get_create(array('sort' => 1, 'topic' => 2) I get http://localhost/test.php?topic=2&sort=1, where the order is important (not http://localhost/test.php?sort=1&topic=2).

How can I make the function look better? My code works, at least now. But I know that I'm wrong. Maybe some elegant solution exists in my case.

\$\endgroup\$
8
  • 2
    \$\begingroup\$ Is that your real code? There appear to be missing semi-colons on the 10th and 14th lines of the first function... \$\endgroup\$ Dec 18, 2017 at 20:11
  • \$\begingroup\$ If you fix the code so that it works, we'll re-open this question \$\endgroup\$
    – Snowbody
    Dec 19, 2017 at 15:44
  • \$\begingroup\$ Yes this is mine, the colons missing because I tried to make code more readable. \$\endgroup\$
    – Medion
    Dec 19, 2017 at 17:45
  • 1
    \$\begingroup\$ @Medion Please don't edit your actual code to make it more readable, we are fully capable of reading code :) Is there anything else that you've edited / simplified before posting here? \$\endgroup\$ Dec 19, 2017 at 17:52
  • 1
    \$\begingroup\$ That's good to hear, however: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question instead. \$\endgroup\$
    – Mast
    Dec 19, 2017 at 18:25

2 Answers 2

2
\$\begingroup\$

How can I make the function look better?

One alternative approach to the switch statement inside get_sort_by_key() is to define a lookup-table using an array:

define('KEY_MAPPING', ['lng','topic','page','sort','search']);

(or it could be a constant defined on a class if such a relevant class exists).

Then the code block in the foreach can be simplified using array_search() to see if a corresponding index is returned. If not (i.e. it returned false) then use 5 as the fallback value.

foreach ($array as $key => $value) {
    $key = strtolower(trim($key));
    $order = array_search($key, KEY_MAPPING);
    if ($order === false) {
        $order = 5;
    }
    $new_array[$order][$key] = $value;
 }

See this playground example for a demonstration.

For small lists of parameters like this the time difference won't be much. I found this SO question from 6 years ago (and the results may have changed since Ariel's answer as PHP internals have changed but likely not by much).

I tried to find some online posts comparing switch statements with array lookups. The first couple results I see in Google are 6-9 years old:

The general consensus is that the array lookup is faster. And typically it can require fewer lines of code. There is a quote somebody said (I can't remember who, so if you know who to give credit please let me know): "The easiest line to debug is the line you didn't write".

\$\endgroup\$
0
2
\$\begingroup\$
  1. The default behavior of array_filter() is greedy about destroying falsey values.

    $new_query = array_filter($new_query, function($element) { return !empty($element);});
    

    is identical in functionality to:

    $new_query = array_filter($new_query);
    
  2. I recommend consistently using curly braces on all constructs and logical tabbing to make your code more readable.

  3. When a scalar or array type variable is sure to exist and you want to check if it is empty/non-empty or falsey/truthy, then don't use empty(). Just use if ($variable) { to check. (e.g. $new_query, $arr_query, and $script)

  4. For get_sort_by_key(), a key-based lookup will always outperform a value-based lookup (in_array() or array_search()). Because php arrays are hash maps, key searches (array_key_exists() or isset()) are the way to go. Here is a basic demonstration of how easy and clean it is to execute a custom key sort: (Demo)

    $lookup = [
        'lng' => 5,
        'topic' => 4,
        'page' => 3,
        'sort' => 2,
        'search' => 1,
        // do not declare a key with a zero value
    ];
    
    uksort(
        $input,
        function($a, $b) use($lookup) {
            return ($lookup[$b] ?? 0) <=> ($lookup[$a] ?? 0);
        }
    );
    

    The above snippet uses descending values for your known keys. The zero value is deliberately excluded from the lookup because when the sorting algorithm finds a key which is not represented in the lookup, it is assigned a sorting value of 0 (it goes to the back of the array). The spaceship operator is a super-versatile tool. If you wanted to sort by your custom order THEN sort alphabetically in an ascending direction, you can simply declare two balanced arrays like this:

    return [$lookup[$b] ?? 0, $a] <=> [$lookup[$a] ?? 0, $b];
    

As for the maintainability of your code, whenever you want to adjust the priority of specific keys for custom sorting, you only need to touch your lookup array (not the uksort() block). This makes readability and maintenance a breeze.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.