Embarrassed Update

Hmm, I really should learn to hit SUBMIT. :-) When I wrote this there was only Boo_Radleys reply, but I got called away to a meeting and apparently hit the wrong button as I left. So ive covered everybody elses points over again. Sorry about that.

---

Weel, lets see, first lets indent this mess...

open DOM_WORK, '>/usr/spool/lpd/dom/dom.work';
&get_text;                                   
sub get_text{                                
  my @text=(<>);                               
  my $line =0;                                 
  foreach $line (@text) {                      
    my $text =$text[$line];                      
    $text =~tr/\x0c-\x0d//d;                     
    select DOM_WORK;                             
    print "$text[$line]";                        
    close DOM_WORK;                              
  }                                    
  &get_domkey;
}
[download]

So whats wrong with this? lets see...

open DOM_WORK, '>/usr/spool/lpd/dom/dom.work';
&get_text;
[download]

First you open a global filehandle, and you dont check to see what happens... The first is a minor nono in my book the second is negligent programming. Then you call a sub, one that isnt defined. I dont like that, but im a top subber. Personally I find that this leads to errors, others disagree. Ill post a link to a debate on the subject in a mo. Either way I think the following is much better

open my $DOM_WORK_FH, '>/usr/spool/lpd/dom/dom.work' or die "Open dom.
+work failed $!";
get_text($DOM_WORK_FH); # you could also pass in the filename and open
+ the file in the sub...
[download]

And then we get to the sub

sub get_text{                                
  my @text=(<>);                               
  my $line =0;                                 
  foreach $line (@text) {
[download]

First the assignment of <> to @text means the whole file will be loaded into memory. Which probably is uneccesary, and if you get a large file will cause serious problems. Add to the fact that the add parens around the <> is distracting. Then you declare and initialize $line to be 0. Ok fine. Only thing is you then iterate over @text with $line. So first off the 0 goes bye-bye, but more importantly $line for each iteration contains a line of text, not the index of that line in the array. (Interestingly the will work once and once only, as the string will eval in the array to 0 and thus give you the first line in the array, but it would do that every time.) So we can probably clean up this mess into

sub get_text {
   my $out_fh=shift #what filehandle to write to  
   while (<>) { # read a line at a time into $_
[download]

So then we get to the processing of each line

    my $text =$text[$line];                      
    $text =~tr/\x0c-\x0d//d;
[download]

The first assignment does nothing useful. in fact it blows the contents of line and would generate large quantities of warnings if you were using them. The second line does some basic character stripping, it seems ok. So this gets reduced to

    tr/\x0c-\x0d//d;  # strip unwanted chars from $_
[download]

And then we get to the really interesting bit,

    select DOM_WORK;                             
    print "$text[$line]";                        
    close DOM_WORK;
[download]

What does this do? The first line sets print to use the global filehandle that you opened earlier. The second line prints out what you think is the line, but really isnt. Even if it was you dont need to quote it. But the last line is of most interest: Did you really want to CLOSE your ouput file after the first line? No I didnt thinks so

   print $out_fh $_;
[download]

Which brings us to the end of the loop and the calling of another sub. Why are you chaining subs like this? Its confusing when you have a lot of subs. Better to call the second subroutine in the same place you call the first. Especially as you arent passing any info from one subroutine to the other.

So in the end the rewritten code would be

use strict;
use warnings;

sub get_text{  
  my $output_file=shift;                           
  open my $out_fh,">".$output_file or die "Failed to open $output_file
+ because $!";
  while (<>) { 
    tr/\x0c-\x0d//d;
    print $out_fh $_;
  }                                    
}          

get_text('/usr/spool/lpd/dom/dom.work');                              
+     
get_domkey();
[download]

Which I think youll agree is a lot easier to read, and actually works.

boo_radley already said it, but almost all of the mistakes you made in this snippet would have been patently obvious had you turned warnings and strict on.

Hope this points you in the right direction,

Yves / DeMerphq
---
Writing a good benchmark isnt as easy as it might look.