in reply to substr in nested foreach loop

sarani:

You didn't show enough code & data to be able to say for sure. But one thing occurs to me: Perhaps you've got blank lines in @plines, so you're asking for characters after the end? Try inserting the following line as the first line in your inner loop to check:

print '"',$pline,'": len=',length($pline),"\n";

--roboticus

Replies are listed 'Best First'.
Re^2: substr in nested foreach loop
by sarani (Sexton) on May 22, 2006 at 12:15 UTC
    roboticus: Thanks :D
    There are no blank lines anywhere in the data file. The lenght reads as 57, and the last substr asks for 8 chars after the offset,48 - that's 56. So no char after the end, unless my basic math is wrong.(Right?) If you'd like to see the entire code, I can put it up...

    UPDATEIt's up on my scratchpad.

Re^2: substr in nested foreach loop
by sarani (Sexton) on May 22, 2006 at 12:53 UTC
    #! /root/bin/perl use strict; use warnings; use subs; sub square; sub read; sub dist; my $r; my $r2; my $i=0; my $xh; my $yh; my $zh; my $f1; my $FILE1; my $hline; my @hlines; $f1=&read; my $x1; my $y1; my $z1; my $f2; my $FILE2; my $pline; my @plines; $f2=&read; open (FILE1, "$f1") or die "Argh! File not opened! $!"; open (FILE2, "$f2") or die "Argh! Second file not opened! $!"; while (<FILE1>) { $hline=<FILE1>; push (@hlines,$hline); } + while (<FILE2>) { $pline=<FILE2>; push (@plines,$pline); } close FILE2; close FILE1; print @hlines; print @plines; &dist; exit; sub dist { foreach $hline (@hlines) { $xh= substr $hline,31,8; $yh= substr $hline,40,8; $zh= substr $hline,48,8; foreach $pline (@plines) { print '"',$pline,'": len=',length($pline),"\n"; $x1= substr $pline,31,8; $y1= substr $pline,40,8; $z1= substr $pline,48,8; $i++; $r2=((&square($xh-$x1))+(&square($yh-$y1))+(&square($zh-$z +1))); $r=sqrt($r2); print "$i. $r2 and square root is $r\n"; if(($r<=2.4)&&($r>=3.6)) { print "match!"} } } print $i; return 1; } sub square { my $num; my $sq; $num=shift; $sq = $num * $num; return $sq; } sub read { print "Enter file name:"; $a=<STDIN>; chomp($a); return $a; }

      Not directly related to your issue, but some comments anyway, in no particular order:

      while (<FILE1>) { $hline=<FILE1>; push (@hlines,$hline); }

      is the same as

      @hlines = <FILE1>;

      Also,

      my $FILE1;

      probably doesn't do what you think. $FILE1 is not the same as the FILE1 you use in your open statement. Either change all places where you use a bare FILE1 to $FILE1 (which would make it a lexical filehandle), or just remove the my $FILE1; declaration.

      In your open call, you don't have to put quotes around the filename. It is however a good idea to use the three-argument form of open, in case someone tries to use something like >datafile as filename and causes you to overwrite the data file that way. So instead of

      open (FILE1, "$f1") or die "Argh! File not opened! $!";

      use something like

      open (FILE1, "<", $f1) or die "Argh! File not opened! $!";

      In your read subroutine, you're using the variable $a without declaring it. With almost any other name this would have caused a compile time error, but $a and $b are special. For that reason, it's best to avoid using either of those names.

      $xh, $yh, $zh, $x1, $y1, $z1, $i, $r and $r2 are only used within your dist subroutine, so you should only declare them in there, and not at the global level

      read is also the name of a built-in function, so it's usually better to avoid using it for any of your own functions unless you explicitely want to override it

      Your shebang line is showing /root/bin/perl as the path to perl. Since /root is usually only accessible by root, this means that you're developing and testing this code as root, which is generally not recommended.

      The use subs; is useless here as far as I can tell.

      Since the two file reads are really unrelated, you can reuse the same filehandle for both of them

      Making the changes above, the code looks something like this:

      #!/root/bin/perl use strict; use warnings; sub square; sub read_filename; sub dist; my $FILE; my $f1 = read_filename; open ($FILE, "<", $f1) or die "Argh! File not opened! $!"; my @hlines = <$FILE>; close $FILE; my $f2 = read_filename; open ($FILE, "<", $f2) or die "Argh! Second file not opened! $!"; my @plines = <$FILE>; close $FILE; print @hlines; print @plines; dist; exit; sub dist { my $i=0; my ($r, $r2); foreach my $hline (@hlines) { my $xh = substr $hline,31,8; my $yh = substr $hline,40,8; my $zh = substr $hline,48,8; foreach my $pline (@plines) { print '"',$pline,'": len=',length($pline),"\n"; my $x1 = substr $pline,31,8; my $y1 = substr $pline,40,8; my $z1 = substr $pline,48,8; $i++; $r2=((square($xh-$x1))+(square($yh-$y1))+(square($zh-$z1)) +); $r=sqrt($r2); print "$i. $r2 and square root is $r\n"; if(($r<=2.4)&&($r>=3.6)) { print "match!"} } } print $i; return 1; } sub square { my $num = shift; my $sq = $num * $num; return $sq; } sub read_filename { print "Enter file name: "; my $fn = <STDIN>; chomp $fn; return $fn; }

      This code can stil be further improved, but it should be a nice step in the right direction.

        Thank you so much! :) I can see I have a logn way to go, but this really helps...
      Well... I don't know what the problem may be, but maybe you should add a
      print length($hline),"\n";
      before the substr that's dying, just to be sure, maybe even print the line.

      But the main reason I write is to suggest using one unpack instead of three substrs... syntax would be (approximately, not tested)

      ($xh,$yh,$zh) = unpack("x31A8x1A8A8",$hline);
      Pretty sure that would be more efficient.

                      - Ant
                      - Some of my best work - (1 2 3)