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.

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.

The thing is there is no reason to sort the hash keys, if you iterate over the list of keys you used to generate the settings hash, the keys will be displayed in the order you specify them. So if you enter the keys in alphabetical order, then you get them back in that order.

There is no reason to initialize the hash values to prepare for logical testing. Multiple values in perl are false, '', 0, and undef are all false. This leads to a good reason not to initialize your hash. What would happen if 0 is a legal value for one of your parameters? Your test would fail. It's safer to test for definedness: if( defined $settings{output} ) { do stuff }. Forgetting that 0 is a defined but false value is a distressingly common bug in perl code. Testing if a variable is defined is a common task. The commonness of this test lead to the addition of the defined or operator (//) in perl 5.10.

So, testing for definedness is good, but in the context of this code it is useless because the loop that sets the parameters initializes every key to a string of 0 or more characters. In this case it makes more sense to test for string length: if( length $settings{output} ) { do stuff }. If you didn't care if 0 was rejected, the empty string is false, so you still don't need to initialize to 0.  if( '' ) { never runs }

Now, you may be thinking that having to worry about do I need to test or length or definedness or whatever gets difficult if you have a big program. And that It would be nice to just remember to test for definedness. But the cursed loop ruins that approach since it initializes everything. The solution is simple:

for my $key (@params) {
    print "$key = ";
    my $value = <>;
    chomp $value;
    $settings{$key} = length $value ? $value : undef;
}
[download]

Now you have a nice, expected undef value to look for. The great thing about this, is that if you have warnings enabled, if you try to use the undef value in a string or math operation, you'll get a warning. This provides a handy alert that you forgot to validate some input or other.

This technique works along the same lines as the redo I used in my second response to you.

If you've got safari access, I have no qualms in suggesting the Camel (Programming Perl) as well. BTW, don't just swallow every bit of advice in PBP. There has been a lot of discussion here and elsewhere about the various topics in the book.


TGI says moo