Something to keep in mind is the deep nested nature of your
hashes (objects). You have a list of hash refs. You call the
tool() function on these objects (without parentheses) which
returns a reference-to-hash. But you need a list of values
in the hash itself. These values are, themselves, objects
which you only care about so you can call the "count" function
on them (once again without parentheses).
I feel like somewhere far back on the design board, the data
structures and APIs were not designed properly. First, it's not
even clear what kind of data you have after you run
values %{$l->tool}.
Yes, it's a list... But a list of what??? Suppose your had a function
called "get_tool_list". Then you can write something like...
foreach my $tool ( $l->get_tool_list() ) { ... }.
Your code would be much better off if you forced yourself to
use
meaningful variable and function names. If the
function names no longer match their purpose, either redesign the API,
the class, or the function.
On the subject of variable naming,
$l is not a recommended variable name.
Not only does it look like
$1, but it doesn't give you
any clues about what the variable is used for. You'd be much better
off writing something like...
my $count = 0;
foreach my $drill (@drills) {
foreach my $tool ($drill->get_tools()) {
$count += $tool->count();
}
}
Now that code looks much cleaner. You might be able to clean
up the code a little more, now that you know what everything does,
using some other looping syntax.
The second block of code is not that bad, although clearly
using 1 grep is better than 2 greps. It's only bad if you
need to run that grep every single time you access an element.
The code structure makes me think that @layer_list won't change
between calls, so it's best to pre-compute that data once and then
directly access @layer_list (instead of recomputing it on each
function call). Of course, I'm making a large number of assertions
about the surrounding context of this code, so use your better judgement.
-Ted