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 a node template that grabs fields from a content type.

In the node--[contenttype].tpl.php file I have two sections (The second is html/php and grabs variables from the first section or from the content type and simply displayed it). The first is strictly php and does three things:

  1. Grabs numbers from one of the content type fields and rearranges them according to some criteria. Setting this output as a variable to be used later in the page.
  2. Grabs two taxonomy terms and sets them as variables to be used later in the page. Also compares these variables to an array as the key in order to set the value as another variable to be used later in the page.
  3. Creates a function for converting stdClass Objects to Arrays and uses it in one of the above two actions.

My question is, should this php even go in the node--[contenttype].tpl.php file? The numbers output and taxonomy output is only ever going to be used in this node (the arrays set for comparison are only relevant with this content type), but the Class function could be used on other pages.

I'm not getting how I can leverage the Drupal system in order to get a strong and easily upgradable code base (for going to Drupal 8 in the future).

How should my mind be processing executable challenges. Should I be asking certain questions in order to determine if something goes into template.php, into a new module, or where its css goes? Should all that css be going into style.css? Can I break it out somehow?

EDIT: Here is my code for node--[contenttype].tpl.php

<?php 
/////**** set link and text for Category and Location section ****////////
$category_to_nid = array(

    "Professional Development" => 34,
    "Curriculum" => 35,
    "Study Labs" => 36,
    "Stage Performances" => 37,
);

$location_to_nid = array(
    "Visit Examples" => 38,
    "We Come To You" => 39);

// to array (from stdClass object)
// for field_location_category to get name 
// without it, get white screen of death because ['taxonomy_term'] is an object not array
// Credit: http://www.if-not-true-then-false.com/2009/php-tip-convert-stdclass-object-to-multidimensional-array-and-convert-multidimensional-array-to-stdclass-object/

    function objectToArray($d) {
        if (is_object($d)) {
            // Gets the properties of the given object
            // with get_object_vars function
            $d = get_object_vars($d);
        }

        if (is_array($d)) {
            /*
            * Return array converted to object
            * Using __FUNCTION__ (Magic constant)
            * for recursive call
            */
            return array_map(__FUNCTION__, $d);
        }
        else {
            // Return array
            return $d;
        }
    }



$location_text_array = objectToArray($node->field_location_category['und'][0]['taxonomy_term']);
$location_text = $location_text_array['name'];
$category_text_array = objectToArray($node->field_location_category['und'][1]['taxonomy_term']);
$category_text = $category_text_array['name'];

$location_link = "http://www.server.com/node/" . $location_to_nid[$location_text];
$category_link = "http://www.server.com/node/" . $category_to_nid[$category_text];

/////**** Check if new category or location ****///////
$display_category_tab = TRUE;
$display_location_tab = TRUE;
if ($category_text != $category_text_previous){
    $display_category_tab = TRUE;
}
else{   $display_category_tab = FALSE;}
if ($location_text != $location_text_previous){
    $display_location_tab = TRUE;
}
else{   $display_location_tab = FALSE;}
/*print $location_text . "<br />";
print $location_text_previous . "<br />";
print $category_text . "<br />";
print $category_text_previous . "<br />";

if($display_location_tab){print "Location TAB" . "<br />";}
if($display_category_tab){print "Category TAB" . "<br />";}
*/
$location_text_previous = $location_text;
$category_text_previous = $category_text; 

/////**** set Taxonomy Term Color ****///////
$color_category_array = array(
    196 => "orange", // "Inspiration for Users"
    177 => "green", // "Visit Examples"
    178 => "blue", // "We Come to You"
    197 => "red", // "Content for Homeschoolers"
    198 => "grey" // "School Partnerships"
    );


$tid_array = array();
for ($i=0; $i < count($node->field_location_category['und']); $i++):  
    $tid_array[] = $node->field_location_category['und'][$i]['tid'];
endfor;

$taxonomy_terms = array_intersect($tid_array, array(196, 177, 178, 197, 198));
$taxonomy_color_class = $color_category_array[$taxonomy_terms[0]];
// Use the following for setting class for color: $color_category_array[$taxonomy_term];




/////**** set Grades $display variable ****///////
if(!empty($content['field_grades'])): 

// Grab Grades string and create into array
// note: because not able to grab as array
$grades_string = render($content['field_grades']);
$grades_string = str_replace("Adult", "13", $grades_string);
$grades_string = str_replace("K", "00", $grades_string);
$new_grades_array = str_split($grades_string, 2);             

// loop for creating $display string based on consecutives
for ($i = 0; $i < count($new_grades_array); $i++):

     // set grade digit for K and Adult
     switch ($new_grades_array[$i]):
           case "K":
               $currentgrade_dig = 0;
               break;
           case "Adult":
               $currentgrade_dig = 13;
               break;
           default:
               $currentgrade_dig = $new_grades_array[$i];
     endswitch;

     // concatenates string to display variable based on situation
     if ($i == 0): // if the first grade listed

          $display = $new_grades_array[$i];

     elseif ($i +1 == count($new_grades_array)): //if the last grade listed

          if ($currentgrade_dig - $last ==1):
               $display .= " - " . $new_grades_array[$i];
          else:
               if ($conseq != FALSE): 
                    $display .= " - " . $last . ", " . $new_grades_array[$i]; 
               else: 
                     $display .= ", " . $new_grades_array[$i];
               endif;
          endif;
         $conseq = FALSE;

     elseif ($currentgrade_dig - $last ==1): // if consecutive number from previous
          $conseq = TRUE;

     else: // if not a consecutive number

          if ($conseq != FALSE): 
               $display .= " - " . $last . ", " . $new_grades_array[$i];
               $conseq = FALSE;
          else:
               $display .= ", " . $new_grades_array[$i];
          endif;

     endif;

     $last = $new_grades_array[$i];

endfor;

$display = str_replace("13", "Adult", $display);
$display = str_replace("00", "K", $display);
$display = str_replace(" 0", " ", $display);
if($display[0]=="0"): 
   $display = substr($display, 1, strlen($display));
endif;

$display = "Grades " . $display;
else:
$display = "All Ages";
endif;




?>


<?php

/**
 * <div id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?> block_1_column clearfix"<?php print $attributes; ?>>
 */

// Check if published
if($content['body']['#object']->status || $user->uid > 0):
?>

        <!--<?php if(isset($content['field_3column_name'])): ?>
            <h2 class="title-main"><?php print render($content['field_3column_name']); ?></h2>
        <?php endif; ?>-->

        <div <?php if(!isset($content['field_3column_icons'])): ?>class="no-icons"<?php endif; ?>>

                <div class="query-item">
                     <div class="query-title big_header_dark-red query-<?php print $taxonomy_color_class; ?>"><?php print $title; ?></div>

                     <div class="query-where">
                        <a href="<?php print $location_link; ?>" class=""><?php print $location_text; ?></a>
                        &nbsp;>&nbsp;
                        <a href="<?php print $category_link; ?>" class=""><?php print $category_text; ?></a>
                        </div>

                     <div class="query-basics-group">
                          <span><?php print $display; ?></span>
                          <?php if(!empty($content['field_duration'])): ?><span><?php print render($content['field_duration']); ?></span><?php endif; ?>
                          <?php if(!empty($content['field_capacity'])): ?><span><?php print render($content['field_capacity']); ?></span><?php endif; ?>
                          <?php if(!empty($content['field_availability'])): ?><span><?php print render($content['field_availability']); ?></span><?php endif; ?>

                     </div>

                      <?php if(isset($content['field_cost']) || isset($content['field_subject_area']) || isset($content['field_note'])): ?>
                     <div class="query-basics-group">
                         <?php if(!empty($content['field_cost'])): ?><span><?php print render($content['field_cost']); ?></span><?php endif; ?>
                         <?php if(!empty($content['field_subject_area'])): ?><span><?php print render($content['field_subject_area']); ?></span><?php endif; ?>
                         <?php if(!empty($content['field_note'])): ?><span><?php print render($content['field_note']); ?></span><?php endif; ?>
                     </div><?php endif; ?>

                     <div class="query-description"><?php 
                        // render(field_view_filed()); instead of just render($content['body']); kept the WYSIWYG formatting
                        print render(field_view_field('node', $node, 'body'));  ?>
                     </div>


                     <?php if(!empty($content['field_register_link'])): ?>
                     <div class="query-link"><?php 
                        // render(field_view_filed()); instead of just render(); kept the WYSIWYG formatting
                        print render($content['field_register_link']);  ?>
                     </div><?php endif; ?>

                </div>          

            <div class="clear"></div>
        </div>

       <?php if(isset($content['field_gallery'])): ?>
            <?php print render($content['field_gallery']); ?>
        <?php endif; ?>

        <?php if(isset($content['field_3column_icons'])): ?>
            <div class="icons"><div class="icons-highlight"><?php print render($content['field_3column_icons']); ?></div></div>
        <?php endif; ?>

<?php
else:

endif;

//dsm("Charity:<pre>" . print_r($content['field_location_category'], TRUE) . '</pre>');
//dsm("Charity:<pre>" . print_r($taxonomy_terms, TRUE) . '</pre>');
?>
share|improve this question
    
According to the FAQ, you should include any pertinent code. If, as this question appears, your question is not about code, or is an abstract question about how best to do something, then it should be on Programmers.SE instead. –  mseancole Oct 10 '12 at 13:53

1 Answer 1

I can't speak specifically to Drupal as I have never used it before. But the following should be helpful in any situation. In order to best answer your specific questions I'm going to have to go through this line by line, which means I need to decipher your code first. Since I'm having to do so anyways, I figured I'd share the results as a review, after all, this is code review.

Style

Be consistent with your style/code. You have two arrays at the very beginning, both are styled slightly differently. Either method is fine, but you should stick to one or the other. Mixing styles makes it appear to be copy-pasted together.

Arrays and Constants

I would have to ask what these arrays are for that they are sequentially ordered in this way. Skimming the rest of this document I see that these arrays are only used once, and are used in such a way that they can be combined. Honestly I think these should actually be refactored into constants, but I can't really see how to do this to demonstrate. However, combining them definitely wont hurt.

$category_to_nid = array(
    "Professional Development" => 34,
    "Curriculum" => 35,
    "Study Labs" => 36,
    "Stage Performances" => 37,
    "Visit Examples" => 38,
    "We Come To You" => 39
);

Comments

Now, I kind of skipped over the first thing I wanted to cover, but let me return to it now. There are three different kinds of comments. Your single line comments //single line comment, your multi-line comments /* multi-line comment */, and finally your doccomments /** doccomment */. While this doesn't really effect performance, it does help with legibility when you use them properly.

//set link and text for Category and Location section

/*
to array (from stdClass object)
for field_location_category to get name
etc...
*/

Adopted Code

Its good to give credit to borrowed code, especially when trying to look it up later, but don't just copy-paste it in. Take the time to manually type it into your own code, and tweak it to your own style and habits while you are at it. This ensures seamless integration and will help you, if not understand it, to at least know its inner workings better. For instance, you use very descriptive names for all of your variables, but in that function you use a very vague variable. In fact, it is just good practice to be descriptive anyways. This makes your code self documenting, making many of these comments unnecessary.

Don't Repeat Yourself

There is a principle called "Don't Repeat Yourself" (DRY). As the name implies, you should do your best to make sure your code is not repetitive. Sometimes this means using loops, sometimes variables, and usually it means using functions. But to fully benefit from this principle you should use them all. I'm not going to discuss each method here, but you should definitely take a look at this principle more in depth. Specifically I want to look at variables here. Variables help make your code DRY by ensuring you don't have to type the same sequences repeatedly. This also reduces the line length of your code, for example:

$und = $node->field_location_category[ 'und' ];
$location_text_array = objectToArray( $und[ 0 ] [ 'taxonomy_term' ] );

//AND

$address = 'http://www.server.com/node/';
$location_lin = $address . $location_to_nid[ $location_text ];

This has the added benefit of making your code easier to edit. Imagine your URL changed. You would then have to update that URL everywhere. Sure, you can use the find/replace tool, but only having to do it once ensures that nothing is accidentally missed AND it makes your code smaller.

Conditions

So a neat little trick here. If you have a variable you want to set to TRUE/FALSE, based on the outcome of a condition, you can just pass that condition to the variable for the same results, for example:

$display_category_tab = $category_text != $category_text_previous;

However, while this is a neat trick, it is unnecessary in this context, because the variables are unnecessary. You only use these variables once. So instead of performing the same expression twice, just use those conditions once, when you actually need them.

if( $category_text != $category_text_previous ) {
    print "Category TAB" . "<br />";
}

Echo vs Print

This is a matter of preference, but typically you see echo being used over print. Both are just fine, but echo seems to be a smidge faster, which I guess is the reason its more widely used. Otherwise there is no difference, at least that I know of.

Alternative Syntax

Every statement/loop has an alternate syntax that is used in templating. This alternate syntax uses a colon instead of the opening brace, and an endXXX instead of the closing brace. As I said, this syntax is used in templating, so seeing it in a the middle of a PHP block is jarring. I would use the traditional syntax instead.

$count = count( $und );
for( $i = 0; $i < $count; $i++ ) {
    $tid_array[] = $und[ $i ] [ 'tid' ];
}

You may notice that I abstracted the count from the loop in the code above. For and while loops call any functions passed into their parameter lists on each iteration, therefore count() is being called each time. This is pretty common and is sometimes overlooked, but it is still good practice to abstract it with a variable. Besides, it also makes the code more legible when using long statements.

Another change you may have noticed was that I am continuing to use that variable I abstracted at the beginning, this makes the code so much shorter, and as a result much easier to read.

Final Notes

I'm not sure what render() does, but if it does what I think it does (converts an array to a string), then you can probably use array functions instead. Without knowing for sure, and not wanting to confuse you, I don't really want to tackle this.

Definitely take a look at incorporating functions. The rest of your code, from where I left off above, could definitely benefit from it.

should this php even go in the node--[contenttype].tpl.php file?

I would say no. Just because of the size and complexity. Honestly this looks like this could be split into multiple pages, thus adopting a framework, such as MVC, would definitely be a possibility.

I'm not getting how I can leverage the Drupal system in order to get a strong and easily upgradable code base (for going to Drupal 8 in the future).

Some of the above suggestions should help. Again, I can't help you specifically with Drupal, but it should help you get started. Having a better understanding of the basics will always help.

How should my mind be processing executable challenges. Should I be asking certain questions in order to determine if something goes into template.php, into a new module, or where its css goes? Should all that css be going into style.css? Can I break it out somehow?

I'm not entirely sure what you are asking for here. It is always best to have styles and javascripts outside of the HTML. It is also sometimes helpful to have PHP external as well, but this really only makes a difference in larger systems. Having these separated makes it easier to reuse. If you are changing the files based on conditions in the PHP, you might want to consider using multiple pages (Professional Development, Curriculum, etc...) and possibly even a framework.

I hope this helps

share|improve this answer
    
That is a beautiful review/answer giving me a lot of food for thought (Yay!). I'm going to have to go through this and let you know if I have any questions. Thank you for attacking this from a programming basics perspective. I feel like I understand the words, grammar, and sentence structure but am not able to "speak" eloquently and this will help! –  SweetTomato Oct 10 '12 at 21:38
    
So one question: I set these: $display_category_tab = TRUE; $display_location_tab = TRUE; outside of the loop so then I can use them outside of the loop, but now that you mention it, variables are "trapped" only inside functions and not loops, correct? –  SweetTomato Oct 10 '12 at 21:43
1  
@SweetTomato: I'm not sure why this question was closed. Initially this seemed off topic, but with the code its better now. As to your question, you are correct, though defining a variable before using it in a loop is still good practice. For instance, if you try to append to an undefined variable from a loop you will get warnings and might accidentally be accessing a string instead. Or, if the array you are looping over is empty, it will just be skipped, meaning the variable wont get defined. So a default value is still a good thing to have. –  mseancole Oct 11 '12 at 13:14
    
@mseancole I'm not sure why this was closed either. I've voted to re-open. –  James Khoury Dec 13 '12 at 3:53

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.