Beefy Boxes and Bandwidth Generously Provided by pair Networks
Problems? Is your data what you think it is?
 
PerlMonks  

check my code

by TechFly (Scribe)
on Jul 14, 2010 at 02:39 UTC ( #849408=perlquestion: print w/ replies, xml ) Need Help??
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`"); }

Comment on check my code
Download Code
Re: check my code
by jwkrahn (Monsignor) on Jul 14, 2010 at 03:11 UTC
    my $confpath; my $servername; my $filepath; my $filemaxage; $confpath = "/export/mksysb"; $filepath = "/export/mksysb"; $filemaxage = "360";

    The usual practice is to define and initialize variables in the same step:

    my $confpath = '/export/mksysb'; my $filepath = '/export/mksysb'; my $filemaxage = 360;

    By the way, $confpath and $filepath have the same value so is there really a need for two variables?

    while(<FILE>){ chomp($servername = $_);

    That is usually written as:

    while ( my $servername = <FILE> ) { chomp $servername;
    if (-M $_ > "$filemaxage"){

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

    You should also check for failure with mkdir and system like you already do with open and unlink.

Re: check my code
by toolic (Chancellor) 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 +.

    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

    ... then from the command line:

    perldoc mksysb.pl
      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;
      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.)
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 ); }
    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)

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>){ ...
    versus
    open( my $fh, '<', "$confpath/mksysbmachinelist.conf" ) or die $!; while( my $line = <$fh> ) { ...

    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
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.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://849408]
Approved by toolic
Front-paged by tye
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others examining the Monastery: (8)
As of 2014-11-27 00:40 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My preferred Perl binaries come from:














    Results (178 votes), past polls