1
\$\begingroup\$

The goal is to loop through an array of days-of-the-week (array values are stored as strings, eg: 'Mon, Tues, Weds', etc) and output some HTML when today's day is encountered.

The file is a CMS's .tpl file, so the PHP sits directly alongside the HTML.

My current, working code is:

<?php
 $dayofweek = date('w');

 foreach ($dataarray['openingDays'] as $day_key => $opening_day):
   $i++;
   //if $i is equal to 7 make it 0 to match array
   $i = ($i == 7 ? 0 : $i);
   //If today's date matches $i
   if ($dayofweek == $i):
?>
<div class="todays-date">
<?php else: ?>
<div class="not-todays-date">
<?php
  endif;
  print $opening_day;
?>
</div>
<?php endforeach; ?>

This is one of the first times I've used ternary and would like to know if it is done correctly. I am also concerned about not initializing $i and would like to know if that is acceptable.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Could you add a dump of dataarray['openingDays']? Does it contain the days for one week or a longer time period? \$\endgroup\$ Commented Apr 11, 2017 at 7:57
  • \$\begingroup\$ Just days of 1 week as a string, "Sun, Mon, Tues, Weds, Thurs, Fri, Sat" \$\endgroup\$
    – MeltingDog
    Commented Apr 11, 2017 at 22:16

2 Answers 2

3
\$\begingroup\$

I would suggest you familiarize yourself with the DateTime class and it's related classes including DateInterval and DatePeriod as these are your most powerful tools for working with date/times, intervals of time, and iterating over intervals of time.

You should have no reason to build an array for days for the week or anything like that.

Let me give an example for how you might approach this problem using these classes.

// current DateTime
$now = new DateTime();
// create a DateTime representing the beginning of the week
$monday = clone $now;
$monday->setTime(0,0,0)
       ->modify('Monday this week');
// create a DateInterval to represent a one day interval
$dayInterval = new DateInterval('P1D');
// create a DatePeriod to represent a full week
$weekPeriod = new DatePeriod($monday, $dayInterval, 7);
// iterate the DatePeriod
foreach($weekPeriod as $day) {
    if($day->format('D') === $now->format('D')) {
        // today matches this day of week
    }
}

Beyond that, I would suggest being cognizant of your styling. Right now: - your indentations are inconsistent - your variable naming style in inconsistent between using snake_case, camelCase (like in $dataarray keys), and no way to differentiate words in your variable name at all (i.e.$dayofweek, $dataarray). You should pick either snake_case or camelCase to make your code more readable and stick with whichever style you choose. I would actually recommend camelCase as PHP, as a language, is moving more in that direction with regard to industry styling standards.

\$\endgroup\$
2
\$\begingroup\$

Your ternary is certainly working but to avoid unexpected behavior or security risks, it is advised to initialize the $i variable. This is not required (like you said, the code is working) but it might cause problems.

Here is an excerpt from the PHP manual:

It is not necessary to initialize variables in PHP however it is a very good practice. Uninitialized variables have a default value of their type depending on the context in which they are used - booleans default to FALSE, integers and floats default to zero, strings (e.g. used in echo) are set as an empty string and arrays become to an empty array.(...) Relying on the default value of an uninitialized variable is problematic in the case of including one file into another which uses the same variable name. It is also a major security risk with register_globals turned on.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.