The checking for allowed values is faulty.
The grep
function takes a code block and a list. It sets the $_
variable to each element in the list in turn. If the block returns a true value, the element is kept. In scalar context, grep
does not return a list of kept elements, but a count.
Your grep
block is {!$args->{$k}}
. This returns true when $args->{$k}
is false and vice versa. The result does not depend on $_
, and therefore doesn't check if the argument is one of the allowed values.
To see if the given value is allowed value, you'll have to test for some form of equivalence, e.g.
if (grep { $args->{$k} eq $_ } @{ $opts->{$k}{allowed} }) {
# this is executed when the arg matches an allowed value
} else {
# the arg is not allowed
}
An Excursion To Smart Matching and List::MoreUtils
If you can use a perl > v10, then smart matching is available. This would express above condition as
use 5.010;
$args->{$k} ~~ $opts->{$k}{allowed}
The lengthy table of possible type combinations states that this is roughly equivalent to the grep
if the arg is a scalar (string/number), and the allowed arrayref holds only normal scalars as well.
However, smart matching was re-marked as experimantal in v18, and behaviour will likely change soon.
In the meantime, it might be better to stick to explicit grep
etc. But we could implement two improvements:
The
grep
will test all elements, even when a match was already found. This can be inefficient. Thefirst
function fromList::Util
core module has the same syntax asgrep
, but stops after the first element. If the block matches a value, this value is returned. If no value matches, it returnsundef
. This makes things complicated whenundef
might be a valid value, or even when false values may be allowed. But in your case, thegrep
could be replaced byuse List::Util 'first'; defined first { $_ eq $args->{$k} } @{ $opts->{$k}{allowed} }
The List::MoreUtils module has even more functionality. It provides for example the
any
function, which corresponds to the mathematical ∃ (there exists) quantifier:use List::MoreUtils 'any'; any { $_ eq $args->{$k} } @{ $opts->{$k}{allowed} }
This only returns a boolean value. While it may not be as efficient as a plain
grep
orfirst
, usingany
is quite self-documenting, and easier to use.Until now, I have assumed that we'll only ever do string comparision to the allowed values. This sometimes works, but it would be better to specify an explicit mode. For example
croak qq(Value for "$k": "$args->{$k}" not allowed) unless $opts->{$k}{mode} eq 'str' and any { $args->{$k} eq $_ } @{ $opts->{$k}{allowed} } or $opts->{$k}{mode} eq 'like' and any { $args->{$k} =~ $_ } @{ $opts->{$k}{allowed} } or $opts->{$k}{mode} eq 'num' and any { $args->{$k} == $_ } @{ $opts->{$k}{allowed} } or $opts->{$k}{mode} eq 'smart' and any { $args->{$k} ~~ $_ } @{ $opts->{$k}{allowed} } or $opts->{$k}{mode} eq 'code' and any { $args->{$k}->($_) } @{ $opts->{$k}{allowed} };
Preventing unknown options
You may or may not want to forbid unknown options in your $args
hash. Especially if you consider composability of classes, you may want to ignore unknown options, as a superclass or subclass may need these.
But if you choose to check for wrong options, you could delete
those elements you already handled:
my $self = {};
for my $k (keys %$opts) {
my $v = delete $args->{$k};
...; # use $v in the rest of the loop
$self->{$k} = $v;
}
croak "Unknown arguments (" . (join ", ", keys %$args) . ") are forbidden" if keys %$args;
or grep
for unknown args:
my @unknown = grep { not exists $opts->{$_} } keys %$args;
croak "Unknown arguments (" . (join ", ", @unknown) . ") are forbidden" if @unknown;
for my $k (keys %$opts) {
...;
}
or you could loop over the combined keys of $args
and $opts
:
use List::Util 'uniq';
for my $k (uniq keys(%$opts), keys(%$args)) {
croak "Unknown argument $k" unless exists $opts->{$k};
...;
}
Scalar Context
I have assumed that you correctly initialized $args
as a hash reference:
my $args = { file => 'file.txt', type => 'foo', head => 1 };
Using parens instead of curlies is syntactically valid:
my $args = ( file => 'file.txt', type => 'foo', head => 1 );
but this doesn't produce a hash. Instead, the =>
and ,
behave like the comma operator in C: the left operand is evaluated and discarded. That is, only the last element is kept:
my $args = 1; # equivalent to above snippet.