Re: [PATCHv5 03/17] gitweb/lib - Very simple file based cache

[Jakub Narebski <jnareb@gmail.com>]

Thank you very much for those comments on code.

On Thu, 7 Oct 2010, Thomas Adam wrote:
> On 6 October 2010 23:01, Jakub Narebski <jnareb@gmail.com> wrote:

> > +# creates get_depth() and set_depth($depth) etc. methods
> > +foreach my $i (qw(depth root namespace)) {
> > +       my $field = $i;
> > +       no strict 'refs';
> 
> For each item, you'll set "no strict refs"?  This might be better off
> outside the loop.  It's still scoped appropriately inside the
> subroutine.

On the other hand this way scope where "no strict 'refs';" is active
is limited... but I guess having "no strict 'refs';" outside loop would
be better.

> 
> > +       my $file = $self->path_to_key($key);
> > +       return undef unless (defined $file && -f $file);
> 
> PBP (Perl Best Practises) will tell you that explicitly returning
> undef is discouraged -- "undef" should be reserved for those errors
> you cannot handle, not ones you don't want to.

Well, Perl Best Practices are practices; sometimes there is good reason
to not take them into account (though probably not in this case).

Explicitly returning undef is discouraged because in list context the
returned undef (or rather 1-element list with 'undef' as sole element)
is true-ish.

On the other hand

> > +# $cache->set($key, $data);
> > +#
> > +# Associates $data with $key in the cache, overwriting any existing entry.
> > +# Returns $data.
> > +sub set {
> > +       my ($self, $key, $data) = @_;
> > +
> > +       return unless (defined $key && defined $data);
> 
> return what?
 
as you can see 'return' statement with no value looks rather cryptic
with statement modifier (conditional).

I should really have run gitweb, caching modules and tests through 
perlcritic...

> > +               last if $read_cnt == 0;
> > +               $size_left -= $read_cnt;
> > +               #last if $size_left <= 0;
> > +       }
> > +
> > +       close $read_fh
> > +               or die "Couldn't close file '$file' opened for reading: $!";
> > +       return $buf;
> > +}
> 
> "use Carp;" would be more useful here, and hence croak() and confess().

For a web application we usually do not want to have too detailed error
message present to client (to web browser) to avoid leaking of sensitive
information.
 
> > +       # ensure that directory leading to cache file exists
> > +       if (!-d $dir) {
> > +               eval { mkpath($dir, 0, 0777); 1 }
> > +                       or die "Couldn't create leading directory '$dir' (mkpath): $!";
> > +       }
> 
> Why is this eval()ed?  It will still return false and set $! appropriately.

IIRC mkpath *dies on error*, rather than returning false.  For better
error handling we would need to use make_path, but File::Path 2.0+
is in [stable] core only since Perl 5.10.
 
[...]
> > +# $data = $cache->compute($key, $code);
> > +#
> > +# Combines the get and set operations in a single call.  Attempts to
> > +# get $key; if successful, returns the value.  Otherwise, calls $code
> > +# and uses the return value as the new value for $key, which is then
> > +# returned.
> > +sub compute {
> > +       my ($self, $key, $code) = @_;
> > +
> > +       my $data = $self->get($key);
> > +       if (!defined $data) {
> > +               $data = $code->($self, $key);
> > +               $self->set($key, $data);
> > +       }
> 
> Can you guarantee $code here?

I don't want to code too defensively, but perhaps check for this is in
order... though what we should do if $code is not code reference?

> 
> unless( defined $code and ref $code eq 'CODE' )

"ref($code) eq 'CODE'" would be enough; 'undef' is not reference, and
ref(undef) returns "".

> {
>         ....
> }
> 
> Wouldn't it be easier to eval{} this and check $@?

So the answer is no.


Thanks again for your comments.
-- 
Jakub Narebski
Poland