Stupid loops, or is it just me?

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

And now, for a question that will test your tolerance for those with no perl skill whatsoever. I have been trying to write this script that takes data from a file which has two indices indicating a position in the sky, in degrees. It should sum over one of the indices, adding every data point that corresponds to a particular vertical position, and divide by the number of horizontal steps (360).

#!/usr/bin/perl -w

use FileHandle;

$fh = new FileHandle "/data/jkerwin/muskyoutput/ravsdec.dat";
die "Can't open RAvsDEC file" unless (defined($fh));
@decavg = ();
$i=0;
$j=0;
@line=();
for ($i=0,$i<147,$i++) {
    $decavg[$i] = 0;
}

while (defined($strip = <$fh>)) {
    ($var1) = (split(/\s+/, $strip))[0];
    ($var2) = (split(/\s+/, $strip))[1];
    ($var3) = (split(/\s+/, $strip))[2];
    $line[0] = $var1;
    $line[1] = $var2;
    $line[2] = $var3;
    $decavg[$line[0]] += $line[2];
}

for ($i=0,$i<147,$i++) {
    $decavg[$i] = $decavg[$i]/360;
    print "$decavg[$i]\n";
}
[download]

Trying to get it to print out what is going into the @decavg array gets me only 9 zeros, which is neither what I want, nor the 147 iterations I expect. Every time I have had the program print up until that point I have gotten some kind of output, but there it fails. This next part is where I have failed time and again. I set up a nested for loop that should assign, for all the vertical degrees, the calculated average value to every horizontal step.

@decavg2d = ();

for ($i=0,$i<147,$i++) {
    for ($j=0,$j<360,$j++) {
    $decavg2d[$i][$j] = 0;
    }
}

for ($i=0,$i<147,$i++) {
    for ($j=0,$j<360,$j++) {
    $decavg2d[$i][$j] += $decavg[$i];
    print "$decavg2d[$i][$j]\n";
    }
}
[download]

Again, when I try to print this out I get a small number of zeroes. Why won't this print out something meaningful? Do I need to use a hash array instead? (I tried, and it gave me the same kind of error). Thanks in advance for any help you can offer me. - jason "newbie" kerwin

Comment on Stupid loops, or is it just me? Select or Download Code

Replies are listed 'Best First'.
Re: Stupid loops, or is it just me? by Aristotle (Chancellor) on Aug 13, 2003 at 00:35 UTC
`($var1) = (split(/\s+/, $strip))[0]; ($var2) = (split(/\s+/, $strip))[1]; ($var3) = (split(/\s+/, $strip))[2]; $line[0] = $var1; $line[1] = $var2; $line[2] = $var3;` [download] Ugh. Why are you working with single values? split returns a list. `@line = split(/\s+/, $strip);` [download] When you say `for ($i=0,$i<147,$i++)` [download] the commas should be semicolons. Also, since all you want to do is iterate over indices 0 to 146, just say so: `for my $i (0 .. 146)` [download] Much easier to read, no? And finally - Perl understands lists. Why use indices where you don't need to? That's rarely necessary and often a source of errors. Instead of `for ($j=0,$j<360,$j++) { $decavg2d[$i][$j] += $decavg[$i]; print "$decavg2d[$i][$j]\n"; }` [download] it is better to say `for my $i (0 .. 146) { for (@decavg2d[$i][0 .. 359]) { $_ += $decavg[$i]; print "$_\n"; } }` [download] Recommended reading: Mark-Jason Dominus' excellent Program Repair Shop and Red Flags article series on Perl.com. Lastly - please get into the habit of using strict and warnings. It will save you a lot of headaches. Makeshifts last the longest.	[reply] [d/l] [select]
Re: Stupid loops, or is it just me? by antirice (Priest) on Aug 13, 2003 at 00:20 UTC
The loop you are showing is iterating over the array (1,1,0). Try replacing the commas in your for statements with semicolons. Hope this helps. antirice The first rule of Perl club is - use Perl The ith rule of Perl club is - follow rule i - 1 for i > 1	[reply]
Re: Stupid loops, or is it just me? by sauoq (Abbot) on Aug 13, 2003 at 00:26 UTC
As antirice has said, you need semicolons in your `for` loops. That should fix the problem. To prevent it in the future, consider writing your loops using a foreach style. For example, rewrite `for ($i=0; $i<147; $i++){ ... }` as: `for my $i ( 0 .. 146 ) { # . . . }` [download] Not only is the syntax simpler making it easier to type and read, you will be less prone to making off-by-one errors too. Update: Yes, I meant "semicolons" not "commas"... thanks rob_au. -sauoq "My two cents aren't worth a dime.";	[reply] [d/l]
Re: Stupid loops, or is it just me? by TomDLux (Vicar) on Aug 13, 2003 at 03:50 UTC
You don't need to initialize the arrays to zero. They begin with each entry undef, which is numerically equivalent to zero. It would only matter if you were only modifying some, and wanted some value at the end. But it would be simpler to check the values for undef at the end, and substitute zero. -- `TTTATCGGTCGTTATATAGATGTTTGCA`	[reply]