6
\$\begingroup\$

I created a dates_in_range() function which checks if all of the dates passed to it ($dates array) are within $start_date and $end_date. If they aren't, it'll return false.

The code works fine, but I'm wondering if there's a way to refactor it better using something like array_filter? Or does this look fine?

function dates_in_range( $start_date, $end_date, $dates = array() ) {

    foreach ( $dates as $date ) {
        if ( ! ( ( (strtotime( $date ) >= strtotime( $start_date ) ) && ( strtotime( $date ) <= strtotime( $end_date ) ) ) ) ) {
            return false;
        }
    }

    return true;
}

$dates = array(
    '2021-11-19 00:00:00',
    '2020-11-20 00:00:00',
);

$start_date = '2021-10-01 00:00:00';
$end_date   = '2021-12-31 00:00:00';

// Returns true
if ( dates_in_range( $start_date, $end_date, $dates ) ) {
    echo "In range.";
} else {
    echo "Not in range.";
}
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Please do not update the code in your question after receiving answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Jan 20 at 18:25
  • \$\begingroup\$ Do the dates come from database query results? if so, is it a flavor of SQL like MySQL? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 21 at 13:46
5
\$\begingroup\$

To me it looks fine, other than it makes me scroll the code to read it. On this ground I agree with Sam, that line ought to be shortened. To do so I would

  • use the fact that you don't need to convert mysql format dates to compare them
  • remove excessive bracing
  • use De Morgan's law to invert the condition

and get this

function dates_in_range( string $start_date, string $end_date, array $dates ): bool
{
    foreach ( $dates as $date ) {
        if ( $date < $start_date || $date > $end_date ) {
            return false;
        }
    }
    return true;
}

Also, I removed the default value for the $dates as I don't see any use in calling this function with only two parameters.

I am not sure about empty arrays though. Your function will return true for one, but it's hard to tell whether it's correct or not. It's like division by zero, and may be an exception must be thrown.

\$\endgroup\$
3
  • \$\begingroup\$ There are pros and cons about using strtotime, your answer assumes the dates are properly mysql formatted dates but the OP doesn't precise it and it may not be the case, for some UI inputs, zeroes can be forgotten, then '2021-11-19 08:00:00' and '2021-11-19 8:00:00' dates would not compare the same in term of alphabetical order, while strtotime produces the right timestamp for both of them. \$\endgroup\$ – Kaddath Jan 21 at 8:51
  • 1
    \$\begingroup\$ I agree. it's better to provide DateTime instances over strings or validate the input data otherwise. \$\endgroup\$ – Your Common Sense Jan 21 at 9:45
  • \$\begingroup\$ Ditto to @YourCommonSense comment... stringified dates was prolific in our code, for comparison and otherwise. It made routine maintenance changes unnecessarily convoluted because the programmer used the date-strings instead of the DateTime class. And surrounding code looked like the coder was thinking "string", not "date AND time" \$\endgroup\$ – radarbob Jan 27 at 18:21
4
\$\begingroup\$

The function seems okay. In Javascript one could use the method Array.every(), but as this StackOverflow question illustrates there isn't really an equivalent method or functional approach in PHP that will stop the loop as soon as one element does not meet the requirement.

You might consider adhering to the PSR PHP Coding standards - especially PSR 12.

PSR-12 has a section about Lines:

2.3 Lines

There MUST NOT be a hard limit on line length.

The soft limit on line length MUST be 120 characters.

Lines SHOULD NOT be longer than 80 characters; lines longer than that SHOULD be split into multiple subsequent lines of no more than 80 characters each. 1

Readabillity suffers because the conditional line is a bit long, partly because of the indentation but also the additional spaces separating parentheses. It may not be an issue for anyone with a wide screen but as is visible in the code snippet one must scroll to the right to see the entire line. While it may only be a micro-optimization, one could pull out the strototime($date) out to a variable, which would not only decrease function calls but also allow for a shorter line. Similarly the calls to strtotime() for the start and end dates could be pulled out of the foreach loop and stored in variables so they only need to be calculated once per function call and can decrease the length of that line with the conditional.

I question whether the third parameter having a default of an empty array is appropriate. If the function is called without any argument, then an empty array will be used and the function will return true. Perhaps using type hinting would be a better technique:

function dates_in_range( string $start_date, string $end_date, array $dates) : bool {

Notice that method signature includes a return type declaration bool. This was added in PHP 7.0 2. Hopefully your code is running on PHP 7.3 or newer, since those are the version that are officially supported 3.

\$\endgroup\$
0

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.