2
\$\begingroup\$

I've developed some PHP that grabs all of a business listing's categories and then displays just the children categories. This is built on Wordpress. I'm still in the learning process and would appreciate some assistance here. Any optimizations you can suggest I make to my code would be really helpful. Please note, if it's really technical, there's a good chance it'll go over my head at this point, so examples are appreciated.

<?php 

$permalink = get_permalink( $id );

$seo = get_the_title()." : ";

$Category_links = ' Found in the ';

$term_list_category = wp_get_post_terms(get_the_ID(), 'listings_categories', array("fields" => "ids"));


//THIS CODE REMOVES PARENTS FROM BEING DISPLAYED IN THE LISTING CATEGORIES
foreach ($term_list_category as $k=>$term) {
    $children = get_term_children( $term, 'listings_categories');
if ($children)
    unset($term_list_category[$k]);
 }

$i = 0;

$count = count($term_list_category);

if ( $count > 0 ){

    foreach ( $term_list_category as $term_category ) {

        $thisCat = get_term_by( 'id', $term_category, 'listings_categories');


        $url = '<a id="'.$term_category.'" slug="'.$thisCat->{'slug'}.'" class="listing-links-cat" href="#" title="'.$thisCat->{'name'}.'" >'.$thisCat->{'name'}.'</a>';

        $i ++;

        $seo .= " " . $thisCat->{'name'} . "";

        $Category_links .= " " . $url . "";

        if($count-1 == $i){
            $Category_links .= " and ";  $seo .= ", ";
        }elseif($count > 1 && $count !== $i){
            $Category_links .= ", ";  $seo .= ", ";
        }

    }
    $Category_links .= " Categories";
?>

<? echo $Category_links;  ?> 
\$\endgroup\$
  • \$\begingroup\$ Welcome to CR, we hope you have a good time here. To ensure the best possible reviews (ie: most relevant to you, and others), it would be better for you to actually specify what you expect a review to bring to the table: Alternative ways to process the data? More efficient in terms of maintenance, or execution speed? Security advice, or more emphasis on coding style. Please take some time to list the things you want us to focus on. \$\endgroup\$ – Elias Van Ootegem Apr 28 '15 at 10:19
3
\$\begingroup\$

One thing that is quite important to really understand. It's something that is said all too often, but it's very true:

Premature optimzation is the root of all evil

By your own admission, you're using wordpress, and post some 30-odd lines of PHP code, asking us what you could do to improve performance. Well, I can tell you that some parts of your code are perhaps not as optimal as they could be, but really: you won't notice any difference even if your code were written as efficiently as possible. The real overhead still is the fact that you're using wordpress, which means there's a ton of code being executed you've probably not even looked at.

Is that a bad thing? No, of course not. What does that mean for you? Well, that you shouldn't be focussing on writing the optimal code just yet. First, you should define "optimized code".

Code can be optimized for:

  • Readability. Very important if you're collaborating, and if ever you have to revisit a piece of code half a year after writing it
  • Debugging. Code that is optimized for debugging is well documented, well structured, and has a lot of functions that all do a distinct task, and do it well, without an awful lot of complexity.
  • Speed. This is, I think what you're after, but at this stage, you shouldn't be. You really shouldn't be thinking about this yet, because code written for performance tends to be error prone, harder to read, and difficult to debug. You can optimize code once you've thoroughly tested every single expression of every statement several times over.
  • Though we seldom see this kind of optimization today, not too long ago, code could also be optimized for storage. Back in the day where a 10Mb drive was worth the equivalent of a years' salary. This point is kind of irrelevant, but just for completeness' sake, I thought I'd mention is.

Having said that, I really do feel as though you should be focussing on writing clean code. Code that is easy to debug, easy to understand and is more self-documenting. I'll go through your code bit by bit, and give a couple of tips to optimize both in terms of readability and execution-time performance (though, again, in terms of speed, there won't be much between the two versions).

$permalink = get_permalink( $id );
$seo = get_the_title()." : "; //more on this var later
//camelCase the vars, it's more common and for many, more readable
$categoryLinks = ' Found in the ';
//spread out function calls that get too long
$termListCategory = wp_get_post_terms(
    get_the_ID(),//this way, you can comment on each param you're passing
    'listings_categories',//like so: this is some string
    array(
        'fields' => 'ids'//assoc array argument specifying something...
    )
);
/** only display children, if term has children, remove it from the array
 * Just like it might be a good idea to use multi-line comments
 * and don't use SHOUT CASE in documentation, why would you?
 */
foreach ($termListCategory as $k => $term)
{//whitespace is good, use it more
    //this assignment is a bit redundant
    $children = get_term_children(
        $term,
        'listings_categories'
    );
    //this is technically valid, but it's not safe
    //think back to apple's infamous goto fail; bug, use brackets
    if ($children)
    {
        unset($termListCategory[$k]);
    }
}

an alternative way to write this loop would be this:

foreach ($termListCategory as $k => $term)
{
    if (get_term_children($term, 'listings_categories'))
    {
        unset($termListCategory[$k]);
    }
}

That, to me at least looks a lot cleaner. This is assuming you can't do anything about the return value of wp_get_post_terms like: structuring the data differently, using different query methods and all that, so that you don't have to fetch data that you're just going to filter from the array anyway.

All that asside, let's continue:

$i = 0;//This doesn't belong. $i is a loop var (99% of the time), why declare + initialize it here?

$count = count($term_list_category);//this is a pointless count call, just write this:
if ($termListCategory) {
//instead of: if ( $count > 0 ){
//an empty array is falsy
    foreach ( $termListCategory as $termCategory )
    {
        $thisCat = get_term_by(
            'id',
            $termCategory,
            'listings_categories'
        );

        //Separate this out into a function!!
        $url = '<a id="' . $termCategory . '" slug="' . $thisCat->{'slug'} . '" class="listing-links-cat" href="#" title="' . $thisCat->{'name'} . '" >' . $thisCat->{'name'} . '</a>';

I've always gotten the hebejeebies when I see string concatenation like this. Please, separate it out into a function, with clear type-hints (if possible), and don't access properties using string constants the way you do. I'd simply write this:

function getUrlFromObject(stdClass $obj, $id)
{
    $url = '<a id="' . $id . '" slug="' . $obj->slug . '" class="listing-links-cat" '
        . 'href="#" title="' . $obj->name . '" >' . $obj->name . '</a>';
    return $url;
}
//call like so, inside the foreach
$url = getUrlFromObject($thisCat, $termCategory);

Well, since you've asked us not to go too technical, I've kept the string concatenation business like you're using, but just in case you want to look into something new, I'd actually use sprintf here, I think, or an actual DOM parser/builder (like DOMDocument or SimpleXMLElement). Anyway, here's the sprintf version:

function getUrlFromObject(stdClass $obj, $id)
{
    return sprintf(
        '<a id="%s" slug="%s" class="listing-links-cat" href="#" title="%s">%3$s</a>',
        $id,
        $obj->slug,
        $obj->name
    );
}

To my eye at least, that looks a lot cleaner than what you have now, but that might be a matter of personal preference, just like your adding spaces between a variable and the postfix-increment operator:

        $i ++;

Whereas I'd write either ++$i; or $i++;. Some people might claim that ++$i; is faster, but honestly: that's a myth that comes from the C-world where there can be a difference between ++i and i++ if the compiler optimization is switched off. Either way, in PHP it doesn't matter, but it's more common to see $i++; anyway (without the spaces).

        $seo .= " " . $thisCat->{'name'} . "";

        $Category_links .= " " . $url . "";

        if($count-1 == $i){
            $Category_links .= " and ";  $seo .= ", ";
        }elseif($count > 1 && $count !== $i){
            $Category_links .= ", ";  $seo .= ", ";
        }

    }
    $Category_links .= " Categories";
?>

I cannot, for the life of me, understand why you're closing the PHP tags, only to open them again and echo here:

<? echo $Category_links;  ?> 

Lastly, what you're doing with these $seo and $Category_links vars is just very error-prone, and clunky-looking. I'd suggest you turn them into arrays, and simply add new values to them. An array of strings can be imploded using any delimiter you like so replace this:

$seo = get_the_title() . ' : ';
//...more stuff
foreach ($termListCategory as $termCategory)
{
}

With something more like:

$seo = array(
    get_the_title() . ' :',
);
$categoryLinks = array();//create empty array
//...more stuff
foreach ($termListCategory as $termCategory)
{
    $seo[] = $thisCat->name;//add this to the seo ARRAY
    $categoryLinks[] = getUrlFromObject($thisCat, $termCategory);
}
$seoString = implode(', ', $seo);//turns array into comma-separated string
$categoryLinkString = implode(', ', $categoryLinks) . ' and Categories';
//or simply directly output the string:
echo implode(', ', $categoryLinks) . ' and Categories';

Is this code faster? Well, probably not. Sure, I'm not calling count for no obvious reason, and I don't use string concatenation that often, but I am manipulating two arrays, which comes at a cost of its own. Using a custom function, and calling that isn't free either, but let's be honest: that's not going to be your major bottleneck. Besides, the upshot is: code that is a lot easier on the eyes:

/**
 * String object into link, pass object, then term (id)
 *
 * @param stdClass $obj
 * @param string $id
 * @return string
 */
function getUrlFromObject(stdClass $obj, $id)
{
    return sprintf(
        '<a id="%s" slug="%s" class="listing-links-cat" href="#" title="%s">%3$s</a>',
        $id,
        $obj->slug,
        $obj->name
    );
}

$permalink = get_permalink( $id );
$seo = array(
    get_the_title() . ' :',
);
//this var was a string, but we've moved it to where we echo the imploded links array (before the imploded array, see below)
$categoryLinks = array();
$termListCategory = wp_get_post_terms(
    get_the_ID(),
    'listings_categories',
    array(
        'fields' => 'ids'
    )
);
foreach ($termListCategory as $k => $term)
{
    //if term does NOT have children, process it
    if (!get_term_children($term, 'listings_categories'))
    {
        $seo[] = $thisCat->name;//add this to the seo ARRAY
        $categoryLinks[] = getUrlFromObject($thisCat, $termCategory);
    }
    //if you want, you can add
    // an else { unset($termListCategory[$k]); } here
    // I just don't see the point of it here
}
$seoString = implode(', ', $seo);//turns array into comma-separated string
//Add the "found in the" bit here
echo 'Found in the ', implode(', ', $categoryLinks) . ' and Categories';

So after putting it all together I have:

  • Removed a lot of concatenation
  • Removed some function calls (count, for example)
  • Added other function calls
  • Added a function definition
  • Removed the secondary loop
  • Removed a fairly pointless if around the second loop
  • IMO simplified the code quite a lot, and made it easier to debug
  • Added some documentation
\$\endgroup\$
  • \$\begingroup\$ This is some spectacular feedback, I look forward to slowly going through it in the next day or two. Really appreciate it! :D I'll let you know how it goes! \$\endgroup\$ – Delto Apr 28 '15 at 19:20

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.