0
\$\begingroup\$

I have a function that implements parts of reading iCal repetition rules, and returns a list of dates when a certain event takes place.

Questions/Concerns of mine

  1. Any comments on the use of anonymous functions? I added them to keep the code as DRY as possible.
  2. One thing I rather dislike is the line return strtotime('+' . $increment . ' ' . $rrule_freq_map[$rrule['FREQ']], $start_timestamp); which casts from integer to string and back for no good reason. Couldn't think of a better way to accomplish the adding of time.
  3. On a Drupal specific note, maybe the use of date in $single_day_output() should be replaced with format_date, but I'm not sure if there are any gains from that.
  4. $next_timestamp() will break if used in more than one location. Currently not an issue, would be interesting to hear suggestions on that.

/**
 * Render the date and times of events.
 *
 * Has some internal helper functions do not make sense outside of this scope.
 */
function _bornholm_custom_calendar_event_dates($datetime) {
  // Ensure we have the date_repeat_split_rrule().
  module_load_include('inc', 'date_api', 'date_api_ical');

  $start_timestamp = strtotime($datetime['value']);
  $start_day = date('Y-m-d', strtotime($datetime['value']));
  $end_day = (isset($datetime['value2'])) ? date('Y-m-d', strtotime($datetime['value2'])) : NULL;
  $end_timestamp = (isset($datetime['value2'])) ? strtotime($datetime['value2']) : NULL;
  $output = '';

  // Build the output for a single days event information.
  $single_day_output = function($timestamp) {
    $output = t('@day d. @date t. @time', array(
      '@day' => t(date('D', $timestamp)),
      '@date' => date('d.m.Y', $timestamp),
      '@time' => date('H:i', $timestamp),
    ));

    return ucfirst($output);
  };

  // Check whether or not the current has a repeat rule.
  $repeats = function ($datetime) {
    if ($datetime['rrule'] !== NULL) {
      list($rrule, $exceptions, $additions) = date_repeat_split_rrule($datetime['rrule']);
      if ($rrule['FREQ'] !== 'NONE') {
        return $rrule;
      }
    }

    return NULL;
  };

  // When an event repeats, step forward the timestamp once.
  $next_timestamp = function($rrule, $start_timestamp) {
    static $count = 0;

    $rrule_freq_map = array(
      'DAILY' => 'day',
      'WEEKLY' => 'week',
      'MONTHLY' => 'month',
    );

    $increment = $count++ * $rrule['INTERVAL'];

    return strtotime('+' . $increment . ' ' . $rrule_freq_map[$rrule['FREQ']], $start_timestamp);
  };

  if ($rrule = $repeats($datetime)) {
    for ($i = 0; $i < $rrule['COUNT'] && $i < 100; $i++) {
      $new_timestamp = $next_timestamp($rrule, $start_timestamp);

      if ($end_day === NULL || $start_day === $end_day) {
        $output .= $single_day_output($new_timestamp);

        if ($end_timestamp) {
          $output .= ' - ' . date('H:i', $new_timestamp + ($end_timestamp - $start_timestamp));
        }
      }
      else {
        $output .= $single_day_output($new_timestamp) . ' ' . t('until') . '<br/>' . $single_day_output($new_timestamp + ($end_timestamp - $start_timestamp)) . '<br/>';
      }
    }
  }
  else {
    if ($end_day === NULL || $start_day === $end_day) {
      $output = $single_day_output($start_timestamp);
      if ($end_timestamp) {
        $output .= ' - ' . date('H:i', $end_timestamp);
      }
    }
    else {
      $output = $single_day_output($start_timestamp) . ' ' . t('until') . '<br/>' . $single_day_output($end_timestamp);
    }
  }

  return $output;
}
\$\endgroup\$
1
\$\begingroup\$

Something to keep in mind about anonymous functions (also known as lambda functions), is that they should only be used for tasks that are only going to be performed once. These functions are not compiled at runtime and so must be explicitly compiled each time they are run. Another thing to keep in mind, just because you can do something, doesn't mean you should. Sure this works, but its confusing and hard to read. Why use lambda functions at all? It looks like what you actually want is a class.

Judging by some of this code I'd hazard to say that you are coming from Java? I say this mostly because of the way you are throwing around lambda functions in a single "main" function. Reminds me of a lot of Java code I've seen. That and your calling an array a map. Just know that PHP is NOT Java. You don't need just one function/class, to house all your other functions. Just pretend they are in a class by default and you'll do fine. Or implement them to a PHP class, they are more or less the same. If you are not from Java, then you might want to look at class tutorials, but first forget you ever heard of lambda functions. You'll probably never need them.

Another thing, that main function, _bornhom_custom_calendar_event_dates(), is entirely too long for a function. Functions should do one thing, and do it well. Classes should also do just one thing, but on a larger scale by using many functions to do smaller things. For example: A class could get a valid date set and have many functions (or methods in a class context) that cared for getting single dates, checking if they were valid, and compiling them together. Abstracting these lambda functions will help immensely with this and the DRY principle.

For quoting DRY principle, I immediately spot code that violates it.

$start_timestamp = strtotime($datetime['value']);
$start_day = date('Y-m-d', $start_timestamp);

Instead of checking if $datetime['value2'] is set repeatedly, do it once to save processing time. And change that name. "value2" is entirely too vague. What differentiates it from value? What even is "value"? Try to be short, but descriptive. Describing what these "values" are supposed to contain will get you started.

$end_day = NULL;
$end_timestamp = NULL;

$value2 = isset( $datetime[ 'value2' ] ) ? strtotime( $datetime[ 'value2' ] ) : FALSE;
if( $value2 ) {
    $end_day = date( 'Y-m-d', $value2 );
    $end_timestamp = $value2;
}

Of course, if you could change your code so that $end_timestamp could have a default value of FALSE instead of NULL, you could just set $end_timestamp above and then check to see if $end_day needed to be set and completely bypass the need of a $value2 variable.

What is function t() as seen in your $single_day_output lambda function? Where does it come from? Just like above, please choose better names.

What is "rrule" and why does it have a child named "rrule"? Maybe the first should be called a "rule set"? I don't know, but the names should be unique.

Break up your long lines. There's no reason to have them so long.

$single_newday = $single_day_output($new_timestamp);
$time = $single_day_output($new_timestamp + ($end_timestamp - $start_timestamp))
$output .= $single_newday . ' ' . t('until') . '<br/>' . $time . '<br/>';

Right now I can't follow your code well enough to help you with any of your specific questions, but if you clean it up a bit I'll take another look. Hope this helps!

\$\endgroup\$

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.