On 8/2/07, Mihir Kamdar <[EMAIL PROTECTED]> wrote:

> #!/usr/bin/perl
>
> my $child_path =
> '/home/user71/RangerDatasource/Customization/TelekomMalaysia/Scripts/Tests/cprogs/files/child'
> ;
> my $parent_path =
> '/home/user71/RangerDatasource/Customization/TelekomMalaysia/Scripts/Tests/cprogs/files'
> ;
> my $write_path =
> '/home/user71/RangerDatasource/Customization/TelekomMalaysia/Scripts/Tests/cprogs/files/output'
> ;
> my $line;

This declaration is being done too soon.  Variables should have the
smallest possible scope.  $line is being used only with a while loop
below.  It should be declared then.

> my %hash;
> my @file;

@file is never used

> my $key ;
> my $value ;

These variables are also being declared too soon.

> my %times ;
> while () {

Infinite loops are a bad idea.  It is better to give it an exit
condition that can be triggered by a signal.  The following code will
exit if the program receives either SIGINT or SIGTERM (control-c or
kill -15):

my $continue = 1;
$SIG{INT} = $SIG{TERM} = sub { $continue = 0 };
while ($continue) {
    print localtime() . "\n";
    sleep 1;
}

This also brings up another problem with your code: you don't sleep
inside the loop.  This will cause your code to consume all available
CPU cycles.  This is generally considered not a good idea.

> opendir my $dh, $child_path or die $!;
>         while (my $file = readdir $dh) {
>                 my $fname = "$child_path/$file";
>                 next unless -f $fname;
>                         unless (exists $times{$file}){
>                                 opendir my $dh1,$parent_path or die $!;
>                                 open (my $IN_FILE,"<","$parent_path/$file")
> or die $!." Parent file not found for $parent_path/$file";
>                                         while ($line=readline($IN_FILE))
>                                         {
>                                         my @cdr=split (/,/, $line) ;
>
> $hash{$cdr[2],$cdr[3],$cdr[6],$cdr[7]}=$line;  #Add some more cdr key fields
> if u want.

This is not a good idea.  The correct way to do this is either
    $hash{$cdr[2]}{$cdr[3]}{$cdr[6]}{$cdr[7]} = $line;
or
    $hash{"@cdr[2,3,6,7]"} = $line;

>                                         }
>                                         close $IN_FILE ;
>                         closedir $dh1 ;
>                 open (my $OUT_FILE,">","$write_path/$file.out") or die $!;
>                 while (($key, $value) = each %hash)
>                         {
>                         print $OUT_FILE $value;
>                         }
>                         close $OUT_FILE;
>                 }
>                 unlink("$parent_path/$file") ;
>                 unlink("$child_path/$file") ;
>         }
> closedir $dh;
> }
>
> Please comment on the above code and tell me if it has any potential
> pitfalls. Right now I am able to test this for just few sample files. But in
> real environment, it has to pick up millions of files, process them and
> write the output.
>
> Your comments will be highly appreciated.
>
> Thanks,
> Mihir
>

-- 
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
http://learn.perl.org/


Reply via email to