Filter by date method

Question

This method allows me to filter by date a data structure list of hash that contains articles. I would like to have your opinion on the code:

=method article_by_date

    $articles->articles_by_date( month => 12 );

Return a list of articles filter by date specified:

    input: month (Int) : optional, month to match
           year (Int) : optional, year to match
    output: Hashref: Details of article

=cut

sub articles_by_date {
    state $check = compile(
        $invocant,
        slurpy Dict[
            month => Optional[Int],
            year  => Optional[Int],
        ]
    );
    my ($self, $arg) = $check->(@_);
    my $month = $arg->{month};
    my $year  = $arg->{year};

    if ( ! defined($year) ) {
        my $dt = DateTime->now;
        $year  = $dt->year;
    }

    my $date  = defined($month)
        ? qr/^\d\d\/$month\/$year\s\d\d:\d\d:\d\d$/
        : qr/^\d\d\/\d\d\/$year\s\d\d:\d\d:\d\d$/;

    my @articles;
    foreach my $article ( @{$self->_get_or_create_cache('articles')} ) {
        $article->{date} =~ $date
            and push(@articles, $article);
    }

    return \@articles;
}

Answer 1

Your code is mostly very readable and good. I have to point out the explicit type checking – this is wonderful!

Minor Style Recommendations

There are a few style thingies where I don't really care what you do, but it is common to put a comma after the last item in a list when it is at the end of a line:

        ...,
        slurpy Dict[
            month => Optional[Int],
            year  => Optional[Int],
        ], # <-- HERE
    );

Also, the

$article->{date} =~ $date
    and push(@articles, $article);

looks goofy:

• TIMTOWTDI, but that may look better with an if:

  push(@articles, $article) if $article->{date} =~ $date;
  

• I prefer to enclose regex applications in /.../, even when this is not syntactically neccessary: The delimiters make it visually clearer:

  push(@articles, $article) if $article->{date} =~ /$date/;
  

  This does not force recompilation of the regex on sufficiently modern perls (the last time I checked).

Naming Improvements

Let's move on to names. I do not know the purpose of your class. But if the class name, the object name, and all method names contain “article”, something may be wrong. (Of course _get_or_create_cache only contains article as an argument, but that seems to be effectively a part of the name).

Assuming the purpose of the class is some collection of articles, then maybe the following may look more natural:

my $articles = Articles->new(...);
my $articles_from_may = $articles->at_date(month => 3);  # instead of articles_by_date

# instead of _get_or_create_cache('articles'):
# array dereference overloading
for my $article (@$articles) {
  ...; 
}
# or a simpler method
for my $article ($articles->all) {
  ...;
}

But I could be misunderstanding the intentions of the class.

As this method is all about dates, I would expect a $date variable to be some kind of date object or string. Nope, it is a regex. I do not know where you stand on Hungarian Notation, but sometimes it can really aid readability: $date_rx or something. Just don't overuse it.

Major Style Issues

Now on to more “serious” stuff.

Define-Or //

Your if ( ! defined($year)) ... is a big waste of screen space. You can use the defined-or operator for shorter initialization:

my $year = $arg->{year} // DateTime->now->year;

Regex Formatting and Correctness

Your two regexes are two lines of unreadable mess. The hypnotic rhythm of backslashes is only broken by the variable interpolations. But I can't tell at a glance what is happening here! We can introduce nonmatching space with the /x flag to make it more readable. You should also change the delimiter so that you don't have to escape the forward slashes.

If there is any chance the date fields in your $article may be generated by an outside source, then the \d is wrong. This character class matches all Unicode digits, or [0-9] in some special circumstances. Either force ASCII semantics via /a flag, or be explicit with your character classes.

my $time_rx = qr/ [0-9]{1,2} : [0-9]{1,2} : [0-9]{1,2} /x;
my $date_rx = defined($month)
    ? qr!\A [0-9]{1,2}/$month/$year     \s+ $time_rx \z!x
    : qr!\A [0-9]{1,2}/[0-9]{1,2}/$year \s+ $time_rx \z!x;

You should also heed Vedran Šego's advice on this topic.

Bugs

Regex Bug: Date Formatting

Now there is a subtle bug here. What happens when the date string is 01/02/2013 13:46:02 and the user calls your method like month => 2? The regex would match 01/2/2013 ... but not /02/! This would mean that you have to sprintf '%02d' the stuff which you'd like to interpolate.

This still assumes that you normalize all your dates (e.g. to UTC), so that your complete omission of time zones if forgivable. (Actually, it isn't. See below for some failure modes).

Rant: Correct Date Handling

Articles Should Contain DateTimes

Handling dates correctly is very complex. Yes, you can achieve some kind of seemingly working code with regexes, but I wouldn't recommend this. It would be preferable for each $article to include a DateTime object (just create them lazily if it seems too expensive to you). Then instead of the regex application:

$article->date->year == $year && $article->date->month == $month

This is shorter, more readable, and more correct, but more expensive than the regex solutions.

The Method Should Have A DateTime As Argument

This still isn't completely correct, because no timezone information for the $year and $month is available. It might be better to require the user to call your method with a DateTime object, which you truncate to the month. Then:

...
my ($self, $date) = &$check;
$date = $date->clone->truncate(to => 'month');
...
$article->date->year == $date->year && ...

Don't Guess The Year!

This would get rid of most bugs, and ambiguity. The latter exists because you try to guess a year. This can be bad UX:

• Assume that on Jan. 1st, I ask for all articles from December. Suprisingly, the query returns no articles, although there was a new article just yesterday!
• Or even better, assume that I ask for all articles from December during the last day of December. However, your program thinks it's already January, and returns an empty array.

Finally, A Solution…

I am not sure, but I think that

...
my $date_min = $date->clone->truncate(to => 'month');
my $date_max = $date_min->clone->add(months => 1);
...
$date_min <= $article->date && $article->date < $date_max

might work without bugs, assuming that the DateTime your method is called with has specified a correct time zone.

Make map, not for.

Finally, your foreach loop is really just a grep in disguise. You want:

my @articles = grep { $date_min <= $_->date && $_->date < $date_max } $self->all;

Answer 2

Most of this depends on what exactly do you want to achieve. For example, @articles will be a list of references to the same articles that are also referenced in $self->_get_or_create_cache('articles'). Depending on what you want, you might want to clone these values, instead of just pushing them to @articles.

The other thing is a regex for dates. Are you certain that your dates will be, for example, 23/08/2013, and not 23/8/2013? If not, this might be better:

my $date  = defined($month)
    ? qr/^\d{1,2}\/$month\/$year\s\d{1,2}:\d{1,2}:\d{1,2}$/
    : qr/^\d{1,2}\/\d{1,2}\/$year\s\d{1,2}:\d{1,2}:\d{1,2}$/;

The same goes for the times, of course.

Depending on how clean your input is, you might want to replace $month and $year with \Q$month\E and \Q$year\E to avoid matching special characters in those variables (i.e., that . is matched as a dot and not as any character).

If it may be possible for the input to have extra spaces, you might want to add \s* after ^ and before $, while also replacing \s behind $year with \s+.

Day of month can be more precisely matched as (0?[1-9]|[1-2]\d|3[01]) (similar for months, hours, minutes, and seconds), but this makes regexes quite comples, so see if you need this.