Hello,
Howdy.
I have this script to gzip logs in different directories. Can some one check it for me. It works fine, but I want to make sure I did it correctly and the best way.
I think it's excellent that you want a review of working code. People often over look this and it's very important!
I think your script could improve a little, but it's a great start. I'll give some suggestions.
BTW, this is my frist script that I wrote.
Even better. Looking good!
Thanks,
Keith Olmstead
#!/usr/bin/perl
Let's move your 'use warnings;' up here and add 'use strict;'. You want to play by the rules and you want to do it for the whole script.
Your code will need some changes to pass the strictures, but this is VERY important for a good programmer in training! ;) See if you can fix the code yourself, but just come back if you get stuck along the way. Remember that you can look up the rules too with 'perldoc strict' at the command line.
my $startdir = "/opt/log/hosts/";
or
use constant START_DIR => '/opt/log/hosts/';
Not that your way is at all wrong though. Just another way to do things. 'perldoc constant' for more information.
use File::Find;
Using modules already? You're a natural! :D
use warnings; my @dirlist; $date=&getDate;
Call your subroutines like this:
my_subroutine();
Note the lack of an ampersand. Save the ampersand for when you need it, like in the call to find below.
The above doesn't mean exactly what you think it means, though it is working in this case.
@logfile = ("cron","messages","maillog","ldap");
A personal nitpick: Learn the difference between "text" and 'text'. ;)
foreach $log (@logfile) {
A little prettier might be:
for my $log (qw(cron messages maillog ldap)) {
find (\&eachFile, "$startdir");
Don't quote a variable unless you are interpolating it into a string, it's not needed.
}
foreach $file(@log){ system("gzip $file") unless ($file =~ m/$date/); #print "$file done!\n" unless ($file =~ m/$date/); } # End for loop
Nice use of comments on the closing braces. This really helps a lot of people. I know one long time programmer who still does this, though a lot of people do grow out of it. No shame in making your life easier, ever! :)
sub eachFile {
if (-e $_ && $_ =~ /$log$/) { push @log, $File::Find::name;}
Is that really a pattern match in that if, or I we just testing for equality? (I honestly don't know.) If it's just equality though, spell that :
$_ eq $log
} # End eachFile
sub getDate { my ($sec, $min, $hour, $mday, $mon, $year) = localtime(time);
You can drop the 'time' if you want, localtime() works on it by default.
$year = $year + 1900;
$mon++;
if ($mon < 10) {$mon = "0" . $mon;}
if ($mday < 10) {$mday = "0" . $mday;}
if ($hour < 10) {$hour = "0" . $mday;}
if ($min < 10) {$min = "0" . $mday;}
if ($sec < 10) {$sec = "0" . $mday;}
$date = "\/$year\/$mon\/$mday\/";
Yuck. :) Read 'perldoc -f sprintf' and see if you can't shorten this up a lot.
return ($date); } # End getDate
Well, that should be plenty to get you started. Again, nice first effort. Good luck with this and future scripts.
James
-- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
