Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

I am having trouble with a simple sub-routine that i wrote. All it does is remove some unwanted characters from a file by substituting them with nothing. The problem is that when I call the sub-routine, i assign it to a variable called @derivative. When i print @derivative, i expect it to print the whole of the file, but infact it only prints the last section. Is there an obvious reason for this that I can't see??? Any help vastly appreciated ................ 


#! /usr/bin/perl -w

use strict;

open (INFILE, $ARGV[0]) or die "unable to open file";
open (OUTFILE, $ARGV[1]) or die "unable to open file";

my $line;
my @derivative = replace($line);

print "@derivative\n";


sub replace {


      $line = @_;

  while (<INFILE>)    {

         $line = $_;
         chomp ($line);
         $line =~ s/data//;
         $line =~ s/=//;
         $line =~ s/detector//;
        
    }

  return $line;

}

[download]

Replies are listed 'Best First'.
Re: sub-routines
by jreades (Friar) on Nov 28, 2002 at 12:45 UTC

    The quick answer is that you only return $line and not the entire file, but there are some other significant issues with the code. You also reset the variable $line twice within the subroutine, and it's not scoped properly.

    Although you can sort of get away with what you're doing here (calling a file within a subroutine that isn't properly scoped to that subroutine), it's hard to read and prone to bugs.

    A better way to think about this is to address the fact that the subroutine operates on a single line at a time, and not the entire file.

    Don't shoehorn in the file operation as well since it doesn't make logical sense. Instead, pass the subroutine a line at a time since that is its logical 'scope' (as distinct from the lexcial scope).

    open (IN "<...") or die("...");
open (OUT ">...") or die("...");

my $line;

while ($line = <IN>) {
    print OUT replace($line); # you could drop the $line and use $_
}

close IN;
close OUT;

exit 0;

sub replace {
    my $line = shift;

    #skip the comp since you're printing out newlines
    $line =~ s/data|=|detector//g;

    return $line;
}

    [download]

    I think that'll do it.

[reply]
[d/l]
[select]
Re: sub-routines
by LTjake (Prior) on Nov 28, 2002 at 12:50 UTC
    Perhaps this is more a point of "style", but to me, your sub replace should either open the file and loop through the contents, or simply replace the text. Openening the file outside the sub, then letting the sub loop through it just feels odd to me. Here's how i might do it: 
    use strict;

open (INFILE, $ARGV[0]) or die "Unable to open file '$ARGV[0]' - $!";
while(<INFILE>) {
    print replace($_);
}
close (INFILE);

sub replace {
    my $line = shift;
    chomp ($line);
    $line =~ s/data|=|detector//g;
    return $line;
}

    [download]
    Tested on this file: 
    xyzzy
data abc
detector = data
hooplah =
enddata

    [download]
    From the command line (note: i opted to use > to print to an OUTFILE rather than do it in the script): 
    C:\>perl a.pl test.txt > out.txt

    [download]
    Results: 
    xyzzy abc  hooplah end

    [download]
    HTH

    Update: Fixed some minor things with the code. Also, as you can see, my output is on one line due to the fact that we're chomping each line. You may wish to leave that out if you want to preserve the lines.

    --
    Rock is dead. Long live paper and scissors!
[reply]
[d/l]
[select]
Re: sub-routines
by Chief of Chaos (Friar) on Nov 28, 2002 at 12:37 UTC
    Hi,
    your are using $line too much.
    For each individual scope you should add  my $line;.
    #!/usr/local/bin/perl -w
use strict;
use warnings;

open (INFILE, "<$ARGV[0]") or die "unable to open file";
open (OUTFILE, ">$ARGV[1]") or die "unable to open file";

while (<INFILE>)    {
  my $line = $_;
  print OUTFILE replace($line);
}

sub replace {
  my $line = shift;
  chomp ($line);
  $line =~ s/data//;
  $line =~ s/=//;
  $line =~ s/detector//;
  return $line."\n"; 
}

close(INFILE);
close(OUTFILE);

    [download]
    Sorry, pasted the wrong statement.
    Now it is correct.
[reply]
[d/l]
[select]
      Your code doesn't make any sense. You're manipulating all the lines of the input, and then you discard them. Also, your replace subroutine just returns the number of arguments passed.

      Abigail

[reply]

Back to Seekers of Perl Wisdom