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

Hi, I wrote up a simple add() and multiply() subroutine script and wanted to have it looked over. I would like it to be as efficient and correct as possible.

As it is now, it works correctly with no errors, just wanted to make sure it is proper form. Thanks for any advice.

use warnings;
use strict;

my $i = $#ARGV;

if ($ARGV[0] eq "add") {
    my $rtn = add($ARGV[1]..$ARGV[$i]);
    print "The sum is: $rtn";
}

elsif ($ARGV[0] eq "multiply"){
    my $rtn = multiply($ARGV[1]..$ARGV[$i]);
    print "Their product is: $rtn";
}

sub add {
    my ($sum);
    $sum=0;
    foreach $_(@_) {
        $sum +=$_;
    }
    return $sum;
}

sub multiply {
    my $product=1;
    foreach $_(@_) {
        $product*=$_;
    }
    return $product;
}

[download]

Replies are listed 'Best First'.
Re: Simple add and multiply subroutines
by jwkrahn (Abbot) on Apr 08, 2008 at 02:13 UTC

    You have a major error in that you are using the range operator which will not work correctly, for example: 

    $ perl -le'my @x = $ARGV[1] .. $ARGV[$#ARGV]; print "@x"' add  12  8  
+2

$ perl -le'my @x = $ARGV[1] .. $ARGV[$#ARGV]; print "@x"' add  2  8  1
+2
2 3 4 5 6 7 8 9 10 11 12

    [download]

    You should probably shift the first argument off @ARGV, something like this: 

    use warnings;
use strict;

my $i = shift;

if ( $i eq 'add' ) {
    my $rtn = add( @ARGV );
    print "The sum is: $rtn";
}
elsif ( $i eq 'multiply' ){
    my $rtn = multiply( @ARGV );
    print "Their product is: $rtn";
}

sub add {
    my $sum = 0;
    foreach ( @_ ) {
        $sum += $_;
    }
    return $sum;
}

sub multiply {
    my $product = 1;
    foreach ( @_ ) {
        $product *= $_;
    }
    return $product;
}

    [download]
[reply]
[d/l]
[select]

      I think I see what you mean, but why would I put $i in the if conditional? Like you have:

      if ( $i eq 'add' ) {
    my $rtn = add( @ARGV );
    print "The sum is: $rtn";
}

      [download]

      I don't understand why it goes there.

[reply]
[d/l]
      I have inserted each one of your answers and everyone of your programs have uninitialized errors. I guess its back to the drawing board
[reply]
Re: Simple add and multiply subroutines
by mr_mischief (Monsignor) on Apr 08, 2008 at 03:19 UTC
    One (almost) never sees a for loop in Perl with the $_ pronoun specified. It is the default loop pronoun so it is assumed or another, more descriptive, variable is specified.

    You can declare and initialize $sum in one line. my $sum = 0;

    If you shift the operation off of @ARGV, you can then use the more natural notation for calling the subs:

    Read more... (449 Bytes)

    Some people place the else or elsif on the same line as the right curly bracket, and some place it on the next. I'm not sure you'll ever see someone skip a line between an if and an else/elsif entirely, and that's probably a good thing.

    Although Perl is only in rare cases sensitive to the presence or absence of white space, it helps readability of your code if you use a space between disparate tokens.

    Read more... (826 Bytes)

    A for loop and a foreach loop are the same thing. There was a widespread habit once upon a time of using "for" only with a C-style set, check, update style loop and using "foreach" for iterating over a list. Many people just use "for" in both cases, but if you're working under a professor or with a group of other programmers who tend to use "foreach", then it's probably a good idea to follow your local convention.

    Another solution to your problem, in which the main program is very short, is this:

    Read more... (1122 Bytes)

    Other options of course exist. One that you'd likely catch all kinds of crap from a professor (or anyone interested in maintainability, likely) is this:

    Read more... (920 Bytes)
[reply]
[d/l]
[select]
Re: Simple add and multiply subroutines
by ysth (Canon) on Apr 08, 2008 at 02:21 UTC
    As written, given arguments add 1 7 9, you are passing 1..9 to add(), which would result in 45. To pass 1,7,9, you would use a slice of the array, like so: add(@ARGV[1..$i]).

    But it would be nicer to separate your arguments up front: 

       my ($operation, @operands) = @ARGV
      or die usage();

   if ($operation eq 'add') {
      my $sum = add(@operands);
...

    [download]
    --
    Online Fortune Cookie Search
[reply]
[d/l]
[select]

      Woot I think I got it. Check it out (I also added comments)

      use warnings;
use strict;

my $i = $#ARGV;                             #Scalar for array length

if ($ARGV[0] eq "add") {                    #Checks for add
    my $rtn = add(@ARGV[1..$i]);     #Calls add subroutine
    print "The sum is: $rtn";
}

elsif ($ARGV[0] eq "multiply"){                    #Checks for multipl
+y
    my $rtn = multiply(@ARGV[1..$i]);       #Calls multiply subroutine
    print "Their product is: $rtn";
}

sub add {                            #add subroutine
    my ($sum);
    $sum=0;
    foreach $_(@_) {                 #Loops through the array
        $sum +=$_;                   #Adds each array element to the r
+est
    }
    return $sum;                     #Returns the sum value
}

sub multiply {                       #multiply subroutine
    my $product=1;                   
    foreach $_(@_) {                 #Loops through the array
        $product*=$_;                #Multiplys each element to last
    }
    return $product;                 #Returns the product value
}

      [download]

      How does that look?

[reply]
[d/l]
Re: Simple add and multiply subroutines
by aufflick (Deacon) on Apr 08, 2008 at 05:39 UTC
    Good job asking for feedback on your code. To elaborate on what jwkrahn said about the range operator, the range operator is for *generating* a list of numbers, but what you want to do is iterate over an existing array.

    So the range operator lets you say:

        1..5

    which is equivalent to the list:

        1, 2, 3, 4, 5

    So if, say, your ARGV array in the original example was ('add', 1, 7, 22, 5) then $ARGV[1]..$ARGV[$i] would evaluate to 1..5, ie: (1, 2, 3, 4, 5). Echoing oko1's comment about a dispatch table being the way to go, here is how I would write your script: 

    use warnings;
use strict;

my %dispatch = (
    add => sub { my $sum = 0; $sum += $_ for @_; $sum; },
    multiply => sub { my $prod = 1; $prod *= $_ for @_; $prod; },
);

my $op = shift @ARGV;

print $dispatch{$op}->(@ARGV);

    [download]
[reply]
[d/l]
[select]
Re: Simple add and multiply subroutines
by oko1 (Deacon) on Apr 08, 2008 at 04:03 UTC

    Not that I'm disagreeing with all the errors/problems other people have pointed out, but this is really the canonical case for using a dispatch table. 

    #!/usr/bin/perl -w
use strict;

my $op = shift or die "You must specify an operation followed by numbe
+rs.\n";
die "Non-numeric input.\n" if grep /\D/, @ARGV;

my %perform = (
    add         =>  sub { local $" = "+"; eval "@ARGV"; },
    multiply    =>  sub { local $" = "*"; eval "@ARGV"; },
);

die "Invalid operation.\n" unless defined $perform{$op};

print $perform{$op}->();

    [download]
    
-- 
Human history becomes more and more a race between education and catastrophe. -- HG Wells
[reply]
[d/l]

      Dispatch table - yes, string eval - OMG NO! especially not for a beginner!

      Your grep test for "Non-numeric input" won't accept floating point numbers or negative numbers.

[reply]

        Nor engineering notation, nor imaginary numbers, nor Roman numerals either. It will, however, handle the integers as shown in the original request. :)

        String eval... OK, you're right. Easy enough to fix, and I see (behind the cut tags) that the poster just before me already did that. 

        
-- 
Human history becomes more and more a race between education and catastrophe. -- HG Wells
[reply]
Re: Simple add and multiply subroutines
by NetWallah (Canon) on Apr 08, 2008 at 06:30 UTC
    ++ to all proposed solutions and explanations.

    If I may add my $0.02, there is something to be said for minimising repetition of similar looking code, particularly if it is likely that the number of repetitions may increase. With that in mind, may I offer: 

    #!/usr/bin/perl -w
use strict;

my %do_op=(
  add       => sub{$_[0] +  $_[1]},
  multiply  => sub{$_[0] *  $_[1]},
  divide    => sub{$_[0] /  $_[1]},
  subtract  => sub{$_[0] -  $_[1]},
  power       => sub{$_[0] ** $_[1]}, 
  equal      => sub{$_[0] == $_[1]? $_[0]:0},
  max        => sub{$_[0] >  $_[1]? $_[0]:$_[1]}, 
  min        => sub{$_[0] <  $_[1]? $_[0]:$_[1]},        
);

sub operate{ 
        my $op=shift;
        my $tot = shift;           
        while (defined (my $x=shift)){
                   $tot = $do_op{$op}->($tot,$x);
          }
          $tot;
};            

my $op = shift or die "You must specify an operation followed by numbe
+rs.\n";
die "Non-numeric input.\n" if grep /\D/, @ARGV;
die "Invalid operation.\n" unless exists $do_op{$op};

print "$op (@ARGV)= ". operate($op, @ARGV) . "\n";

    [download]
    Now, you are on your way to implementing a reverse polish engine ...

         "As you get older three things happen. The first is your memory goes, and I can't remember the other two... " - Sir Norman Wisdom

[reply]
[d/l]
      use List::Util qw( reduce );

my %ops = (
    add => sub { reduce { $a + $b } @_ },
    multiply => sub { reduce { $a * $b } @_ },
    # etc ...
);

my $op = shift or die "You must specify an operation followed by numbe
+rs.\n";
die "Non-numeric input.\n" if grep /\D/, @ARGV;
die "Invalid operation.\n" unless exists $ops{$op};

print "$op (@ARGV)= ". $ops->(@ARGV) . "\n";

      [download]
      Except, even that's too much repetition. 
      use List::Util qw( reduce );

my %ops = (
    add => '+',
    multiply => '*',
    # etc ...
);

%ops = map {
    $_ => eval q(sub { reduce { $a ) . $ops{$_} . q( $b } @_ })
} keys %ops;

# And so forth.

      [download]
      Learn CPAN, young padawan. It is rare that someone hasn't already solved at least part of whatever it is you're looking at. Writing core Perl for more than 50% of a program is a sign of "I don't know CPAN," not brilliance.

      Update: Fixed typo s/op/ops/ inside eval.

      My criteria for good software:
      1. Does it work?
      2. Can someone else come in, make a change, and be reasonably certain no bugs were introduced?
[reply]
[d/l]
[select]
        Brilliant!(++) ( Except for a small typo: s/\$op/\$ops/ , inside the "eval").

        Education never stops.

        Update:Code Re-written incorporating dragonchild and mr_mischief's ideas: 

        #!/usr/bin/perl -w
use strict;
use List::Util qw( reduce );

# Definition Syntax: <verb> <operation>;   <operation> can be SIMPLE, 
+as in "+"
#   COMPLEX ops are defined by arrayref, and contain an array of <term
+s>.
#   <complex-op> = [<simple-op>,<pre-code>,<post-code>,<last-code>]  (
+all ,<*-code> is optional)
#   <*-code> can use pre-defined vars: ($a,$b)=@_[0,1];  $count=scalar
+ @_; $result=result


my %ops = ( qw|add + subtract - multiply * divide / power ** |,
             qw|greater > lesser < |,
                 #OP  ,Pre,Post,LAST
    xor      => ["&&",'$a=~$a'],
    all      => ["&&",'', '1 : 0'], 
    none     => ["||",'', '1 : 0', '!$result'], 
    any      => ["||",'', '1 : 0'],
    min      => ["<",'', '$a: $b'],
    max      => [">",'', '$a: $b'],
    mean     => ["+",'','','$result /=$count'],
    geomean  => ["*",'','','$result ** (1/$count)' ], 
    equal    => ["-",'','0:$b', '$result &&=1'], 
);

sub op_JIT_compile{
   my $op = shift;
   return if ref $ops{$op} eq 'CODE'; # Optimize for "Already compiled
+"
   my $opval = $ops{$op};
   # Complex case first - Handle "pre,post" op requirements.
   if (ref $opval eq 'ARRAY'){ # Allow for "pre", "post", and "last" o
+peratons 
         ##  Avoid "undef",s by passing in extra "?'s at end.      
      my ($realop,$pre,$post,$last) = (@$opval,('') x 4); 
      my $post_ternary = $post eq '' ? '' : qq|?$post|; 
      my $eval_this_sub = q|sub { ($a,$b) = my @F=@_; my $count=scalar
+ @F;| . 
                                      q|;my $result= reduce {|
                                                  . $pre . 
                                                   q|; $a | 
                                                  .  $realop  .
                                                  q| $b| 
                                                  . $post_ternary . 
                                                  q| } @F ;|
                                        . $last .
                                      q| }|;
      #print qq[$op = $eval_this_sub\n\tOPS(realop,pre,post,last;)=$re
+alop,$pre,$post,$last;\n];                            
       $ops{$op} = eval $eval_this_sub;
       return;
   }
   # This is the "simple" operator case ..
    $ops{$op} = eval q| sub { reduce { $a | .  $opval  . q| $b } @_ }|
+;
}
 

my $op = lc shift or die "You must specify an operation followed by nu
+mbers.\n";
die "Non-numeric input. $_\n" if grep /\[^-]?\D/, @ARGV;
die "Invalid operation ($op).\n" unless exists $ops{$op};
op_JIT_compile($op);
print "$op (@ARGV)= ". $ops{$op}->(@ARGV) . "\n";

        [download]
        Note that the eval used is a string eval, using only program variables (no user-supplied data). This eval can be performed ahead of time, or JIT (as programmed). The eval is performed only once per function-name. Specifically, it would NOT be performed every time the user provides input (if the program had an input loop).

        Thanks to dragonchild, and mr_mischief for making this interesting/educational.

             "As you get older three things happen. The first is your memory goes, and I can't remember the other two... " - Sir Norman Wisdom

[reply]
[d/l]
      Your equal sub has a painful concession to a special case. The value presented for 0 equaling 0 is false. I prefer it to be true, although then the concession is that the return is no longer the value from the call.

      BTW, if you're really wanting to cut down on duplicated code and don't mind torturing the language a bit, you can do this: 

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

my $op = shift @ARGV;

my $minmax = '$result ? $_[0] : $_[1];';
my $settruth = '$result ? 1 : 0;';

my %ops = (
    'add'      => { 'op' => '+' },
    'multiply' => { 'op' => '*' },
    'subtract' => { 'op' => '-' },
    'divide'   => { 'op' => '/' },
    'lesser'   => { 'op' => '<' },
    'greater'  => { 'op' => '>' },
    'power'    => { 'op' => '**' },
    'xor'      => { 'op' => '&&', 'pre'  => '$_[0] = ! $_[0];' },
    'all'      => { 'op' => '&&', 'post' => $settruth },
    'none'     => { 'op' => '||', 'post' => $settruth, 'last' => '$tot
+ = ! $tot; $tot += 0;' },
    'any'      => { 'op' => '||', 'post' => $settruth },
    'min'      => { 'op' => '<',  'post' => $minmax },
    'max'      => { 'op' => '>',  'post' => $minmax },
    'mean'     => { 'op' => '+',  'last' => '$tot /= scalar (@_ + 1);'
+ },
    'geomean'  => { 'op' => '*',  'last' => '$tot = $tot ** ( 1 / scal
+ar (@_ + 1) );' },
    'equal'    => { 'op' => '-',  'post' => '$result = $result ? 0 : $
+_[1];',
        'last' => 'local $" = "||"; $tot = 1 if 0 == $tot && 0 == eval
+ "@_";'}
);

sub make_op {
    my $op = shift;
    my $result = 0;
    $ops{$op}{'sub'} = sub {
        ( eval $ops{$op}{'pre'} ) if defined $ops{$op}{'pre'};
        $result = ( eval $_[0] . $ops{$op}{'op'} . $_[1] ) + 0;
        $result = ( eval $ops{$op}{'post'} ) if defined $ops{$op}{'pos
+t'};
        return $result;
    };
}

sub do_op {
    my $op = shift;
    my $tot =  shift;
    make_op( $op ) unless (
        exists $ops{$op}{'sub'}
        && defined $ops{$op}{'sub'}
        && ref $ops{$op}{'sub'} eq 'CODE'
    );
    foreach ( @_ ) {
        $tot = $ops{$op}{'sub'}( $tot, $_ );
    }
    if ( exists $ops{$op}{'last'} ) {
        eval $ops{$op}{'last'};
    }
    return $tot;
}

printf "%-0.03f\n",  do_op( $op, @ARGV );

      [download]

      I know, I know... It's ugly to do from 1 to 3 evals for each iteration and possibly another one or two at the end. The data structures could have clearer names. At least only the op you need gets its sub created. I did say the language would be tortured a bit, didn't I? It's kind of a fun little toy, though.

[reply]
[d/l]
[select]

      Might as well toss this in, then: 

      ...
my $op = shift or die "You must specify an operation followed by numbe
+rs.\n";
###########################################################
die "At least two numbers required.\n" unless @ARGV > 1;
###########################################################
die "Non-numeric input.\n" if grep /\D/, @ARGV;
...

      [download]

      Despite jwkrahn's warning, I'd want to implement an "eval" wrapped around the operation, too - after carefully vetting the data. It's quite nice for those "Divide by zero" errors. 

      
-- 
Human history becomes more and more a race between education and catastrophe. -- HG Wells
[reply]
[d/l]

Back to Seekers of Perl Wisdom