Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have created a wordpress theme options using array, and it is working. There are 5 type of input text,textarea,checkbox,select and upload.

Kindly check this function and give suggestion and tweaks.

<?php
add_action( 'admin_menu', 'admin_enqueue_scripts' );
function admin_enqueue_scripts() {
    wp_enqueue_style( 'theme-opt-css', get_template_directory_uri() . '/admin/stylesheet/theme-opt.css' );
    wp_enqueue_script( 'theme-opt-js', get_template_directory_uri() . '/admin/javascript/theme-opt.js' ); 
} 
function wp_gear_manager_admin_scripts() {
    wp_enqueue_script('media-upload');
    wp_enqueue_script('thickbox');
    wp_enqueue_script('jquery');
}

function wp_gear_manager_admin_styles() {
    wp_enqueue_style('thickbox');
}

add_action('admin_print_scripts', 'wp_gear_manager_admin_scripts');
add_action('admin_print_styles', 'wp_gear_manager_admin_styles');

$themename = "Foundation";
$shortname = "fo";
$mx_categories_obj = get_categories('hide_empty=0');
$mx_categories = array();
foreach ($mx_categories_obj as $mx_cat) {
    $mx_categories[$mx_cat->cat_ID] = $mx_cat->cat_name;
}
$categories_tmp = array_unshift($mx_categories, "Select a category:");  
$number_entries = array("Select a Number:","1","2","3","4","5","6","7","8","9","10" );
$colorscheme = array("Default", "White", "Blue", "Red", "Purple");

$options = array (

array(  "name" => "Main Set Up",
            "type" => "heading",
            "desc" => "Set your logo and color scheme.",
       ),

array(  "name" => "Logo Display",
            "desc" => "The URL address of your logo (best is 400px x 65px). (Leaving it empty will display your blog title)",
            "id" => $shortname."_logo",
            "type" => "upload",
            "std" => ""),       

array(   "name" => "Blog Color Scheme",
            "id" => $shortname."_color",
            "type" => "select",
            "std" => "Default",
            "options" => $colorscheme),
array(   "name" => "Blog Width",
            "id" => $shortname."_width",
            "type" => "text",
            "std" => "1440"),

array(  "name" => "Navigation Settings",
                    "type" => "heading"),   

array(  "name" => "Exclude Categories",
                    "desc" => "Enter a comma-separated list of the <a href='http://support.wordpress.com/pages/8/'>Category ID's</a> that you'd like to exclude from the main category navigation. (e.g. 1,2,3,4)",
                    "id" => $shortname."_cat_ex",
                    "std" => "",
                    "type" => "text"),  

array(  "name" => "Featured section",
            "type" => "heading",
            "desc" => "This section customizes the featured area on the top of the content and the number of stories displayed there.",
       ),       
array(  "name" => "Featured section category",
            "desc" => "Select the category that you would like to have displayed in Featured list on your homepage.",
            "id" => $shortname."_story_category",
            "std" => "Uncategorized",
            "type" => "select",
            "options" => $mx_categories),

array(  "name" => "Number of highlight reel posts",
            "desc" => "Select the number of posts to display ( Upto 5 is good).",
            "id" => $shortname."_story_count",
            "std" => "1",
            "type" => "select",
            "options" => $number_entries),

array(  "name" => "Sidebar Set Up",
    "type" => "heading",
    "desc" => "Set your sidebar layout.",
       ),

array( "name" => "Disable Tabs box?",
    "desc" => "Tick to disable Tabs box.",
    "id" => $shortname."_distabs",
    "type" => "checkbox",
    "std" => "false"),

array( "name" => "Disable Search box?",
    "desc" => "Tick to disable Search box.",
    "id" => $shortname."_search",
    "type" => "checkbox",
    "std" => "false"),

array( "name" => "Disable About box?",
    "desc" => "Tick to disable About box.",
    "id" => $shortname."_dispop",
    "type" => "checkbox",
    "std" => "false"),  

array( "name" => "Disable Ads box?",
    "desc" => "Tick to disable Ads box.",
    "id" => $shortname."_disads",
    "type" => "checkbox",
    "std" => "false"),

array( "name" => "Disable Flickr box?",
    "desc" => "Tick to disable Flickr box.",
    "id" => $shortname."_disflickr",
    "type" => "checkbox",
    "std" => "false"),

array( "name" => "Disable Follow Me box?",
    "desc" => "Tick to disable Follow Me box.",
    "id" => $shortname."_disfollow",
    "type" => "checkbox",
    "std" => "false"),

array( "name" => "Disable Video box?",
    "desc" => "Tick to disable Video Me box.",
    "id" => $shortname."_disvideo",
    "type" => "checkbox",
    "std" => "false"),


array(  "name" => "About Me Settings",
            "type" => "heading",
            "desc" => "Set your About me image and text from here .",
       ),           

array("name" => "About me Image",
            "desc" => "Enter your avatar image url here.",
            "id" => $shortname."_img",
            "std" => "",
            "type" => "text"),    

array("name" => "About me text",
            "desc" => "Enter some descriptive text about you, or your site.",
            "id" => $shortname."_about",
            "std" => "Integer eget dui ante, a vestibulum augue. Suspendisse lorem diam, viverra a interdum in, facilisis eget mauris. Etiam cursus ligula at dolor ultrices adipiscing sodales metus lacinia. Etiam id justo consectetur lorem auctor scelerisque nec varius ante. Ut condimentum nisl nec enim porttitor ut auctor neque adipiscing. Praesent ac eleifend nunc.",
            "type" => "textarea"),    

array(  "name" => "Featured Video Settings",
            "type" => "heading",
            "desc" => "Displays a featured video on the homepage .",
       ),   

array(  "name" => "Featured Video category",
            "desc" => "Select the category that you would like to have displayed in the videos section on your homepage.",
            "id" => $shortname."_video_category",
            "std" => "Select a category:",
            "type" => "select",
            "options" => $mx_categories),

array(  "name" => "Twitter, Facebook, Flickr account",
            "type" => "heading",
            "desc" => "",
            ),  
array(  "name" => "Your Twitter account",
            "desc" => "Enter your Twitter account name",
            "id" => $shortname."_twitter_user_name",
            "type" => "text",
            "std" => ""),
array(  "name" => "Your Facebook account",
            "desc" => "Enter your Facebook account name",
            "id" => $shortname."_facebook_user_name",
            "type" => "text",
            "std" => ""),   
array(  "name" => "Your Flickr account",
            "desc" => "Enter your Flickr account name",
            "id" => $shortname."_flickr_user_name",
            "type" => "text",
            "std" => ""),   


array(  "name" => "Header Banner Ad  (468x60px)",
            "desc" => "Enter your AdSense code, or your banner url and destination, or disable header ad.",
                    "type" => "heading"),

array(  "name" => "Adsense code",
                    "desc" => "Enter your adsense code here.",
                    "id" => $shortname."_ad_head_adsense",
                    "std" => "",
                    "type" => "textarea"),

array(  "name" => "Banner Ad Header - Image Location",
                    "desc" => "Enter the URL for this banner ad.",
                    "id" => $shortname."_ad_head_image",
                    "std" => "wp-content/themes/GoodThemeLead/images/ad-big.gif",
                    "type" => "text"),

array(  "name" => "Banner Ad Header - Destination",
                    "desc" => "Enter the URL where this banner ad points to.",
                    "id" => $shortname."_ad_head_url",
                    "std" => "#",
                    "type" => "text"),

array(  "name" => "Disable Ad",
                    "desc" => "Disable the ad space",
                    "id" => $shortname."_ad_head_disable",
                    "std" => "false",
                    "type" => "checkbox"),  

array(  "name" => "Content Banner Ad  (468x60px)",
            "desc" => "Enter your AdSense code, or your banner url and destination, or disable content ad.","type" => "heading"), 

array(  "name" => "Adsense code",
                    "desc" => "Enter your adsense code here.",
                    "id" => $shortname."_ad_content_adsense",
                    "std" => "",
                    "type" => "textarea"),

array(  "name" => "Banner Ad Content - Image Location",
                    "desc" => "Enter the URL for this banner ad.",
                    "id" => $shortname."_ad_content_image",
                    "std" => "wp-content/themes/GoodThemeLead/images/ad-big.gif",
                    "type" => "text"),

array(  "name" => "Banner Ad Content - Destination",
                    "desc" => "Enter the URL where this banner ad points to.",
                    "id" => $shortname."_ad_content_url",
                    "std" => "#",
                    "type" => "text"),


array(  "name" => "Disable Ad",
                    "desc" => "Disable the ad space",
                    "id" => $shortname."_ad_content_disable",
                    "std" => "false",
                    "type" => "checkbox"),

    array(  "name" => "Banner Ads Settings",
            "type" => "heading",
            "desc" => "You can setup four 125x125 banners for your blog from here",
       ), 

    array("name" => "Banner-1 Image",
            "desc" => "Enter your 125x125 banner image url here.",
            "id" => $shortname."_banner1",
            "std" => "wp-content/themes/GoodThemeLead/images/ad-small.gif",
            "type" => "text"),     

    array("name" => "Banner-1 Url",
            "desc" => "Enter the banner-1 url here.",
            "id" => $shortname."_url1",
            "std" => "#",
            "type" => "text"),    


    array("name" => "Banner-2 Image",
            "desc" => "Enter your 125x125 banner image url here.",
            "id" => $shortname."_banner2",
            "std" => "wp-content/themes/GoodThemeLead/images/ad-small.gif",
            "type" => "text"),    

    array("name" => "Banner-2 Url",
            "desc" => "Enter the banner-2 url here.",
            "id" => $shortname."_url2",
            "std" => "#",
            "type" => "text"), 

    array("name" => "Banner-3 Image",
            "desc" => "Enter your 125x125 banner image url here.",
            "id" => $shortname."_banner3",
            "std" => "wp-content/themes/GoodThemeLead/images/ad-small.gif",
            "type" => "text"),    

    array("name" => "Banner-3 Url",
            "desc" => "Enter the banner-3 url here.",
            "id" => $shortname."_url3",
            "std" => "#",
            "type" => "text"),

    array("name" => "Banner-4 Image",
            "desc" => "Enter your 125x125 banner image url here.",
            "id" => $shortname."_banner4",
            "std" => "wp-content/themes/GoodThemeLead/images/ad-small.gif",
            "type" => "text"),    

    array("name" => "Banner-4 Url",
            "desc" => "Enter the banner-4 url here.",
            "id" => $shortname."_url4",
            "std" => "#",
            "type" => "text"),
);

function mytheme_add_admin() {

    global $themename, $shortname, $options;

    if ( $_GET['page'] == basename(__FILE__) ) {

        if ( 'save' == $_REQUEST['action'] ) {

                foreach ($options as $value) {
                    update_option( $value['id'], $_REQUEST[ $value['id'] ] ); }

                foreach ($options as $value) {
                    if( isset( $_REQUEST[ $value['id'] ] ) ) { update_option( $value['id'], $_REQUEST[ $value['id'] ]  ); } else { delete_option( $value['id'] ); } }

                header("Location: themes.php?page=theme-options.php&saved=true");
                die;

        } else if( 'reset' == $_REQUEST['action'] ) {

            foreach ($options as $value) {
                delete_option( $value['id'] ); 
                update_option( $value['id'], $value['std'] );}

            header("Location: themes.php?page=theme-options.php&reset=true");
            die;

        }
    }

      add_theme_page($themename." Options", "$themename Options", 'edit_themes', basename(__FILE__), 'mytheme_admin');

}

function mytheme_admin() {

    global $themename, $shortname, $options;

    if ( $_REQUEST['saved'] ) echo '<div id="message" class="updated fade"><p><strong>'.$themename.' settings saved.</strong></p></div>';
    if ( $_REQUEST['reset'] ) echo '<div id="message" class="updated fade"><p><strong>'.$themename.' settings reset.</strong></p></div>';
?>
<div id="<?php echo $shortname; ?>" class="wrap">
<h2><b><?php echo $themename; ?> options</b></h2>
<div class="theme-opts-c">
<div class="theme-opts-author">
    <div class="about-me">
        <h1 class="thank">Thank You!</h1>
        <h2 class="visit">Visit </h2>
    </div>
</div>
<form method="post">

<div class="options-grids" >

<?php foreach ($options as $value) {        
    if ($value['type'] == "text") { 

    /* ----------------------Start Input Text -------------------*/
    ?>

    <div class="option-box">
        <h1 class="option-title"><?php echo $value['name']; ?></h1>
        <div class="option-input">
            <input name="<?php echo $value['id']; ?>" id="<?php echo $value['id']; ?>" type="<?php echo $value['type']; ?>" value="<?php if ( get_settings( $value['id'] ) != "") { echo get_settings( $value['id'] ); } else { echo $value['std']; } ?>" size="40" />
        </div>          
        <div class="option-desc"><?php echo $value['desc']; ?> </div>

    </div>
    <?php 
    /* ----------------------End Input text-------------------*/

    } elseif ($value['type'] == "textarea") {

    /* ----------------------Start textarea -------------------*/

    ?>
    <div class="option-box">
        <h1 class="option-title"><?php echo $value['name']; ?></h1>
        <div class="option-input">
            <textarea name="<?php echo $value['id']; ?>" id="<?php echo $value['id']; ?>" cols="40" rows="5"/><?php if ( get_settings( $value['id'] ) != "") { echo get_settings( $value['id'] ); } else { echo $value['std']; } ?>
    </textarea>             
        </div>  
        <div class="option-desc"><?php echo $value['desc']; ?> </div>
    </div>
    <?php 
    /* ----------------------End textarea -------------------*/

    } elseif ($value['type'] == "select") { 

    /* ----------------------Start Select -------------------*/
    ?>

        <div class="option-box"> 
            <h1 class="option-title"><?php echo $value['name']; ?></h1>
            <div class="option-input">
                <select name="<?php echo $value['id']; ?>" id="<?php echo $value['id']; ?>">
                    <?php foreach ($value['options'] as $option) { ?>
                    <option<?php if ( get_settings( $value['id'] ) == $option) { echo ' selected="selected"'; }?>><?php echo $option; ?></option>
                    <?php } ?>
                </select>           
            </div>
            <div class="option-desc"><?php echo $value['desc']; ?> </div>   
        </div>

    <?php 
    /* ----------------------End Select-------------------*/

    } elseif ($value['type'] == "checkbox") { 

    /* ----------------------Start checkbox -------------------*/
    ?>

        <div class="option-box"> 
            <h1 class="option-title"><?php echo $value['name']; ?></h1>
            <div class="option-input">
                <?php if(get_settings($value['id'])){ $checked = "checked=\"checked\""; }else{ $checked = ""; } ?>
                    <input type="checkbox" name="<?php echo $value['id']; ?>" id="<?php echo $value['id']; ?>" value="true" <?php echo $checked; ?> />
            </div>
            <div class="option-desc"><?php echo $value['desc']; ?> </div>
        </div>   

    <?php 

    /* ------------------- end checkbox -------------------*/

    }elseif ($value['type'] == "upload") { 

    /* ----------------------Start upload -------------------*/
    ?>

        <div class="option-box"> 
            <h1 class="option-title"><?php echo $value['name']; ?></h1>
            <div id="upload-opt" class="option-input">
                <input id="upload_image" type="text" size="36" name="<?php echo $value['id']; ?>" value="<?php echo $gearimage; ?>" />
                <input id="upload_image_button" type="button" value="Upload Image" />

            </div>
            <div class="option-desc"><?php echo $value['desc']; ?> </div>
        </div>   

    <?php 

    /* ------------------- end upload -------------------*/

    } elseif ($value['type'] == "heading") { 

    /* ----------------------Start heading -------------------*/
    ?>

        <div class="option-heading">  
            <h2 class="option-heading-title"><?php echo $value['name']; ?></h2>
            <div class="option-heading-desc"><?php echo $value['desc']; ?> </div>
        </div>

    <?php /* ----------------------End Heading -------------------*/} ?>
<?php } ?>
</div>
        <div class="opt-submit">
            <div class="submit">
                <input name="save" type="submit" value="Save changes" />    
                <input type="hidden" name="action" value="save" />
            </div>
        </div>
</form>
    <div class="opt-reset">
        <form method="post">
            <div class="reset">
                <input name="reset" type="submit" value="Reset" />
                <input type="hidden" name="action" value="reset" />
            </div>
        </form>
    </div>
</div>
</div>
<?php
}
add_action('admin_menu', 'mytheme_add_admin'); ?>
share|improve this question
add comment

1 Answer

up vote 1 down vote accepted

Mostly, it just appears that your code is violating the "Don't Repeat Yourself" (DRY) Principle. As the name implies, your code should not repeat itself. There are a few reasons for this, efficiency and reusablity chief among them. Your first violation of DRY is sort of a combination of the two. When you have a similar value being reused, without using a variable, and that value is something mutable like a directory, then it is inefficient and harder to reuse as you will have to change every instance of that value if it ever needs to be changed. Additionally, if that value, or part of it, is being fetched from a function, then you are making your code work harder than it needs to. Using functions has a higher processing cost than using a variable, so calling the function once and setting its return to a variable is more efficient. Another added benefit of abstracting this value is shorter line lengths, which means more legible code.

$dir = get_template_directory_uri() . '/admin/';
wp_enqueue_style( 'theme-opt-css', $dir . 'stylesheet/theme-opt.css' );
wp_enqueue_script( 'theme-opt-js', $dir . 'javascript/theme-opt.js' );

That's still sort of violating DRY, what with that first parameter and filename being so similar, but after toying around with str_replace() and substr_replace() for a few minutes I decided it was more hassle than it was worth, and creating a function just for that seems unnecessary unless it will be reused a lot more.

I don't know if its an issue with with the uploaded script or not, but some of your functions are not indented. I'd make sure this isn't the case with your actual code, and if it is, I would fix it. Indentation is a pretty big point for legibility.

There is a quote that goes along with the above principle: "You can either do something once, or you can do something many times. There's no such thing as doing something only twice." I probably slaughtered that quote, but its pretty close. Essentially it just means that you should use loops or functions to simulate repetition in tasks. For example:

$scripts = array(
    'media-upload',
    'thickbox',
    'jquery'
);

foreach( $scripts AS $script ) {
    wp_enqueue_script( $script );
}

The above can also be applied to those add_action() function calls too, just use a multi-dimensional array.

Here's a neat trick: If you have a range of numbers you want in an array, you can use PHP's range() function to populate it for you. This makes it a little easier to manipulate that range, if needs be, and helps with legibility.

$number_entries = range( 1, 10 );
$number_entries = array_unshift( $number_entries, 'Select a Number:' );

I have no idea what this $options array is, nor what its being used for, but that is just too large to throw in the middle of your code like that. You should see if you can't abstract it away from your code somehow. I'm thinking maybe a config file or database. I'm just going to ignore this for now.

Globals are evil. Forget you ever heard of them. You will NEVER need a global. EVER. You should avoid globals at all costs. There are a number of reasons for this, but there are two main ones: They are insecure because any application can use them once they are declared; They are hard to maintain because they are difficult to read and trace. Everything in your script appears to be contained in this one file, but they could just as easily have come from another file, or be manipulated in another file. There are a few different ways we can avoid globals, but the easiest in this case would simply be to inject them into our functions as parameters. $shortname isn't even used in this first function, so it can be left out altogether.

function mytheme_add_admin( $themename, $options ) {

You are violating the Arrow Anti-Pattern. This pattern is illustrated by code that is heavily or unnecessarily indented, usually so that it comes to points like an arrow. This should be avoided to enhance legibility. There are a couple of ways we can avoid violating this principle. One is to return early from functions to stop execution. Another is to manipulate our if logic so that it contains the smaller block of code and then return early. If we do the latter, then we can neglect the else statement as it is implied, thus we remove a level of indentation from the larger block of code.

$basename = basename( __FILE__ );
if ( $_GET[ 'page' ] != $basename ) {
    $options = "$themename . Options";
    add_theme_page(
        $options,
        $options,
        'edit_themes',
        $basename,
        'mytheme_admin'
    );

    return;
}

//else implied, rest of code here

Its perfectly fine to use Yoda syntax or normal syntax, but you should be consistent and choose either one or the other, not both. There is no right or wrong way here. Originally Yoda syntax was considered good practice because a non-variable value can't be defined with a value like a variable, thus avoiding accidentally reassigning a new value to a variable in a statement. However, this practice fell out of common practice because most languages stopped supporting assignments in statements. PHP still allows this, so Yoda syntax still makes sense, but isn't necessary if you are vigilant.

//standard
if( $_GET[ 'page' ] == $basename ) {

//because second equals sign was forgotten
//page is now $basename
if( $_GET[ 'page' ] = $basename ) {

//yoda
if( 'save' == $_REQUEST[ 'action' ] ) {

//a string cannot be given a value
//so this throws fatal errors preventing accidents
if( 'save' = $_REQUEST[ 'action' ] ) {

Don't iterate over the same array twice, especially if no changes were made to that array to make a second iteration necessary. Your two foreach loops can be combined. I'm not even sure that first loop was necessary. You are updating the $data[ 'id' ] in the first loop, then the second loop checks that same value against the $_REQUEST array to determine if it should be updated or deleted. Seems kind of redundant.

foreach( $options AS $value ) {
    $id = $value[ 'id' ];
    update_option( $id, $_REQUEST[ $id ] );//is there a reason for this?


    if( isset( $_REQUEST[ $id ] ) ) {
        update_option( $id, $_REQUEST[ $id ]  );
    } else {
        delete_option( $id );
    }
}

Typically when you reload a page with a header(), you use exit, not die. I don't know if there is really any difference in this, but figured I'd mention it.

header("Location: themes.php?page=theme-options.php&saved=true");
exit;

My last bit of advice would be to consider using includes instead of directly outputting HTML with PHP. Its cleaner and will allow you to reuse the HTML if necessary. That first bit of HTML, including the foreach loop with all those if/else statements can be one include. Then inside that foreach loop, each if/else statement can lead to another include.

//new include 'text.inc'
<div class="option-box">
    <h1 class="option-title"><?php echo $value['name']; ?></h1>
    <div class="option-input">
        <input name="<?php echo $value['id']; ?>" id="<?php echo $value['id']; ?>" type="<?php echo $value['type']; ?>" value="<?php
            if( get_settings( $value['id'] ) != '' ) {
                echo get_settings( $value['id'] );
            } else {
                echo $value['std'];
            }
        ?>" size="40" />
    </div>          
    <div class="option-desc"><?php echo $value['desc']; ?> </div>

</div>

//back in the main file
if( $value[ 'type' ] == 'text' ) {
    include 'text.inc';
} elseif( $value[ 'type' ] == 'textarea' ) {
    //etc...

Side note: The if/else statement in the above include can be rewritten as a ternary statement. Some people do not like ternary because it is usually abused, but as long as it is short and simple, I see no problems with it.

//original
if( get_settings( $value['id'] ) != '' ) {
    echo get_settings( $value['id'] );
} else {
    echo $value['std'];
}

//regular ternary
echo get_settings( $value[ 'id' ] ) ? get_settings( $value[ 'id' ] ) : $value[ 'std' ];
//PHP >= 5.3 allows short ternary
echo get_settings( $value[ 'id' ] ) ?: $value[ 'std' ];

When you are comparing the same value against multiple possibilities, you should use a switch statement rather than if/else statements. They are a bit faster and a little easier to read and maintain.

switch( $value[ 'type' ] ) {
    case 'text' :
        //etc...
    break;

    case 'textarea' :
        //etc...
    break;

    //etc...
}

Hope this helps!

share|improve this answer
    
thanks, it really helps. i will follow your guidance –  Script Jan 8 '13 at 1:26
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.