Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl Monk, Perl Meditation
 
PerlMonks  

Re^2: improving the aesthetics of perl code

by tcf03 (Deacon)
on Jan 21, 2005 at 14:46 UTC ( #423985=note: print w/replies, xml ) Need Help??


in reply to Re: improving the aesthetics of perl code
in thread 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;

Replies are listed 'Best First'.
Re^3: improving the aesthetics of perl code
by jdporter (Chancellor) 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; } } }
    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.
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; } }

    Something like this, perhaps:

    foreach (@tmp) { push @hosts, $1 if /^([\w.]+)$/; }
    I may be missing something blindingly obvious, though. :-)

      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 ;-)

      I'd write that as
      push @hosts, map { /^([\w.]+)$/ ? $1 : () } @tmp;
      Not that I make any claims for its aesthetics. :-)
        Why not...
        @hosts = map { /^([\w.]+)$/ ? $1 : () } @tmp;
        update: might as well get rid of the @tmp while we're at it...
        @hosts = map { /^([\w.]+)$/ ? $1 : () } <CFGFILE>;


        -- All code is 100% tested and functional unless otherwise noted.
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>';
    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.

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?

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; }

    I prefer to use map over foreach when possible:

    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.

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;

    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

    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.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://423985]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others meditating upon the Monastery: (3)
As of 2022-05-18 20:19 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?
    Do you prefer to work remotely?



    Results (71 votes). Check out past polls.

    Notices?