jhumphreys:

"Not working" isn't much of a description. If I had to guess (and I do, since I don't know what "Not working" means), I'd guess that it appears to hang, since you don't reprompt if there's an error. Perhaps you ought to put an error message in there. For example:

OUTER1: while (<STDIN>) {
   chomp;
   for $to (%rates) {
      last OUTER1 if $to eq $_;
   }
   print "I can't find a conversion for '$_'!\n";
}
[download]

Also, there are better ways to check your hash. They are, after all, optimized for searching. Read perldoc perldsc and perldoc -f exists for clues to simplify your loops!

...roboticus

When your only tool is a hammer, all problems look like your thumb.

roboticus-

You're right, I should've given a better description. Thanks again for your help.

-j

If you want to put label on a loop so you can use 'next', 'last', 'redo', there's no need to label loops you don't go to. Also, I would suggest using a more meaningful name. Instead of calling the labels OUTER0 and OUTER1, how about naming them SOURCE_CURRENCY and TARGET_CURRENCY?

Going through the elements of %rates isn't what you want to do. The whole point of a hash is to look up keys and determine the corresponding value. Just like the point of an array is to look up the i-th element. I would suggest:

if ( ! defined $rates{$_} {
   print "Don't know how to handle unrecognized rate '$_'.\n";
   # handle error in some way.
}
print $rate{$_}
[download]

But since the code is becomming longer, i would definitely suggest using a named variable to store user input, such as $source_currency and $target_currency.

Also, using %rates the way you have, generates a list of a key and it's corresponding value, another key with it's value, for some random ordering of keys. If you really wanted to look at the keys one by one, you can use keys %rates to generate just that list. Similarly, you can use values %rates to generate a list of just the values. The keys and values lists will be in the same order, but may not be the same as you would get on a different machine, or even if add or delete an element.

TomDLux-

Excellent feedback. Thanks for your help.

-j

IO::Prompt::Hooked handles the details of prompting, detecting an interactive environment, and accepting callbacks to validate user input and to issue meaningful warnings if the user input is invalid. It continues to loop upon invalid input until it gets good input, or until an optional "tries" count reaches zero. Here's an example of how it could be used for your project.

use strict;
use warnings;

use Scalar::Util qw( looks_like_number );
use IO::Prompt::Hooked;

my %rates = (
  'pounds'        => 1,
  'usd'           => 1.6,
  'marks'         => 3.0,
  'french francs' => 10.0,
  'yen'           => 174.8,
  'swiss francs'  => 2.43,
  'drachma'       => 492.3,
  'euro'          => 1.5
);


my ($from, $to) = get_targets(\%rates);

my $value = get_value();

my $converted = $value * ($rates{$to} / $rates{$from});
printf "$value $from is %.2f $to.\n", $converted;


# Manage prompting for "from" and "to" currencies.
sub get_targets {
  my $currencies = shift;
  return
    map { get_currency($_, $currencies) }
    ('Enter your starting currency:', 'Enter your target currency:');
}

# Prompt for an individual currency.  5 tries.  Die if we don't get
# good input.
sub get_currency {
  my ($msg, $curr) = @_;
  my $in = prompt(
    message  => $msg,
    tries    => 5,
    validate => sub { return exists $curr->{lc shift}; },
    error    => sub {
      my $valid = join ', ', keys %{$curr};
      return "Currency must be one of the following: ($valid).\n"
        . "$_[1] tries remaining.\n";
    },
  );
  die 'Too many tries. Consult someone who can read.' unless defined $
+in;
  return lc $in;
}

# Prompt for a value to convert.  Die if we don't get good input after
# five tries.
sub get_value {
  my $input = prompt(
    message  => "Enter an amount to convert:",
    default  => 0,
    tries    => 5,
    validate => sub { return looks_like_number shift; },
    error    => sub {
      return "Amount must be a number.  $_[1] tries remaining.\n";
    },
  );
  die "Unable to obtain a valid value for conversion." unless defined 
+$input;
  return $input;
}
[download]

Here we prompt for a "from" and a "to" currency. We provide the user five tries at getting it right, and upon receiving invalid input, we prompt with additional information enumerating the valid inputs. If after five tries the user can't figure out how to get it right, we die. Then we move on to getting a value, and again loop until we actually get a number (or until the allotted number of tries expires). Finally, we perform the conversion, and output it, rounded to two decimal places.

This is sort of a shameless plug for the module. What I like is that it handles the logic and looping. All the module's user has to do is provide callback subs that perform the validation, and that provide an error string in case of bad input. The error string could also be a simple string instead of a subref, but the subref provides the opportunity to output the number of remaining tries. The module is based on IO::Prompt::Tiny, but while it's still smaller and easier to use than the granddaddy, "IO::Prompt", it's not tiny either. ;)

As for how this example code turned out: You might say, "But it's bigger than the original code." ...Yes, it is. But it is doing more; it's validating both the targets and the numeric values, it's providing meaningful messages for invalid input, and it's dieing if the user can't seem to figure out how to provide valid input after a reasonable number of tries.

Update: Changed all hash keys to lower case per frozenwithjoy's observation. Output will be 'usd' instead of 'USD'. Does it matter? ;)

I like this; however, USD isn't working for me (presumably because of the lc in validate => sub { return exists $curr->{lc shift}; },

Edit: I fixed it by changing that line to:

 validate => sub { my $chosen_curr = shift; return exists $curr->{$cho
+sen_curr}; },
[download]

And by removing lc from return lc $in;

Good call. :) It might be better to just change the hash so that all the keys are lower cased. I'll update.




Dave

Dave-

Thanks for your feedback. Obviously, I'm just beginning in Perl. But will study.

Best,

-j

#!/usr/bin/env perl
# convert3.pl
use warnings;
use strict;

my ( $value, $from, $to, $rate, $rates, %rates );
%rates = (
    pounds          => 1,
    USD             => 1.6,
    marks           => 3.0,
    "french francs" => 10.0,
    yen             => 174.8,
    "swiss francs"  => 2.43,
    drachma         => 492.3,
    euro            => 1.5
);
print "Enter your starting currency: ";
OUTER0: while (<STDIN>) {
    chomp;
  INNER0: for my $cur (keys %rates) { # added keys so you don't loop t
+hrough values, too
        $from = $cur; # set $from currency
        last OUTER0 if $from eq $_;
    }
    print "Currency not recognized, try again: "; # Added to avoid con
+fusion when an unknown currency is entered
}
print "Enter your target currency: ";
OUTER1: while (<STDIN>) {
    chomp;
  INNER1: for my $cur (keys %rates) { # added keys so you don't loop t
+hrough values, too
        $to = $cur; # set $to currency
        last OUTER1 if $to eq $_;
    }
    print "Currency not recognized, try again: "; # Added to avoid con
+fusion when an unknown currency is entered
}
print "Enter your amount to convert: ";
$value = <STDIN>;
chomp $value; # added to fix format of output
$rate  = $rates{$to} / $rates{$from};
print "$value $from is ", $value * $rate, " $to. \n";

__END__
Enter your starting currency: french francs
Enter your target currency: swiss francs
Enter your amount to convert: 100
100 french francs is 24.3 swiss francs.
[download]

frozenwithjoy-

Thanks for your help!

Best,

-j