PHP-generated table with colored cells

Question

How can I make my PHP script more easier? It seems quite difficult and long but I don ́t know how can I make it shorter. This code is inserted in my HTML5 code on my PHP tutorial webpage, but I need it to be shorter.

<?php
$n=1;
$k=0;
for ($line=0;$line<1000;$line++)
{
 echo "<tr>";
 if ($k==0)
{
 echo "<td style=\"background:rgb(". $n*25 .", ". (10-$n)*25 .", 0);\" colspan=$n>$n"; echo"</td>";
 for ($p=0;$p<10-$n;$p++)
 {
 echo "<td>"; echo"</td>"; 
 }
 $n++;
 if ($n==11)
 {
 $k=1;
 $n=9;
 }
 echo"</tr>";
}
 if ($k==1)
 {
 echo "<td style=\"background:rgb(". $n*25 .", ". (10-$n)*25 .", 0)\" colspan=$n>$n"; echo"</td>";
 for ($column=0;$column<10-$n;$column++)
 {
 echo "<td>"; echo"</td>"; 
 }
 $n--;
 if ($n==1)
 {
 $k=0;
 }
 echo"</tr>";
 } 
}

?>

Answer 1

First of all you should use a deeper indent than one space. Changing it to three spaces makes the logic more readable:

if($k==0)
{
 $k=0;
}

vs:

if($k==0)
{
 $k=0;
}

The next thing is, that you named your variables in a way, that makes them not readable. $k and $n are not readable at all. If you rename $n to $colspan and $k to $countAsc they express their meaning in your logic, which improves your code style by a lot. Other programmers (like me) can read way easier into your code then and have a faster understanding of your formulas.

Speaking of formulas, you should not put the calculations into the echo statement, but extract them, to separate the calculation of your values from its usage. That increases the overview of your variables by alot.

$red = $colspan * 25;
$green = (10 - $colspan) * 25;
/* ... */ "background:rgb(" . $red . "," . $green . ",0);" /* ... */

Speaking of the echo statements, you can separate the PHP from the HTML code. Since PHP 5.6 this is really easy due to the short echo tags <?= 'string to echo' ?>. This also enables you to indent the HTML tags as well, giving the reader an easier way to understand your overall structure.

<?php
$red = $colspan * 25;
$green = (10 - $colspan) * 25;
?>
<td style="background-color:rgb(<?= $red ?>, <?= $green ?>, 0);" colspan="<?= $colspan ?>">
 <?= $colspan ?>
</td>

As a next step you can safely extract the building of the <td ... ></td> out of the if clauses. They are equal in both cases and therefore are always echoed, no matter the if conditions. This is also true for your <td></td> loop. Both inner for loops are exactly the same, given the difference, that the variables have a different names. In addition to that, the $countAsc variable only has two states: Being 1 or 0. Because of that it can safely be changed to a boolean variable.

$colspan = 1;
$countAsc = TRUE;
for ($line = 0; $line < 1000; $line++): ?>
 <tr>
 <?php
 $red = $colspan * 25;
 $green = (10 - $colspan) * 25;
 ?>
 <td style="background-color:rgb(<?= $red ?>, <?= $green ?>, 0);" colspan="<?= $colspan ?>">
 <?= $colspan ?>
 </td>
 <?php
 for ($column = 0; $column < 10 - $colspan; $column++)
 {
 ?>
 <td></td>
 <?php
 }
 if ($countAsc) ...

After extracting all these things you can see, that the if clause only determines, whether the iteration of the $colspan variable is rising or falling. Therefore the if clause should represent only that, to make the logic more readable:

if ($countAsc && $colspan >= 10)
{
 $countAsc = false;
}
elseif (!$countAsc && $colspan <= 1)
{
 $countAsc = true;
}

After this logic is extracted, the calculation for the $colspan is that simple, that it can be shortened to the short if syntax (condition ? true value : false value;) to be in a more readable manner:

$colspan = $countAsc ? $colspan + 1 : $colspan - 1;

Defining the conditions in a more fitting way reduces the necessarity to redefine the $colspan at all and therefore makes your logic less complex. Also if you have huge for loops, it is more readable to use the for(condition): ... endfor; syntax, because like this you can see first hand what type of "closing bracket" is displayed at the end of the loop, giving the code, again, a better readability. The whole code is:

<?php
$colspan = 1;
$countAsc = TRUE;
for ($line = 0; $line < 1000; $line++): ?>
 <tr>
 <?php
 $red = $colspan * 25;
 $green = (10 - $colspan) * 25;
 ?>
 <td style="background-color:rgb(<?= $red ?>, <?= $green ?>, 0);" colspan="<?= $colspan ?>">
 <?= $colspan ?>
 </td>
 <?php
 for ($column = 0; $column < 10 - $colspan; $column++)
 {
 ?>
 <td></td>
 <?php
 }
 if ($countAsc && $colspan >= 10)
 {
 $countAsc = false;
 }
 elseif (!$countAsc && $colspan <= 1)
 {
 $countAsc = true;
 }
 $colspan = $countAsc ? $colspan + 1 : $colspan - 1;
 ?>
 </tr>
<?php endfor ?>

• Philipp Maurer: Running example: sandbox.onlinephpfunctions.com/code/…

Answer 2

I am going to borrow a couple of Philipp's variable names because they are solid advice.

First, my suggested code:

$colspan=1;
$increment=1; // 1 means ASC, -1 means DESC
echo "<table>";
 for($line=0; $line<20; ++$line){
 $red=$colspan*25;
 $green=250-$red;
 echo "<tr>";
 echo "<td style='background-color:rgb($red,$green,0);' colspan='$colspan'>$colspan</td>";
 echo str_repeat("<td></td>",10-$colspan);
 echo "</tr>";
 if($colspan==10){
 $increment=-1;
 }elseif($colspan==1 && $increment==-1){
 $increment=1;
 }
 $colspan+=$increment;
 }
echo "</table>";

If you copy the above into www.phptester.net it will render the html so you can see what is happening.

Now an itemized list of suggestions:

• Use $increment as a dual purpose variable: 1. convey direction and 2. act as literal incrementation amount
• Simplify the calculation of $green by merely subtracting from 250.
• Segment the calculations performed in each iteration before and after the block of echoes for improved readability.
• (Ordinarily I used escaped double quotes inside my echoes (\") but phptester doesn't like them.)
• use str_repeat() to display your empty table cells because it is more intuitive of the action and uses a shorter syntax.
• The only time $increment should be changed to -1 is when $colspan==10; there is no use checking if $increment==1 because it will be in this case.
• In the elseif put the $colspan condition first because it only occurs once every 18 iterations compared to $increment==-1 which occurs 9 times every 19 iterations. The logic here is prioritizing the quickest failure condition so that php can "short-circuit" the condition and move on.
• At the end of each iteration use the "combined operator" (assignment and addition) to modify $colspan using the appropriate $increment value.
• If you place a higher value on code brevity than readability, you can eliminate the declarations of $red and $green and write the calculated values into the echo line, but this will surely be a hindrance to future developers who read your code. (I believe this will do more harm than good.)
• Again, you can reduce code lines by concatenating the str_repeat() declaration to its previous line (and this line-crunching is true of the entire table row contents), but I feel this will lead to a reduction in readability.

p.s. I don't like to bounce in and out of php tags (<?php and ?>) over and over because I think it looks messy. Since my snippet spends the majority of its time using php, I just stay in it.

Answer 3

If I could add anything the others have not added yet, would be to avoid doing two statements in one line. For example:

echo "<td>"; echo"</td>";

The compiler does not care if you use two lines, but humans do!

It adds better readability, and it shows where you can improve. You can make that one line, or even use a comma to echo the two strings. For example:

echo "<td></td>";

echo "<td>", "</td>";

echo "<td>";
echo "</td>";