in reply to Re: Not A Rockstar File Manipulator Today
in thread Not A Rockstar File Manipulator Today

The good:

The bad: 

use strict;
use warnings;

my %settings; 
foreach my $key (qw(input output pattern replacement)) {
    print "$key = "; 
    my $setting = <STDIN>;
    chomp $setting;
    $settings{$key}=$setting;
}

open IN, '<', $settings{input} 
    or die "Can't open input file $settings{input}: $!"; 

die "Output file $settings{output} already exists\n" 
    if  -e $settings{output};

open OUT, '>', $settings{output} 
    or die "Can't open output file $settings{output}: $!";

while (<IN>) {
    s/$settings{pattern}/$settings{replacement}/g;    
    print OUT $_;
}

close (IN);
close (OUT);

[download]

Now, there are still some problems with this code. The big one is that using a hash and keys for storing the settings, looses us the benefits of using strictures in the first place. If I misspell 'output' when I use it, I'll get no warnings. Perl happily autovivifies a value in the hash and returns an undef.

There are ways around this issue. The easiest is to use Hash::Util to lock the hash, so that attempts to change the hash or access nonexistent keys become a fatal error. 

use Hash::Util qw(lock_hash);
my %settings; 
foreach my $key (qw(input output pattern replacement)) {
    print "$key = "; 
    my $setting = <STDIN>;
    chomp $setting;
    $settings{$key}=$setting;
}
lock_hash %settings;

[download]

Another thing is the use of global file handles. I should really be using lexical filehandles. Global handles are global variables, with all the attendant problems.


TGI says moo

Replies are listed 'Best First'.
Re^3: Not A Rockstar File Manipulator Today
by ptoulis (Scribe) on Nov 16, 2008 at 00:03 UTC
    Wow! I see your points TGI, thanks! About the '||' and 'or' thing this was my bad, I was aware of the difference. I think using and's and or's is pretty cool. Coming from a Java/C# background I'd say they are most expressive features. I have not been into big Perl projects yet, so I guess I have not faced the drawbacks.
    Anyway, I cleaned up more code using the map{} magic and current 'version' is:
    use strict;
use warnings;

my @params=qw(input output pattern replacement);
my %settings; @settings{@params}=("0")x @params;
foreach my $key (sort keys %settings) {
    print "$key="; 
    chomp($settings{$key}=<>);
    }
open IN,"<",$settings{input} or die "not a rockstar today"; 

-e $settings{output} and die("File Exists..will not override!")
    or 
open(OUT,">",$settings{output}) or die "Error while opening file: $!";

print OUT map { if(s/$settings{pattern}/$settings{replacement}/g) 
    {$_} else {$_} } (<IN>);

close (IN) and close (OUT);

    [download]

    As a freshman, I just can't enough of this kind of writing! :)
[reply]
[d/l]

      The second take is much more readable. But you are doing some extra, unnecessary work when you initialize your settings hash. What does setting each key to '0' do for you? It's easier to let perl create the keys in the loop as it goes. Also, why sort the keys of the settings hash. Sort may be a simple looking command, but it can get expensive to sort big arrays. Just put your params array in the order you want to use. So you wind up iterating over your param array twice, and sorting it once.

      Any setting value will always be defined, as at least the empty string, so there is no need to initialize your hash values. 

      foreach my $key (@params) {
    print "$key = ";
    my $value = <>;  # must at minimum be "\n"
    chomp $value;       # must at minimum be ""
    redo unless length  $value;  # rerun loop if value is empty string
+.
}

      [download]

      Also the or between checking for the output file and the opening it is unnecessary. If you die from the open failing, you die, and execution stops. The following is identical in behavior to yours: 

      -e $settings{output} and die "File Exists..will not override!";

open(OUT,">",$settings{output}) or die "Error while opening file: $!";

      [download]

      It's amazing how you can do more with less in perl.

      I think "The Right Thing To Do"tm as we end the script is to check the results of the handle closes. 

      close IN  or warn "Error closing input $!\n";
close OUT or warn "Error closing output $!\n";

      [download]

      Perl is a lot of fun to play with. When I first started I went crazy for the ternary operator ($foo = $bar > 3 ? 'BIG' : 'small';) and used for all sorts of things. Keep playing and experimenting. You will learn from it. Keep your old code and come back and look at it months later. You'd be amazed at what you learn from this, too.

      If you can manage it on a student budget pick up PBP. It is an excellent book. It has tons of advice from grizzled veterans who've stepped in many of the traps that you are just discovering. For fun, and if you want to blow your mind, take a look at Higher Order Perl. Dominus does a great job introducing functional programming techniques in perl.

      Also, I can't recommend wandering around the The Portland Pattern Repository enough.

      Mainly, keep having fun and learning new stuff.


      TGI says moo

[reply]
[d/l]
[select]
        Hi TGI, about sorting the settings hash, I thought the user should be prompted first for input then for output,then pattern, replacement , which happens to be the alphabetical order of the problem parameters! Also I set the initial value to 0 because we could use a simple if to test the settings (e.g. if($settings{output}) {open...}.

        Your links are great! Safari has an online version for 'Best Practices' and looks like a great book. 'Higher Order Perl' needs you to have stomach, but I am totally in for it. Perl has some much flexible and expressive structures that make functional programming really really relevant.
[reply]
[d/l]
[select]

In Section Seekers of Perl Wisdom