On Fri, Apr 22, 2011 at 10:08 PM, Tiago Hori <tiago.h...@gmail.com> wrote:
> Hi List, > > Howdy. Before we start, instead of using wikipedia, if you can get your hands on it and feel like spending a few hours of your time, Perl Best Practices[0] is the way to go. However, assuming that you are a busy person (and, as a PhD student, broke and can't afford books), here's something that will alleviate most of your style woes: perltidy[1]. perltidy is a nifty utility that will grab your code and vomit a "tidied" version. In fact, let's go grab your original post and run it through perltidy.. http://ideone.com/VAdfe Which is looking pretty nice already, eh. You can change how it indents too, so you'll get the braces however you prefer them and sosuch. In any case, after running a couple of your scripts through perltidy, some of the style should rub off you and you'll start doing it naturally. That aside, some corrections to your code.. You are using a bunch of global variables -This is generally a big red flag, as later on if the program grows or you make a change it might have weird effects at a distance. For example, all the processing of $seqnum is inside readFasta, and thenonce outside to print the results. Instead of that, you might want to declare two versions of $seqnum: One inside of readFasta, and one outside, to receive the results of that function. Basically: my ($seqnum) = readFasta( $ARGV[0] ); sub readFasta { my $seqnum = 0; ... return $seqnum; } We'll get back to this later. Then, you are using a lot of bareword filehandles - stuff like TARGETS. Here's the chance to introduce another nifty utility - perlcritic[2]. You may recall me mentioning Perl Best Practices before - The book isn't only about style, but also about avoiding common mistakes in Perl. Bareword filehandles can occasionally be a big bag of nasty; You'll want a lexical filehandle instead: open( my $target, "<$ARGV[0]" ); @targets = <$target>; close $target; But that's not good enough yet : ) That open may be trouble. You are using the two-argument version, which is magical. For safety's sake, you'll want the three-argument version: open my $target, '<', $ARGV[0]; But we aren't done with open yet. What if the file wasn't found? Your program will go on merrily, but without a file to read from, that seems hardly what you'd like it do you. So make it stop: open my $target, '<', $ARGV[0] or die "File not found: $ARGV[0]: $!"; You could do that, of course, but that's a bunch of extra typing. You could use the autodie pragma[3] which will do the 'or die's for you. Beyond that, there's only one big error in what you sent, though. in readFasta, you do this: if ( @ARGV == 0 ) { die "No FASTA file specified.\n"; } But that should be if ( @_ == 0 ) { die "No FASTA file specified.\n"; } And then this: open(FILE, "<@_[0]"); Which should probably be open my $file, '<', $_[0]; A nitpick, but you also have an $index variable that you don't use. You can probably also let go of that grep and replace it with a smart match: if ($seqheader[$index] ~~ @targets) { ... Since I'm kind of bored, here's my shot at your program; I've never had to deal with FASTA files, so I left some stuff in readFasta untouched: http://ideone.com/kk0OI Brian. [0] http://oreilly.com/catalog/9780596001735/ [1] http://search.cpan.org/~shancock/Perl-Tidy-20101217/bin/perltidy and http://search.cpan.org/~shancock/Perl-Tidy-20101217/lib/Perl/Tidy.pm [2] http://search.cpan.org/~elliotjs/Perl-Critic-1.115/bin/perlcritic [3] http://perldoc.perl.org/autodie.html
