Can you please help me in optimizing the code given below.

Sure, but clean up your code first then re-post it. As is, it is almost unreadable.

• Add use strict; and eliminate these warnings:
  Read more... (6 kB)

  That will save everybody here from having to track down that the reason for this:

  Global symbol "$sourcefile" requires explicit package name at - line 1
  +58.
  [download]

  Is because of lines like this:

  open(OLD,"/inside29/urchin/test/logfiles/$filename") || die ("Cud not 
  +open the $sourcefile");
  [download]

  and this

  open(OLD,"consolidatedlog.txt") || die ("C not open the $sourcefile");
  [download]

  It is quite handy if your error messages refer to the same files as those that cause the errors!

  And sorry for mentioning this, but what is all this?

  my $yesterday = time() - ( 24 * 60 * 60 ); ## Why use 'my' here ######
  +###########
  
  ## And not here -v-v-v-v-v-v-v-v- #####################
  ($sec,$min,$hour,$mday,$mon,$year,$wday,$yday,$isdst) = localtime($yes
  +terday);
  $year += 1900;
  $mon++;
  $d = "$mday"; ## <<< Why copy $mday to $d? And why quote "$mday"?  ###
  +######################
  if ($d <10) {
          $d = "0$d"; ## <<< What does this do? #################
  }
  $m ="$mon";
  if ($m<10) {
          $m = "0$m"; ## <<< And this? ####################
  }
  $yesterday = "$year" . "$m" . "$d"; ## Why all those "s? #######
  [download]

  Please don't take this to heart, but asking us to optimise your code, when it is so ... um ... messy, is rather pushing your luck a little.

  ---

  Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.

  Lingua non convalesco, consenesco et abolesco.

  Rule 1 has a caveat! -- Who broke the cabal?

As mentioned it is hard to read your code, it is long, it does not use strict and warnings, it does not adhere to the smallest possible test case ideal. That said a couple of hints to tidy things up.

It does look like you are taking each business and then comparing all the file entries against it before reading the next business off the array. This means you are reading the input files $number_of_business times. Reading the file is slow, itterating through an array held in memory is fast.

• For getting a formatted date/time consider use POSIX; and the strftime function that provides.

perl -MPOSIX -le'print strftime "%X %x", localtime(time)'
12:28:59 18/03/05
[download]

• If you need to match each line against all those businesses consider building an array of pre-compiled regex using the qr command and then read each line in turn and run it against each compiled regex 'till you get a match (assuming only one business can match each line)

Hopefuly this code can give you a couple of ideas ...

#!/usr/bin/perl
use warnings;
use strict;

my @businesses=(
                ['foo', 'bar', 'baz'],
                ['een', 'twee', 'drie'],
                ['ichi', 'ni', 'san'],
                ['hydrogen', 'helium', 'lithium'],
);
my %regexen;

foreach my $group (@businesses) {
    print "making regex from group @{$group} ... ";
    my $regex=join "|", @$group;
    print "\\$regex\\\n";
    my $compiled_re=qr/$regex/;
    $regexen{$regex}=$compiled_re;
}

while (my $line = <DATA>) {
    for my $group (keys %regexen) {
        next unless $line =~ /$regexen{$group}/;
        print "The line $line matched the bussiness group $group\n";
    }
}

__DATA__
nosuch
foo
this
that
helium
ballon
ichi
foot
een
[download]

Cheers,
R.

#!/usr/bin/perl
use strict;
use warnings;
use FileHandle;

# create a hash of businesses and their target files
my %businesses =
(
    "corp.home.ge.com"         => "new_corp_home_ge_com.log",
    "scotland.gcf.home.ge.com" => "new_scotland_gcf_home_ge_com.log",
    "marketing.ge.com"         => "new_marketing_ge_com.log",
);

#loop the data
while (<DATA>)
{
    # check if we have a line that contains a http:// address
    # and store that in $1
    #\" is just to prevent editor from screwing syntax higlight
    if ( m-http://([^/\"]+)- )
    {
        # do we have a entry in the hash for that business
        if ( $businesses{$1} )
        {
            #yes. so create a filehandle in the hash for writing to th
+at
            unless ( ref $businesses{$1} )
            {
                my $fh = new FileHandle;
                $fh->open (">$businesses{$1}");
                $businesses{$1} = $fh;
            }

            #print to the business' filehandle
            $businesses{$1}->print ($_);
        }
        else
        {
            #no so emmit a warning
            warn "unknown business in logfile";
        }
    }

}
__DATA__
16/Jan/2005:00:00:40 -0500 "GET /ge/ige/1/1/4/common/cms_portletview2.
+html HTTP/1.1" 200 1702 0 "http://marketing.ge.com/portal/site/insura
+nce/" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" "-" "marke
+ting.ge.com"
16/Jan/2005:00:00:40 -0500 "GET /portal/site/transportation/menuitem.8
+c65c5d7b286411eb198ed10b424aa30/ HTTP/1.1" 200 7596 0 "http://geae.ho
+me.ge.com/portal/site/transportation/" "Mozilla/4.0 (compatible; MSIE
+ 5.5; Windows NT 5.0)" "-" "geae.home.ge.com"
16/Jan/2005:00:00:41 -0500 "GET /ge/ige/26/83/409/common/cms_portletvi
+ew.html HTTP/1.1" 200 7240 0 "http://marketing.ge.com/portal/site/ins
+urance/" "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" "-" "ma
+rketing.ge.com"
[download]

    print $businesses{$1} $_;

    #yields: 
    #Scalar found where operator expected at C:\t.pl line 36, near "} 
+$_"
    #(Missing operator before  $_?)
[download]

Hi Holli,
I tried your code with little modification and it was
really helpful.Thanks a lot. But i am stuck.
I have some lines having https:// and some lines having no
http:// at all.
I tried out some combimations of reg ex,but no luck.
Please advise.
Thanks

I just noticed the address is also at the very end of every string, so:

m-"([^\"]+)"$-
[download]

will do.


holli, /regexed monk/

If you do not need to know if it was https you can reduce them all to http:// when you read each line in with something like s!https://!http://!; just after the while(<DATA>) {. Note the use of ! as a regex delimiter stops you having a quoting nightmare with the //

Cheers,
R.

Pereant, qui ante nos nostra dixerunt!

Just an asside for the OP. If you are just starting out with Perl don't get into the habit of saying "use FileHandle". The FileHandle module was replaced by IO::File some considerable time ago.

NOTE: This class is now a front-end to the IO::* classes.

(from the POD of FileHandle.pm)


holli, /regexed monk/

my $log = '/var/log/httpd-access.log';
open my $fh, "/usr/bin/egrep 'corp.home.ge.com|scotland.gcf.home.ge.co
+m' $log |") or die "egrep failed on $log: $!";
while ( <$fh> ) {  ... }
close ($fh);
[download]

use strict;
use warnings;

my @b = ( "corp.home.ge.com",
          "scotland.gcf.home.ge.com",
          "marketing.ge.com",
          "home-school.com" );
my %b; $b{$_} = 0 for @b;

for (<DATA>) {
    $_ = lc $_;
    m/^https?:\/\/(?:www.)?(.*?)[\/\n]/;
    $b{$1}++ if exists $b{$1};
}

print "$_ => $b{$_}\n" for @b;

__DATA__
http://corp.home.ge.com/page/whatever.php3
https://scotland.gcf.home.ge.com
http://sub.marketing.ge.com/
HTTP://marketing.ge.com/
http://www.home-school.com/mypage.html
http://home-school.com/mypage.html
https://MARKETING.ge.com/testpage.html
[download]

#!/usr/bin/perl

use strict;
use warnings;

#-------------------------------------------------------

sub generate_date_str ($) {
    my ($time) = @_;

    my ($mday,$mon,$year) = (localtime($time))[3,4,5];
    $year += 1900;
    $mon++;

    return sprintf("%04d%02d%02d", $year, $mon, $mday);
}

#-------------------------------------------------------

sub get_matching_filenames ($$) {
    my ($dir, $match_str) = @_;

    opendir(DIR, $dir) or die "couldn't open directory \"$dir\"";
    my @names = grep {/$match_str/} readdir(DIR);
    closedir(DIR);

    return @names;
}

#-------------------------------------------------------

sub consolidate_logs ($$$) {
    my ($destination_file, $dir, $filename_str) = @_;

    my @files = get_matching_filenames($dir, $filename_str);

    open(OUT,"> $destination_file") or die "Could not open file \"$des
+tination_file\" for writing";

    foreach my $source_file (@files) {
        print "Processing of log \"$source_file\" started at " . local
+time() . "\n";

        open(OLD,"< $dir/$source_file") or die("Could not open file \"
+$dir/$source_file\" for reading");
        while (<OLD>) {
            print OUT $_;
        }
        close(OLD);

        print "Processing of log \"$source_file\" ended at " . localti
+me() . ".\n";
    }

    close(OUT);
}

#-------------------------------------------------------

sub split_logs ($$$) {
    my ($source_file, $business_list, $filename_prefix) = @_;

    foreach my $business (@$business_list) {
        my ($domain, $file) = @$business;

        my $outfile = "/inside29/urchin/test/newfeed/$filename_prefix-
+$file";
        my $newigebusiness = $domain;

        print  "Creating of log for $newigebusiness started at " . loc
+altime() . "\n";

        open(OUT,">> $outfile") || die("Could not open out file \"$out
+file\" for appending");
        open(OLD,"< $source_file") || die ("Could not open the consoli
+dated file \"$source_file\" for reading");
        while (<OLD>) {
            if ((index($_,$newigebusiness))> -1) { 
                print OUT $_;
            }
        }
        close(OLD);
        close(OUT);

        print "Log for $newigebusiness created at " . localtime() . "\
+n";
    }
}

#-------------------------------------------------------

my @businesses = (
 [ "\"corp.home.ge.com\"", "new_corp_home_ge_com.log" ],
 [ "\"scotland.gcf.home.ge.com\", "new_scotland_gcf_home_ge_com.log" ]
+,
 [ "\"marketing.ge.com\"", "new_marketing_ge_com.log" ]
);

my $consolidated_log = "consolidatedlog.txt";
my $logfiles_dir = '/inside29/urchin/test/logfiles';

my $today = generate_date_str( time() );
my $yesterday = generate_date_str( time() - (24 * 60 * 60) );

consolidate_logs($consolidated_log, $logfiles_dir, $yesterday);

split_logs($consolidated_log, \@businesses, $today);
[download]

consolidate_logs function could be then optimized to this

sub consolidate_logs ($$$) {
    my ($destination_file, $dir, $filename_str) = @_;

    my @files = get_matching_filenames($dir, $filename_str);

    open(OUT,"> $destination_file") or die "Could not open file \"$des
+tination_file\" for writing";

    foreach my $source_file (@files) {
        print "Processing of log \"$source_file\" started at " . local
+time() . "\n";

        system("cat $dir/$source_file >> $destination_file");

        print "Processing of log \"$source_file\" ended at " . localti
+me() . ".\n";
    }

    close(OUT);
}
[download]

(using "cat" program instead of perl code for simply transferring large quantities of data)
or even to this

sub consolidate_logs ($$$) {
    my ($destination_file, $dir, $filename_str) = @_;

    system("ls $dir | grep $filename_str | xargs -iX cat $dir/X >> $de
+stination_file");
}
[download]

split_logs function could be simplified to this

sub split_logs ($$$) {
    my ($source_file, $business_list, $filename_prefix) = @_;

    foreach my $business (@$business_list) {
        my ($name, $file) = @$business;

        my $outfile = "/inside29/urchin/test/newfeed/$filename_prefix-
+$file";

        print  "Creating of log for $name started at " . localtime() .
+ "\n";

        system("grep \"$name\" $source_file >> $outfile");

        print "Log for $name created at " . localtime() . "\n";
    }
}
[download]

again - using external program ("grep" this time) for simple string matching but in big quantities of data.

bartek