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 the following code below I was wondering if there is a way to clean it up/improve it especially near the if statements. I am new to programming and this is what I came up with and its working fine I just think there might be a better way to accomplish it.

    <?php 

require 'DB.php';

try {

    $stmt = $conn->prepare('SELECT * FROM equipment');
    $stmt->execute();
    } catch(PDOException $e){
        echo'ERROR: ' . $e->getMessage();
    }

echo "<table class='tableTwo'>
<tr>
<th>Equipment Type</th>
<th>Unit Number</th>
<th>Last Updated</th>
<th>Current Hours</th>
<th>Scheduled PM</th>
</tr>";

while($row = $stmt->fetch())
  {
  echo "<tr>";
  echo "<td>" . $row['equipType'] . "</td>";
  echo "<td>" . $row['unitNumber'] . "</td>";
  echo "<td>" . $row['lastUpdate'] . "</td>";

  if ($row['nextPM'] - $row['currentHours'] > 100) {

  echo "<td class='good'>" . $row['currentHours'] . "</td>";
  echo "<td class='good'>" . $row['nextPM'] . "</td>";

  } else if ($row['nextPM'] - $row['currentHours'] < 100 && $row['nextPM'] - $row['currentHours'] > 50){

  echo "<td class='soon'>" . $row['currentHours'] . "</td>";
  echo "<td class='soon'>" . $row['nextPM'] . "</td>";

  }

  else {
  echo "<td class='bad'>" . $row['currentHours'] . "</td>";
  echo "<td class='bad'>" . $row['nextPM'] . "</td>";
  }
  echo "</tr>";
  }
echo "</table>";
?>
share|improve this question

2 Answers 2

My own personal preference when outputting things like this is to put the portion where data is actually retrieved in a separate file and then use the alternate PHP structure syntax. While this probably isn't everyone's preferred coding style, in my opinion it makes it easier to structure HTML so that its readable among the PHP. From what I have seen, this appears to be relatively common in templates.

Just a quick once-over your file to show you what I mean:

<?php
require 'DB.php'

try {
    $stmt = $conn->query('SELECT * FROM equipment');
} 
catch(PDOException $e){
    echo'ERROR: ' . $e->getMessage();
    die();
}

?>
<table class='tableTwo'>
    <tr>
    <th>Equipment Type</th>
    <th>Unit Number</th>
    <th>Last Updated</th>
    <th>CurrentHours</th>
    <th>Scheduled PM</th>
    </tr>
<?php while($row = $stmt->fetch()): ?>
    <tr>
    <td><?php echo htmlspecialchars($row['equipType']); ?></td>
    <td><?php echo htmlspecialchars($row['unitNumber']); ?></td>
    <td><?php echo htmlspecialchars($row['lastUpdate']); ?></td>
<?php if ($row['nextPM'] - $row['currentHours'] > 100): ?>
    <td class='good'><?php echo htmlspecialchars($row['currentHours']); ?></td>
    <td class='good'><?php echo htmlspecialchars($row['nextPM']); ?></td>
<?php elseif ($row['nextPM'] - $row['currentHours'] < 100 && $row['nextPM'] - $row['currentHours'] > 50): ?>
    <td class='soon'><?php echo htmlspecialchars($row['currentHours']); ?></td>
    <td class='soon'><?php echo htmlspecialchars($row['nextPM']); ?></td>
<?php else: ?>
    <td class='bad'><?php echo htmlspecialchars($row['currentHours']); ?></td>
    <td class='bad'><?php echo htmlspecialchars($row['nextPM']); ?></td>
<?php endif; ?>
    </tr>
<?php endwhile; ?>
</table>

Again, if I were doing this I would separate that top half into another file and have that file include your "template" which would output the actual HTML. You could also use a template engine like Savant3 to make the separation a little cleaner and the templating better.

Demonstration with savant3:

Your main file (index.php perhaps?):

<?php
require 'DB.php'
require_once 'Savant3.php'
$tpl = new Savant3();

try {

    $stmt = $conn->query('SELECT * FROM equipment');
    $tpl->data = $stmt; 
} 
catch(PDOException $e){
    echo'ERROR: ' . $e->getMessage();
    die();
}

$tpl->display('table.tpl.php');

?>

Then in your template file (table.tpl.php in this example):

<table class='tableTwo'>
    <tr>
    <th>Equipment Type</th>
    <th>Unit Number</th>
    <th>Last Updated</th>
    <th>CurrentHours</th>
    <th>Scheduled PM</th>
    </tr>
<?php foreach($this->data as $row): ?>
    <?php $class = "bad";
    if ($row['nextPM'] - $row['currentHours'] > 100) {
        $class = "good";
    }
    elseif ($row['nextPM'] - $row['currentHours'] < 100 && $row['nextPM'] - $row['currentHours'] > 50) {
        $class = "soon";
    }
    ?>
    <tr>
    <td><?php echo htmlspecialchars($row['equipType']); ?></td>
    <td><?php echo htmlspecialchars($row['unitNumber']); ?></td>
    <td><?php echo htmlspecialchars($row['lastUpdate']); ?></td>
    <td class='<?php echo $class; ?>'><?php echo htmlspecialchars($row['currentHours']); ?></td>
    <td class='<?php echo $class; ?>'><?php echo htmlspecialchars($row['nextPM']); ?></td>
    </tr>
<?php endforeach; ?>
</table>

Note that I changed the while into a foreach because I personally don't like doing an assignment and comparison in the same statement. You can foreach over a PDOStatement (as far as I know) just as well as you can fetch from it.

EDIT: Also, you don't necessarily need to prepare the statment with the PDOs if there are no arguments. Of course, that's all up to preference again. If you have arguments, though, definitely use the prepared statements.

share|improve this answer
    
I feel it is important to demonstrate proper security when posting example code. Can you please update your example to include XSS protection? –  Rob Apodaca Jan 25 '13 at 3:30
    
That should do it...it's pretty basic since its just htmlspecialchars, but it should work so long as they don't actually need html to be displayed. –  Los Frijoles Jan 26 '13 at 17:23

First of all read Los Frijoles post, he is right :)

In addition to that you should extract large (not fitting on the screen/iframe) conditions in a variable (readability over performance) and move the common parts of your if branches out of your if. (I also use the short tag in templates, but this is an other discussion :P)

<?php foreach($this->data as $row): ?>
    <?php 
        $isGood=$row['nextPM'] - $row['currentHours'] > 100;
        $isSoon=$row['nextPM'] - $row['currentHours'] < 100 && 
                $row['nextPM'] - $row['currentHours'] > 50;
        if ($isGood) $class="good";
        else if ($isSoon) $class="soon";
        else $class="bad";
    ?>
    <tr>
        <td><?=$row['equipType'] ?></td>
        <td><?=$row['unitNumber'] ?></td>
        <td><?=$row['lastUpdate'] ?></td>
        <td class='<?=$class ?>'><?=$row['currentHours'] ?></td>
        <td class='<?=$class ?>'><?=$row['nextPM'] ?></td>
    </tr>
<?php endforeach; ?>

Of course you should move this logic into your other php file, but for demonstration this is fine.

share|improve this answer

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.