WordPress/PHP – Refining an if/else statement

1 Views

I am looking at adding an if/else statement, showing different HTML as required. My question is how I could possibly refine this code? Is there a more refined way of presenting this code? 

<?php if ('Images' == get_post_meta($post->ID, 'project_type', true)) { ?>
                  <h1>Images</h1>
<div class="images">...</div>
                <? } elseif ('Slideshow' == get_post_meta($post->ID, 'project_type', true)) { ?>
                  <h1>Slideshow</h1>
<div class="slideshow">...</div>
                <? } elseif ('Video' == get_post_meta($post->ID, 'project_type', true)) { ?>
                    <h1>Video</h1>
<div class="video">...</div>
                <? } elseif ('Audio' == get_post_meta($post->ID, 'project_type', true)) { ?>
                    <h1>Audio</h1>
<div class="audio">...</div>
                <?php } ?>
Post Views: 1

Related posts

Leave a Reply

3 comments 

  1. <?php

// Only call the function once (performance)
$post_id = get_post_meta($post->ID, "project_type", true);

// Use a whitelist to validate
$whitelist = array("Images", "Slideshow", "Video", "Audio");

// Check if given post ID is valid
if (in_array($post_id, $whitelist) === true) { ?>
    <h1><?= $post_id ?></h1>
    <div class="<?= strtolower($post_id) ?>">...</div>
<?php } ?>

    Yes, this approach doesn’t take into account what might be happening within your <div> element. As this isn’t part of your question. If there is a lot going on I’d propose an object oriented approach. Another bad approach would be a switch statement.

    <?php

switch (get_post_meta($post->ID, "project_type", true)) {
    case "Images": ?>
        <h1>Images</h1>
        <div class="images">...</div>
        <?php break;

    case "Slideshow": ?>
        <!-- Same story again ... -->
        <?php break;
} // End switch

    With OOP we can create something like the following:

    <?php

namespace Stackoverflow;

abstract class MyBaseTemplate {

    protected $title;

    protected $class;

    protected $content;

    public function __toString() {
        return "<h1>{$this->title}</h1><div class='{$this->class}'>{$this->content}</div>";
    }

}

class Images extends MyBaseTemplate {

    public function __construct() {
        $this->title   = "Images";
        $this->class   = "images";
        $this->content = "...";
    }

}

class Slideshow extends MyBaseTemplate {

    // Init

}

// In the other file instead of if/else and switch

$post_id = get_post_meta($post->ID, "project_type", true);
$class   = "\Stackoverflow\{$post_id}";

if (class_exists($class) === true) {
    echo new $class();
}

  2. Sometimes it can be cleaner to put all that markup in echo statements, but only if it is simple markup and the result is actually more readable than mixing in the php statements as above. Example:

    <?php
if (...) {
    echo "<h1>Images</h1>"
    echo "<div class="images">...</div>"
} else if(...) {
    echo "<h1>...</h1>"
}
?>

  3. Fleshgrinder is right, you could make this better by only using get_post_meta one time.

    If the contents of the html is different for each of these, I would probably use a switch rather than else/elseif/etc…

    $project_type = get_post_meta($post->ID, 'project_type', true);
switch ( $project_type ) {

case 'Images':
    echo '<h1>Images</h1>';
    echo '<div class="images">...</div>';
    break;

case 'Slideshow':
    echo '<h1>Slideshow</h1>';
    echo '<div class="slideshow">...</div>';
    break;

case 'Video':
    echo '<h1>Video</h1>';
    echo '<div class="video">...</div>';
    break;

case 'Audio':
    echo '<h1>Audio</h1>';
    echo '<div class="audio">...</div>';
    break;
}