Re: Efficiency in code to cycle log file

From: Jim Gibson (
Date: 07/29/04

Date: Wed, 28 Jul 2004 17:09:07 -0700

In article <UwVNc.39538$>, Domenico Discepola
<> wrote:

> Hello. I wrote a subroutine that creates log file entries in a text file
> using the following format: script name version date time status message. A
> sample entry would be:
> script v1.0.2 2004/07/12 12:29:18 (I): Message 1 here
> script v1.0.2 2004/07/12 12:29:48 (I): Message 2 here
> etc.
> The important part is that the first 5 "fields" are space seperated whose
> positions will not change. I now wanted to cycle the log file if the date
> of the log file entry was greater than a certain value, based on today's
> date. In other words, delete all log file entries older than X days. The
> code I posted works well enough for me but it is not very elegant. Right
> now, the script simply creates a "temporary" file with the log file entries
> I want to keep, leaving the original file untouched. I would consider it a
> learning experience if someone can provide me with some suggestions for
> tightening the code.

I can offer up some suggestions for 'improving' your program that, if
you had been reading this group for a few months, you would already
have read several times.

> #!perl
> use strict;
> use warnings;
> use diagnostics;
> use Date::Pcalc qw( Delta_Days );
> our $g_file_log = "c:\\work\\jps.log";
> our $g_file_log_tmp = "c:\\work\\jps.log.tmp";

You are better off using single quotes here, because you are not doing
any interpolation of variables. With single quotes, you do not need to
escape the backslashes. Even better, use forwardslashes as perl lets
you do so.

> sub cycle_log {
> my ( $cycle ) = @_;

You may also use:

   my $cycle = shift;

> my ( $sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $isdst) =
> localtime(time);
> #Because year is 2 digits and month starts at 0

Actually, for the past four years $year has been 3 digits. :)

> $year += 1900;
> $mon += 1;
> open(CFILE, "< ${g_file_log}") || die "Cannot open ${g_file_log} file\n";

Better: use the 3 argument version of open and don't put the file name
in quotes. Add the reason the open failed to the die statement (special
variable $!) and delete the "\n" to get more info.

You should also open the output file here, too (see below).

> while (<CFILE>) {

here (see below).

> my @a = split / +/, $_;

Better would be: my @a = split;

i.e., use the defaults that perl gives you.

> my ( $iyyyy, $imm, $idd ) = split /\//, $a[2];

Use alternate delimiters on a regular expression to avoid having to
escape special characters:

   ... = split m|/|, $a[2];

You could also use the unpack function to extract the fixed-width,
fixed-location fields in one step. Something like (untested):

   my( $y, $m, $d ) = unpack('x14 A4 x A2 x A2', $_;

> my $diff = Date::Pcalc::Delta_Days( $iyyyy, $imm, $idd , $year, $mon,
> $mday, );
> if ( $diff >= $cycle ) {
> #print "Will be deleted because > $cycle days: $iyyyy, $imm, $idd \n";
> next;
> } else {
> open (my $file, ">>${g_file_log_tmp}") || return 2;

Do you really want to reopen the file each time? That seems terribly
inefficient! Why not open the file before the loop?

> ## code improvement part here ####
> my $string;
> foreach my $el ( @a ) {
> chomp ( $el );

Only the last element of the @a array will have a newline at the end.
Chomping all of the other elements is unnecessary and wasteful. Add the
chomp above and remove this one.

> $string = $string . "${el} ";

You don't need the braces. "$el " works fine.

> }
> $string =~ s/(^.+)(\s$)/$1/;

There is an easier way to trim blanks from the end of a line that
doesn't require capturing, evaluating an expression, anc substituting:

   $string =~ s/\s*$//;

> $string = $string . "\n";
> print $file $string;

You don't really need to decompose the line and then put it back
together to print it. You should probably replace your loop with:

   while( my $line = <CFILE> ) {
      ... # extract $diff from $line
      print $line if $diff >= $cycle;

> close $file;

Since you are now going to open the file before the loop, move the
close to after the loop, and check to see if the close works (usually
does, but checking it puts you in the top percentile of paranoid
programmers! :)

> ###################################
> }
> }
> close (CFILE) || die "Cannot close $g_file_log file\n";

Well, there you go. You are already in that top percentile! But
wouldn't you want to know the reason such a rare event occurred? (add
$! to your error message).

> }
> sub main {
> &cycle_log( 2 );
> }
> main();

Perl is not C. You don't need to define a main subroutine. Execution
will start with the first non-subroutine statement (usually).

> exit 0;

Exits are not required either, but they don't hurt.

I am sure others could offer even better suggestions, but I tried to
show you the easy stuff.