From: Uri Guttman <[EMAIL PROTECTED]>
Date: Wed, 08 Sep 2004 00:17:15 -0400
>>>>> "BR" == Bob Rogers <[EMAIL PROTECTED]> writes:
BR> use strict;
use warnings should be here too.
Thank you; that sounds like a good idea. (Though usually my main
interest in manipulating perl warnings is shutting them up in production
code.)
BR> use Getopt::Long;
BR> use Pod::Usage qw();
BR> # use Data::Dumper;
BR> # define instance accessors.
BR> sub BEGIN {
BR> no strict 'refs';
BR> for my $method (qw(option_definers additional_options
BR> man_p help_p usage_p)) {
BR> my $field = '_' . $method;
i like "_$method" better.
I don't; it's harder to see.
BR> *$method = sub {
BR> my $self = shift;
BR> @_ ? ($self->{$field} = shift, $self) : $self->{$field};
why are you returning $self when assigning in the accessor? in general
doing side effects in ?: is frowned upon. here are two different ways to
do that, with your semantics and a more common one which always returns
the field value . . .
This is boilerplate I lifted from somebody else's code; it strikes me as
ugly, but I haven't changed it much, because I haven't thought of
anything significantly better. This particular coder (I've forgotten
who now) was fond of doing chains of accessors:
my $obj = Some::Class->slot1($value1)->slot2($value2);
I never do this myself, though I sometimes still take advantage of slot
assignments returning the object.
BR> sub new {
BR> my $class = shift;
BR> $class = ref($class)
BR> if ref($class);
that idiom is not needed nor is it useful . . .
Very true. The slot accessors used to do this, as I mentioned above;
when I took this out of the code, it speeded the entire suite of scripts
by about 15%! But obviously I overlooked taking it out of the
constructor.
. . . construction from an object is better called 'clone' as it may
actually copy much (if not all) of the object into the new one.
Indeed; in fact, there was a 'clone' method on the base class from which
I borrowed the 'new' method, but I didn't bother to take it.
BR> my $self = bless {} => $class;
i don't like using => like that.
Why? I'm curious -- this is another inherited idiom, so I'm not wedded
to it.
ever heard of vertical whitespace or comments?
As a matter of fact, yes. Lower-than-normal comment density is part of
what makes this an "early version" (though it's true that I never said
so explicitly). Also, I don't use much vertical whitespace, because it
makes less code fit on the screen.
use explicit returns. you may get burned with the return of last value
thing. it is easier to see what is returned when you see 'return'
As a matter of fact, this almost never gives me problems; I get burned
more often by an overlooked 'return' that causes something later on in
the sub to be skipped unexpectedly.
for some reason my emacs/supercite is screwing up the indent. but your
habit of putting statement modifiers on the next line is unusual. i
would put them on the same line if possible (that is if the combined
line isn't too long). it just reads odd to me to see all those if's on
the next line.
For a long time, I resisted using statement modifiers at all. It is
true that they are more compact, but they change the flow of control in
ways that can make the code harder to read. I find it distracting that
in
hairy_function_b(....)
if hairy_function_a(....);
the hairy_function_a gets called before hairy_function_b, in the reverse
of the natural reading order. Combining them on the same line makes it
worse, as it makes the true flow of control even less obvious.
also using ||= will simplify many of those lines (as i did above).
$cl_options->{usage} ||= sub { $self->pod2usage(2); }
if $self->usage_p;
This strikes me as still worse; there are two circumstances that could
alter the effective flow of control, but they are being expressed in two
different ways, still out of order.
Mind you, I do use "||=" and modifers on the same line from time to
time, but I try to do so only if I think the intent is transparent. I
use "||=" mostly for defaulting arguments to subs, and try to restrict
same-line modifiers to cases where the branch will be taken almost all
of the time. (Though I will confess to having taken some shortcuts in
getting this code ready to post. As you can see, it's still not yet in
CVS. ;-)
BR> sub _add_to_class_table {
BR> my ($self, $table, $class_visited_p, $class) = @_;
BR> if (! $class_visited_p->{$class}) {
unless is nice there.
I started using 'unless' until I wound up creating constructs like the
following:
unless (<some-expression>) {
<some-code>
}
elsif (<some-other-expression>) {
<some-more-code>
}
The fact that <some-more-code> is executed iff *both* <some-expression>
and <some-other-expression> are true just bent my brain in a funny way.
So now I tend to avoid 'unless' altogether.
event better since this if block is the whole sub, just return early and
save an indent and a block
return unless $class_visited_p->{$class} ;
rest of code is here but no {} or indent.
Sometimes I do that if I'm short on indentation, but I try to avoid
nonlocal exits when I can. (But as you have seen, I am inconsistent
here, too.)
BR> sub class_precedence_table {
BR> my $self = shift;
BR> $self->{_class_precedence_table} = shift, return($self)
BR> if @_;
make that a regular if block. hidden side effects (return) with ,
expressions are hard to see.
Agreed. I should have taken my own medicine.
. . .
BR> close(IN);
no need for the close as the handle goes out of scope and gets
closed.
True. But I always put in explicit 'close' calls when I'm done with a
handle, because that makes my intent clear -- it tells the reader that
I'm not doing anything tricky.
de morgan to the rescue!
unless ($pod_file && -r $pod_file) {
I agree that this reads better. But I would almost prefer:
if (! ($pod_file && -r $pod_file)) ...
due to the brain-bending mentioned above.
BR> sub document_options {
BR> my ($self, $fh) = @_;
BR> $fh ||= *STDOUT;
$fh ||= \*STDOUT;
passing glob refs are better than passing globs
OK; I'll take your word for it.
i can't get into the actual design or debugging but i hope my comments
are useful.
uri
Not as useful as dealing with the substance, but still helpful; thank
you.
And peace to the other posters who objected to having my code "ripped
to shreds". Thanks for trying to defend me from mean 'ol Uri (;-), but
I really don't need it. In fact, I can't remember the last time I got
feedback at this level, from anybody. It's useful to hear another
experienced coder's opinions of how my code reads -- especially since my
own Perl coding style has changed greatly in the last year and a half.
From: Uri Guttman <[EMAIL PROTECTED]>
Date: Wed, 08 Sep 2004 12:49:10 -0400
>>>>> "SQ" == Sean Quinlan <[EMAIL PROTECTED]> writes:
SQ> . . . And I don't think anyone has accused Uri of being the
SQ> most diplomatic monger. =P
why thank you! the code quality is usually more important than the
coder's ego :)
I agree. Besides, this coder's ego is not so easily bruised.
. . . i am wary of the idea of using OO like
that as well. when i see many utilities with similar options, i tend to
think it would be better as one main utility with commands and
options. that would remove both the code and doc redundancy. it could
still be very modular (load modules based on the command selected).
uri
A little of that might be possible, especially where I've started coding
a next-generation script before retiring the original version, but most
of these programs use the same modules to do very different things.
They are almost all DNA manipulation programs, 80 of them (including
some CGI scripts), but there the resemblance ends. Some of them design
oligonucleotides (short DNA strings) for various applications, some
manipulate sequences and sequence databases, and then there are some
utility programs for support and testing. FWIW, the whole pile is about
50K lines of Perl (code+POD).
There are 23 modules that define one or more command-line options. A
key module is ModGen::ObjectStreams::PCRFactory, mentioned previously,
which is used to design oligonucleotides for PCR ("polymerase chain
reaction", used to amplify selected DNA). This module is used directly
or indirectly from 13 scripts, for which it provides a dozen options or
so, including --pcr-temp, --pcr-length, and --misprime-max-tm, to name
three. These options all have essentially the same meaning wherever
they appear, but the other inputs and outputs can be radically
different, depending on whether the oligos are to be used for gene
repair, sequencing, or building a gene from scratch. Too much
consolidation would create a different sort of documentation problem.
I hope this gives a better feel for what I'm up against . . .
-- Bob
_______________________________________________
Boston-pm mailing list
[EMAIL PROTECTED]
http://mail.pm.org/mailman/listinfo/boston-pm
