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 DateTime
s
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;