Hi --
On 7 October 2010 00:00, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> Thank you very much for those comments on code.
Sure.
> 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.
Definitely.
>>
>> > + Â Â Â 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).
Do not ever underestimate Conway's book, Jakub -- seriously. It's
not a Bible, sure, but to ignore it or otherwise eschew its advise is
to shoot yourself in the foot. :)
> I should really have run gitweb, caching modules and tests through
> perlcritic...
And your other patches. :)
>> > + Â Â Â Â Â Â Â 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.
Unless we use fatalstobrowser, it will still end up in Apache's error
log. I don't see a problem here.
>> > + Â Â Â # 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.
Umm, maybe. I've not verified this, but we shouldn't rely on this either.
> 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?
confess() ahem, or otherwise die() since it's a complete fail here.
Appropriate logging is necessary here.
>>
>> unless( defined $code and ref $code eq 'CODE' )
>
> "ref($code) eq 'CODE'" would be enough; 'undef' is not reference, and
> ref(undef) returns "".
Sure, but it still assumes $code is a reference, and there's no
guarantee of that in the code path.
>> {
>> Â Â Â Â ....
>> }
>>
>> Wouldn't it be easier to eval{} this and check $@?
>
> So the answer is no.
Fantastic! :)
-- Thomas Adam
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html