Generating filters to remove query variables from a URL

Question

This function takes a URL structure which is most often a query string (but sometimes not) and generates filters to remove query variables from the URL.

If you're looking at an archive like

http://example.com/?genre=rock&band=deep-purple

you'll get the following output:

[ROCK X] [DEEP PURPLE X]

so you can remove those "filters".

I have a lot (too much maybe) of nested structure-- if anyone has any general optimization or refactoring pointers for me I'd really appreciate it!

<?php
function event_filters() {
    $output = '';
    $filterclear = get_bloginfo('wpurl').'/events';
    $filterclass = 'filter-clear cp block fl search-tag tdn pr con ic-right ic-x c2h btn bg1 c2';
    $queryString = $_SERVER['QUERY_STRING'];
    $home = get_bloginfo('wpurl');

    parse_str($queryString, $params);   
    $filter_count = count($params);

    // IF IS SINGLE DAY VIEW
    if(tribe_is_day()) {
        $day_title = ucwords(strftime( '%e de %B ', strtotime( get_query_var('eventDate') )));
        $output .= '<a class="'.$filterclass.'" title="Remove filter: ' . $day_title . '" href="'.$filterclear.'">'.$day_title.'</a>';
    }

    foreach ($params as $key => $term) {
        $tags = explode(' ',$term);
        $tag_count = count($tags);

        $term = get_term_by('slug', $term, $key);
        $term_name = $term->name;

        if($filter_count == 1 && $tag_count <= 1) {
            // IF IS ONE KEY AND ONE TERM
            $output .= '<a class="'.$filterclass.'" title="Remove filter: ' . $term_name . '" href="'.$filterclear.'">'.$term_name.'</a>';
        } else {
            if($tag_count > 1) {
                foreach($tags as $tag) {    
                    // IF THERE IS MORE THAN ONE TERM PER KEY, LOOP THROUGH EACH TERM    
                    $the_tag = get_term_by('slug', $tag, $key);
                    $tag_name = $the_tag->name;

                    $filterclear = str_replace($tag, '', $queryString);
                    $filterclear = str_replace(array('=+', '++', '+&'), array('=', '+', '&'), $filterclear);

                    if(substr($filterclear, -1) == '+'){
                        $filterclear = substr($filterclear,0,-1);
                    }

                    $output .= '<a class="'.$filterclass.'" title="Remove filter: ' . $tag_name . '" href="' . $home . '?'.$filterclear . '">'.$tag_name.'</a>';
                }
            } else {
                // OR ELSE THERES JUST ONE TERM PRESENT
                $filterclear = remove_query_arg($term,$queryString);
                $output .= sprintf("<a class=\"$filterclass\" title=\"Remove filter: $term_name\" href=\"$blogurl/?%s\">%s</a>", $filterclear, $term_name);
            } // END TAG_COUNT
        } // END FILTER_COUNT
    } // END FOR EACH

    return $output;
}
?>

Answer 1

Breaking Up Is Hard To Do

My first suggestion would be to break up your one function into many. No need to do all of that in just one function. The best way to do this would be to go about it logically. That is to say, break up your function by their purpose. For instance you could create a get_params() function like the following.

function getParams($queryString) {
    parse_str($queryString, $params);
    return $params;
}

This is a poor example because it doesn't take much from your code, but it does show how your functions should be specialized. Break them up so that they do only one thing and do it well. Even if you don't find yourself reusing that function it will help you in the long run. Don't believe me? Try it and then run your program again. Where is that error coming from now? I am almost positive it will have narrowed the amount of code you need to look at and will probably tell you in which function to look through. Here's a couple more small functions for you. Not to say there aren't more, these are just some maybe not so obvious ones to get you started.

function getOutput($class, $title, $href) {
    return '<a class="' . $class . '" title="Remove filter: ' . $title . '" href="' . $href . '">' . $title . '</a>';
}

function getTermBy($key, $term) {
    return get_term_by('slug', $term, $key);
}

That last function might not be all that great, but if this were a class you could make it so that you'd only have to pass the $term to it, which would be better and cleaner. I tend to tell everyone to ignore OOP until they are ready for it, just as I am telling you. Work on the suggestions in this answer and once this all becomes second nature I think you'd be ready for it. For the time being, just breaking up your code will make it easier to transition into classes should you ever decide to convert this over to one.

What's This Now!

It may be common knowledge what some of these functions do ('get_bloginfo()`), but I haven't a clue. I'm assuming this is wordpress or something. Never got into it myself so I'm not sure. Never the less, you should be commenting your code to let people know what you're doing. You don't have to go into great detail, just enough to say what you expect the outcome to be. I do see that you have some comments, but they seem to be more structural rather than functional.

norunonfunctionsasitishardtoread

Look up, now look down, now back up at me. Sadly these run-on functions are hard to read.

$day_title = ucwords(strftime( '%e de %B ', strtotime( get_query_var('eventDate') )));

But with the power of good code separation your code could look as good as mine!

$eventDate = get_query_var('eventDate');
$eventDateTime = strtotime($eventDate);
$formattedTime = strftime('%e de %B', $eventDateTime);
$day_title = ucwords($formattedTime);
//OR
$day_title = get_query_var('eventDate');
$day_title = strtotime($day_title);
$day_title = strftime('%e de %B', $day_title);
$day_title = ucwords($day_title);

I've always wanted to do that :) Anyways, now that I've had my fun... It won't hurt anything to separate code like this, it will just increase the number of lines in your program. It will still run the same and the slight increase in filesize will be neglible and wont make it run any slower, or if it does, it will be so miniscule as to be unnoticeable.

Consistency

You seem to be struggling with how you want your code to look. This is very common in a new programmer who is still unsure as to what style they want to adopt. You should pick one and stick with it though. Right now you have oneword, camelCase, and under_score variables/functions with no consistency. The only other reason I could think of for code to look like this is if it were all copy-pasted in without fully understanding it, which should never be the case, nor do I think it is. This is not to say you need to pick just one. You can mix and match. Some people use camelcase for variables and underscore for functions, or vice-versa. Its entirely up to you, there is no right or wrong, just preference. But once you pick one you like you should stick with it.

If you ever join a team this will become a big factor, though your preferences will mater little and you'll have to conform your style to the project lead's. They do this for consistency and being consistent with your own code will get you in the hang of this.

Final Thoughts

I've not found your issue, but implementing these suggestions might help you find it. I had a little fun with the answer, so my apologies, but it should still be helpful and understandable.