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.) 2. %WriteMakefileArgs in "Makefile.PL" is indented by 12 spaces. Why so many? You also have some three-line spaces in that file, which is also uncommon. 3. bin/example.pl should really be under eg/ or examples/ or whatever. 4. In the same file, "use strict;" should be separated from the shabang with an empty line. 5. my ($name) = $AUTOLOAD; ==> no need for the parentheses here (I don't think they hurt in this case, but they are confusing.) 6. my $validate_name_regex = qr/^[a-zA-Z_]+[a-zA-Z0-9_]*$/; ==> you can omit the first "+" in this case. 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). 9. "$proretset" - what does this variable mean? I cannot parse the meaning of its identifer. Otherwise, looks fine. Regards, Shlomi Fish -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ Best Introductory Programming Language - http://shlom.in/intro-lang Larry Wall has been changing the world. By modifying its very source code. Please reply to list if it's a mailing list post - http://shlom.in/reply .
