4
\$\begingroup\$

There are two arrays, $states and $weekdays. I want to loop through them to create a form, an HTML table, or possibly some other element).

Here's my example:

// addDay() Adds 1 day to the date
// format() Returns a date in a new format
// get_date() returns todays date in unix

$weekdays = [ 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday', 'Sunday' ];
$states   = [ 'start', 'finish', 'total', 'overtime' ];
$date     = get_date();

foreach ( $states as $state ) { ?>
  <tr class="<?php echo $state; ?>">
    <th><?php echo $state; ?></th>
    <?php
    $d = 0;
    foreach ( $weekdays as $weekday ) { ?>
      <td>
        <input name="date<?php echo $date->addDay( $d )->format('[Y][m][d]') . '[' . $state . ']'; ?>" />
      </td>
    <?php
    $d++;
      } ?>
  </tr>
<?php
}

I think the code can be cleaner. Ignore the fact that there aren't comments. Focus on the HTML / PHP intertwining. Can it be separated further or tidier?

Is there a method of sprintf but for sections of code perhaps? or would you echo the whole thing with double quotes for the variables?

Note — I am using PHP7, so be as fancy as you want with the latest syntax;

\$\endgroup\$
  • \$\begingroup\$ echo date->addDay - is that supposed to be echo $date->addDay? if so, where does $date come from? is it a Carbon DateTime object, a Zend_Date object, or something else? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 23 '17 at 21:06
  • \$\begingroup\$ Added a date variable for you, but imagine all functions work, all variables are needed. \$\endgroup\$ – Andrew Lazarus Oct 23 '17 at 21:57
  • \$\begingroup\$ Updated code to show functions – The question is more about formatting of php/html when woven together. \$\endgroup\$ – Andrew Lazarus Oct 23 '17 at 22:38
  • \$\begingroup\$ This being Code Review, we really don't like to leave anything to the imagination. See A guide to Code Review for Stack Overflow users. \$\endgroup\$ – 200_success Oct 23 '17 at 22:47
  • \$\begingroup\$ @SamOnela carbon datetime \$\endgroup\$ – Andrew Lazarus Oct 23 '17 at 22:49
2
\$\begingroup\$

To be honest, I was able to pickup and read you code really easily and understand it, which is the end goal. So what you have already is fine if you want to stick with it.

In saying that, there are a few things I try to do, when mixing html and php.

  1. Keep all the logic out of the html. This often involves building an array of pre-formatted values or structured data.

  2. I like the long form of foreach/endforeach, for/endfor etc. Just because I find them easier to spot in amongst the html, instead of looking for a } that could be part of a javascript, etc

  3. I prefer the short form of <?= instead of <?php echo

I note you have preset values for your arrays, which contain no html, otherwise I would be wrapping those echo statements in htmlentities() or the likes.

And lastly not sure if it is intended behaviour, but you use ->addDay(0), then addDay(1), then addDay(2), but when you get to 2, you will have actually added (1+2 = 3 days) to the date, and so on.

<?php

// addDay() Adds 1 day to the date
// format() Returns a date in a new format
// get_date() returns todays date in unix

$weekdays = [ 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday', 'Sunday' ];
$states   = [ 'start', 'finish', 'total', 'overtime' ];
$date     = get_date();

$weekdays_formatted = [];
foreach ($weekdays as $idx => $weekday) {
    $weekdays_formatted[] = $date->addDay( $idx )->format('[Y][m][d]');
}

?>

<?php foreach ( $states as $state ): ?>
<tr class="<?= $state; ?>">
    <th><?= $state; ?></th>
    <?php foreach ( $weekdays_formatted as $weekday_formatted ): ?>
        <td>
            <input name="date<?= "{$weekday_formatted}[{$state}]"; ?>" />
        </td>
    <?php endforeach; ?>
</tr>
<?php endforeach; ?>
\$\endgroup\$
  • \$\begingroup\$ I like your solution but personally I would avoid PHP complex string syntax in favor of a standard output operator, i.e. <=$weekday_formatted?>[<?=$state?>] \$\endgroup\$ – Your Common Sense Oct 25 '17 at 7:28
2
\$\begingroup\$

Initial Thoughts

Initially I was thinking of suggesting a view class that could hold the states and weekdays in instance variables, then have methods to get the header for each state and then display the cells for each weekday, but that seems like a lot of extra overhead just to display a table, and wouldn't provide the desired separation of business logic and display templates.

Simple improvements

As GrumpyCrouton mention in the SO answer, PHP Short echo tags could be used to shorten the inline echo statements (i.e. <?php echo to <?=).

$d could be eliminated by using the associative array syntax of foreach

foreach (array_expression as $key => $value)
     statement 1

foreach($weekdays as $d => $weekday) {
    //$d doesn't need to be incremented since it is the keys - e.g. 0, 1, 2, etc.
}

Or just used array_keys() since the weekday value isn't used...

foreach(array_keys($weekdays) as $d) {
    //$d doesn't need to be incremented since it is the keys - e.g. 0, 1, 2, etc.
}

One could define a template using Heredoc syntax, like:

$cellTemplate = <<<CELL
<td>
    <input name="date-%s" />
</td>
CELL;

Then that could be used for each table cell:

foreach ( $states as $state ) { ?>
  <tr class="<?= $state; ?>">
    <th><?= $state; ?></th>
    <?php
    foreach ( $weekdays as $d => $weekday ) { 
        $dateName = $date->addDay( $d )->format('[Y][m][d]') . '[' . $state . ']';
        echo sprintf($cellTemplate, $dateName);
      } 
      ?>
  </tr>
<?php
}
?>

A demonstration can be seen here. Note that instead of using the Carbon DateTime I am using PHP's DateTime class along with DateInterval to accomplish the date addition with DateTime::add().

Template Engines

One could also utilize a template engine, like Smarty, Symfony’s Twig, Laravel’s Blade, etc.

Here is an example with a Smarty template. I like bumperbox's notion of storing the formatted weekdays in an array and using that in later on. That array could also be generated using array_map() (though accessing $date inside the callback would require a use statement):

$formattedWeekdays = array_map(function($d, $weekday) use ($date) {
    return $date->addDay( $d )->format('[Y][m][d]');
}, array_keys($weekdays), $weekdays);
$smarty->assign('states', $states);
$smarty->assign('formattedWeekdays', $formattedWeekdays);
$smarty->display('../templates/weekdays.tpl');

And that template file (i.e. weekdays.tpl) might look like this:

<table>
    {foreach $states as $state}
    <tr class="{$state}">
        <th>{$state}</th>
        {foreach $formattedWeekdays as $formattedWeekday }
        <td>
            <input name="date-{$formattedWeekday}[{$state}]" />
        </td>
        {/foreach}
    </tr>
    {/foreach}
</table>    

That way the display logic is separate from the business logic. As this wikibooks page alludes to, the HTML is separated from the PHP code so multiple people (e.g. graphics designer, developers, etc.) can manage the separate parts since they are de-coupled.


1http://php.net/manual/en/control-structures.foreach.php

\$\endgroup\$
  • \$\begingroup\$ Does this help other develops take on the code and appreciate these updates? Or does it obfuscate the code further? \$\endgroup\$ – Andrew Lazarus Oct 24 '17 at 12:50
  • \$\begingroup\$ do you mean specifically the class abstraction, or all the suggestions? if you mean the class abstraction, the goal isn't obfuscation but rather to separate the PHP logic from the HTML. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 24 '17 at 17:39
  • 1
    \$\begingroup\$ Your solution reminded me this awesome library, github.com/Herzult/SimplePHPEasyPlus :) \$\endgroup\$ – Your Common Sense Oct 25 '17 at 7:25
  • \$\begingroup\$ It's the least I could do... well actually, I guess doing nothing would be the least, but you get the point, right? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 27 '17 at 20:39

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.