Re: [PATCHv5 03/17] gitweb/lib - Very simple file based cache
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Oct 6, 2010, 23:00 UTC
- Message-ID
- <201010070100.33478.jnareb@gmail.com>
- In-Reply-To
- <AANLkTinPCOfknoN4aO_EdPwKorRiM6NU6ep0z_nnNug8@mail.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:
Show 8 quoted lines
> > +# 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.
Show 7 quoted lines
> > > + 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
Show 10 quoted lines
> > +# $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...
Show 11 quoted lines
> > + 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.
Show 7 quoted lines
> > + # 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. [...]
Show 16 quoted lines
> > +# $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 "".
Show 5 quoted lines
> {
> ....
> }
>
> Wouldn't it be easier to eval{} this and check $@?So the answer is no.
Thanks again for your comments.
-- Jakub Narebski Poland