Beefy Boxes and Bandwidth Generously Provided by pair Networks
We don't bite newbies here... much
 
PerlMonks  

Subroutine question

by Limo (Scribe)
on Sep 18, 2000 at 21:58 UTC ( [id://32990]=perlquestion: print w/replies, xml ) Need Help??

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

Here's the code:
sub file_processing { $script = "/export/home/ssesar/Perl/another_script.pl -e"; $open = "$script $arg1 $file1"; open(FILE1, "$open |") or die "can't do it\n"; while (<FILE1>) { chomp; next if /^\#/; next if /None/; next if /Unkno/; next if /unkno/; next if /NONE/; @_ = split ( /\n/, $_); @array1 = @_; } close FILE1; }
I'd like to know if I could replace:
$open = "$script $arg1 $file1"; open(FILE1, "$open |") or die "can't do it\n";
with:
$open = "$script $arg2 $file2"; open(FILE2, "$open |") or die "can't do it\n";
and:
@array1 = @_;
with:
@array2 = @_;
...within the same subroutine. Not a whole lot to re-type, if I needed to create 2 different subroutines, but I'd like to learn to program as efficiently as possible.

Replies are listed 'Best First'.
Re: Subroutine question
by Fastolfe (Vicar) on Sep 18, 2000 at 22:25 UTC
    The other posters already indicate that passing these items as arguments to your subroutine would be the best way to handle it, but I'm looking at your subroutine and wondering what you're trying to do. Just FYI, <FILE> works line-by-line, so you shouldn't have to split on \n. You could re-write your function like this:
    sub file_processing { my ($script, $arg, $file) = @_; my @ret; open(FILE, "$script $arg $file |") or die "can't do it: $!"; while (<FILE>) { chomp; next if /^#|none|unkno/i; push(@ret, $_); } close(FILE); return @ret; }
    Note also that your open call is going to be insecure if any of your 3 arguments is provided by the user. You probably want to do something like this for better security, unless you know you can trust all three arguments. Running Perl with taint-checking (-T) enabled helps if you're dealing with potentially bad/mischievous data.
    my $pid = open(FILE, "-|"); if (!$pid) { die "Couldn't fork: $!" unless defined $pid; exec($script, $arg, $file) or die "Couldn't exec: $!"; }
    This has the same effect, but instead of passing the command to /bin/sh (or whatever your shell is) to parse into a command and arguments (which might include things like semi-colons allowing an evil person to execute other programs), it uses exec to forcibly pass things as discrete, known arguments directly to the script.
RE: Subroutine question
by reyjrar (Hermit) on Sep 18, 2000 at 22:07 UTC
    could you just pass $script $arg and $file as arguments to the sub routine?
    ie in the main loop: $stuff_to_do[0] = "/path/to/script -e;;arguments;;file"; ... ... foreach $line (@stuff_to_do) { ($script,$arg,$file) = split(/;;/,$line); &file_processing($script,$arg,$file); } ... then your subroutine would be: sub file_processing { my ($script, $arg, $file) = @_; .... } I think that's what you're trying to do, but not sure.. -brad..
      I think that's what he's trying to do, but he's also modifying the global @array1, and he wants to modify the global @array2 the second time around.

      Personally, I'd consider that poor style, and rewrite it as...

      @array1 = file_processing($script,$arg1,$file1); @array2 = file_proceesing($script,$arg2,$file2); ... sub file_processing { my ($script,$arg,$file) = @_; my @results; ... return @results; }
      Actually, I wouldn't... I'd call the sub "processfile", and make several other tweaks if I was writing this myself, but that's just me.
Re: Subroutine question
by mirod (Canon) on Sep 18, 2000 at 22:10 UTC

    It looks like you are not passing arguments to your subroutine, and using @array1 as a global variable too.

    You will have to pass the arg and file as parameters, and get the array as the return value.So the call will go from:

    # you use strict do you? my $arg1= "foo"; my $file1="bar"; my @array1; file_processing(); foreach (@array1) { process( $_); }

    to

    my $arg1= "foo"; my $file1="bar"; my @array1= file_processing( $arg1, $file1); foreach (@array1) { process( $_); }

    The subroutine should look like this:

    sub file_processing { my( $arg, $file)= @_; # added my @array; # added $script = "/export/home/ssesar/Perl/another_script.pl -e"; $open = "$script $arg $file"; # changed open(FILE, "$open |") or die "can't do it\n";# changed while (<FILE>) { # changed chomp; next if /^\#/; next if /None/; next if /Unkno/; next if /unkno/; next if /NONE/; @_ = split ( /\n/, $_); @array = @_; } close FILE; return @array; # added }
Re: Subroutine question
by cwest (Friar) on Sep 18, 2000 at 22:06 UTC
    I am completley confused. Please update this post with more information in _context_.

    We can help you much better this way.

    --
    Casey
       I am a superhero.
    

      The basic point is that it is poor programming style (and ineffecient to boot) to have two subroutines that essentially do the exact same thing.

      From my standpoint as someone who has learnt to code that painful way (is there any other), I've developped the standard methodology that if I have duplicated code (or code that closely duplicates functionalities I've used elsewhere) then I need to go back and look at condensing the duplicates into a single subroutine.

      In this case, then, you might start by changing the subroutine as follows:

      sub file_processing { my ($array_ref, $script, $arg, $file) = @_; open(FILE, $script . ' ' . $arg . ' ' . $file . ' |') or die ("can +'t do it: $!\n"); while (<FILE>) { chomp; next if /^\#/; next if /none/i; next if /unkno/i; push @{$array_ref}, split ( /\n/, $_); } close FILE; }

      Notice, however, that there are some areas that could use some more work:

      1. The regexp that matches none/unknown/# could be improved and probably condensed into a single one looking something like this: /(?:#|unknown|none)/
      2. The assignment back to $array_ref may not do exactly what you expect/need -- a little testing would be in order

      And finally, you'd call this script by doing the following:

      &file_processing(\@array1, $script1, $arg1, $file1); &file_processing(\@array1, $script2, $arg2, $file2);

      Which, of course, using the heuristic of duplicated code = BAD, could be condensed into a single array of arrays that are looped over and passed in in turn.

      Does that help?

      my $arg1 = "foo"; my $file1 = "file_1.gz; my $arg2 = "bar"; my $file2 = "file_2.gz"; sub file_processing { $script = "/export/home/ssesar/Perl/another_script.pl"; $open = "$script -e $arg1 $file1"; open(FILE1, "$open |") or die "can't do it\n"; while (<FILE1>) { chomp; next if /^\#/; next if /None/; next if /Unkno/; next if /unkno/; next if /NONE/; @_ = split ( /\n/, $_); @array1 = @_; } close FILE1; }
      1.I want this subroutine to be run as it reads
      2. Then, I want this subroutine to be run a second time with the follo +wing changes: <CODE> $open = "$script -e $arg2 $file2"; open(FILE2, "$open |") or die "can't do it\n"; while (<FILE2>) {
      and...
      @array2 = @_;

        Right, here's what we're saying:

        my @array1; my @array2; my @args = ( [ \@array1, "foo", "file_1.gz"], [ \@array2, "bar", "file_2.gz"] ); foreach (@args) { &file_processing($args->[0], $args->[1], $args->[2]); } sub file_processing { my ($array_ref, $arg, $file) = @_; my $script = "/export/home/ssesar/Perl/another_script.pl"; open(FILE, $script . ' ' . $arg . ' ' . $file . ' |') or die ("c +an't do it: $!\n"); while (<FILE>) { chomp; push @{$array_ref}, $_ unless /^(?:\#|none|unknow)/i; } close FILE; }

        This should do what you want as best as I can tell -- open a file and push into an array (either array 1 or array 2 depending on which iteration) any line that doesn't start with UNKNOW/unknow/#/NONE/none.

        Your lines

        @_ = split ( /\n/, $_); @array1 = @_;

        look very strange -- as someone else pointed out, if you're reading in the file line by line (which is what <FILE> implies) then the final split on \n is useless.

        In addition, you're only assigning the last value of @_ to @array1 (meaning the other values are lost) unless you've undefined $/, in which case you should just use my $file = <FILE> rather than the while loop.

        Hope this helps.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://32990]
Approved by root
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others perusing the Monastery: (3)
As of 2024-04-26 00:55 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found