Laying out php code for readability and maintenance...

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
CoderGoblin
DevNet Resident
Posts: 1425
Joined: Tue Mar 16, 2004 10:03 am
Location: Aachen, Germany

Laying out php code for readability and maintenance...

Post by CoderGoblin »

OK not sure where to really post this so mods feel free to move it....

As a fairly long member, if not regular member poster, on this forum it has come to my attention that often people do not structure code in such a way as to make it easily readable and maintainable. Te purpose of this topic is to simply give people something to think about. I am not saying that this is the only way to do things but I have found it useful. This is often called templating and some people argue against it but that is for you to decide...

Php and HTML can mix and this can often lead to the following looking PHP code:

Code: Select all

<html>
  <head>
    <title>Mix</title>
  </head>
  <body>
<?php 
 
  // This function simple outputs different strings
  function decide() {
    if (!empty($_GET['testvar']) {
      echo '<p>This is the test var: '.$_GET['testvar'].'</p>';
    } else {
      echo "<p>Well you didn't pass in a test var</p>";
    }
  }

  // The $out variable is used to show some output
  if (!empty($_GET['myvar']==1) {
?>
     <p>hello myvar has a value of one</p>
<?php
  } else {
?>
    <p>hello myvar does not have a value of one</p>
<?php 
}
decide(); 
?>
  </body>
</html>
If I haven't done any typos then this is perfectly valid and will work but is it really readable ? I don't think so, so I'll start to implement just a couple of rules...

First rule...
1. Have a main PHP block at the top of the file. This should contain all your 'logic' often called business logic.
2. All functions go at the top of the php block.
3. Where necessary set variables for output later.

Compare the previous output with the following...

Code: Select all

<?php
  function decide() {
    if (!empty($_GET['testvar']) {
      echo '<p>This is the test var: '.$_GET['testvar'].'</p>';
    } else {
      echo "<p>Well you didn't pass in a test var</p>";
    }
  }

  // The $out variable is used to show some output
  if (!empty($_GET['myvar']==1) {
     $out="<p>hello myvar has a value of one</p>";
  } else {
    $out="<p>hello myvar does not have a value of one</p>";
  }
 
?>
<html>
  <head>
    <title>Mix</title>
  </head>
  <body>
<?php echo $out; ?>
<?php decide(); ?>
  </body>
</html>
I'll leave you to decide which one prefer to read but if you don't like the second one consider this...

Often you need to use something like sessions or the 'header' command to redirect the user based on parameters passed in, either through a form or directly set via $_GET. Both session_start and headers need to be sent before any output. Even a blank line will throw this. If you have a <?php start on the first line and you do not directly output and output within the php block you are less likely to run into any problems with the error message 'headers already sent'.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

You're following along the lines that coding logic should be separated from markup. Such is the idea of views and templating. Your suggestion is a step shy of a template.

Of course though, I agree, it is much more readable.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
CoderGoblin
DevNet Resident
Posts: 1425
Joined: Tue Mar 16, 2004 10:03 am
Location: Aachen, Germany

Re: Laying out php code for readability and maintenance...

Post by CoderGoblin »

CoderGoblin wrote: This is often called templating and some people argue against it but that is for you to decide...
I'd already mentioned that :lol: Must say that my purpose in creating this topic was to allow others to reference it when people post failing code which is very difficult to read with their mix of PHP and HTML. Also possibly for header redirection errors.
dekindia
Forum Newbie
Posts: 3
Joined: Tue Sep 18, 2007 9:28 am

Post by dekindia »

very readable,thx
mrkite
Forum Contributor
Posts: 104
Joined: Tue Sep 11, 2007 4:19 am

Post by mrkite »

I personally can't stand echoing out html code in quotes.

Code: Select all

<?
function blah()
{
   //do something
   echo "<p>something else</p>";
  // do more stuff
}
?>
That drives me nuts, especially when people start escaping quotes on html attributes.

I always prefer to break out of php:

Code: Select all

<?
function blah()
{
   //do something
?>
    <p>something else</p>
<?
  // do more stuff
}
?>
User avatar
CoderGoblin
DevNet Resident
Posts: 1425
Joined: Tue Mar 16, 2004 10:03 am
Location: Aachen, Germany

Post by CoderGoblin »

Down to personal preference but most people I have been in contact with find echo to be a lot more readable. When you start mixing php and HTML you can easily lose readability and therefore make the code a lot harder to maintain. When trying to echo/create a variable out a large block of text you could use the Heredoc syntax which means you don't need to escape any quotes. When it's a single line with HTML and attributes you can use single quotes.
* single quoted
* double quoted
* heredoc syntax
.

The other thing to bear in mind is to produce variables rather than output text directly in the main php block. Functions should normally return values as opposed to directly outputting. This makes life a lot easier when also adding things like translation.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: Laying out php code for readability and maintenance...

Post by Christopher »

CoderGoblin wrote:
CoderGoblin wrote: This is often called templating and some people argue against it but that is for you to decide...
I'd already mentioned that :lol: Must say that my purpose in creating this topic was to allow others to reference it when people post failing code which is very difficult to read with their mix of PHP and HTML. Also possibly for header redirection errors.
I think scottayy was perhaps thinking of something like this. A small, but I think conceptually significant difference.

view.php

Code: Select all

<?php
  function decide() {
    if (!empty($_GET['testvar']) {
      return '<p>This is the test var: '.$_GET['testvar'].'</p>';
    } else {
      return "<p>Well you didn't pass in a test var</p>";
    }
  }

  // The $out variable is used to show some output
  if (!empty($_GET['myvar']==1) {
     $out="<p>hello myvar has a value of one</p>";
  } else {
    $out="<p>hello myvar does not have a value of one</p>";
  }

  include 'template.php';
template.php

Code: Select all

<?php
<html>
  <head>
    <title>Mix</title>
  </head>
  <body>
<?php echo $out; ?>
<?php echo decide(); ?>
  </body>
</html>
(#10850)
mrkite
Forum Contributor
Posts: 104
Joined: Tue Sep 11, 2007 4:19 am

Post by mrkite »

CoderGoblin wrote:Down to personal preference but most people I have been in contact with find echo to be a lot more readable. When you start mixing php and HTML you can easily lose readability and therefore make the code a lot harder to maintain. When trying to echo/create a variable out a large block of text you could use the Heredoc syntax which means you don't need to escape any quotes.
The benefit for me is when it comes to syntax highlighting. You'll lose all syntax highlighting on your html with heredoc or singlequote echos.

Of course the better solution is to use a templating system like smarty or an MVC framework like cake.
User avatar
CoderGoblin
DevNet Resident
Posts: 1425
Joined: Tue Mar 16, 2004 10:03 am
Location: Aachen, Germany

Post by CoderGoblin »

mrkite wrote:Of course the better solution is to use a templating system like smarty or an MVC framework like cake.
The initial suggestion is indeed a step short of templating but as stated, the whole idea is to make code display more readable/therefore more maintainable and to try to avoid problems like "headers already sent". Just bear in mind that templating does not necessarily involve using Cake or Smarty, you can achieve templating with simple PHP as shown by aborint's example. Most of the legacy code I maintain actually has a body variable. This is appended to throughout the main php area and simply output at the end. Not a templating system but uses the same principle of never stopping/starting PHP just to output HTML. It is still easily readable.
Post Reply