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.)

#!/usr/local/bin/perl

This is a module: you don't need a shebang line.

use Carp qw/croak cluck/;

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;
use feature qw/say switch/;

Remove this line because:

  • I can only find one instance of say and adding a \n to a print is no huge effort.
  • The 'switch' feature is experimental. (More on how to work around the code using it below.)
  • It's contrary to your later statement in the POD: "Every reasonable effort has been made to maintain compatibility with previous versions of Perl."
$|++;

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.

sub codon_to_aa { ... }

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.

sub connect_db { ... }

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.

sub read_fasta_file { ... }

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.

sub usage { ... }

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.

sub AUTOLOAD { ... }

This seems unnecessary: perl will report if nonexistent functions are called.

sub DESTROY { ... }

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

    kcott,

    ++ for a great analysis. I usually skip over AM questions, glad I didn't with this one.

    Regards...Ed

    "Well done is better than well said." - Benjamin Franklin

Re^2: Bioinformatics project feedback request.
by runrig (Abbot) on Jul 02, 2013 at 16:02 UTC
    The $|++ is a well known idiom; however, it works on the assumption that $| >= 0 is TRUE

    No, it doesn't. It works on the little known fact that $| can only be 0 or 1. Try: $| = -1; print "$|\n";

      Interesting. I certainly didn't know that and it's not documented in perlvar.

      $ perl -E '$| = 2; say $|; $| = -1; say $|; $| = 0; say $|; $| = undef +; say $|' 1 1 0 0

      Thanks for the information.

      -- Ken