On Tue, May 29, 2012 4:42 am, Shlomi Fish wrote:
> Hi Joel,
>
> On Tue, 29 May 2012 14:48:27 +0700
> Joel Jacobson <j...@trustly.com> wrote:
>
>> Hi!
>>
>> I'm about to submit a module to CPAN and would like some feedback on the
>> proposed module name, and any other feedback on the code or interface.
>>
>> http://prepan.org/module/429En4oFbi
>> https://github.com/joelonsql/dbix-pg-callfunction
>>
>> This module will be used at a financial institute processing millions of
>> transactions,
>> so I urge you to try to help me find any eventual bugs before putting it
>> in
>> production.
>>
>
> OK, here's what I ran into:
>
> 1. Regarding the choice of licence, see:
> http://www.shlomifish.org/philosophy/computers/open-source/foss-licences-wars/#which-licence-same-as-perl
> (including the links) and
> http://perlbuzz.com/2010/06/artistic-license-20-makes-dual-license-boilerplate-unnecessary.html
>
> (Yes, I know that most people still do not follow this advice, but I'm
> still going to try.)
>
It's kind of pointless to twit the end-users about this when it's the end
result of an automated process. Complain on MetaCPAN to the maintainers of
the module-making tools.
[skipping the style complaints]
>
> 3. bin/example.pl should really be under eg/ or examples/ or whatever.
>
Yes. Absolutely agree.
[more style complaints skipped]
>
> 6. my $validate_name_regex = qr/^[a-zA-Z_]+[a-zA-Z0-9_]*$/; ==> you can
> omit the first "+" in this case.
>
Yup. I'm guessing this is the result of an editing session that didn't
completely clear away the redundant code.
> 7. «$_ =~ $validate_name_regex || croak "invalid format of argument name:
> $_" for @arg_names;»
>
> I was wondering where "$_" comes to play until I saw the trailing "for"
> (past the 80 columns). I suggest converting it into a foreach loop with a
> lexical variable:
>
> foreach my $name (@arg_names)
> {
> if ($name !~ $validate_name_regex)
> {
> croak...
> }
> }
>
> 8. You may wish to cache the prepared SQL statements in "call()" based on
> the $name somewhere for making the execution faster (instead of preparing
> a statement each time the function is called. (Though DBI may already do
> this for you).
>
YES! I don't know how frequently the functions get called, but if it's in
a tight loop it's something that should be addressed.
> 9. "$proretset" - what does this variable mean? I cannot parse the meaning
> of its identifer.
"Return set", obviously. Don't know what the "pro" stands for, but who
cares? It's another style complaint.
>
> Otherwise, looks fine.
>
> Regards,
>
> Shlomi Fish
>
-john
