[RESOLVED] Code "best practices" question

[ixalmida]

Just out of curiosity, I am wondering what the more experienced coders here would consider best practice for jumping in and out of PHP code in a web page. I generally prefer to keep my PHP and HTML in larger blocks, but some people like to use the <?php ?> tags like they were going out of style.

For example, do you prefer this?

<?php
  if ($table == "Forecasted") {
?>
      <td align="center" style="font-weight:bold; font-size:10pt">Dispose Project</td>
<?php
  }
?>
<?php
  if ($table == "Hold") {
?>
      <td align="center" style="font-weight:bold; font-size:10pt">Retrieve Project</td>
<?php
  }
?>

Or, this?

if ($table == "Forecasted")
  {
  echo '<td align="center" style="font-weight:bold; font-size:10pt">Dispose Project</td>\n';
  }
if ($table == "Hold")
  {
  echo '<td align="center" style="font-weight:bold; font-size:10pt">Retrieve Project</td>\n';
  }

There's no question that the 2nd example is much more readable code, but there are always trade-offs. In this case, what you gain on the code view side would be lost somewhat in the resulting HTML view.

The "PHP Bible" is fairly silent on the issue. Is there a best-practice here?

[bradgrafelman]

I prefer something different; separating PHP code from HTML entirely:

if ($table == "Forecasted")
  {
  readfile('content/forecasted.inc');
  }
if ($table == "Hold")
  {
  readfile('content/hold.inc');
  } 

On a different topic altogether, I also prefer to use CSS rather than tables/styling attributes/etc. :p

[nrg_alpha]

I personally wouldn't bother making a line like that an include (as I tend to reserve includes for chunk of code such as headers and footers that can be changed to see those changes propogate site wide). My personal preference is the second OP's listing.. And a note to the OP, that \n at the end needs to be ecapsulated in double quotes, otherwise you are actually outputting \n on screen.

if ($table == "Forecasted") { 
  echo '<td align="center" style="font-weight:bold; font-size:10pt">Dispose Project</td>' . "\n"; 
} else if ($table == "Hold") { 
  echo '<td align="center" style="font-weight:bold; font-size:10pt">Retrieve Project</td>' . "\n"; 
}

And yeah, unless those td tags are for a table used for displaying tabular data, best to learn to build 'table-less' sites. Less markup, and easier to maintain / update / change via external CSS stylesheets.

[ixalmida]

Interesting...I kind of like your idea but I'm not sure if it would always be practical. Imagine wanting to include a variable in the middle of the echo statement - that could get messy.

Oh, and I am all about CSS - but that code wasn't mine. The developer who wrote that used CSS very sparingly.

[nrg_alpha]

ixalmida;10899904 wrote:

Interesting...I kind of like your idea but I'm not sure if it would always be practical. Imagine wanting to include a variable in the middle of the echo statement - that could get messy.

That shouldn't be a problem..

echo 'Lorem ipsum dolor sit amet, consectetur' . "$variable" . 'adipiscing elit. Donec euismod venenatis erat.';

Obviously, take the route that is most comfortable for you. I was simply stating my personal preference. I personally like keeping includes down to a minimum and for stuff that is meant to be modular (as in the reasons already mentioned in my previous post). But use whatever methodology works for you.

[ixalmida]

nrg_alpha;10899895 wrote:

...that \n at the end needs to be ecapsulated in double quotes, otherwise you are actually outputting \n on screen.

Wow - I didn't know that! But then, I usually encapsulate in the reverse manner, i.e. - double-quotes outside, and single inside, so I've never noticed a problem.

nrg_alpha;10899895 wrote:

And yeah, unless those td tags are for a table used for displaying tabular data, best to learn to build 'table-less' sites. Less markup, and easier to maintain / update / change via external CSS stylesheets.

This is all financial data, so there's a LOT of tabular data. But again, the code I posted isn't mine - it's just a sample of the code I need to rewrite. I just want to do it right when I fix it.

I just wish my head wouldn't spin whenever I think about fixing 7 years worth of code written by a young kid who never intended to become a developer.

[nrg_alpha]

ixalmida;10899906 wrote:

I just wish my head wouldn't spin whenever I think about fixing 7 years worth of code written by a young kid who never intended to become a developer.

<----- never intends to become a developer :p
(not professionally anyway.. just want to learn as much as I can so I can build my own sites to high enough standards.... I'm working on it.)

[ixalmida]

nrg_alpha;10899905 wrote:

That shouldn't be a problem...

Sorry, I meant to reply to bradgrafelman. He was saying that he keeps all his HTML in an include. I was first thinking you'd want to include PHP variables in the middle of your HTML, so you'd still have little sections of PHP.

Plus, it would get ugly trying to remember which variables are in which includes. I have enough trouble remembering to put my session_start() above my authentication functions.

[Weedpacket]

ixalmida wrote:

I was first thinking you'd want to include PHP variables in the middle of your HTML, so you'd still have little sections of PHP.

Templates by their very nature usually do need other bits interpolated before they become real pages - [man]sprintf[/man] or [man]vsprintf[/man] are useful in this context (even more useful than their C antecedents, even). For richer markup a "template" often itself becomes a function. Of course, that just pushes the original issue down another level (now you're wanting to figure out how to do the template function), but the problem domain is now a lot smaller (e.g., error message feedback, form layout) and can be addressed more specifically.

[bradgrafelman]

As Weedpacket points out, templates can be more than just plain text documents; Smarty templates, for example, even allow you to include loops and other PHP functions in your templates.

Granted, it might seem overkill for just one line of HTML, but I was thinking more in the grander scheme of things where you might have larger blocks of HTML.

[Weedpacket]

If it is just a matter of one line - this specific case, where we're talking about a table cell's contents depending on the value of a cell:

// There should be a better name than $mapping,
// and you wouldn't really construct the array on the fly like this.
$mapping['Forecasted'] = 'Dispose Project';
$mapping['Hold'] = 'Retrieve Project'; 
echo "<td align\"center\" style=\"font-weight:bold; font-size:10pt\">$mapping[$table]</td>";

Incidentally, I strongly suspect that this is supposed to be a <th> element, not a <td> element; between that and a touch of CSS the line could become

echo "<th>$mapping[$table]</th>";

Or

<th><?php echo $mapping[$table]?></th>

[jazz_snob]

In the past I've used what I call "view layer helper functions"

<td><?php print draw_what_your_supposed_to_draw($table);?></td>

Where I snippet out the attributes in <td> for appearance for clarity ...use CSS you'll thank yourself later!

Thereby encapsulating the logic about what to draw into a function you can put into a library file and unit test 🙂

name your function whatever you feel is appropriate (ie at a glance you can tell what its doing)

[ixalmida]

Just checking back to make sure I mark my threads resolved and I decided to review this one. I'm glad I did because I missed some replies!

I like jazz_snob's idea of table-drawing functions. I'll have to think about how I could implement that.

Just for the record, I rarely use inline styling - the code listing was from a previous developer who knew little about CSS. My main CSS file is about 600 lines of code. Of course, I keep learning; the file used to be 2,200 lines of code, but I re-styled and optimized the entire site. I really like to write code in as plain English as possible. I probably would have done something like...

// Figure out which columns exist...
switch($table['field'])
{
  case "Forecasted":
    $header = "Dispose Project";
    break;
  case "Hold":
    $header = "Retrieve Project";
    break;
  case "Booked":
    $header = "Show Project";
    break;
  [..]
}

// Show columns if they exist...
foreach($table as $value)
{
  echo "<th>$value</th>";
}