Ok, here's a simplistic way to replace one of the string of elsifs with a hash:
my %vars = (
'dir-mode' => \$d_mode,
'file-mode' => \$f_mode,
'link-mode' => \$l_mode,
'dir-owner' => \$d_own,
'file-owner' => \$f_own,
'link-owner' => \$l_own,
'dir-group' => \$d_grp,
'file-group' => \$f_grp,
'link-group' => \$l_grp,
);
for my $testy (keys %{ $config{$test}{$tester} }) {
unless ($vars{$testy}) {
warn "Unexpected test key '$testy'";
next;
}
${ $vars{$testy} } = $config{$test}{$tester}{$testy};
}
This is simply saying: we have a string of (els)ifs, and whichever string we match we're going to execute almost the same code, so let's abstract out the one bit that varies (the variable name in this case) and let the hash associate the string to match with the variable name to modify, leaving the rest of the code constant.
However, we also have a series of similar variable names, which is also often a sign that a hash is needed. So let's look at the point we're using these variables: I'd characterise the wanted_c subroutine as doing something like:
- where specified, update any of the mode, owner or group of the file
- but select a different set of updates depending on whether the file to update is a directory, a link or a plain file
so I'd be inclined to map that to a data structure that looks like: my $wanted_this_time = {
mode => $wanted_mode,
owner => $wanted_owner,
group => $wanted_group,
};
but pick one of three such structures depending on the type of file: my $wanted = {
if_directory => {
mode => $dir_mode,
owner => $dir_owner,
group => $dir_group,
},
if_link => { ... }
if_file => { ... }
};
and having made that conceptual transformation, we can now get rid of the matrix of individually named variables:my %wanted;
my %validfile = map +($_ => 1), qw/ dir file link /;
my %validupdate = map +($_ => 1), qw/ mode owner group /;
for (keys %{ $config{$test}{$tester} }) {
my($filetype, $updatetype} = split /-/, $_, 2;
unless ($validfile{$filetype} && $validupdate($updatetype}) {
warn "Unexpected directive '$_'";
next;
}
$wanted{$filetype}{$updatetype} = $config{$test}{$tester}{$_};
}
[...]
sub wanted_c {
stat($_);
my $wanted =
(-d _ ? $wanted{'dir'})
: (-l _ ? $wanted{'link'})
: (-f _ ? $wanted{'file'})
: return; # ?? warn unexpected type?
return unless $wanted; # nothing to change
if (defined $wanted->{'mode'}) {
change_mode($_, $wanted->{'mode'});
}
if (defined $wanted->{'owner'}) {
change_owner($_, $wanted->{'owner'});
}
if (defined $wanted->{'group'}) {
change_group($_, $wanted->{'group'});
}
}
Note that you could as easily represent this data using arrays rather than hashes, but it then becomes much less self-documenting unless you also use named constants for the indexes into each array.
In general, whenever I see code that needs to cope with several different cases, my instinct is to ask: what effort will be required when another new case needs to be added to the list? How far can I reduce that effort? How can I minimise the danger of getting it wrong when I need to add a new case, for example by adding it in one place and not in another?
Usually, the answer is: use data structures to minimise code duplication; where possible, encapsulate all the information for the cases in a single data structure, so all the work involved in adding a new case is reduced to adding a new entry into that data structure.
It may be overkill for this script, but we can take that extra step by taking advantage of code references. The data structures might then look something like this: my %filetypes = (
'dir' => sub { -d _ },
'link' => sub { -l _ },
'file' => sub { -f _ },
);
my %updatetypes = (
'mode' => \&change_mode,
'owner' => \&change_owner,
'group' => \&change_group,
);
sub wanted_c {
stat($_);
for my $filetype (keys %wanted) {
next unless &{ $filetypes{$filetype} };
my $wanted = $wanted{$filetype};
for my $update (keys %$wanted) {
&{ $updatetypes{$update} }($_, $wanted->{$update});
}
}
}
However, I suspect that would not be desirable, at least for the update types, since it makes it much harder to add interdependencies between the update types such as (for example) updating ownership before mode.
Hugo |