Unusual Subroutines


The blog and musings of Christopher Allen-Poole

A code review

Recently saw this in a codebase, I updated the formatting (which was notably worse) and the variable names to protect the innocent.

function restrict_post_type_list() {
    global $typenow;
    global $wpdb;
     if ($typenow == 'duck'){
        $types = $wpdb->get_col("
            SELECT DISTINCT meta_value
            FROM ". $wpdb->postmeta ."
            INNER JOIN ".$wpdb->posts." ON (".$wpdb->posts.".ID = ". $wpdb->postmeta .".post_id AND ". $wpdb->posts .".post_type = 'duck')
            WHERE meta_key = 'type'
            ORDER BY meta_value
        ");

    ?>
        <select name="duck_type_restrict" id="type">
            <option value="">View all types</option>
            <?php foreach ($types as $type) { ?>
            <option value="<?php echo esc_attr( $type ); ?>" <?php if(isset($_GET['duck_type_restrict']) && !empty($_GET['duck_type_restrict']) ) selected($_GET['duck_type_restrict'], $type); ?>>
            <?php
            echo ($type == 'duck')? 'Quack':'Bark';
            ?>
            </option>
            <?php } ?>
        </select>
        <?php
    }
}

My immediate takeaway? "Does that select statement even need to exist?" If there is more than two "types", then the following would happen:

<select>
    <option value="">View all types</option>
    <option value="duck">Quack</option>
    <option value="$val1">Bark</option>
    <option value="$val2">Bark</option>
    <option value="$val3">Bark</option>
    <option value="$val4">Bark</option>
</select>

So either we have a very clear "requirements are not clear" problem, a more severe user experience problem, or a bad case of code stink. It didn't take long to confirm which one was the case.

The function needed to be re-written. A first draft:

function restrict_post_type_list() {
    global $typenow;
     if ($typenow == 'duck'){
              ?>
        <select name="duck_type_restrict" id="type">
            <option value="">View all types</option>
            <?php foreach (array('dog', 'duck' ) as $type) { ?>
            <option value="<?php echo esc_attr( $type ); ?>" <?php if(isset($_GET['duck_type_restrict']) && !empty($_GET['duck_type_restrict']) ) selected($_GET['duck_type_restrict'], $type); ?>>
            <?php
            echo ($type == 'duck')? 'Quack':'Bark';
            ?>
            </option>
            <?php } ?>
        </select>
        <?php
    }
}

But this would likely be even better as it simplifies things:

function restrict_post_type_list() {
    global $typenow;
    if ($typenow == 'duck'){
        // isset and !empty is a duplication of logic.
       // And move the logic somewhere where it would be evaluated less.
       $duckType = empty($_GET['duck_type_restrict'])? '': $_GET['duck_type_restrict'];
       ?>
       <select name="duck_type_restrict" id="type">
           <option value="">View all types</option>
           <?php 
               // use keys and values to get rid of in-iteration conditionals
              foreach (array('dog' => 'Bark', 'duck' => 'Quack') as $type => $noise) { ?>
           <option value="<?php 
            // no longer need to escape known values.
           echo $type; ?>" <?php selected($duckType, $type); ?>>
           <?php
              echo $noise;
           ?>
           </option>
           <?php } ?>
           </select>
        <?php
    }
}
blog comments powered by Disqus