How do I stop repeating code in similar functions?

computerbarry · June 4, 2014, 11:09pm

Hi all

I’m working on a bit of Drupal 7 theming, generally modifying a few fields, nothing to complex.
One thing that is frustrating me is I keep repeating 80% of the same code in each function as shown below for a particular content type.

What is the best practice, how do I combine this code/functions so I don’t DRY ?
The only things different are the function names and the first span class.

function theme1_field__field_event_date_and_time__event($variables) {
  $output = '';
  $output .= '<span class="time"></span>';
  
  // Render the items.
  $output .= '<div class="field-items"' . $variables['content_attributes'] . ' style="display:inline-block">';
  foreach ($variables['items'] as $item) {
    $classes = 'field-item';
    $output .= '<div class="' . $classes . '"' . $variables['item_attributes'] . '>' . drupal_render($item) . '</div>';
  }
  $output .= '</div>';
  
  // Render the top-level DIV.
  $output = '<li class="list-group-item"><div class="' . $variables['classes'] . '"' . $variables['attributes'] . '>' . $output . '</div></li>';
  
  return $output;
}

function theme1_field__field_event_location__event($variables) {
  $output = '';
  $output .= '<span class="map-marker"></span>';
  
  // Render the items.
  $output .= '<div class="field-items"' . $variables['content_attributes'] . ' style="display:inline-block">';
  foreach ($variables['items'] as $item) {
    $classes = 'field-item';
    $output .= '<div class="' . $classes . '">' . drupal_render($item) . '</div>';
  }
  $output .= '</div>';
  
  // Render the top-level DIV.
  $output = '<li class="list-group-item"><div class="' . $variables['classes'] . '"' . $variables['attributes'] . '>' . $output . '</div></li>';
  
  return $output;
}

Any suggestions on the best way to go about this?
Thanks, Barry

computerbarry · June 5, 2014, 12:07am

Could I write some sort of class here, or would that be a bit overkill?
Maybe with different parameters?

Any ideas?

I’m also wondering, is there a simple way to make numerous functions use the same code, like in CSS?

example:
.bigtext, .smalltext {color:white}

couldn’t we have:
function somename1 (), function somename2() {
…
}

?

computerbarry · June 5, 2014, 3:29pm

To simplify things:

function theme1_field__field_event_date_and_time__event($variables) {
  $output = '';
  $output .= '<span class="time"></span>';

//10+ lines of duplicate code

}

function theme1_field__field_event_location__event($variables) {
  $output = '';
  $output .= '<span class="map-marker"></span>';

//10+ lines of duplicate code

}

Can you see the problem?

Thanks, Barry

John_Betong · June 5, 2014, 5:00pm

Functions can call other functions.

Try this:


<?php 
define ('SPC', '&nbsp;&nbsp&nbsp;');

$variables = date('H:i:s');

echo '<br /><br />';
$var = theme1_field__field_event_date_and_time__event($variables);
echo $var;

echo '<br /><br />';
$var = theme1_field__field_event_location__event($variables);
echo $var;

// following common functions could/should be in an include file
// include './pathToCommonfunctin/commonFunctions.php';
function theme1_field__field_event_date_and_time__event($param = 'No $time passing')
{
  $result = '<b>function: </b>' .__FUNCTION__;

  $result .= '<span class="time"></span>'; 

  $result .= '<br />'; 
  // 10+ lines of duplicate code 
  $param   =  SPC .'<b>source: </b>theme1_field__field_event_date_and_time__event($variables)'
           . '<br />' .SPC .SPC .'<b>time: </b>' .SPC .$param;  
  $result .= tenLinesOfDuplicateCode( $param );


  return $result;
} 

function theme1_field__field_event_location__event($param = 'No $time passing')
{ 
  $result = '<b>function: </b>' .__FUNCTION__;

  $result .= '<span class="map-marker"></span>'; 

  $result .= '<br />'; 
  //10+ lines of duplicate code 
  $param   = SPC .'<b>source: </b> theme1_field__field_event_location__event($variables)'
           . '<br />' .SPC .SPC .'<b>time: </b>' .SPC .$param;  
  $result .= tenLinesOfDuplicateCode( $param );

  return $result;
}  

function tenLinesOfDuplicateCode( $param = 'Default: No time specified')
{
    $result = SPC .'<b>function: </b>' .__FUNCTION__ ;

  $result .= '<br />' .SPC .'<b>contents: </b>'; 
    for($i2=1; $i2 < 10; $i2++)
    {
        $result .= 'line: ' .$i2 .', &nbsp; '; // with a comma
    }
  $result .= 'line: ' .$i2; // no comma

  $result .= '<br />' .SPC .$param;

  return $result;
}

Output:

function: theme1_field__field_event_date_and_time__event
function: tenLinesOfDuplicateCode
contents: line: 1, line: 2, line: 3, line: 4, line: 5, line: 6, line: 7, line: 8, line: 9, line: 10
source: theme1_field__field_event_date_and_time__event($variables)
time: 22:58:34

function: theme1_field__field_event_location__event
function: tenLinesOfDuplicateCode
contents: line: 1, line: 2, line: 3, line: 4, line: 5, line: 6, line: 7, line: 8, line: 9, line: 10
source: theme1_field__field_event_location__event($variables)
time: 22:58:34

computerbarry · June 5, 2014, 5:46pm

Cool thanks John

Though this seems more confusing than my original code, the amount of code is still the same, no?

Would a class be better?
Here is a small snippet of a class I’m using for something else, wondering if we can use this approach:

class NodeFieldQuery extends EntityFieldQuery {
  function __construct($node_type, $order_by) {

$query = new NodeFieldQuery('event','field_event_date_and_time');

Can you see what I’ve done here, wouldn’t this be a little easier and less code?

I’m still learning PHP as I’m sure you can tell, information much appreciated thanks.

Barry

John_Betong · June 5, 2014, 6:40pm

Hi Barry,

There are many different ways to code and no doubt your technique and style will change with more practise.

If all you want to do is reduce the duplication then try this. Please note the commonFunction() is verbose because I find it a lot easier to quickly scan and find errors.


 <?php define ('SPC', '&nbsp;&nbsp&nbsp;');

$variables = array 
(
  'content_date'       => date('H:i:s'),
  'content_attributes' => 'content_attributes',
  'items'              => array('One','two','three'),
  'item_attributes'    => 'item_attributes',
  'classes'            => 'classes',
  'attributes'         => 'attributes',
);
echo '<pre>'; print_r($variables); echo '</pre>';

echo '<br />' .theme1_field__field_event_date_and_time__event($variables);

echo  '<br />' .theme1_field__field_event_location__event($variables);


//=================================
function theme1_field__field_event_date_and_time__event($variables)
{
  echo  '<b>function: </b>' .__FUNCTION__ .'<br />';;

  $output = '<span class="time"></span>';

  $output .= commonFunction($variables);  

  return $output;
}

//=================================
function theme1_field__field_event_location__event($variables)
{
  echo  '<b>function: </b>' .__FUNCTION__ .'<br />';

  $output = '<span class="map-marker"></span>';
  
  $output .= commonFunction($variables);  
  
  return $output;
}  

//=================================
function commonFunction($variables)  
{
  echo  SPC .'<b>function: </b>' .__FUNCTION__ .'<br />';

  $output = '';

  // Render the items.
  $output .= '<div class="field-items"' 
          .     $variables['content_attributes'] 
          .     ' style="display:inline-block">';

  foreach ($variables['items'] as $item)
  {
    $classes = 'field-item';
    $output .= '<div class="' 
            .   $classes 
            .    '"' 
            .    $variables['item_attributes'] 
            .  '>' 
            .    'drupal_render($item)' 
            . '</div>';
  }
  $output .= '</div>';
  
  // Render the top-level DIV.
  $output .= '<li class="list-group-item"><div class="' 
          .    $variables['classes'] 
          .   '"' 
          .   $variables['attributes'] 
          .   '>' 
          .    $output 
          . '</div></li>';

  return $output;
}


//=================================
function drupal_render($var)
{
  echo  SPC .'<b>function: </b>' .__FUNCTION__ .'<br />';

  return $var; 
}

Classes are a whole new ball game, have fun

John

fretburner · June 5, 2014, 6:46pm

The only actual difference between the two functions seems to be the class name that’s applied to the span element, so why not just do something like this?


function theme1_field__field_event__event($type, $variables) {
    $output = '';
    $output .= "<span class=\\"$type\\"></span>";

    //10+ lines of duplicate code
}

I’m not familiar with the Drupal naming convention for theme functions, but you’d basically want to make it generic so that it applies to any event type, with the type passed as the first parameter.

computerbarry · June 5, 2014, 8:14pm

Thanks John

There are many different ways to code and no doubt your technique and style will change with more practise.
If all you want to do is reduce the duplication then try this.

Code examples much appreciated, hopefully once I get a bit more established as you say
I like your coding style here.

Hi fretburner
Yes this is exactly the kind of thing I’m thinking of:

function theme1_field__field_event__event($type, $variables) {  
    $output = '';  
    $output .= "<span class=\\"$type\\"></span>";  

    //10+ lines of duplicate code  
}

The only actual difference between the two functions seems to be the class name that’s applied to the span element…

At the moment yes, though I need the two different function names as these function names are unique to the drupal fields, I also have a third function which also has the same code excluding the <span>. So in total we have:


function theme1_field__event($variables) {
// this is generally what the event fields default to
}
function theme1_field__field_event_location__event($variables) {
}
function theme1_field__field_event_date_and_time__event($variables) {
}

What do you suggest now?

you’d basically want to make it generic so that it applies to any event type, with the type passed as the first parameter

And could you elaborate on this.

FYI

theme_1 = the name of our theme
field = we’re targeting a field (can be other things like, block or view)
field_event_location = the unique field name
event = the content type name

Putting all that together = theme1_field__field_event_location__event

oddz · June 5, 2014, 10:55pm

You would just call the function which you copied the code from. If the file that function exists in hasn’t been included yet than you would need to manually call drupal_load_include to bring in the file.


function theme1_field__field_event_location__event($variables) {
  $output = foobar($variables); // you need to figure out what this function is
  $out = 'mystuff'.$out;
  return $out;
}

Though there is likely a *better way to achieve your end goal than doing that. I would have to understand the whole scope to actually recommend anything else.

fretburner · June 5, 2014, 11:19pm

I’ve just seen that oddz already posted a similar reply, but as I’ve already written a response I’ll post it anyway in case it’s useful.


function theme1_field__event($variables)
{
    return build_event_item($variables);
}

function theme1_field__field_event_location__event($variables)
{
    $prefix = '<span class="map-marker"></span>';
    return build_event_item($variables, $prefix);
}

function theme1_field__field_event_date_and_time__event($variables)
{
    $prefix = '<span class="time"></span>';
    return build_event_item($variables, $prefix);
}

function build_event_item($variables, $prefix_markup = '')
{
    // Render the items.
    $items = '';
    foreach ($variables['items'] as $item) {
        $classes = 'field-item';
        $items .= '<div class=' . $classes. '>' . drupal_render($item) . '</div>';
    }

    // Render the containing markup.
    $output = <<<EOD
    <li class="list-group-item">
      <div class="{$variables['classes']}" {$variables['attributes']} >
        $prefix_markup
        <div class="field-items" {$variables['content_attributes']} style="display:inline-block">
          $items
        </div>
      </div>
    </li>
EOD;

    return $output;
}

computerbarry · June 6, 2014, 12:58am

It works fretburner
I see what you have done here, this is exactly the type of approach I was referring to.

Likewise with oddz suggestion which I tried first though the <span> elements were being printed outside of the <li>, not sure if I was doing it correctly.
I’ll digest all this properly tomorrow and get back with updates, very tired now (:

Also hoping to see what oddz suggests once I explain a little further, maybe we can fine tune this even more

Thanks again all for your time.

Chat soon, Barry

computerbarry · June 8, 2014, 12:04am

Though there is likely a *better way to achieve your end goal than doing that. I would have to understand the whole scope to actually recommend anything else.

I’m general just changing/adding a bit of markup, new attributes to some fields inside my event content type.

The only other alternative I could see oddz was to create a load of template files field–field-event-date-and-time.tpl.php and so on.
I was under the impression that if possible, its better to make the changes inside your template.php, faster ?

What other ways are there?
I’ll soon have more fields I need to change, things can soon add up.
Is there a best approach, wouldn’t my template.php soon become heavy with code for making tweaks to fields, or would I be better creating template files?

Nice to hear you view on this and how you do things yourself.

And thanks again fretburner, currently using your method.

Thanks,
Barry

oddz · June 8, 2014, 12:46am

Why do you need to add additional attributes and change the markup in the first place.

computerbarry · June 8, 2014, 11:09am

So I can customise my site more. Sometimes I’ll need to add spans and other html elements, custom graphics, microdata, data-attributes amongst other things.
Isn’t this what the theming layer is for so we can build our own theme to how we like it?

So whats best, template.php or .tpl.php ?
A mixture or do you recommend a different approach oddz?
And are you saying what I’m doing above is wrong?

I just want to make sure I’m doing things correctly before I dive into a big build.
Appreciate your feedback thanks.

Cheers, Barry

oddz · June 9, 2014, 2:43am

It’s difficult to speak on a generic level about this topic. Could you provide some concrete examples of what your actually trying to accomplish from a design standpoint.

computerbarry · June 9, 2014, 10:28am

In a nutshell oddz,
I’d just like to understand the best way of modifying fields, best practice, writing less code and speed.
Generally, how to add extra markup and attributes to fields when I need extra customisation per theme for individual fields.

Cheers, Barry

oddz · June 9, 2014, 10:04pm

Well to be honest I’ve never needed to override the HTML output of a field provided by core or a contrib module. Having said that I have created my own custom fields, and used token replacement in views to customize HTML. I’ve also created custom view modes and displays for fields to solve other problems. There are many different ways to achieve things in Drupal. Though on a generic level it is difficult to recommend any one approach. Typically though if you have to repeat code like you have done there is a better means of solving the problem at hand. Typically… not always.

computerbarry · June 9, 2014, 10:22pm

There are many different ways to achieve things in Drupal.

As I’m finding out

I have created my own custom fields, and used token replacement in views to customize HTML

I have come across tokens though haven’t really used them yet, generally been learning and working with code, rather than the admin.

Though on a generic level it is difficult to recommend any one approach

Might sound like a silly question, but what exactly do you mean when you say ‘generic level’ slightly confusing me, what does it mean?

Thanks, Barry

oddz · June 9, 2014, 10:39pm

I mean why do you need to add attributes or classes. Do you need them as hooks for other libraries to apply css or javascript perhaps.