>>>>> "BC" == Brent Chapman <Brent@GreatCircle.COM> writes:
BC> Oh, I _hate_ it when Perl gets "smart" like that... I'd rather code it
BC> clearly (split(/\s+/)) than count on somebody to know that " " is a
BC> special case kinda-sorta-but-not-quite the same thing.
But you'd appreciate the feature if you were an awk programmer. That's
part of the whole perl mindset.
BC> While I'm up on my soapbox about Perl programming style, let me mention
BC> another Perl-ism that I hate: conditionals following code bodies.
You may not like them, but I use them whenever it's proper. Especially
"unless". There are times when the code just makes more sense that way. I
would agree that they don't make any sense if you have a block of
conditional code, but a structure like
print "Blah.\n" if $DEBUGGING;
makes a while heck of a lot more sense than
if ($DEBUGGING) {
print "Blah.\n";
}
and I'm happy that perl offers the construct.
BC> In all seriousness, though, there is a code maintenance principle that,
BC> when working on existing programs, you code in the style they were
BC> written in, even if that's not precisely your own style.
Majordomo started getting really messy around 1.92, and I think it's much
more than issues of style. To me it seems that the "patch it" mentality
has led to what looks like mush. Compare the following chunk of
do_subscribe in 1.94:
if ($approved
|| ($sub_policy =~ /auto/i &&
&check_and_request("subscribe", $clean_list,
$subscriber, "check_only")) # I don't think this check is doing the right thing. Chan 95/10/19
|| (($sub_policy !~ /closed/ )
&& &addr_match($reply_to, $subscriber,
(&cf_ck_bool($clean_list,"mungedomain") ? 2 : undef)))
) {
&lopen(LIST, ">>", "$listdir/$clean_list")
|| &abort("Can't append to $listdir/$clean_list: $!");
The whole block is indented, and there are two lines in the else clause
(way at the end). Here's what's in the version I run:
$addresses_match = &addr_match($reply_to,
$subscriber,
(&cf_ck_bool($clean_list,
"mungedomain") ?
2 :
undef));
# Approval will be needed if:
# the request is not approved, and
# subscribes are not automatically accepted, and either
# a. the list is closed or
# b. the addresses don't match.
$approval_needed = (!$approved &&
$sub_policy !~ /auto/i &&
($sub_policy =~ /closed/i ||
!$addresses_match));
if ($approval_needed) {
&request_approval($sm, $clean_list, $subscriber);
return 0;
}
&lopen(LIST, ">>", "$listdir/$clean_list")
|| &abort("Can't append to $listdir/$clean_list: $!");
The complicated approval mechanism was just grafted onto the original code
and pieces were added without rewriting. Nobody commented anything. It
took some time to untangle everything and figure out what was going on, but
now I can understand the code and adding to it wouldn't be too difficult.
The next step is to abstract further and break this down into functions
like lock_list, lock_file, add_address_to_list, etc.
BC> I haven't been paying close attention to the discussions regarding 2.0
BC> development (I keep intending to go back and read them someday), but I
BC> think that this issue of coding style is something that needs to be
BC> addressed right along with all the other logistical aspects of the
BC> project that you've been discussing.
Codifying coding styles makes some sense, but it has to be to some degree
flexible. You don't like to put the conditional after the action. I do,
where it's warranted. If a prerequisite to contribution be that I write
perl code that looks like C, I'll pass. If, on the other hand, we decide
how to name functions and variables, to indent four spaces, to follow
perlstyle when it comes to block notation and indentation, to try as hard
as possible to limit to three nested blocks maximum and to document every
damn block (for example) than I'm in. (I really like comments. Big, long,
explanatory comments. I like no unexplained lines of code.)
I still think design issues are an order of magnitude more important. We
have decide what we're going to do before we decide to do it with a
southern or an English accent.
- J<
References:
|
|
