Beefy Boxes and Bandwidth Generously Provided by pair Networks
Think about Loose Coupling

Re: Review of my script

by Tux (Abbot)
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

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?

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://1033635]
NodeReaper adjusts the cross hairs

How do I use this? | Other CB clients
Other Users?
Others making s'mores by the fire in the courtyard of the Monastery: (4)
As of 2018-03-19 01:47 GMT
Find Nodes?
    Voting Booth?
    When I think of a mole I think of:

    Results (231 votes). Check out past polls.