5
\$\begingroup\$

This is an alphabetical navigation which shows only the letter that have posts in both that letter and the currently filtered "genero" taxonomy term.

I use multiple taxonomy queries to find "artistas" posts that, for example, are tagged as both "rock" and "funk".

The function works perfectly and outputs exactly what I want it to, but the multiple loops are really lagging the load time and I'm uncertain of how to optimize the function.

enter image description here

<?php
function empty_alfa($current, $i, $taxonomyvar) { 
    $output = '<li class="'.$current.'"><a>'.strtoupper($i).'</a></li>';
    return $output;
}

function is_alfa() {
    $alfa = filter_input(INPUT_GET, 'alfa', FILTER_SANITIZE_STRING | FILTER_SANITIZE_STRIPPED);
    $is_alfa = ( ! $alfa ? is_tax('alfa') : true );
    return $is_alfa;
}

function has_artists($i, $genre) {
    $has_artists = false;

    if(term_exists( $i, 'alfa' )) {
        $alfas[] = array(
            'taxonomy' => 'alfa',
            'terms' => $i,
            'field' => 'slug',
        );

        $genres[] = array(
            'taxonomy' => 'genero',
            'terms' => $genre,
            'field' => 'slug',
            'operator' => 'AND', 
        );

        $termquery['post_type'] = 'artistas';

        if(empty($genre)) {
            $termquery['tax_query'] = $alfas;
        } else {        
            $termquery['tax_query'] = array_merge($genres, $alfas);
                $termquery['tax_query']['relation'] = "AND";
        }

        $has_artists = get_posts($termquery);
    }

    return $has_artists;
}

function alfa_nav_output($home, $uri, $taxonomyvar) {
    foreach(range('a', 'z') as $i) :
        $current = ($i == $taxonomyvar) ? "bg1 round-res" : "";
        $empty_alfa = empty_alfa($current, $i, $taxonomyvar);

        if ( term_exists( $i, 'alfa' ) ){ 
            if( has_artists($i,$genre) && $i != $taxonomyvar ) {
                if(empty($genre)) {
                    $link = $home.'?alfa='.$i.$orden;
                } else {
                    $genrestring = (is_array($genre) ? implode('+',$genre) : $genrestring = $genre );
                    $link = $home.'?alfa='.$i.'&genero='.$genrestring.$orden;
                }
?>
                    <li class="<?php echo $current; ?>">
                        <?php echo sprintf('<a class="alfa-link" href="%s">%s</a>', $link, strtoupper($i) ) ?>
                    </li>
<?php
            } else {
                echo $empty_alfa;
            }
        } else {
            echo $empty_alfa;
        }
    endforeach;
}

function bam_artist_alfa() {
        $taxonomy = 'alfa';  
        $taxonomyvar = get_query_var($taxonomy);

        $uri = my_url();
        $home = 'http://buenosairesmusic.com/';

        if(strstr($uri, '/artista/') ) {
            $uri = str_replace('/artista/','/?alfa=', $uri);
            $uri = substr_replace($uri ,"",-1);
        }

        $all_link = removeqsvar($uri, 'alfa');
        $last = $all_link[strlen($all_link)-1]; 

        if($last == '?') $all_link = substr_replace($all_link ,"",-1);

        if($all_link == $home) $all_link = $home.'artistas';

        $all_current = (is_alfa() ? null : ' bg1 round-res' );
    ?>
                <ul class="bbw bo alfa-nav c2">
                    <li class="all-link<?php echo $all_current; ?>">                    
                        <a<?php if(is_alfa()) echo ' href="'.$all_link.'"'; ?>>A&ndash;Z</a>                    
                    </li>
<?php
    $query = $_SERVER['QUERY_STRING'];
    $genre = null;
    $orden = (isset($_GET) && isset($_GET['orden']) ? '&orden=fecha' : null);

    if( strstr($uri,'/genero/') ) {
        $genre = str_replace('/genero/','/?genero=', $uri);
        $genre = substr_replace($uri ,"",-1);
        $genre = explode('/',$genre);
        $genre = end($genre);
    } elseif( isset($_GET) && isset($_GET['genero']) ) {
        $genre = $_GET['genero'];
    }

    if(!empty($genre)) {
        $spaces = strpos($genre,' ');
        $genre = ($spaces === false ? $genre : explode(' ', $genre) );
    } 

    alfa_nav_output($home, $uri, $taxonomyvar);
?>          
                 </ul>
<?php } ?>
\$\endgroup\$

migrated from stackoverflow.com Apr 26 '12 at 0:50

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

  • \$\begingroup\$ Perhaps... but the amount of traffic that this site gets makes it ideal to maximize the potential for getting help. \$\endgroup\$ – j-man86 Apr 25 '12 at 4:18
  • 2
    \$\begingroup\$ Have to agree with Cygal. There's not enough information here to provide a decent answer, and adding more would really only confuse the issue as what you already have is so large and dificult to look through. I am planning to return to give you something to look at, but I am only able to stick around for a short time this morning. In the mean time, use microtime to start benchmarking your code to determine where the bottleneck is. I would suggest focusing on database queries. I'll try to post something before the end of the day. \$\endgroup\$ – mseancole Apr 25 '12 at 11:31
5
\$\begingroup\$

To find bottle necks in your code, take the following code and wrap it around the code you wish to benchmark. Take a look at the documentation for more information.

$start = microtime(true);
//code to benchmark.
$time = microtime(true) - $start;
echo "task took $time seconds";

Now, on to your current code. I can't tell you the parts that are running slow because: One, there is just too much here; And two, there's not enough here. Kind of an Oxymoron, but it makes sense, honest! So, instead I will just look at your code and give feedback. I will limit myself to just looking for redundancies so that it focuses more on your speed issues than anything else.

Now, ignoring what I just said, here's one fix you can do that is more design oriented rather than performance oriented. However it could have the added benefit of increasing your programs performance as well, which is why I mentioning it. The most glaring issue I have with your code is that your functions are HUGE! Functions are, first and foremost, meant to hold repetitive tasks. Secondly they are meant to make your code cleaner. I actually had to return to the top of your code while reading this to ensure that I wasn't looking at procedural code because I had forgotten I was looking at a function and thought that you had put another function in the middle of your code.

Update: Upon further observation, I still found this to be true. Your has_artists() function is in the middle of your bam_artist_alfa() function. Don't do this. Its hard to tell from your code because of how long it is, so I don't know if it was just a typo, or if you legitamately have it this way. This may even be part of your performance problem.

So first thing to do, is break your functions up so that they are cleaner. Added benefit will be that it will be easier to debug and read and it will be easier to wrap a function in those microtime blocks I showed you earlier than those long lines of code you have now. And as I already mentioned, it could have the added benefit of boosting your performance. Here's an example:

function bam_artist_alfa() {
    $taxonomy = 'alfa';  
    $uri = my_url();
    $home = 'http://buenosairesmusic.com/';

    // save the terms that have posts in an array as a transient
    $start = microtime(true);
    check_taxonomy();//new function
    $time = microtime(true) - $start;
    echo "It took $time seconds to save the terms";
    //plus all other code, I won't copy it all here
}

function check_taxonomy() {//one of those new functions in bam_artist_alfa
    if ( false === ( $alphabet = get_transient( 'bam_archive_alphabet' ) ) ) {
        // It wasn't there, so regenerate the data and save the transient
        $terms = get_terms($taxonomy);

        $alphabet = array();
        if($terms){
            foreach ($terms as $term){
                $alphabet[] = $term->slug;
            }
        }
        set_transient( 'bam_archive_alphabet', $alphabet );
    } 
}

etc...

I didn't actually change any of your code here, I only moved a chunk of it to a new function check_taxonomy(). Break your code up into similar, logical, blocks and this will help your code drastically. Both in readability, and hopefully, performance.

Here's the first performance issue I have spotted. Change the following:

        $genre = explode('/',$genre);
        $genre = end($genre);

To this:

        $genre = substr($genre, - strrpos($genre, '/'));

If you only want the last bit of a string that you know the starting positon of, you can just use substring. Exploding it into an array just to get the last element probably takes up more resources than are necessary. I'm not 100% sure about this, but it makes sense. Use that microtime function on both methods to determine which is faster for you.

Now, for the second performance issue I spotted. Your entire has_artists() function. I'm having a real problem getting through all these arrays, some of which are identical. If you find yourself reusing information, set it up higher in the code to be reused as the code progresses. Even if it is not used for certain parts, it will not hurt anything to have it declared. I ended up rewritting this entire function. Compare yours to mine. The size difference alone is obvious. There was a lot of redundancy here, and even reduced, it is still rather confusing and could probably be compressed more, but I don't understand it enough to try.

    function has_artists($i, $genre) {
        $temp = array(
            'taxonomy' => 'alfa',
            'terms' => $i,
            'field' => 'slug',
        );

        if( ! empty($genre)) {
            $temp['taxonomy'] = 'genero';
            $temp['terms'] = $genre;

            if(is_array($genre)) { $temp['operator'] = 'AND'; }
            $genres[] = $temp;

            $termquery['tax_query']['relation'] = "AND";
        }

        $alfaquery = array();//was not defined and should be
        $alfaquery[] = $temp;
        $termquery['tax_query'] = array_merge($genres, $alfaquery);
        $termquery['post_type'] = 'artistas';

        $has_artists = get_posts($termquery);

        return $has_artists;
    }

This is about as far as I got. The rest looks like it will benefit from the above changes. Try applying these methods to your other code to see if that helps.

Update for your update

I would move $have_artists = false; to the beginning of your has_artists() function, outside of the if/else statement. That way it can serve as a default value should you ever decide to extend it.

You did move a good portion of your code from the bam_artist_alfa() function, but you misunderstood what I was trying to tell you. The code in alfa_nav() function should be broken up further into multiple other functions. As it is, it is still very hard to read, especially with that HTML thrown in there. Later, you will find that removing the HTML entirely from your classes/functions will improve your code readability immensly. However, it is not important in this case because you are still working on separating everything into its logical parts. I'm not going to rewrite this function for you, but I will point out some issues and tell you how I would break it up. Maybe that will give you and idea of where to start. First some issues I found in your new function.

$uri and $home were never defined, yet you are using them in this new function. Pass them as a function argument, or retrieve it somehow or this function will not work.

I don't know what removeqsvar() does, but from the looks of it, you just reinvented PHP's substr.

Don't use an if statement without those brackets {}. Especially if you aren't going to be consistent about it. It makes your code harder to read.

Declare $is_alfa like so. The way you are currently doing it is very redundant. No need to check if get is set, just check if the element you are looking for is and whether it is empty. If you are using PHP 5.2 or better, you can use filter_input function to accomplish this easier. I did so in my example below. If you are not able to use this function, continue to do it the way you were, but using the suggestions I gave before I mentioned filter_input.

$alfa = filter_input(INPUT_GET, 'alfa', FILTER_SANITIZE_STRING | FILTER_SANITIZE_STRIPPED);
$is_alfa = ( ! $alfa ? is_tax('alfa') : true );

All the above could be thrown into a new function called is_alfa().

Stop checking if a TRUE/FALSE variable is true, just do the following.

$all_current = $is_alfa ? null : ' bg1 round-res';

Whether the above variable ONLY has TRUE/FALSE values is irrelevant. Unless you are using the absolute equality operator('===') any value besides FALSE/NULL/''/0 will be evaluated as TRUE.

The above could also be moved into its own function. It could be made reusable by passing a variable to it to replace those 'alfa' strings, that way the same could be accomplished for any value you wanted to check in a similar way.

For the purposes of this answer, I would say to move the HTML output into its own function. I would do the following, or something similar to it first though.

<?php if($is_alfa) : ?><a href="<?php echo $all_link; ?>"><?php endif; ?>
A&ndash;Z
<?php if($is_alfa) : ?></a><?php endif; ?>

The above if format is very common in View files(MVC), however it doesn't make a difference as it is a designer preference. But since you are already using it for a foreach loop in your main code body, I figured I would point out the more common place to use it. The foreach loop you are doing this for currently is out of place and inconsistent with the rest of your code.

I will stop here, mostly because I have developed a headache. Not from your code, its just been a long day, I actually really enjoy doing this. With everything I've shown you here, you should be able to take the last half of your function and finish separating it into logical pieces. Once that is done, you can then use those microtime functions around each new function as you call it to determine how long each takes to run. That way you will be able to determine where your program bottlenecks. Don't just do it to these new functions, do it to your old ones too, such as the very first one you use get_query_var().

\$\endgroup\$
  • \$\begingroup\$ Nice answer! Upvoted. I really think speed is a matter of "SQL queries in a loop" here though (if he has no more than a few hundred artists). \$\endgroup\$ – Quentin Pradet Apr 25 '12 at 19:02
  • \$\begingroup\$ @Cygal: I agree, though this can't hurt! \$\endgroup\$ – mseancole Apr 25 '12 at 19:04
  • \$\begingroup\$ I've updated my code... if you see anything else let me know! In general that code takes 0,177800893784 seconds to execute. \$\endgroup\$ – j-man86 Apr 25 '12 at 22:02
  • \$\begingroup\$ @j-man86: I've updated my answer to suit. One thing I did not mention in my update: To get an accurate reading, you will have to run it many times. The easiest way to do it would be to run it in loop a set number of times. Don't do this quite yet, finish setting up the rest of your code first and then do each separately. \$\endgroup\$ – mseancole Apr 25 '12 at 23:16
  • \$\begingroup\$ OK great... I added my latest code. I think I'm getting closer! \$\endgroup\$ – j-man86 Apr 26 '12 at 0:25
3
\$\begingroup\$

First of all, notice that declaring has_artists() in bam_artists_alfa is confusing. It doesn't make the function "local", since you can call it outside of has_artists() too.

It's hard to tell where the bottleneck is! You only have one visible loop, but who knows what get_posts(), get_terms(), set_transient() or get_query_var() do? Do they have loops? Do they query the database? Did you benchmark the time taken by your function? Do you know where you spent most of your time?

Usually, the best option you have is to make sure there is no request to the database in inner loops. Do all your requests beforehand and only work on PHP data.

You should also make sure to avoid mixing displaying and computations, since it will make the optimization easier to reason about.

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

One thing you can do is take out the array declarations from the has_artists method which gets called many times inside your loop.

Also, when you declare $genres[] in two different cases, the only difference is 'operator' => 'AND'.

Lastly, instead of printing each <li class ... as you loop, append the results to a variable and print the contents afterwards.

I did a test with the following code:

for($x=1, $x < 10000; $x++) {
    printf("%s", $x);
}

This was slow compared to the following code:

$out = ''; for($x=1, $x < 10000; $x++) { $out = sprintf("%s%s", $out, $x);}; echo $out;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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