If that script were 20-40 lines, posting it here (not as a NON-linking URL -- see Markup in the Monastery -- at pastebin) would be appropriate.
Since it's 80-some lines, you'd do better to pick a section about which you're particularly concerned, to post (as a runnable snippet) here.
We're here to help you learn; not to provide free programming services*1, which is fundamentally what you've asked for. See On asking for help and the other basic Q&A in PerlMonks FAQ.
One direct comment: You start out using numeric values ($countf=0;) for example, but then proceed to use an alpha test, ge, on $countf at line 56, and revert to a numeric test in 59.
Perl's DWIM factor will often save you from goofs like that... but you can't count on it forever.
Update: Fixed some punct and quoted code.
*1 ...or -- for the next 20 minutes only -- take advantage of the special offer of free programming services (except for a modest $2k registration fee, taxes, shipping, handling and modest surcharges) at ww.inc. :-)
Update:
OP revised since answers started appearing leaving several to appear out of contest. Author has been advised that doing so removes context for several replies (including this one).
Original post included only a pastebin address for code,
http://pastebin.com/P5mK7k5G (original was * NOT * a link) which was very different from what's now posted.
In that code, lines 54-59 read:
# Changes subject based on amount of files found, over 4000 will sugge
+st a possible error.
my $subject = '';
if ($fCount ge '4000') {
$subject = '[POSSIBLE ERROR] Import has '.$fCount.' regular fi
+les and '.$fCountL.' large files waiting to be processed!';
}
elsif ($fCount < '4000') {
Thanks to planetscape for pointing this out.
| [reply] [d/l] [select] |
| [reply] |
shadowfox:
First, if you use code tags, then the script will be easy to read. Second, this thread would become useless if/when that link dies--so please edit the node and insert your code to make the link more useful. This is a learning site, after all, and we encourage people to search the site for answers to their problems.
...roboticus
When your only tool is a hammer, all problems look like your thumb.
| [reply] |
No need to use pastbin. You can add your code here just fine using code-tags (and readmore-tags)...
Some first comments:
use variable names which mean something, also to others :)
use consistent indentation in your code to improve readability
use lexical file and directory handles
this:
# Sorts files in $dir and sets $latest->{file} with newest file.
my $latest = (sort {$b->{mtime} <=> $a->{mtime}}
map {{mtime => -M $_, file => $_}}
<$dir/*>)[-1];
my $newM = (stat $latest->{file})[9];
# Sorts files in $dir and sets $oldest->{file} with oldest file.
my $oldest = (sort {$a->{mtime} <=> $b->{mtime}}
map {{mtime => -M $_, file => $_}}
<$dir/*>)[-1];
my $oldM = (stat $oldest->{file})[9];
to determine the oldest and newest file, could easier be writen as:my ($newM, $oldM) = (sort {-M $a <=> -M $b} <./*>)[0,-1];
No need for maps etc. (oh, and better use glob...)
HTH,
| [reply] [d/l] [select] |
Thanks for the tip about the code tags, I ignorantly used
[code][/code] to start with and it didn't work so I pastbined it here to avoid cluttering the front page. Good call on the readmore tags though, it made me realize the tag syntax I was using was the problem, I should have investigated it further.
Anyway, I shorted it down to the part of concern, there probably isn't much that can be improved to the smtp email sending and logging so I took it out from the example. Thanks for the comments so far, I'll make some reflected changes tomorrow and report back with an update.
| [reply] [d/l] |
# Sorts files in $dir and sets $latest->{file} with newest file and se
+ts $oldest->{file} with oldest file.
my ( $latest, $oldest ) = (
sort { $a->{ mtime } <=> $b->{ mtime } }
map { { mtime => -M $_, file => $_ } }
<$dir/*>
)[ 0, -1 ];
my ( $newM, $oldM ) = map +( stat )[ 9 ], $latest->{ file }, $oldest->
+{ file };
# Opens $dir and counts number of files
opendir(DIR, $dir);
LINE: while(my $FILE = readdir(DIR)) {
next LINE if($FILE =~ /^\.\.?/);
$fCount++;
}
closedir(DIR);
# Opens $dirL and counts number of files
opendir(DIRL, $dirL);
LINE: while(my $FILE2 = readdir(DIRL)) {
next LINE if($FILE2 =~ /^\.\.?/);
$fCountL++;
}
closedir(DIRL);
You should always verify that opendir worked correctly. And, you don't include the directory name in your regular expression test so you are testing the wrong files and you can use grep to get your counts:
# Opens $dir and counts number of files
opendir my $DIR, $dir or die "Cannot opendir '$dir' because: $!";
my $fCount = grep "$dir/$_" !~ /\A\.\.?\z/, readdir $DIR;
closedir $DIR;
# Opens $dirL and counts number of files
opendir my $DIRL, $dirL or die "Cannot opendir '$dirL' because: $!";
my $fCountL = grep "$dirL/$_" !~ /\A\.\.?\z/, readdir $DIRL;
closedir $DIRL;
| [reply] [d/l] [select] |
Hey sorry i got busy with other tasks, only just had a chance to get back on this. So I tried the grep suggestion but i had a problem with the count of files in the directory being 2 more than were actually there, even when its empty? I wasn't sure why this would happen, there are no hidden or system files or sub directries in this windows directory. So I tried another approach, as you can see below. Will grep save time and how can I manage the output to ignore the 2 'ghost' items its counting?
Also I had to add in a bit to check for .lock files that are created when import jobs start, I was wondering if this can be consened also? I'm curious if anyone has some reading that pertains to what types of things can be run in the same "query" for lack of a better term, cause I end up doing things seperately for each occourance I need for lack of understanding.
# Looks in $dir and sets $latest->{file} with newest file and sets $ol
+dest->{file} with oldest file.
my ( $latest, $oldest ) = (
sort { $a->{ mtime } <=> $b->{ mtime } }
map { { mtime => -M $_, file => $_ } }
<$dir/*>
)[ 0, -1 ];
my ( $newM, $oldM ) = map +( stat )[ 9 ], $latest->{ file }, $oldest->
+{ file };
# Looks in $dirL and sets $latestL->{file} with newest file and sets $
+oldestL->{file} with oldest file.
my ( $latestL, $oldestL ) = (
sort { $a->{ mtime } <=> $b->{ mtime } }
map { { mtime => -M $_, file => $_ } }
<$dirL/*>
)[ 0, -1 ];
my ( $newML, $oldML ) = map +( stat )[ 9 ], $latestL->{ file }, $oldes
+tL->{ file };
# Opens $dir and counts number of files
my @files = <$dir/*>;
my $fCount = @files;
# Opens $dirL and counts, large files are processed seperately thus $d
+irL
my @filesL = <$dirL/*>;
my $fCountL = @filesL;
# Changes Newest/Oldest file display in email based on if any files ar
+e found.
if (defined($latest->{file})){
$latestF = 'Newest File: '. $latest->{file}. ' with a timestamp of
+ '. scalar localtime $newM;
$oldestF = 'Oldest File: '. $oldest->{file}. ' with a timestamp of
+ '. scalar localtime $oldM;
}
# Changes Newest/Oldest file display in email based on if any large fi
+les are found.
if (defined($latestL->{file})){
$latestFL = 'Newest File: '. $latestL->{file}. ' with a timestamp
+of '. scalar localtime $newML;
$oldestFL = 'Oldest File: '. $oldestL->{file}. ' with a timestamp
+of '. scalar localtime $oldML;
}
# Looking for the lock files generated when pLock(job1) job starts to
+see when import started.
my $pLock ='\\\\server\\E$\\directory1\\job1.lock';
my $pLockStat='No lock file for Job1 was found, this import is not cur
+rently running.';
if ( -f $pLock ) {
my @time = timeconv( -M $pLock );
my $pLockWhen=$time[1].' hours, '.$time[2].' minutes, and '.$
+time[3].' seconds ago';
$pLockStat='Lock file found for Job2, this import was started
+ '. $pLockWhen;
}
# Looking for the lock files generated when eLock(job2) job starts to
+see when import started.
my $eLock ='\\\\server\\E$\\directory1\\job1.lock';
my $eLockStat='No lock file for Job2 was found, this import is not cur
+rently running.';
if ( -f $eLock ) {
my @time2 = timeconv( -M $eLock );
my $eLockWhen=$time2[1].' hours, '.$time2[2].' minutes, and '
+.$time2[3].' seconds ago';
$eLockStat='Lock file found for Job2, this import was started
+ '. $eLockWhen;
}
| [reply] [d/l] |