Test Coverage and Simplicity

| 1 Comment

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 });
}
... into:
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.

1 Comment

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.

Modern Perl: The Book

cover image for Modern Perl: the book

The best Perl Programmers read Modern Perl: The Book.

affiliated with ModernPerl.net

Categories

Pages

About this Entry

This page contains a single entry by chromatic published on June 3, 2011 1:57 PM.

Autogenerated Test Stubs was the previous entry in this blog.

Four New Perl Books Underway is the next entry in this blog.

Find recent content on the main index or look in the archives to find all content.


Sponsored by Blender Recipe Reviews and the Trendshare how to invest guide

Powered by the Perl programming language

what is programming?