The following fragment is extracted from your program:
sub sep_id {
...
open (TMPFILE, "> $tmp_file") or (die "Could not open $tmp_fi
+le: $!");
select TMPFILE;
foreach $water (keys %wat_freq) {
$freq=0;
$code=undef;
if ($wat_freq{$water} >= $C_NETSIZE) {
@ids=();
foreach (@info) {
if (substr($_,13,5,) == $water ) {
push @ids, substr($_,8,5);
}
}
@ids=sort {$a <=> $b} @ids;
$code = join ":", $water . "+", @ids;
$code =~ s/ /0/g;
printf "%02d:+%s:\n",$wat_freq{$water}, $code;
}
}
select STDOUT;
You are missing a close TMPFILE after you created the file. Now what happens is that the last file created will not closed properly, although files created earlier are automatically closed by the next open of the TMPFILE in your loop.
Update:
Having looked at your code, which is ugly, I just want to add a few comments:
1) Add use strict; to the beginning of your code to prevent autovivification of variables.
2) Avoid using global variables. And if you must, try to make your intension clear on how the global variables are used. For example, instead of doing
my $SOME_GLOBAL_VARIABLE = 0;
some_sub_with_side_effects();
...
sub some_sub_with_side_effects {
$SOME_GLOBAL_VARIABLE = 1;
}
Do this instead:
my $SOME_GLOBAL_VARIABLE = some_sub_without_side_effects();
...
sub some_sub_without_side_effects {
return 1;
}
3) Remove the { } around the function calls, they are unnecessary and ugly.
4) Use IO::File my $f = new IO::File "file.txt", "w" or open my $f, "< file.txt" when openning your files. In other words, avoid using direct file handles, use lexical/anonymous file handles that are associated to the variables they are attached to. The lexical file handles are closed automatically when the associated variables fall out of scope, so you don't have to worry about closing them.
5) When modifying a Perl built-in variable, always create a scope and localize it first. You had a $| = 1; in your sub that turned off bufferring, however the effect is not cancelled when your sub finishes, this side effect carries on to other subs. This will not happen if you do local $| = 1;
Anyway, good luck and happy Perl programming.
| [reply]
[d/l]
[select] |
Hi Roger,
Thanks a lot for the feedback and useful criticism.
> 1) Add use strict; to the beginning of your code to prevent autovivification of variables.
Yes, by the time I posted it here I had already used strict everywhere. Initially I had error messagse that I could not understand but eventually got over.
> Do this instead: my $SOME_GLOBAL_VARIABLE=some_sub_without_side_effects();
You are right. But the way I needed to use was to have several variables to be returned, which would be easier with global variables than with several functions. :)
> 3) Remove the { } around the function calls, they are unnecessary and ugly.
I was intending to use eval... In retrospect, that was not required. Yes, I would be removing them now. :)
> 4) Use IO::File ...
Yes, I would be using it from now on. Thanks. :)
> 5) When modifying a Perl built-in variable, always create a scope and localize it first...
This has been a problem for me to understand, I need better books for advanced settings. I am not sure which are actually global and which are not.
My array sizes are huge so I was not sure if I should use subroutines which might duplicate all the variables upon passing the info, but I could not verify if it actually happens. These problems exists, but I could somehow write my first code.
> Anyway, good luck and happy Perl programming.
Thanks. It's been an ugly code, but I am happy to have shfted from FORTRAN for these things. :)
Sorry I could not reply earlier, was out for some trip.
Thanks a lot again. :)
| [reply] |
But the way I needed to use was to have several variables to be returned, which would be easier with global variables than with several functions
Yes you can do that in Perl quite easily:
my ($var1, $var2, $var3) = MyFunction();
sub MyFunction {
return ('VAR1', 'VAR2', 'VAR3');
}
| [reply]
[d/l] |