Tests… your safety net when refining you code.

Anyone who knows my development style will know that my personal approach is very test oriented, and I tend towards verbosity first, then refining the code to be cleaner and/or more more idiomatic after the code works… and it proven to work.

So what does it look like?

Recently in my $dayjob I had to loop around an array and apply a modification before returning that array to another piece of logic. Here is my rough approach to the problem, but replacing $dayjob with a hypothetical bit of code based on the awesome TrueAchievements.com twelve days of christmas challenge.

# 12_days_of_christmas.t

use strict;
use warnings;

use Test2::V0 -target => 'TA';

isa_ok $CLASS, 'TA';
can_ok $CLASS, 'TwelveDaysOfChristmas';

done_testing;

Yep, that’s about it to start with a simple test that confirms that my module/method exist:

$ prove -lv
t/12_days_of_christmas.t .. Can't locate TA.pm in @INC

The above is Perl telling me, “Hey Lance, how about you actually create that file my lad”. So I go ahead and do that:

#TA.pm

use strict;
use warnings;

sub TwelveDaysOfchristmas {}

1;

As you can say I have created the bare bone of the library, and my subroutine literally does nothing. So at this stage I press up arrow and return, re-running “prove -lv“, which gives me:

$ prove -lv
t/12_days_of_christmas.t ..
ok 1 - TA->isa('TA)
not ok 2 - TA->can('TwelveDaysOfChristmas')

Yeah… you probably spotted the typo already, I did not type the name of the sub routine and the name in the “can_ok” test the same. I am a terrible typist, I make mistakes like that all the time. So I really do tend to write tests like this early and often. And I run them all the time (sometimes with the Linux “watch” command) so that I get the fastest warning I did something stoopid that I can.

It also suggests to me that my natural verbosity is hurting me here, so I can make a small change and shorten the sub name in the test, then the module and all is well and my terminal says:

$prove -lv
t/12_days_of_christmas.t ..
ok 1 - TA->isa('TA')
ok 2 - TA->can('TwelveDaysOfChristmas')
1..2
ok
All tests successful

Obviously in this example it is all a bit contrived; in reality in my $dayjob I am adding methods to objects. So often I am just adding a basic test of the method. But when I am wiring up a new service (linking from controller down to infrastructure classes via domain objects etc.) then I do like to start with a test similar to this that proves I have connected everything up correctly. And I like to know I can call the method from the sontroller level BEFORE I even start building the method.

So… I want my new TwelveDaysOfChristmas() sub to loop around an array of XBox achievements and set them all to “done”. So lets go…

my $got = TA::TwelveDaysOfChristmas(
    [
        ['Foo',''],
        ['Bar',''],
    ]
);

is $got, 
    [
        ['Foo','Done'],
        ['Bar','Done'],
    ]
,
'Correctly set the elements to DONE';

And as you might have guessed I run the tests again and get this (edited for brevity) test output:

# +---------+---------+
# | GOT | CHECK |
# +---------+---------+
# | <UNDEF> | <ARRAY> |
# +---------+---------+

As expected the test fails, because I want an array, and currently the sub is empty so it returns undef. Next I will often put the least return value I can, something like “return 666;” just so I can see the test failure change:

# +-----+---------+
# | GOT | CHECK |
# +-----+---------+
# | 666 | <ARRAY> |
# +-----+---------+

Again this is just a little sanity testing, I made a change in the sub, and the test output proved to me as a developer that I am still in control. So next what do I typically add:

sub TwelveDaysOfChristmas {
    return [
        ['Foo','Done'],
        ['Bar','Done'],
    ];
}

This makes all the tests pass, though it’s entirely useless code. But now I have it all confirmed that if the sub returns the right data, then the test will pass. So at this stage I remove the bugus just return the data and start working on the actual code.

Now at this stage I will confess I “might” use every Perl developers most used commands “ warn ” and of course “ Data::Dumper ” as I draft the logic. SO maybe something like this:

sub TwelveDaysOfChristmas {
    my @achievements = @_;

    for my $achievement (@achievements) {
       warn '-->', $achievement[0];
    }
}

Now, if you read the above carefully, you will have already predicted that the tests failed. The warn is wrong as I have not dereferenced the array properly.

$ prove -lv
t/12_days_of_christmas.t .. Global symbol "@achievement" requires explicit package name...

So I change that line to be “warn ‘—>’, $achievement->[0]” and all is well with the world again… no not really the tests fail again. This time it looks like this:

$prove -lv
t/12_days_of_christmas.t .. --->$VAR1 = [
          'Foo',
          ''
        ];

Hmmm…. is this what I expected? No not really no, why? Oh because in the test I have passed and arrayref NOT and array as the code expects. So I have spotted another small mistake I have made. Lets sort that out.

sub TwelveDaysOfChristmas {
    my $achievements = shift;

    for my $achievement (@$achievements) {
       warn '-->', $achievement[0];
    }
}

Which returns this:

$ prove -lv
t/12_days_of_christmas.t .. --->$VAR1 = 'Foo';
--->$VAR1 = 'Bar';

Excellent, I am looping around the achievements correctly now, though I am warning the wrong value… but not to worry. So lets carry on.

sub TwelveDaysOfChristmas {
    my $achievements = shift;

    my @cheevos;
    for my $achievement (@$achievements) {
       push @cheevos, [$achievemnet->[0], 'Done'];
    }
    return \@cheevos;
}

Now we run the tests again and this time… success!

(NB: I have ripped out the isa_ok and can_ok tests, so now it is just the one test)

$ prove -lv
t/12_days_of_christmas.t ..
ok 1
1..1
All tests successful.

So, the code now works, it is doing what we want. Job done; time to go home right? Wrong… this code is ugly (and it’s intentionally so) but I got here fast and it’s proven to work.

So now I can feel pretty happy cleaning the code:

sub TwelveDaysOfChristmas {
    my $achievements = shift;

    my @cheevos = map {[$_->[0],'Done']} @$achievements;

    return \@cheevos;
}

Ok… this is smaller and cleaner and uses the more idiomatic “map” and importantly still works. But can we make it tidier?

sub TwelveDaysOfChristmas {
    my $achievements = shift;

    [map {[$_->[0],'Done']} @$achievements ];
}

Hmm… ok. That is still working and a bit shorter. Less code means less bugs people. What else can we do?

sub TwelveDaysOfChristmas {
    [map {[$_->[0],'Done']} @{$_[0]} ];
}

Ok great, we have taken my original 6 line verbose code and used a more idiomatic approach and cut down to 1 line of perl.

I have a confession to make, the last version there makes me really queasy. Looking at it, I am pretty certain that no newbie developer joining my project will understand that. The experienced developers will be able to understand it… but only after a mental effort to read that line.

So I personally would probably stop somewhere around this stage:

sub TwelveDaysOfChristmas {
    my $achievements = shift;

    my @cheevos = map {[$_->[0],'Done']} @$achievements;

    return \@cheevos;
}

It’s a little easier to understand I think. We get some achievements, we add ‘Done’ and push it into a fresh array, and return that. As new developer might find the map a bit hard to follow (maybe, or maybe it’s just me). But they can tell right a way that we take the input, do “something” to it and then return it.

So the concept of what I am trying to do I think is pretty clear; even if the Done bit is not super clear. Rather than going for the one line solution I might be happier working on making the code a bit more legible. Maybe some changes like this:

sub set_achievements_to_done {
    my $achievements = shift;

    my @done_achievements = map {[$_->[0],'Done']} @$achievements;

    return \@done_achievements;
}

So for me; this one give a bit more of a hint to my colleagues that the sub should make achievements be “Done”. So immediately the ‘Done’ in the map is easier to understand. So hopefully the more complicated code is easier to digest as the intent is expressed in the sub name?

So there you have it, a small example of my personal approach to coding. I start with a very simple test to make sure that the wiring is correct. Then write a verbose simplistic solution that makes my test pass.

Once I have a working solution, THEN, and only then I start looking at the code and trying to make it shorter and more idiomatic. I want to try and make it use more efficient methods for my language (Perl in this case) and reduce the line count… less lines means less bugs, seriously it reduces bugs in the long-term.

Trust me on this. I have not stats to quote. But it makes sense to me. I write a 6 line sub. It’s shiny and good. But 6 months later a colleague adds something else, 12 months later a new developer adds something else. a year after that another new developer adds one more thing. But they make a small change that actually messes up my original code.

My one line solution might be hard to read; but it’s a single statement so it’s not very likely that someone will slip something unexpected into the middle of it. Unlikely… not impossible.

I also have a test. And would normally write a lot more than that. But even a single test that proves the method does what I intended is a great way of being sure that even if someone comes along later; they “should” not be able to break my code without the test failing.

I like my code relatively verbose though. I want it to express to developers what I want it to do more than I want it to tell the computer what I want it to do. So I will sacrifice brevity for legible.

I’d be really interested in your thoughts on what I ‘ve shown above. I don’t promise there are no bugs in the above. I also don’t claim this is the best code ever, I wrote the code whilst sitting on the sofa while writing the blog post; so I have not given it my full attention to be honest.

I guess the take-away I wanted to get across is that, tests are a tool for me as a developer. I use them to catch the silly mistakes I make as much as proving that my code does what I want it to.

Lance