Bob's Blog

In the last article, we looked at a way to show the word NEW on pages when the resource was published recently. It's common to develop a quick snippet to solve some immediate problem, then later think of ways to make it more generic and more flexible. In this article and the next ones we'll enhance the snippet to give us more control over what it does.

Starting Point

For reference, here is the snippet as it appeared in the last article:

<?php
if ( ((strtotime($modx->resource->get('publishedon'))) + 1209600) > time()) {
    if ($modx->resource->get('published')) {
        return '<span class="new_resource">NEW!</span>';
    } else {
        return '';
    }
}

Next Steps

When you're working on PHP code, you want to improve these aspects of your code: speed, memory efficiency, features, flexibility, user-friendliness, and readability. Experienced coders disagree about the order in which these enhancements should be applied. I'm going to make suggestions about how to do it, but you should be aware that others do it differently and that I don't always follow my own advice (though I'm often sorry later when I don't).

What I usually do next is to make the code readable. If it's not readable, working on it is more likely to introduce errors, and the errors will be difficult to detect. You may have heard or read the advice, "make it work, then make it fast," which suggests that you should forget about efficiency at this point, but as I make the code more readable, I often spot ways to make it more efficient and correct any logic errors.

Let's make the code more readable by adding some comments and improving the structure:

<?php
/* NewResource snippet */

/* Get the publishedon date (if any) and
   convert it to a unix timestamp */
$pTime = strtotime($modx->resource->get('publishedon'));

/* Get the current time as a timestamp */
$currentTime = time();

/* Get the resource's age in seconds */
$age = $currentTime - $pTime;

/* Set $interval as the number of seconds in two weeks */
$interval = 1209600;

/* See if the resource is published */
$published = $modx->resource->get('published');

/* Set the return value */
$output = '<span class="new_resource">NEW!</span>';

/* See if publication date is within the last two weeks */
if ($age < $interval ) {
    /* Yes, it's recent. If published, return NEW! */
    if ($published)
        return $output;
} else {
    /* Not recent, return an empty string */
    return '';
}

Coding Issues

Our code is much more readable now. We've extracted some of the values and calculations and made them into variables at the top of the snippet. This makes them much easier to modify without introducing errors, puts them all in one place where we, or another coder, can find them. Doing this also makes the code infinitely easier to follow. Notice that we've also changed the age test in the if statement to make it more intuitive.

This version is less efficient than the previous version and will run slower, but the difference is probably a matter of milliseconds. If this snippet were used in a time-critical situation, you'd probably change it back to look more like the original. It's easy enough to do by just cutting and pasting to replace the variables in the code with the right-hand side of their values variables at the top and removing the variable at the top. This would be the very last step you'd perform in working on the snippet.

There is one glaring error in coding style that you may have noticed. The if statement that tests whether the resource is published has no curly braces. This is a violation of general PHP coding guidelines and also a violation of the MODX coding standards. It works, so why is it a problem? Imagine that you later decide that you want to set a placeholder in addition to returning "NEW!", so you do this:

if ($published)
    $modx->setPlaceholder('new_resource', $returnVal);
    return '<span class="new_resource">NEW!</span>';

Because there are no curly braces, the return statement is not controlled by the if statement. In other words, the scope of the if statement only includes the line that immediately follows it. That means the return statement is going to execute whether the resource is published or not, introducing an error that might be difficult to fix. It's a harmless error in this instance, but skipping the curly braces is a bad habit that is guaranteed to bite you in the long run.

To correct this, we add a couple of curly braces to the if statement:

if ($published) {
    return '<span class="new_resource">NEW!</span>';
}

Here's the full corrected code:

<?php
/* NewResource snippet */

/* Get the publishedon date (if any) and
   convert it to a unix timestamp */
$pTime = strtotime($modx->resource->get('publishedon'));

/* Get the current time as a timestamp */
$currentTime = time();

/* Get the resource's age in seconds */
$age = $currentTime - $pTime;

/* Set $interval as the number of seconds in two weeks */
$interval = 1209600;

/* See if the resource is published */
$published = $modx->resource->get('published');

/* Set the return value */
$output = '<span class="new_resource">NEW!</span>';


/* See if publication date is within the last two weeks */
if ($age < $interval ) {
    /* Yes, it's recent. If published, return NEW! */
    if ($published)
        return $output;
} else {
    /* Not recent, return an empty string */
    return '';
}

Our code still has a significant logic error, but we'll look at that in the next article.