Re: checking values of variables
by davido (Cardinal) on Aug 12, 2013 at 18:15 UTC
|
All of these variables are related in some way. I don't know how they're related, but obviously they must be, or you wouldn't be testing them all at once as you are. It seems that their relationship is, at least, that failure to be set somehow will generate a call to help(). Since they're all related, it makes more sense (to me) to group them together in a single container. And since they seem to each have descriptive names, the container would be a hash.
You also mentioned somewhere in this thread that you might not be testing each one against an empty string. I assume that means you might want to test them all against either a predetermined value, or a pattern match, or a range of permissible values.
This would be fun to code in a more elegant way, but I would at least want to know what sorts of tests would be required for each value; tests against a single value, an enumerated list of possibilities, a pattern match, etc. But my thought is that you would also have a hash that associates these keys with the values they're allowed to have. Then you can do a Boolean test by iterating over the elements of the inputs and comparing them against the same elements of the validating hash. Sometimes simply associating coderefs with the validating hash's keys is helpful; each coderef would return true or false if the value passed in as a parameter validates against what you want to permit.
| [reply] [d/l] |
Re: checking values of variables
by toolic (Bishop) on Aug 12, 2013 at 14:25 UTC
|
Instead of declaring 50 variables, you could declare a single hash.
my %params;
# some code here which may or may not add keys to %params
print "help\n" unless %params;
| [reply] [d/l] |
|
I guess that would not work, because the OP apparently wants to trigger the help function if any of the variables is absent, not if they are all missing.
So using a hash, it would have to be something like this:
help() if 22 > scalar values %params; # scalar unnecessary, just to make the context clearer
But that would not be very robust, for example if the %params may have some elements other that the list of mandatory parameters.
So an approach more reliable than just counting the hash values would be to test each and every single mandatory param:
for (qw / server database user password ... initversion /) {
help() and last unless (defined $params{$_} and $params{$_});
}
The exact test on the %params values would depend on how the hash is initialized. | [reply] [d/l] [select] |
|
| [reply] |
Re: checking values of variables
by Laurent_R (Canon) on Aug 12, 2013 at 20:45 UTC
|
So you have really a hash to start with. No point of doing this long list of assignments if the hash is not valid and complete. I therefore come back to the solution I first gave here Re^2: checking values of variables as an answer to toolic's post, just adapting it to the hashref at hand:
for (qw / server database user password ... initversion /) {
help() and last unless (defined $cfg->params($_) and $cfg->params
+($_));
Although, to tell the truth, my suggestion was just to give a minimal solution. In real life, I would first create an array of the mandatory parameters, and check against that array, rather than a hard coded list. Something like this:
my @mandatory_params = qw / server database user password ... initvers
+ion /;
for (@mandatory_params) {
help() and last unless (defined $cfg->params($_) and $cfg->params
+($_));
}
The point of doing that is that the list of parameters is likely to be useful elsewhere, storing it into an array enables to have it defined in one single place. If you need to change it, there is only one place where it needs to be changed.
And, BTW, I do not know what the rest of your program does, but I would most probably not do a long list of assignments of scalar variables with the values of the cfg->param hash; in this case, this is almost like hard coding things than need not be hard coded. Copy the hash ref values into a simple hash, take a simple reference to it, do what you need to gain an access that is easier to understand in your eyes, but don't store all these values into individual scalars, you are losing flexibility and maintainability.
Update: I had not seen davido's answer when I started to make mine, but I think we agree on the basic issues, even though our solutions might not be exactly be the same.
| [reply] [d/l] [select] |
|
this question garnered a fair amount of interest and I appreciate the replies.
| [reply] |
|
| [reply] |
|
|
Re: checking values of variables
by Laurent_R (Canon) on Aug 12, 2013 at 15:53 UTC
|
for my $param ($server, $database, $user, $password, ...
$sendmail, $initversion)
{
help() and last if ($param eq q{});
}
| [reply] [d/l] |
Re: checking values of variables
by ramlight (Friar) on Aug 12, 2013 at 15:08 UTC
|
The code that have posted is rather cumbersome, but it does have one significant advantage: it will be much easier to understand a year or so down the road when a small change has to be made - especially if someone else has to make the change. So while there are more elegant ways of coding this, the code you have is about as clear an expression of your desired behavior as you will find. | [reply] |
|
use List::MoreUtils qw(any);
...
help() if any { $_ eq q{} } $server, $user, $database, $password, ...
+;
| [reply] [d/l] [select] |
|
While your suggested code is a good solution; I was thinking along the lines of being able to match more than just an empty string for each case. So List::MoreUtils could be used to express the code this way:
help() unless all { $_ } $server, $user, $database, ...
or without List::MoreUtils:
help() unless $server && $user && $database ..
but I still think that the original code makes it very clear what is going on and provides future flexibility. I tend to believe that code golf and maintainablity are often at odds.
| [reply] [d/l] [select] |
|
|
|
| [reply] |
Re: checking values of variables
by BillKSmith (Monsignor) on Aug 12, 2013 at 18:47 UTC
|
Are you validating the named parameters of a function or method? If so, see Params::Validate.
| [reply] |
|
here's the whole thing; should have posted it in the 1st place.
# open configuration file
my $cfg = new Config::Simple($config_file)
or die "Can't open configuration file: $config_file\n";
$server = $cfg->param('server');
$database = $cfg->param('database');
$user = $cfg->param('user');
$password = $cfg->param('password');
$logdir = $cfg->param('logdir');
$basedir = $cfg->param('basedir');
$inputdir = $cfg->param('inputdir');
$inputextension = $cfg->param('inputextension');
$cleanupdays = $cfg->param('cleanupdays');
$countyid = $cfg->param('countyid');
$unzipdir = $cfg->param('unzipdir');
$sleeptime = $cfg->param('sleeptime');
$mail_host = $cfg->param('mail_host') || 'mail@xxx.org';
$support_addr = $cfg->param('support_addr') || 'support@xxx.org'
+;
$support_cc = $cfg->param('support_cc') || q{};
$ar_court_text = $cfg->param('ar_court_text');
$ar_court_ori = $cfg->param('ar_court_ori');
$debug_print = $cfg->param('debug_print');
$delete_txt_file = $cfg->param('delete_txt_file');
$from_email_address = $cfg->param('from_email_address');
$to_email_address = $cfg->param('to_email_address');
$cc_email_address = $cfg->param('cc_email_address');
$send_email = $cfg->param('send_email');
$init_version = $cfg->param('init_version');
&help()
if ( $server eq q{}
|| $database eq q{}
|| $user eq q{}
|| $password eq q{}
|| $logdir eq q{}
|| $basedir eq q{}
|| $inputdir eq q{}
|| $inputextension eq q{}
|| $cleanupdays eq q{}
|| $countyid eq q{}
|| $sleeptime eq q{}
|| $unzipdir eq q{}
|| $mail_host eq q{}
|| $support_addr eq q{}
|| $ar_court_text eq q{}
|| $ar_court_ori eq q{}
|| $debug_print eq q{}
|| $delete_txt_file eq q{}
|| $from_email_address eq q{}
|| $to_email_address eq q{}
|| $send_email eq q{}
|| $init_version eq q{}
);
| [reply] [d/l] |
|
use constant CONFIG_ITEMS => [ qw(
server database user
password logdir basedir
inputdir inputextension cleanupdays
countyid unzipdir sleeptime
unzipdir mail_host support_addr
ar_court_text ar_court_ori debug_print
delete_txt_file from_email_address to_email_address
send_email init_version
] );
my %config;
foreach my $item ( CONFIG_ITEMS ) {
my $value = $cfg->param($item);
help($item) unless defined $value and length $value;
$config{$item} = $value;
}
sub help {
die "Parameter $_[0] wasn't set in config file.";
}
It would still be more fun if we were checking for valid config values, rather than just the existence of a value, but at minimum, this seems to be what you're asking for.
If you are checking for validity also, then make CONFIG_ITEMS a hashref such that:
use constant CONFIG_ITEMS => {
server => sub { my $param = shift; ...some Boolean test },
database => sub { ... },
user => ...
};
...or...
use constant CONFIG_ITEMS => {
server => qr/some pattern/,
database => qr/some pattern/,
...
};
And then...
foreach my $item( keys %{CONFIG_ITEMS} ) {
my $value = $cfg->param($item);
help() unless CONFIG_ITEMS->{$item}->($value);
$config{$item} = $value;
}
...for example.
There's probably a module for this, but it's not that hard to implement from scratch for a trivial use. As things get more complex, start looking for a well tested solution.
| [reply] [d/l] [select] |
|
| [reply] |