Function for iterating out an unordered list of custom taxonomy terms

Question

I've created a function that generates an <ul> of my wordpress custom taxonomy terms, displaying parent > child > second child heirachy. Functionally it is working perfectly and replaces my old hard-coded menu.

The problem is I'm inexperienced and recognise it as an ugly, inefficient function but could not figure out how to make it better. I tried use 'While()' loops to simplify things but came unstuck. Could someone help me understand how to improve this. The final output (including CSS and jQuery to collapse deactivated menu items) is included below.

//Display Navigation Menu

function taxonomy_nav(){

  //Get Parent Terms
  $getParents = [
    'taxonomy'     => 'kernal_category',
    'parent'        => 0,
    'number'        => 0,
    'hide_empty'    => false  
  ];

  //Assign parent terms to variable
  $parent_terms = get_terms( $getParents );  

  //Loop through parent terms
  foreach ( $parent_terms as $parent_term ) {   

    //Store children terms 
    $childrenCheck = get_term_children( $parent_term->term_id, $parent_term->taxonomy );

    //If parent term has children...
    if ( ! is_wp_error( $childrenCheck ) && ! empty ( $childrenCheck ) ) {

      //Output parent term and start new children list
      echo '<li><a href="'. get_term_link( $parent_term ) .'">'. $parent_term->name.'</a>';
      echo '<ul>';

        //Loop through child items and output name
        foreach ( get_terms( 'kernal_category', array( 'hide_empty' => false, 'parent' => $parent_term->term_id ) ) as $child_term) {

            //Check if children have children
            $secondChildrenCheck = get_term_children( $child_term->term_id, $child_term->taxonomy );

              //Generate list for second children
              if ( ! is_wp_error( $secondChildrenCheck ) && ! empty( $secondChildrenCheck ) ) {

                echo '<li><a href="' . get_term_link( $child_term ) . '">' . $child_term->name.'</a>';
                echo '<ul>';

                //If child has child, look through child
                foreach ( get_terms( 'kernal_category', array( 'hide_empty' => false, 'parent' => $child_term->term_id ) ) as $second_child_term) {

                 echo '<li><a href="'. get_term_link( $second_child_term ) . '">'.$second_child_term->name . '</a></li>';       

                }

                echo '</ul>';
                echo '</li>';       

              } else {

              echo '<li><a href="'. get_term_link( $child_term ) .'">'. $child_term->name.'</a></li>';

              }
        }

      echo '</ul>';
      echo '</li>';

    } else {

      echo '<li><a href="'. get_term_link( $parent_term ) .'">'. $parent_term->name.'</a></li>';

    }     

  }

Answer 1

The first thing I noticed was your indentation. On some lines the indentation is not consistent. Try to keep your indentation consistent as it helps others read your code.

You also write a lot of comments in your code but comments are usually not necessary. I only write comments to explain certain design choices which are not clear from the code (for example doing some low-level code because it improves performance rather than readability) or I write comments to explain a difficult algorithm or procedure of steps. A lot of your comments distract from the actual code or are confusing. For example

//Store children terms 
$childrenCheck = get_term_children( $parent_term->term_id, $parent_term->taxonomy );

The comment says something about storing children terms but the method called is called get_term_children implying there is something fetched, not stored. I assume you mean storing the children terms in the variable, but in that case a comment stating //fetching children term would be better as most developers understand that the children terms are stored in the variable. No comment would even be better as most experienced developers read the code and immediately see that the children terms are fetched.

If comments help you understand your code better and you are the only one reading your code, by all means. But you should also try to read and understand code without comments.

A general rule of thumb is that a lot of nested structures is not a good sign. You now have a foreach inside an if inside a foreach inside an if inside a foreach. What you can do for example for each foreach statement is take all the code inside the foreach statement and extract it into a separate function. Try to give each of these functions descriptive names what they are doing.

You also have a lot of repetition when it comes to echoing. The following

echo '<li><a href="'. get_term_link( $parent_term ) .'">'. $parent_term->name.'</a></li>';

is used quite a lot throughout your code. You can extract this to a method, something like

function printListItem($term) {
  echo '<li><a href="'. get_term_link( $term ) .'">'. $term->name.'</a></li>';
}

In this way when you have to change something to each list item at the same time (for example, adding a html class to the lis) you only have to do it in one place.

The check for children can also be extracted into a separate function, say

function hasChildren($term) {
  $children = get_term_children( $term->term_id, $term->taxonomy );
  return !is_wp_error($children) && !empty($children);
}

Than you can use it like this:

foreach ( $parent_terms as $parent_term ) {   
  if (hasChildren($parent_term) {
    ...
  }
}

I think this is a good set of improvements for you to start with to improve the readability of your code. Just keep practicing and eventually you will write clearer code and understand how to improve your own code :)

Answer 2

As pointed out, there is a lot of code repetition and this is a big problem when any changes get made. It's so easy to quickly make a change to one piece of code and miss the 3 other versions of it which do the same thing.

Rather than factor out these parts of code into functions though, you can re-factor the code into a recursive function. The idea being that you just use the same code to do each layer of the process rather than loops within loops calling helper functions to do the more common tasks.

One other thing I have changed is to build the output into a string rather than echo it out directly. I would normally then return this string to the calling code and let that do whatever it wants with it. This allows more control over the output generated, also allows the output to be re-used (although not sure how that would be necessary in this case) rather than having to re-generate the output each time. But as I'm not sure how this is called, I just echo the combined string at the end.

function taxonomy_nav( string $category = null, array $settings = null  ) : string {
    if ( $category == null )    {
        $settings = [
            'taxonomy'     => 'kernal_category',
            'parent'        => 0,
            'number'        => 0,
            'hide_empty'    => false
        ];
        $parent_terms = get_terms( $settings );
    }
    else    {
        $parent_terms = get_terms( $category, $settings );
    }

    foreach ( $parent_terms as $parent_term ) {
        $list =  '<li><a href="'. get_term_link( $parent_term ) .'">'. $parent_term->name.'</a>';

        $childrenCheck = get_term_children( $parent_term->term_id, $parent_term->taxonomy );

        if ( ! is_wp_error( $childrenCheck ) && ! empty ( $childrenCheck ) ) {
            $list .= '<ul>'
                . taxonomy_nav ( 'kernal_category', array( 'hide_empty' => false, 'parent' => $parent_term->term_id )  ) 
                . '</ul>';
        }

        $list .= '</li>';
    }

    echo $list;
}

As you can see, in the if ( ! is_wp_error( $childrenCheck ) block, it just starts the sub list and then calls itself to do the processing for any child nodes.

This also shows how the list item itself is only ever generated by the one line of code, not repetition and any changes are easily identified.