Rendering drop-down boxes for country and region

Question

In PHP how can I follow the DRY (don't repeat yourself) principle in this piece of code? The 2 classes in question share an awful lot of code.

<?php

interface iSelect
{
    public static function display();
}


class CountrySelect implements iSelect {

    static $template = 'select_template.php';

    public static function display() {

        if ( class_exists( 'View' ) ) {

            // Get the full path to the template file.

            $templatePath = dirname( __FILE__ ) . '/' . static::$template;

            $viewData = array(
                "options" => '_countries',
                "optionsText" => 'name',
                "optionsValue" => 'geonameId',
                "value" => 'selectedCountry',
                "caption" => 'Country'
            );

            // Return the rendered HTML
            return View::render( $templatePath, $viewData );

        }
        else {
            return "You are trying to render a template, but we can't find the View Class";
        }
    }
}

class RegionSelect implements iSelect {

    static $template = 'select_template.php';

    public static function display() {

        if ( class_exists( 'View' ) ) {

            // Get the full path to the template file.

            $templatePath = dirname( __FILE__ ) . '/' . static::$template;

            $viewData = array(
                "options" => '_regions',
                "optionsText" => 'name',
                "optionsValue" => 'name',
                "value" => 'selectedCity',
                "caption" => 'Region'
            );

            // Return the rendered HTML
            return View::render( $templatePath, $viewData );

        }
        else {
            return "You are trying to render a template, but we can't find the View Class";
        }
    }
}

?>

The code in question is for php templating. Here is the template:

<div class="form-group col-sm-6">
    <div class="select">
        <span class="arr"></span>
        <select data-bind="options: <? echo $options ?>,
            optionsText: '<? echo $optionsText ?>',
            optionsValue: '<? echo $optionsValue ?>',
            value: <? echo $value ?>,
            optionsCaption: '<? echo $caption ?>'">
        </select>
    </div>
</div>

Here is the View.php which takes the $ViewData and the $template file and then renders it:

<?php

/** View.php **/

class View {

    /**
     * -------------------------------------
     * Render a Template.
     * -------------------------------------
     * 
     * @param $filePath - include path to the template.
     * @param null $viewData - any data to be used within the template.
     * @return string - 
     * 
     */
    public static function render( $filePath, $viewData = null ) {

        // Was any data sent through?
        ( $viewData ) ? extract( $viewData ) : null;

        ob_start();
        include ( $filePath );
        $template = ob_get_contents();
        ob_end_clean();

        return $template;
    }
}
?>

Answer 1

In PHP how can I follow the DRY (don't repeat yourself) principle in this piece of code? The 2 classes in question share an awful lot of code.

One approach would be to create a common superclass and extract the common elements of the function. As everything except the viewData is the same, just pass that to the newly created common function:

<?php
interface iSelect {
    public static function display();
}

abstract class AreaSelect implements iSelect {
    public static function displayData($viewData) {
            if ( class_exists( 'View' ) ) {

                // Get the full path to the template file.

                $templatePath = dirname( __FILE__ ) . '/' . static::$template;

                // Return the rendered HTML
                return View::render( $templatePath, $viewData );

            }
            else {
                return "You are trying to render a template, but we can't find the View Class";
            }
    }
}

class CountrySelect extends AreaSelect {
    public static function display() {
        $viewData = array(
            "options" => '_countries',
            "optionsText" => 'name',
            "optionsValue" => 'geonameId',
            "value" => 'selectedCountry',
            "caption" => 'Country'
        );
        parent::displayData($viewData);
    }
}

class RegionSelect extends AreaSelect {
    public static function display() {
        $viewData = array(
            "options" => '_regions',
            "optionsText" => 'name',
            "optionsValue" => 'name',
            "value" => 'selectedCity',
            "caption" => 'Region'
        );
        parent::displayData($viewData);
    }
}

The displayData is now so generic, that it might make sense to create a AbstractSelect class instead of an AreaSelect as it is likely that other selects will use the exact same method as well.

Extract

You should always be careful when using extract like this. If you ever have an entry that is called filePath, you would overwrite the $filePath argument and thus break your code.

Additionally, if $viewData is ever user-controlled, you would have an LFI vulnerability. It's unlikely, but I would always try to make generic methods such as this as secure as possible (the calling code is unlikely to expect a security issue), even for unlikely use (that's also why I would probably check filePath for directory traversal, just in case, as well as html-encode all variables in the template); at the very minimum, I would document the issue.

Misc

• why are you checking if View exists (it should always exist, right?), but not if $template exists (it's more likely that one of many templates gets misplaced than that the single view class gets lost)?
• your placement of curly brakets is not consistent.
• your comments aren't really needed. Stuff like // Return the rendered HTML is already clear from the code. It would be better to remove comments such as these, and add PHPDoc style comments documenting the methods behaviour instead.

Answer 2

Why do you have two same classes? You can make parent abstract class instead of interface and move all logic to parent.