Beefy Boxes and Bandwidth Generously Provided by pair Networks
Clear questions and runnable code
get the best and fastest answer
 
PerlMonks  

Re: Review of my script

by Tux (Monsignor)
on May 15, 2013 at 08:24 UTC ( #1033635=note: print w/ replies, xml ) Need Help??


in reply to Review of my script

A small note to start with:

#------Check options----- # Check if host is defined and alive if (!defined $host){ print "\nNo host specified\n\n"; showUsage(); } else { print "\nChecking if $host is alive.. \n"; $ping = Net::Ping->new(); if ($ping->ping($host)){ print "$host is alive\n\n"; }else { print "$host is unreachable\n\n"; exit; } $ping->close(); }

That reads awful for several reasons

  • Reverted if branches if (not expr) {} else {} => if (expr) {} else {}
  • showUsage () does an exit which is unclear from the code, as showUsage is at the bottom.

And you should make clear difference between wanted output (STDOUT) and error/diagnostics (STDERR).

I'd put showUsage () at the top of the script, which also functions as immediate documentation for someone starting to read the script, making that if/else easier, like:

sub showUsage { my $err = shift and select STDERR; print "\n------------------------------------------\n"; print "-- Usage for the database backup script --\n"; print "------------------------------------------\n"; print "\n--host \n"; print " Specify the host IP\n\n"; print "--engine \n"; print " Specify the database engine. This could be postgres o +r mysql. \n\n"; print "--password \n"; print " Give the password needed to connect with the database + server\n\n"; print "--user \n"; print " Specify user to connect to the database server.\n"; print " The user must have permissions to connect to the data +base(s)\n\n"; print "--port \n"; print " If another port than the standard must be used to con +nect you could \n"; print " specify this with the --port option\n\n"; exit $err; } GetOptions ( "help|?" => sub { showUsage (0); }, "host=s" => \$host, "engine=s" => \$engine, "password=s" => \$password, "user=s" => \$user, "port=s" => \$port, "all" => \$all, ) or showUsage (1); # no need for a $help option here #------Check options----- # Check if host is defined and alive unless (defined $host) { warn "\nNo host specified\n\n"; showUsage (); } # That exits, so no need for an else print "\nChecking if $host is alive.. \n"; Net::Ping->new ()->ping ($host) or die "$host is unreachable\n"; print "$host is alive\n\n";

Enjoy, Have FUN! H.Merijn


Comment on Re: Review of my script
Select or Download Code
Replies are listed 'Best First'.
Re^2: Review of my script
by Jackiejarvis (Novice) on May 15, 2013 at 08:28 UTC

    Thank you! I appreciate any kind of feedback/suggestion. I would change my code!

    Other feedback is always welcome!

Log In?
Username:
Password:

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

How do I use this? | Other CB clients
Other Users?
Others pondering the Monastery: (11)
As of 2015-07-07 23:14 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    The top three priorities of my open tasks are (in descending order of likelihood to be worked on) ...









    Results (93 votes), past polls