37
\$\begingroup\$

I'm struggling for a while now with the readability of my code, I after I tried to get as much insight as possible (for my standards). On my level, I think I understand and use it all right for my level.

But I'm still having big chunks of mixed html/css in the presentation. Often I have a mediocre complex multi-dimensional array as a return value and on the actual presentation page, I iterate through it, but still do a lot of stuff with it.

So I'm looking now into template engines like Smarty, but I can't get my head around it, how I would save some actual code with it in examples like the following where I iterate and work with the array in the presentation:

$courseinfo = new courseinfo($_SESSION['course_short']);
$row = $courseinfo->get_all(); 
$default = $courseinfo->get_default();
$prices = $courseinfo->get_prices();
$month_min_show = 5;

//color settings for prices
$colorlow = '#6F6'; 
$colormid = '#09F'; 
$colorhigh = '#F90'; 
$colorspecial = '#F0F';
$colorfull = 'rgba(255,0,0,0.3)';


/* CONTENT CALENDAR
-----------------
-----------------
*/

echo '<div id="calendar">';
$count['month'] = 0;
foreach($row as $month)
{
    $lastcourse = end($month['course']);
    $laststart = $lastcourse['date'];
    $enddate = new DateTime($laststart);
    $enddate->modify('+ '.($lastcourse['length']-1).' days');
    $iterate = new DateTime('01-'.date('m',strtotime($laststart)).'-'.$month['year']);
    if (!isset($stored['year']) || isset($stored['year']) && $stored['year'] != $month['year'])
    {

        if($count['month'] > $month_min_show) { break;}     // don't show next year if $month_min_show months already displayed
        if(isset($stored['year'])) { echo '<br /><br /><br /><div style="margin-top:-10px"></div>';}
        echo '<span class="year" style="float:left;">'.$month['year'].'</span>';
        echo '<div style="float:right;margin-top:-20px;padding-right:5px;">';
        echo '<div style="float:left;font-size:12px;font-weight:bold;">PRICES '.$month['year'].'</div>';

        echo '<div class="pricelegend" style="background-color:'.$colorfull.'">fully booked</div>';
        if (in_array('low',$prices[$month['year']])) { echo '<div class="pricelegend" style="background-color:'.$colorlow.'">'.$default['price_low'].' &euro;</div>'; }
        if (in_array('mid',$prices[$month['year']])) { echo '<div class="pricelegend" style="background-color:'.$colormid.'">'.$default['price_mid'].' &euro;</div>'; }
        if (in_array('high',$prices[$month['year']])) { echo '<div class="pricelegend" style="background-color:'.$colorhigh.'">'.$default['price_high'].' &euro;</div>'; }
        if (in_array('custom',$prices[$month['year']])) { echo '<div class="pricelegend" style="background-color:'.$colorspecial.'">Special Offer</div>';}


        echo '</div><div style="clear:both;"></div><hr width="800px;" align="left"/>';
    }


    echo '<div class="m_start">'.mb_strtoupper($month['monthname'],'UTF-8').'<br />';
        echo '<span class="yearsmall">'.$month['year'].'</span>';
    echo '</div>';
    echo '<div class="courses">';
    echo '<div style="float:left;width:10px;">&nbsp;</div>';
    while($iterate<=$enddate)
    {
        $dayname = strftime('%a',$iterate->format('U'));
        if ($dayname == "So" OR $dayname == "Sa") { $daycolor = "#999"; } else { $daycolor = "#FFF";}

        echo '<div class="dayname" id="'.$iterate->format('dmY').'" style="color:'.$daycolor.'">'.$dayname.'</div>';

        $iterate->modify('+ 1 days');
    }
        echo '<br />';

    $lineswitch = 0;
    foreach($month['course'] as $course) 
    { 
        $date = $course['date'];
        $date = new DateTime("$date");
        $coursewidth = $course['length']*20-2;
        if($course['class'] == 'low') { $pricecolor = $colorlow; }
        elseif($course['class'] == 'mid') { $pricecolor = $colormid; }
        elseif($course['class'] == 'high') { $pricecolor = $colorhigh; }
        else {$pricecolor = $colorspecial;}
        if($course['user'] >= $course['usermax']) { $pricecolor = $colorfull;  }
        if(isset($_SESSION['course_id']) && $_SESSION['course_id'] == $course['id']) { $bordercolor = 'border-color:#FFF';} else {$bordercolor = '';}

        if($course['user'] < $course['usermax']) { echo '<a class="clink" id="'.$course['id'].'" href="'.$_SESSION['book_url'].'?course='.$course['id'].'" target="_self">'; }
        echo '<div class="course" style="background-color:'.$pricecolor.';'.$bordercolor.';width:'.$coursewidth.'px;margin-top:'.$lineswitch*17 .'px;margin-left:'.(10+($date->format('d')-1)*20).'px">';

        if($course['user'] < $course['usermax'])
        {
            echo '<span class="coursestart">&nbsp;'.$date->format('d').'</span>';


            if($course['length'] > 1) 
            {
                echo '-';
                $date->modify('+ '.($course['length']-1).' days');
                echo '<span class="courseend">'.$date->format('d').'&nbsp;</span>';
            }
        }
        else { echo '<span style="color:#000;">x</span>'; }

        echo '</div>';
        if($course['user'] < $course['usermax']) {echo '</a>';}
        unset($date);
        if ($lineswitch == 0) { $lineswitch = 1;} else {$lineswitch = 0;}
    }
    echo '</div>';

    echo '<div class="m_end"></div>';
    echo '<div style="clear:both;"></div><br />';
    $stored['year'] = $month['year'];
    $count['month']++;

}       
echo '</div>';

Here's an example of the array I'm iterating through:

Array ( [04.2012] => 
    Array ( [monthname] => April [year] => 2012 [course] => 
        Array ( 
           [0] => Array ( [id] => 106 [date] => 2012-04-02 14:00:00 [length] => 3 [class] => mid [price] => 110 [user] => 0 [usermax] => 20 [day] => 02 [week] => 14 [dayname] => Mo [hours] => 3 ) 
           [1] => Array ( [id] => 107 [date] => 2012-04-03 10:00:00 [length] => 3 [class] => mid [price] => 110 [user] => 0 [usermax] => 20 [day] => 03 [week] => 14 [dayname] => Di [hours] => 3 ) 
           [2] => Array ( [id] => 108 [date] => 2012-04-05 14:00:00 [length] => 3 [class] => mid [price] => 110 [user] => 0 [usermax] => 20 [day] => 05 [week] => 14 [dayname] => Do [hours] => 3 )   
          ) 
       ) 

This is quite a bit of code as you can see, just so you can get an idea how I still have to work a lot with the array.

  • So how could I split this into smaller chunks or just make it more readable and easier to work with ?

Hope I could make clear what I want here...and sure if you find anything else that's totally stupid in this code, give me a word!

I wouldn't know how a template engine would help as it still is a lot of ifs and dynamic changes in there.

Sidenote: I'm working alone and always will, so the separation is just for me.

\$\endgroup\$

migrated from stackoverflow.com Nov 28 '11 at 15:04

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ I would suggest using a PHP framework such as cakePHP. These frameworks use an MVC (model View Controller) architecture which will help you separate your business logic from presentation code. \$\endgroup\$ – GFuller Nov 28 '11 at 11:11
28
\$\begingroup\$

Thank you for the perfect example of the real-life piece of presentation logic.
Most people pushing some primitive templating solutions just have no idea of such a complex cases existence.

Three rules for you to get it right:

  1. Use PHP as a template engine.
  2. Output not an HTML tag nor text constant using PHP echo but use straight HTML only.
  3. Move ALL data preparations to the business logic part.

Format ALL your data in the business logic.
Pass only scalars to the template. No datetime objects!
No complex logic - use only basic PHP syntax in the template.

So, foreach your data twice:

  • first time to do all the data preparations and formatting.
  • and next time to do the actual output in the template.

So, PHP code become like this

$count['month'] = 0;
foreach($row as $i => $month)
{
    $month['lastcourse'] = end($month['course']);
    $month['laststart']  = $month['lastcourse']['date'];
    $month['enddate']    = new DateTime($month['laststart']);
    $month['enddate']->modify('+ '.($month['lastcourse']['length']-1).' days');
    $month['iterate']    = new DateTime('01-'.date('m',strtotime($month['laststart'])).'-'.$month['year']);
    $month['showyear']   = (!isset($stored['year']) || isset($stored['year']) && $stored['year'] != $month['year']);
    $month['monthname']  = mb_strtoupper($month['monthname'],'UTF-8');
    $row[$i]             = $month;
}   

while template as clean as this

<div id="calendar">
<? foreach($row as $month): ?>
  <? if ($month['showyear']): ?>
    <? if ($stored['year']): ?>
        <br /><br /><br /><div style="margin-top:-10px"></div>
    <? endif ?>
        <span class="year" style="float:left;"><?=$month['year']?></span>
        <div style="float:right;margin-top:-20px;padding-right:5px;">
        <div style="float:left;font-size:12px;font-weight:bold;">PRICES <?=$month['year']?></div>
        <div class="pricelegend" style="background-color:<?=$colorfull?>">fully booked</div>
        some code removed
        </div><div style="clear:both;"></div><hr width="800px;" align="left"/>
  <? endif ?>
        <div class="m_start"><?=$month['monthname']?><br />
        <span class="yearsmall"><?=$month['year']?></span>
        </div>
    <div class="courses">
    <div style="float:left;width:10px;">&nbsp;</div>
<? endforeach ?>
</div>

I am not going to reformat all your code, but just to give you an idea.

\$\endgroup\$
  • \$\begingroup\$ After reading this now for a couple of times, I totally dig this! It's a lightweight and plain simple approach to handle my problem. I will try to rewrite my own code now and see how it goes - might come back for further questions. I will leave the question still open in case someone else has a few more words to say! Thanks so much!! \$\endgroup\$ – Anonymous Nov 28 '11 at 11:36
  • 3
    \$\begingroup\$ Feel free to ask. I am developing such an approach for years and hope I have something to share from the experience I've got. \$\endgroup\$ – Col. Shrapnel Nov 28 '11 at 11:40
  • 2
    \$\begingroup\$ Inline styles and shorthand PHP tags are usually a no-go. Apart from that good job. :) \$\endgroup\$ – Daveo Nov 28 '11 at 12:11
  • 2
    \$\begingroup\$ @Daveo The shorthand PHP tags that cause trouble (currently) are <? and <%. Using <?= is fine. \$\endgroup\$ – Levi Morrison Nov 28 '11 at 19:17
6
\$\begingroup\$

Working with this batch:

//color settings for prices
$colorlow = '#6F6'; 
$colormid = '#09F'; 
$colorhigh = '#F90'; 
$colorspecial = '#F0F';
$colorfull = 'rgba(255,0,0,0.3)';

// ...
// ...
// ...

if (in_array('low',$prices[$month['year']])) { echo '<div class="pricelegend" style="background-color:'.$colorlow.'">'.$default['price_low'].' &euro;</div>'; }
if (in_array('mid',$prices[$month['year']])) { echo '<div class="pricelegend" style="background-color:'.$colormid.'">'.$default['price_mid'].' &euro;</div>'; }
if (in_array('high',$prices[$month['year']])) { echo '<div class="pricelegend" style="background-color:'.$colorhigh.'">'.$default['price_high'].' &euro;</div>'; }
if (in_array('custom',$prices[$month['year']])) { echo '<div class="pricelegend" style="background-color:'.$colorspecial.'">Special Offer</div>';}

One way to do it would be to use something like this:

# place your colors into an array instead
$colors = array();
$colors['low'] = '#6F6'; 
$colors['mid'] = '#09F'; 
$colors['high'] = '#F90'; 
$colors['special'] = '#F0F';
$colors['full'] = rgba(255,0,0,0.3);

# price legend array, since your code appears to be checking each one
$priceLegend = array();
$priceLegend[] = 'low';
$priceLegend[] = 'mid';
$priceLegend[] = 'high';
$priceLegend[] = 'custom';

// ...
// ...
// ...

Then on the html output side:

<div class="pricelegend" style="background-color:<?php echo $colors['full']; ?>">fully booked</div>
<?php
    foreach($priceLegend as $key => $value) {
    if (in_array($value,$prices_array[$months_array['year']])) {
        $priceBackclr = $colors_array[$value];
        $priceDisplay = $defaults_array['price_'.$value];
        $priceDisplay .= ($priceDisplay != 'Special Offer') ? ' &euro' : '';
        ?>
        <div class="pricelegend" style="background-color:<?php echo $priceBackclr; ?>"><?php echo $priceDisplay; ?></div>';
        <?php
    }
}
?>
</div><div style="clear:both;"></div><hr width="800px;" align="left"/>

It looks like a lot more code, but making changes should not be so tough; make one change here then the others will follow.. I mean, for the price legend's (for example) div style or something..

Anyway, it is just one of the many ways to do it.. And you can apply this to other sections of code... There are better methods out there; hope more will post here.

[EDIT] Additional option/code provided as a result of on-going comment exchange [ EDIT]

Place this function in one of your includes if you'd like:

# function does the in_array for you
function findInArrayAndReturnAsArray($valueToFind, $arrayToFindIn, $outputType) {
    # use $outputType so that if you would like, 
    # you can reuse this function to do other stuff like find 
    # another value in another array

    $outputArray = array();

    if ($outputType = 'price') {
        if (in_array($valueToFind, $arrayToFindIn)) {

            $outputArray[] = $colors_array[$valueToFind];
            $temp = $defaults_array['price_'.$valueToFind];
            $temp .= ($priceDisplay != 'Special Offer') ? ' &euro' : '';
            $outputArray[] = temp;
        }
    } 

    return $outputArray;

}

Then in your template, if you need to display it, use it this way:

# go through each item in the price legend array and grab the results and then echo them with the html
foreach($priceLegend as $key => $value) {
    $priceLegendDisplay = findInArrayAndReturnAsString($value , $prices_array[$months_array['year']], 'price');
    echo = '<div class="pricelegend" style="background-color:'.$priceLegendDisplay[0].'">'.$priceLegendDisplay[1].'</div>';
}

The function can help DRY out your code. It can act flexible enough to accommodate other actions based on $outputType values you set and code for. Imagine that function stored away somewhere, and then your output page simply calls it. Then if you can, create your own class that will contain functions like these that handle how to output data. It might appear like a controller and view from the MVC design pattern, but with fine tuning you can turn it into a pure view. So I guess what I'm saying is.. To use a design pattern, object oriented approach, as well as keeping in mind to stay true to Don't Repeat Yourself principle.

\$\endgroup\$
  • \$\begingroup\$ +1 for your effort or even more for the idea - because I see were you want to push me. Your code is more automated one could say. In this special case it wouldn't make sense though, but that doesn't lower the idea. Thanks for that too! Great to have different input!! \$\endgroup\$ – Anonymous Nov 28 '11 at 13:11
  • \$\begingroup\$ Glad to have helped. I was actually going to post about creating more classes an DRY'd out functions, and then make them work dynamically with css, but I wasn't sure it would make sense since it'd somewhat be like suggesting a lot of code reformating.. But anyway, thanks!! \$\endgroup\$ – Nonym Nov 28 '11 at 14:16
  • \$\begingroup\$ At this stage it might be a bit too much for me to reformat my whole coding, but I'm always eager to learn, especially when it's about structuring. Could you give a rough example how you would do that please ? \$\endgroup\$ – Anonymous Nov 28 '11 at 15:01
  • \$\begingroup\$ @danontheline, is there a chance that you're already using a design pattern for your code? I would definitely suggest OOP and DRY programing, that's for sure, but I wanna ask first in case you already have a design pattern in use.. \$\endgroup\$ – Nonym Nov 29 '11 at 11:08
  • \$\begingroup\$ Okay, here are two links that I feel might give you a good refresh on OOP: net.tutsplus.com/tutorials/php/oop-in-php and phpfreaks.com/tutorial/oo-php-part-1-oop-in-full-effect ..Next, I personally apply OOP even to small projects like a simple survey or petition page. I go procedural when I need to whip up something quick to see if the idea works. Then I go OOP on it. :D Try to apply it to all your projects. As for DRY.. A rough example would be.. Let me update my answer... \$\endgroup\$ – Nonym Nov 29 '11 at 21:56
1
\$\begingroup\$

You might also consider using a template engine like Twig or Smarty.

They both provide complete separation of display and logic.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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