in reply to Bioinformatics project feedback request.
Overall, this code seems well laid out and easy to read: that's a good start. I'll just run through it, top to bottom, and comment as I see appropriate. (I don't know your level of Perl — ask if something needs further explanation.)
This is a module: you don't need a shebang line.
See Carp. carp() and croak() give warnings/errors with short messages; cluck() and confess() use long messages. The short message forms are usually what you want in a module and, as they're exported by default, you can just write:
use Carp;
Remove this line because:
Don't make this global for the entire module. Find out where you need it and localise it:
{ local $| = 1; # code needing unbuffered output here }
Note that I've shown an assignment (=) instead of an increment (++).
The $|++ is a well known idiom; however, it works on the assumption that $| >= 0 is TRUE but never tests it. If someone ever decided to do the reverse to get buffered output (i.e. $|--) you could easily run into problems (due to an undocumented feature, that's not correct - see discussion below). Using local $| = 1;, you know exactly what you've got and where you've got it.
See also autoflush() in IO::Handle.
Here you're using the experimental 'switch' feature I mentioned earlier. Instead of the given/when/.../default, you can use if/elsif/.../else or one of the forms shown in perlsyn - Basic BLOCKs. Here's another alternative that's closer to how your original code looks:
$ perl -Mstrict -Mwarnings -le ' sub codon_to_aa { my ($codon) = @_; for ($codon) { return /GC./i ? "A" : /TG[TC]/i ? "C" : /GA[TC]/i ? "D" : do { print "Sorry"; "" }; } } for (qw{GCT TGT XXX}) { print "$_ : ", codon_to_aa($_); } ' GCT : A TGT : C Sorry XXX :
This subroutine also contains the only say() I found. print() is obviously fine but I'm wondering if carp() or croak() might be more appropriate here.
I would tend to restrict that function to just what its name implies (i.e. making a connection). It would return the database handle ($dbh) which could be reused for multiple queries.
I don't know why you've chosen to use sysopen. If you're sure you need it, mention the portability issues in the "INCOMPATIBILITIES" section of your POD. I think open would be fine here — use the 3-argument form.
I'd say this is surplus to requirements. What you do need, however, is a "SYNOPSIS" section at the start of your POD with much the same information. You show GCCBioParser which does not match the package name. You show a new() constructor which does not exist and none of your subroutines appear to be object methods. Until I got down to here, I'd assumed this was intended to be a functional module, not an object-oriented one. Also, do not recommend to users of this module that they use Indirect Object Syntax — see perlobj - Invoking Class Methods.
This seems unnecessary: perl will report if nonexistent functions are called.
Unless you're planning to add other functionality, this is also unnecessary: perl will free memory, you don't need to do it yourself.
-- Ken
|
|---|
| Replies are listed 'Best First'. | |
|---|---|
|
Re^2: Bioinformatics project feedback request.
by flexvault (Monsignor) on Jul 02, 2013 at 12:04 UTC | |
|
Re^2: Bioinformatics project feedback request.
by runrig (Abbot) on Jul 02, 2013 at 16:02 UTC | |
by kcott (Archbishop) on Jul 02, 2013 at 16:36 UTC |