Re: Re: Re: Re: Re: Documenting Methods/Subs

[pdcawley]

That's certainly a possibility. However (and I'm aware that this may sound like me attempting to move the goalposts), in well factored code the method is likely to be short enough that anyone reading the code will be able to see at a glance that bar will be expected to do_that_thang so the validator declaration is unlikely to introduce any real clarification.

Actually, my whole argument for not having much in the way of code documentation is predicated on the belief that well factored code doesn't need much in the way of code documentation and, more strongly, that code that does need substantial explanatory documentation probably isn't well factored enough.

[adrianh]

I totally agree with the assertion that well factored code needs little in the need of code documentation. However the fact that perl lacks the ability to talk succinctly about type means that some things are less obvious than they are in other languages unless you add explicit assertions.

Consider the example of specifying a logger:

sub new {
   my $class = shift;
   my %self = validate(@_, {logger => { isa => 'Logger' } } );
   return( bless \%self, $class );
};

The Logger object is not used by new(). It may not be used anywhere near the constructor code - especially if in a subclass. Without the assertion it's not obvious what kind of thing the logger should be.

If I passed something that wasn't a Logger the problem wouldn't show up until it was used - potentially quite a distance from the incorrect call to new() making debugging harder.

Writing a test that concisely states "this has to be a logger object in all subclasses" is pretty much impossible.

Writing a comment has the usual problem of getting out of sync with the code, and doesn't solve the debugging issues.

Having an assertion (using validate or something like Carp::Assert::More seems a better solution.

[pdcawley]

Part of the problem here is that your new could be improved. Personally I'm not a fan of having a 'new' that takes arguments; I prefer to leave that to factory methods.

sub new {
  my $class = shift;
  return bless {}, $class;
}

sub new_with_logger {
  my $proto = shift;
  my $logger = shift;
  return $proto->new->set_logger($logger);
}

sub set_logger {
  my $self = shift;
  my $logger = shift;
  # If you insist...
  eval { $logger->isa('Logger') } or 
    croak "New logger must be an instance of Logger class"
  $self->{logger} = $logger;
  return $self;
}

Personally, I could do without the assertion at this point, relying instead on programmer's ability to read; okay, so the errors may happen some way away, but the error should be explicit enough for the programmer to realise what's amiss. However, if you insist on asserting that the logger is a Logger, the correct place to do this is definitely within the setting method itself, that way you catch all errors when someone tries to set the logger to an illegal value (assuming they don't just monkey directly with the hash, in which case they deserve all that's coming to them.)

Your comment about attempting to assert that 'this has to be a logger object in all subclasses' being impossible seems to miss the point. After all, a later subclass could take as its logger argument either a Logger or a logspec, which can be used to build an appropriate logger. All of a sudden your assertion about the logger argument is incorrect.

[adrianh]

Part of the problem here is that your new could be improved.

Well, I was trying to keep the example simple :-)

After all, a later subclass could take as its logger argument either a Logger or a logspec, which can be used to build an appropriate logger. All of a sudden your assertion about the logger argument is incorrect.

In which case, I would change it.

My aim when I do this sort of thing is to make the assumptions (or contracts if you prefer) of my classes to be explicit. Where possible I like to make them explicit using code rather than comments or documentation.

If my contracts are wrong, I fix them. Having them in code makes it very obvious when they're wrong - which is a plus in my book :-)