I just now rewrote this method:
sub save_with_fetchable
{
my $self = shift;
my @dirty = map { $_ => $_->fetchable->is_changed ? $_->fetchable : () } @_
$self->txn_do(sub { $_->insert_or_update() for @dirty });
}sub save_with_fetchable
{
my ($self, @items) = @_;
$self->txn_do(sub
{
for my $item (@items)
{
$item->insert_or_update;
my $fetchable = $item->fetchable;
$fetchable->insert_or_update if $fetchable->is_changed;
}
}
);
}... after Devel::Cover complained
that my tests didn't exercise the $_->fetchable->is_changed
branch condition in all of its forms. The tests indeed do exercise that, but I
rewrote the code anyway.
Novice testers introduced to test code coverage often go through wild gyrations to make sure that every part of their code gets 100% coverage. Complete coverage is a good rule of thumb if and only if your tests exercise the intent of the code completely. It's relatively easy to exercise every statement and branch and conditional while letting trivial bugs go untested and unfixed.
With that said, code that's easy to test tends to be easier to verify and to maintain.
In this case, rewriting compact code—clever code—into something simple enough for D::C to verify makes the code easier to read. The design of the code is slightly better. Add to that the fact that the module containing this code now has 100% test coverage, so that any subsequent changes of coverage will have an obvious tie to subsequent changes of the code, and this was an easy decision to make.
It's nice when improving test coverage fixes bugs, but it's even nicer when improving test coverage leads to improving code and its design.
I find it unnecessarily difficult to discern the intent in either way you’ve written it. It is easy to make the code very much clearer:
sub save_with_fetchable { my $self = shift; my @dirty = ( @_, grep { $_->is_changed } map { $_->fetchable } @_, ); $self->txn_do( sub { $_->insert_or_update() for @dirty } ); }If you need the updates of fetchables properly interleaved with their corresponding items (or maybe even if not, I cannot decide) then I’d write a closely parallel version:
sub save_with_fetchable { my $self = shift; my @dirty = map { $_, grep { $_->is_changed } $_->fetchable, } @_; $self->txn_do( sub { $_->insert_or_update() for @dirty } ); }If the problem in question is at all amenable to expression in terms of lists, then I find that code which produces, transforms and reduces them is almost always more readble and clearer in intent than code that uses explicit conditionals in any form, particularly code that iterates procedurally.
To be fair, as far as your original question is concerned, you will easily get 100% coverage on these ways of writing the code: even if you pass a list of items whose fetchables are either all dirty or clean, Neither would test the full range of implemented intents.
However, it occurs to me that both of these flawed tests would still correctly assert the correctness of the insert_or_update line if they pass.
So writing the function in terms of list transformation reveals that it combines two separate concerns: augmenting a list of items by their corresponding dirty fetchables, and unconditionally saving a list of records. These concerns can be separated into distinct methods for easier testing, but that is not imperative. The clarity gained is what’s important: it can guide whatever you do next, even if that is just writing tests for this combined-concerns method.