Re: improving the aesthetics of perl code

Replies are listed 'Best First'.
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;` [download] but not in `sort { $a <=> $b; } @foo;` [download] So for the most part you're basically in violent agreement with perlstyle.	[reply] [d/l] [select]
Re^3: improving the aesthetics of perl code by g0n (Priest) on Jan 21, 2005 at 17:07 UTC
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; [download]	[reply] [d/l]
Re^3: improving the aesthetics of perl code by jdporter (Paladin) on Jan 21, 2005 at 22:44 UTC
Regarding this: `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; } } }` [download] 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;` [download] 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;` [download] hth.	[reply] [d/l] [select]
Re^3: improving the aesthetics of perl code by pingo (Hermit) on Jan 21, 2005 at 16:23 UTC
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; } }` [download] Something like this, perhaps: `foreach (@tmp) { push @hosts, $1 if /^([\w.]+)$/; }` [download] I may be missing something blindingly obvious, though. :-)	[reply] [d/l] [select]
Re^4: improving the aesthetics of perl code by Tanktalus (Canon) on Jan 21, 2005 at 16:29 UTC
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]
Re^4: improving the aesthetics of perl code by jdporter (Paladin) on Jan 21, 2005 at 22:25 UTC
I'd write that as `push @hosts, map { /^([\w.]+)$/ ? $1 : () } @tmp;` [download] Not that I make any claims for its aesthetics. :-)	[reply] [d/l]
Re^5: improving the aesthetics of perl code by sleepingsquirrel (Chaplain) on Jan 21, 2005 at 22:43 UTC
Re^3: improving the aesthetics of perl code by wfsp (Abbot) on Jan 21, 2005 at 16:44 UTC
Looks good to me too (but that's not saying that much!) If a print statement is a bit unwieldy I often use join to get it to read better. `print join "\n", '<center>', '<h3>MSHIP - my status of host IP's</h3>', '</center>';` [download] 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]
Re^3: improving the aesthetics of perl code by g0n (Priest) on Jan 21, 2005 at 16:15 UTC
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]
Re^3: improving the aesthetics of perl code by hardburn (Abbot) on Jan 21, 2005 at 22:23 UTC
`foreach my $uitem (sort @uphosts) { # sort is for cleanliness... print li("$uitem\n"),br; }` [download] I prefer to use `map` over `foreach` when possible: `print join br, map li $_, sort @uphosts;` [download] 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]
Re^3: improving the aesthetics of perl code by BrentDax (Hermit) on Jan 24, 2005 at 09:56 UTC
`print "<tt>�\n"; print "\n<c�enter�><h3>�MSHIP - my statu�s of host IP's<�/h3><�/cent�e +r>\n�"; my $now=�strft�ime "%m/%�d/%Y %H:%M�:%S", (loca�ltime�); print "<cen�ter>$�now</�cente�r>\n"�; print hr;` [download] 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 statu�s of host IPs</h3></center> <center>$now</center> <hr /> END` [download] 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]
Re^3: improving the aesthetics of perl code by Anonymous Monk on Jan 24, 2005 at 15:01 UTC
For subroutine comments, use POD, I disagree with that. POD is used to make user documentation. Unless you play silly tricks with `=for` or `=begin`, anything that you document with POD, will end up in the user manual. That gives you two options: You will make some subroutines part of the API while they shouldn't be. This breaks encapsulation, and in the future, it will give you the choice to break backwards compatibility, or to support code you'd rather not support. It will restrict your freedom to redo the internals of your module. Document that the description that follows is about code the user shouldn't use. This makes the documentation clumsy, as it could easily be littered with pieces of documentation the user isn't supposed to use. POD is for the user, comments are for the coder. And while they may be the same, they usually aren't.	[reply]
Re^4: improving the aesthetics of perl code by hardburn (Abbot) on Jan 24, 2005 at 18:57 UTC
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]


Welcome to the Monastery
	PerlMonks