Re^2: improving the aesthetics of perl code

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]

Comment on Re^2: improving the aesthetics of perl code Download Code

Replies are listed 'Best First'.
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
Why not... `@hosts = map { /^([\w.]+)$/ ? $1 : () } @tmp;` [download] update: might as well get rid of the @tmp while we're at it... `@hosts = map { /^([\w.]+)$/ ? $1 : () } <CFGFILE>;` [download] -- All code is 100% tested and functional unless otherwise noted.	[reply] [d/l] [select]
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]