CGI/Perl Guide | Learning Center | Forums | Advertise | Login
Site Search: in

  Main Index MAIN
INDEX
Search Posts SEARCH
POSTS
Who's Online WHO'S
ONLINE
Log in LOG
IN

Home: Perl Programming Help: Intermediate:
review of my code

 



jackiejarvis
New User

May 15, 2013, 1:37 AM

Post #1 of 4 (541 views)
review of my code Can't Post

Hello everyone, I'm new in perl and I'm a student in IT. I need to write some scripts for my internship. Over 3 weeks there is a jury who judge over the internship and they will see my scripts. But it could be that some of the people are experts in perl or scripting. I would like to ask if some of you will check my script and give some suggestions and feedback. The script is to make a dump of all the databases from remote. The scripts works fine normally. Many thanks for the suggestions!


Code
#!/usr/bin/perl 
# (c) Odeurs Dieter
# Script to back-up the databases

use strict;
use warnings;
use Getopt::Long;
use Net::Ping;
use Time::Local;

# Variables
my $host;
my $engine;
my $user;
my $password;
my $port;
my $cmd;
my @args = ();
my $ping;
my $all;
my $timestamp;
my $filename;
my $help;
my $error;
my $result = "";
my @result;

#-----------------------------------------------
# Show usage
#-----------------------------------------------
sub showUsage{
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 or 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 database(s)\n\n";
print "--port \n";
print " If another port than the standard must be used to connect you could \n";
print " specify this with the --port option\n\n";
exit;
}

# Read Options
GetOptions(
"host=s" => \$host,
"engine=s" => \$engine,
"password=s" => \$password,
"user=s" => \$user,
"port=s" => \$port,
"all" => \$all,
"help" => \$help
);

if ($help){
showUsage();
}

#Creating timestamp
my ($sec, $min, $hour, $day, $month, $year, $wday, $yday, $isdst) = gmtime(time);
$timestamp = sprintf("%4d-%02d-%02d_%02d-%02d", $year + 1900, $month + 1, $day, $hour, $min);

#------Check options-----
# Check if host is defined and alive
if (defined $host){
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();
} else {
print "\nNo host specified\n\n";
showUsage();
}

#If a password is set we need to set PGPASSWORD environment variable
if ($password && $password ne ''){
if ($engine eq "postgres" ){
$ENV{'PGPASSWORD'} = $password;
} else {
push(@args, qq{--password=$password});
}
}

if ($host && $host ne ''){
push(@args, qq{--host $host});
}

if ($port && $port ne ''){
push(@args, qq{--port $port});
}

if ($user && $user ne ''){
if ($engine eq "postgres" ){
push(@args, qq{--username $user});
} else {
push(@args, qq{--user $user});
}
}

#Check if database engine is defined and valid
if (defined $engine) {
# When a engine is defined the trigt sub will be called
if ($engine eq "postgres" ){
postgres();
} elsif ($engine eq "mysql" ){
mysql();
} else {
print "Unknown database engine\n";
showUsage();
}
} else {
print "Please specify the database engine postgres|mysql\n";
showUsage();
}

#------------------------------------------------
# Postgres
#------------------------------------------------
sub postgres{
if ($all) {
$filename = "/home/dieter/dumps/postgres/" . "Fulldump" . "_" . $timestamp . ".sql";
print "Taking dump of all databases...\n";
$cmd = "pg_dumpall @args > $filename";
@result = `$cmd`;
if ($? !=0) {
print "Done but with errors\n\n";
} else {
print "Dump saved in $filename\n\n";
}
} else {
#Find al the databases on the specified host
print "Checking databases.. \n";
$cmd = "psql -At @args -c 'SELECT datname FROM pg_database'";

#Print the non-standard databases and store them in an array
my @databases;
@result = `$cmd`;
if ($? !=0) {
print "Some errors occured..\n\n";
exit;
}
print "Databases to back-up: \n";
foreach $result (@result) {
chomp $result;
if ($result !~ "template" && $result !~ "postgres"){
print " - " . $result . "\n";
push(@databases, $result);
}
}

#Back-up each database
foreach my $database (@databases){
#Generate a filename for each database
$filename = "/home/dieter/dumps/postgres/" . $database . "_" . $timestamp . ".sql";
$cmd = "pg_dump -C @args $database > $filename";
print $cmd . "\n" ;
@result = `$cmd`;
if ($? !=0) {
print "Done but with errors\n\n";
exit;
}else {
print "DONE \n";
}
}
}

}

#-----------------------------------------------
# MySQL
#-----------------------------------------------
sub mysql{
#-------One database in one file------
if ($all) {
#-------All database in one file------
$filename = "/home/dieter/dumps/mysql/" . "Fulldump_" . $timestamp . ".sql";
print "Taking dump of all databases...\n";
$cmd = "mysqldump @args --all-databases > $filename";
@result = `$cmd`;
if ($? !=0) {
print "Done but with errors\n\n";
exit;
}else {
print print "Full dump saved in $filename\n";
}
} else {
#list databases and exclude the default
my @databases;
print "Checking databases..\n";
$cmd = "mysql @args -Bse 'show databases'";
@result = `$cmd`;
if ($? !=0) {
print "Some errors occured..\n\n";
exit;
}
print "Databases to back-up: \n";
foreach $result (@result) {
if ($result !~ "information_schema" && $result !~ "mysql"){
print " - $result \n";
push (@databases, $result);
}
}

push(@args, qq{--single-transaction});
#back-up of the selected databases
foreach my $database (@databases){
chomp $database;
$filename = "/home/dieter/dumps/mysql/" . $database . "_" . $timestamp . ".sql";
print "Taking dump of $database ...\n";
$cmd = "mysqldump @args $database > $filename";
@result = `$cmd`;
if ($? !=0) {
print "Done but with errors\n\n";
exit;
}else {
print "Dump saved in $filename\n";
}
}
}

print "Dump completed!\n\n";
}
exit;



BillKSmith
Veteran

May 15, 2013, 5:34 AM

Post #2 of 4 (534 views)
Re: [jackiejarvis] review of my code [In reply to] Can't Post

Well done! But you did ask for criticism.

Declaring all your lexical variables at the top of the program largely defeats the advantage of "use strict". All lexical variables should be declared in the smallest possible scope. Your use of them as globals in subroutines makes it even worse. It is far better to supply all inputs to a subroutine as arguments.

Your usage section should be a "here document" and a single print.

I would prefer an all perl solution rather than an external program to access the data base. I believe that the DBI module could do the job.

You should consider formatting the time stamp with the function strftime in POSIX.

You have several logic errors. The values undef, numeric zero, and null string are all false. Any other value is true.


Code
if ($? !=0) {


The "!=0" is unnecessary.


Code
if ($password && $password ne ''){


This is even worse. if $password is a null string, it is false and the second test is not performed. On the other hand if it is not, the second test is performed and always passes. I suspect that you mean:


Code
if (defined $password && $password ne ''){


Note: There are several other analagous errors.


Be prepared to defend your style of indenting and nesting. It is not my first choice, but all that really matters is that you are consistent. You are.
Good Luck,
Bill


jackiejarvis
New User

May 15, 2013, 5:40 AM

Post #3 of 4 (532 views)
Re: [BillKSmith] review of my code [In reply to] Can't Post

Thank you for the feedback!
I would change some things in script !
I like any kind of feedback I'm new to perl so I only can learn new things to get a better code


FishMonger
Veteran / Moderator

May 15, 2013, 7:22 AM

Post #4 of 4 (526 views)
Re: [jackiejarvis] review of my code [In reply to] Can't Post

See my answer on your cross post at http://forums.devshed.com/perl-programming-6/review-of-my-script-945218.html#post2877605

 
 


Search for (options) Powered by Gossamer Forum v.1.2.0

Web Applications & Managed Hosting Powered by Gossamer Threads
Visit our Mailing List Archives