Beefy Boxes and Bandwidth Generously Provided by pair Networks
go ahead... be a heretic

Is this code logical?

by Win (Novice)
on Feb 24, 2006 at 14:07 UTC ( #532543=perlquestion: print w/ replies, xml ) Need Help??
Win has asked for the wisdom of the Perl Monks concerning the following question:

Can someone please tell me whether there is anything wrong with the logic of the following bit of code.
my $new_outputfile = $outputfile."_B"; open (RESULT_FILE, "<$outputfile"); open (CLEANED_RESULT, "+>$new_outputfile"); our @data = <RESULT_FILE>; chomp @data; my %seen; for ( @data) { next if $seen{$_}++; print CLEANED_RESULT "$_\n"; } remove $outputfile or warn $!; rename "$new_outputfile", "$outputfile" or warn $!;
It does not appear to be doing what I ask of it.

Update: The output file is present as a directory location and file name like: C:/Perl_activate/filename.txt

Comment on Is this code logical?
Download Code
Re: Is this code logical?
by CountOrlok (Friar) on Feb 24, 2006 at 14:16 UTC
    Where does the function "remove" come from? It isn't a built-in function. Do you mean "unlink"?
    The parameters for "rename" should be the other way around and do not need quotes around them.
    Why declare "our @data" and not "my @data"?
    If the file is huge, the process will be slow since you're slurping the whole file in. It would be better to read and process line by line.


      The file names are the correct way round. I want the content of the old file name to be the same as the content of the new file name.
Re: Is this code logical?
by trammell (Priest) on Feb 24, 2006 at 14:43 UTC
    Failing to check the return value of open() is pretty illogical.
Re: Is this code logical?
by merlyn (Sage) on Feb 24, 2006 at 14:52 UTC
      This is very good advice, in principle, especially for the benefit of other readers, but for the OP evidence is that won't listen in any case. Indeed I'm afraid to have to do an ad hominem attack -and I shamelessly admit I'm doing one!-, but I doubt he's really willing to learn, let alone to listen. Which makes one wonder why he keeps asking, instead! Mistery...
Re: Is this code logical?
by dragonchild (Archbishop) on Feb 24, 2006 at 14:56 UTC
    If you're using Perl 5.005_03 or higher, this is how I'd write your code:
    open my $results, '<', $outputfile or die "Cannot open '$outputfile' for reading: $!\n'; open my $cleaned, '>', $new_outputfile or die "Cannot open '$new_outputfile' for writing: $!\n"; { my %seen; local $_; while ( <$results> ) { chomp; next if $seen{ $_ }++; print $cleaned $_, "\n"; } } # Always close in the reverse order of opening. close $cleaned; close $results; unlink $results or die "Cannot unlink '$outputfile': $!\n"; File::Copy::move( $new_outputfile, $outputfile ) or die "Cannot rename '$new_outputfile' to '$outputfile': $!\n";
    The changes:
    • Use 3-arg open for safety.
    • Use lexicals for filehandles instead of globals. (That's the difference between 'my $fh' and 'FH'.)
    • Always check the return values of system commands.
    • If you fail a system command, then stop processing immediately. It's dangerous to continue unless you have a way of cleaning up what went wrong.
    • Don't read the file into memory. It might be small now, but it will probably grow.
    • If you intend on using $_, localize it so that you don't trample on anyone.
    • You're not paying for your whitespace, so use whitespace liberally. It will increase readability in the long run.
    • If you open it, then close it as soon as you're done with it.
    • If you open two things, close them in reverse order.

    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?
      This does not work I'm afraid. This is perl, v5.8.4 built for MSWin32-x86-multi-thread may be the reason why.

      Content restored by Arunbear, using RSS feed.

      Update: I wish to delete this node because it is not of relevance and I have got the code to work and do as I wish.

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://532543]
Approved by Corion
Front-paged by grinder
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others browsing the Monastery: (7)
As of 2014-09-18 08:17 GMT
Find Nodes?
    Voting Booth?

    How do you remember the number of days in each month?

    Results (109 votes), past polls