in reply to Improve my Code: Subroutines

Unless you have a compelling reason to have separate arrays, I suggest you use an AoA to put everything in the same data structure. This makes it easier to keep associated values together. 

my @radsigma;
for ( my $i=1; $i<=100; $i+=2 ) {
    my $sig;
    if ( $r <2 e-5 ) {
        $sig = sigmalow($r);
    }else{
        $sig = sigmahigh($r);
    }
    push @radsigma, [ $r, $sig ];
}

[download]

The "constant" $pi is used in multiple places and should be declared only once at the top of the program. There is also a pragma to make it more like a real constant. 

use strict;
use warnings;
use constant PI => 3.1416;

[download]

You've done way too much work to get the subroutine arguments. This is how it's generally done: 

sub sigmalow {
    my $rad = shift;   # shift it off the front of @_
    my $rad = $_[0];   # or explicitly assign the value

[download]

About your loop, there is no reason to declare $i on a separate line (see above). Also, C style for loops are generally ugly and unperlish. Your example is one of the few cases where these may actually be cleaner than the alternatives, but you should at least be aware that there are alternatives: 

for my $i (1..100){
    next unless $r % 2;

for (0..49){
    my $i = $_ * 2 + 1;

for my $i ( grep {$_ %2} (0..99 ) ){

[download]

A lot of your code is crowded horizontally. Using more blank spaces helps readability.

Replies are listed 'Best First'.
Re^2: Improve my Code: Subroutines
by ikegami (Patriarch) on Jun 13, 2009 at 21:33 UTC
    Arguments are usually done with 
    sub f {
   my ($arg1, $arg2) = @_;
   ...
}

    [download]

    or 

    sub f {
   my $arg1 = shift;
   my $arg2 = shift;
   ...
}

    [download]

    Using $_[0] is rarer.

    Also, C style for loops are generally ugly and unperlish

    True, but none of your alternatives are particularly better. I would move the my into the loop, though. 

    for (my $i=1; $i<=100; $i+=2) {    # 1, 3, 5, ..., 99

    [download]
[reply]
[d/l]
[select]
Re^2: Improve my Code: Subroutines
by cheech (Beadle) on Jun 14, 2009 at 19:41 UTC
    Thanks for the advice.. everything makes sense except the alternative loop examples, but I'll get there.
[reply]
      ... everything makes sense except the alternative loop examples ...

      They were meant to illustrate the Perl style for loop (aka foreach loop), so you would be aware of it. The C style loop actually makes more sense in your program, but that is usually not the case.

      The basic idea of the Perl for loop is that it loops over a list of values, aliasing each one to the loop variable in turn. The following should help clarify: 

      # foreach $variable ( @list_of_values ){ ...
# or
# for $variable ( @list_of_values ){ ...

# $variable is aliased to each successive item in list
# @list_of_values can be a literal list, an array or an
# expression that evaluates to a list:

for my $var ( 1..4 ){ ...        # expression using range operator

for my $var ( @array ){ ...      # array

for my $var ( 6, 7, 'A', 'B' ){  # literal list
    print "$var  ";
}

# if a variable is not specified, $_ is used by default

for ( 6, 7, 'A', 'B' ){
    print "$_  ";
}

      [download]
[reply]
[d/l]

In Section Seekers of Perl Wisdom