in reply to improving the aesthetics of perl code
Coding style is very much a matter of personal preference. For instance, I think perlstyle says you should omit the last ; in a block. Personally I disagree - if you add some code after that line later, it generates a syntax error unless you spot it and change it. But that might just be me.....
Having maintained other peoples code for far too many years, the one thing I would say is comment. Lots and lots of comment. Not just:
#this bit does xyz
sub xyz....
which can be difficult to see in a long script, but
#######################################
# subroutine xyz - does xyz, abc, zz plural z alpha
#######################################
sub xyz..
Or your own personal variant of it. Also, give variables reasonably explanatory names, and make fairly liberal use of whitespace.
Those would be my suggestions anyway. Why not post a code snippet and let the monks bitch about it? :-)
charles.
Addendum: applause for wanting to make your code easier to read though - most people wouldn't bother!
Re^2: improving the aesthetics of perl code
by TimToady (Parson) on Jan 21, 2005 at 16:36 UTC
|
For instance, I think perlstyle says you should omit the last ; in a block.
No, that's not what it says at all. It says:
Semicolon omitted in "short" one-line BLOCK.
where "one-line" should be taken to include the curlies, and "short" should be taken to mean something that can be seen as a kind of visual "pill". So it's saying it's okay to omit the semicolon in
sort { $a <=> $b } @foo;
but not in
sort {
$a <=> $b;
} @foo;
So for the most part you're basically in violent agreement with perlstyle. | [reply] [d/l] [select] |
|
Apologies, that'll teach me to do my research properly! :-)
| [reply] |
Re^2: improving the aesthetics of perl code
by tcf03 (Deacon) on Jan 21, 2005 at 14:46 UTC
|
heres a snippet of code which Ive cleaed up a bit - Ive posted earlier viersions of this elsewehere on perlmonks and added some of the suggestions that I received.
#!/usr/bin/perl -T
# mship.cgi
# Just add hosts ip addresses, fqdn, or just hostnames
# gives a simple status of hosts up or hosts down
# next step is to add logging. The aim is not
# a full featured network monitor, just a quickie list
# of whats up and whats down - Its probably not useful
# in a large environment...
# Ted Fiedler <fiedlert@gmail.com>
use strict;
use warnings;
use Net::Ping;
use CGI qw/:standard/;
use POSIX qw(strftime);
# Clean up our UNIX environment
# for more infor read perldoc perlsec
$ENV{'PATH'} = '/bin:/usr/bin';
delete @ENV{qw(IFS CDPATH ENV BASH_ENV)};
my (@hosts, @uphosts, @downhosts);
if ( -e "ship.cfg" ) {
open (CFGFILE, "ship.cfg") || die $!;
while (my @TMP=<CFGFILE>) {
next if ( /#/ );
# Lets untaint the data after making sure it only matches
# word characters... For more info read perldoc perlsec.
foreach $_(@TMP) {
if ($_ =~ /^([\w.]+)$/) {
$_ = $1;
push (@hosts, $1);
} else {
next;
}
}
}
} else {
# or enter your hosts here...
# This is the most secure/preferred way to do this
@hosts = qw/sapdev0 sapprd0 saptst0 pcbackup vader/;
}
# I haven't decided what else to do here - it just semantics...
my @verbage=("are", "hosts", "are", "hosts");
# udp is default
# other values can be icmp (if youre root)
# or TCP, which is expensive on the wire
my $p = Net::Ping->new('udp');
# Iterate through my hosts and get their status
foreach my $host(@hosts) {
chomp($host); # Cleans up ship.cfg if you're using it
if ($p->ping($host)) {
push (@uphosts, $host); # its either UP
} else {
push (@downhosts, $host); # or its DOWN...
}
}
$p->close();
print header, start_html;
print "<tt>\n";
print "\n<center><h3>MSHIP - my status of host IP's</h3></center>\n";
my $now=strftime "%m/%d/%Y %H:%M:%S", (localtime);
print "<center>$now</center>\n";
print hr;
# Just cleaning up the verbage is are host hosts etc...
if (scalar(@downhosts) == 1 ) {
$verbage[2]="is";
$verbage[3]="host"
}
if (scalar(@uphosts) == 1 ) {
$verbage[0]="is";
$verbage[1]="host";
}
# We don't care about things that don't exist...
unless (scalar(@downhosts) == 0) { print p,"There $verbage[2] ",scalar
+(@downhosts)," $verbage[3] unreachable\n",br }
print "There $verbage[0] ",scalar(@uphosts)," $verbage[1] alive\n";
print p,"The following $verbage[1] $verbage[0] alive: \n",br;
foreach my $uitem (sort @uphosts) { # sort is for cleanliness...
print li("$uitem\n"),br;
}
# Again, we don't care about things that don't exist
unless (scalar(@downhosts) == 0) { print p,"The following $verbage[3]
+$verbage[2] unreachable: \n",br }
foreach my $ditem (sort @downhosts) {
print li("$ditem\n"),br;
}
print "</tt>\n";
print start_form,
p,submit('Refresh'),
end_html;
| [reply] [d/l] |
|
open (CFGFILE, "ship.cfg") || die $!;
while (my @TMP=<CFGFILE>) {
next if ( /#/ );
# Lets untaint the data after making sure it only matches
# word characters... For more info read perldoc perlsec.
foreach $_(@TMP) {
if ($_ =~ /^([\w.]+)$/) {
$_ = $1;
push (@hosts, $1);
} else {
next;
}
}
}
That's totally wrong. Ordinarily, you either read from a file a line at a time, using while, or you read the entire (rest of the) file by assigning to a list. You can't meaningfully combine the two approaches, which is what it looks like you've tried to do. Below is how to write your loop using the one-fell-swoop approach, with the other transformations included:
open CFGFILE, "< ship.cfg" or die "read ship.cfg: $!";
@hosts =
map { /^([\w.]+)$/ ? $1 : () }
grep !/#/,
<CFGFILE>;
}
close CFGFILE;
Here's something similar but using a line-at-a-time approach:
open CFGFILE, "< ship.cfg" or die "read ship.cfg: $!";
while (<CFGFILE>) {
/#/ and next;
/^([\w.]+)$/ or next;
push @hosts, $1;
}
close CFGFILE;
hth.
| [reply] [d/l] [select] |
|
Looks fine to me, but I am not exactly an expert myself. :-) One thing that I guess could be written differently is this:
foreach $_(@TMP) {
if ($_ =~ /^([\w.]+)$/) {
$_ = $1;
push (@hosts, $1);
} else {
next;
}
}
Something like this, perhaps:
foreach (@tmp) {
push @hosts, $1 if /^([\w.]+)$/;
}
I may be missing something blindingly obvious, though. :-) | [reply] [d/l] [select] |
|
Not blindingly obvious, no. What you probably missed is the -T at the top of the code. This means that the original code will untaint parts of @TMP (assigning to $_ should affect the original array entry) at the same time as making a wholly untainted @hosts. Since @TMP doesn't seem to be used again, the $_=$1 part seems somewhat useless ;-)
| [reply] |
|
push @hosts, map { /^([\w.]+)$/ ? $1 : () } @tmp;
Not that I make any claims for its aesthetics. :-) | [reply] [d/l] |
|
|
print join "\n",
'<center>',
'<h3>MSHIP - my status of host IP's</h3>',
'</center>';
I would have considered $ping instead of $p.
I prefer a space either side of an = (sometimes you do too, sometimes you don't!).
I think that 2 space tabs use less real estate than 4 if there is deep nesting.
foreach $_(@TMP)
could be written as
for (@TMP)
} else {
Is that what they call cuddled?
It's controversial but I put the else on a new line.
All very minor.
| [reply] [d/l] [select] |
|
I can't speak for anyone else, but I can't see much to complain about in that code. I try not to use single line {} blocks unless its very short is the only thing that springs to mind.Anyone else?
| [reply] |
|
print join br, map li $_, sort @uphosts;
But maybe that's just because I like writing LISP in Perl :)
"There is no shame in being self-taught, only in not trying to learn in the first place." -- Atrus, Myst: The Book of D'ni.
| [reply] [d/l] [select] |
|
print "<tt>\n";
print "\n<center><h3>MSHIP - my status of host IP's</h3></cente
+r>\n";
my $now=strftime "%m/%d/%Y %H:%M:%S", (localtime);
print "<center>$now</center>\n";
print hr;
Personally, I'd probably use a heredoc for this:
my $now=strftime "%m/%d/%Y %H:%M:%S", localtime;
print <<"END";
<tt><center><h3>MSHIP - my status of host IPs</h3></center>
<center>$now</center>
<hr />
END
Of course, I wouldn't write HTML like that, but continuing in that direction will only send me way off-topic...
=cut
--Brent Dax
There is no sig.
| [reply] [d/l] [select] |
Re^2: improving the aesthetics of perl code
by hardburn (Abbot) on Jan 21, 2005 at 22:19 UTC
|
For subroutine comments, use POD, not ugly flowerbox comments. Better still, name your subroutines and clearly label the input params, and your documentation will take care of itself.
"There is no shame in being self-taught, only in not trying to learn in the first place." -- Atrus, Myst: The Book of D'ni.
| [reply] |
|
| [reply] |
|
Why not play tricks with =for? It should not be difficult to modify the pod2text converter (or whatever other convert you want) to accept =for private blocks. Then you can have two converters, one for basic API information, and another for private docs. Javadocs have a similar concept, and I think it's a good idea. Getting your developers to follow it is simply a matter of corporate coding policy.
In fact, I think I should probably implement that, or track down a converter that does it already.
"There is no shame in being self-taught, only in not trying to learn in the first place." -- Atrus, Myst: The Book of D'ni.
| [reply] [d/l] [select] |
Re^2: improving the aesthetics of perl code
by Anonymous Monk on Jan 24, 2005 at 15:21 UTC
|
Lots and lots of comment.
When I was a programming novice, I didn't comment much.
When I was a programming acolyte, I commented a lot.
Now I am a programming master, and I try to minize the comments in my programs.
Why, you may ask. Doesn't more comments make it easier to understand what code does? Yes, it does - however, comments don't solve the cause. Comments fix the symptoms. Comments are used when the program isn't clear. If you write clearer programs, you need less comments.
It's like taking painkillers. They cause the headache to go away, but it's better to stop banging your head.
| [reply] |
|
|