check my code

TechFly has asked for the wisdom of the Perl Monks concerning the following question:

I have been writing Perl for a relatively short time, and am just starting to make code that is useful. I wrote the following script to create mksysbs for my unix boxes. It runs from a central location on a NIM box, and creates the mksysbs. It checks to see if any of the mksysbs are old, and deletes them if they are. I humbly submit my code in hopes that someone can take a look and offer pointers. A better way to do things, or a different way, or a more efficient way, or anything that occurs to you that a perl initiate should know. Thanks.

# ====================================================================
+==
#
# NAME: mksysb.pl
#
# AUTHOR: TechFly
# Email: TechFlyG<a~t>Gmail
# DATE  : 7-13-2010
#
# PURPOSE: Create an mksysb of servers listed in $path\mksysb.conf.
#          Also, purge all mksysb's that are over an age set by the 
#          filemaxage parameter.  
#
# ====================================================================
+==

use strict;
use warnings;
my $confpath;
my $servername;
my $filepath;
my $filemaxage;

$confpath = "/export/mksysb";
$filepath = "/export/mksysb";
$filemaxage = "360";

open(FILE, '<', "$confpath/mksysbmachinelist.conf") or die $!;
while(<FILE>){
    chomp($servername = $_);
    if (-e "$filepath/$servername") {
        print("\n\n$servername\n");
        }else{
        print("\n\n$servername does not exist\n");
        mkdir("$filepath/$servername");
        }
        
    foreach(<$filepath/$servername/$servername*>){
        print "$filemaxage";
        if (-M $_ > "$filemaxage"){
            print("   $_ will not be purged\n");
            }else{
            unlink($_) or print ("Cannot delete file $!");}
        }
system("nim -o define -t mksysb -a server=master -a mk_image=yes -a lo
+cation=$filepath/$servername/$servername\_`date +%m%d%Y` -a source=$s
+ervername $servername\_`date +%m%d%Y`");
    }
[download]

Comment on check my code Download Code

Replies are listed 'Best First'.
Re: check my code by jwkrahn (Abbot) on Jul 14, 2010 at 03:11 UTC
`my $confpath; my $servername; my $filepath; my $filemaxage; $confpath = "/export/mksysb"; $filepath = "/export/mksysb"; $filemaxage = "360";` [download] The usual practice is to define and initialize variables in the same step: `my $confpath = '/export/mksysb'; my $filepath = '/export/mksysb'; my $filemaxage = 360;` [download] By the way, `$confpath` and `$filepath` have the same value so is there really a need for two variables? `while(<FILE>){ chomp($servername = $_);` [download] That is usually written as: `while ( my $servername = <FILE> ) { chomp $servername;` [download] `if (-M $_ > "$filemaxage"){` [download] You are converting `$filemaxage` to a string and then Perl has to convert it back to a number in order to do a numerical comparison. What's wrong with always quoting "$vars"? mkdir("$filepath/$servername"); system("nim -o define -t mksysb -a server=master -a mk_image=yes -a lo +cation=$filepath/$servername/$servername\_`date +%m%d%Y` -a source=$s +ervername $servername\_`date +%m%d%Y`"); [download] You should also check for failure with mkdir and system like you already do with open and unlink.	[reply] [d/l] [select]
Re: check my code by toolic (Bishop) on Jul 14, 2010 at 03:12 UTC
You can use the perlcritic tool to automatically check your code for best practices. It will point out some tips such as: `Useless interpolation of literal string at line 22, column 13. Bareword file handle opened at line 26, column 1. Glob written as <...> at line 36, column 10. See page 167 of PBP. Readline inside "for" loop at line 36, column 10. See page 211 of PBP +.` [download] You can also convert your comments to POD and get a manpage for free: `=head1 PURPOSE: Create an mksysb of servers listed in $path\mksysb.conf. Also, purge all mksysb's that are over an age set by the filemaxage parameter. =cut` [download] ... then from the command line: `perldoc mksysb.pl` [download]	[reply] [d/l] [select]
Re^2: check my code by graff (Chancellor) on Jul 14, 2010 at 05:09 UTC
Right ++! The template that I use for starting all my perl source files is something like this, which is pretty much what you see in most CPAN modules: `#!/usr/bin/perl =head1 NAME =head1 SYNOPSIS =head1 DESCRIPTION =cut use strict;` [download] I normally fill in the POD sections before I write any actual code. (This helps me establish and maintain focus, which tends to make the code easier and quicker to write.)	[reply] [d/l]
Re: check my code by graff (Chancellor) on Jul 14, 2010 at 05:27 UTC
If there has to be a big long system call like that, I'd be inclined to make it as non-repetitive as possible -- build it up as an array of args, and use the list form of the system call: `use POSIX; ... my $datedname = join('_', $servername, strftime("%m%d%Y", localtim +e()); my %opts = ( server => 'master', mk_image => 'yes', source => $servername, location => "$filepath/$servername/$datedname", ); my @cmd = qw/nim -o define -t mksysb/; for my $o ( keys %opts ) { push @cmd, '-a', "$o=$opts{$o}" } push @cmd, $datedname; print " command-line: @cmd\n"; system( @cmd ); }` [download] You might want to get just one date string to use on all iterations of the loop -- assign the strftime string to a variable before going into the loop, and then append that variable to each server name. (updated code snippet to add a missing quotation mark)	[reply] [d/l]
Re: check my code by derby (Abbot) on Jul 14, 2010 at 10:32 UTC
All good comments so far but you should also use lexical filehandles instead of barewords: `open(FILE, '<', "$confpath/mksysbmachinelist.conf") or die $!; while(<FILE>){ ...` [download] versus `open( my $fh, '<', "$confpath/mksysbmachinelist.conf" ) or die $!; while( my $line = <$fh> ) { ...` [download] The reasons for preferring lexical filehandles instead of bareword are barewords are really package variables and those have their own issues if another file is all ready open under that package variable, it will be closed. if you also have a subroutine named the same, your code will fail silently. Lexical filehandles have been around since perl 5.6 (2000) but it's only recently (perl 5.8.9) that the perldoc has been updated to reflect that lexicals are really the way to go. -derby	[reply] [d/l] [select]
Re: check my code by TechFly (Scribe) on Jul 14, 2010 at 21:14 UTC
Thanks for the advice guys. I modified the script to use many of the comments.	[reply]