Am I developing poor style?

el-moe has asked for the wisdom of the Perl Monks concerning the following question:

Replies are listed 'Best First'.
Re: Am I developing poor style? by btrott (Parson) on Jan 17, 2001 at 00:49 UTC
You could use the sum function from List::Util: `use List::Util qw/sum/; $header{"Number of Drills"} = sum map $_->count, map { values %{ $_->tool } } @drill_arr;` [download] I haven't benchmarked this so don't know how it compares to yours. I'd guess it'd be faster since yours has to build up a code ref then get rid of it, etc. And sum is implemented in C, so it's fast. Update: okay, benchmarked them: `timethese(-6, { 'sum' => sub { my $n = sum map $_->{count}, map { values %{ $_->{tool} } } @drill_arr; }, 'sub' => sub { my $n = &{ sub { my $ret; foreach my $l ( @drill_arr ) { foreach ( values %{$l->{tool}} ) { $ret += $_->{count}; } } return $ret; } }; }, });` [download] Results: `Benchmark: running sub, sum, each for at least 6 CPU seconds... sub: 7 wallclock secs ( 6.00 usr + 0.00 sys = 6.00 CPU) @ 15 +883.17/s (n=95299) sum: 6 wallclock secs ( 6.02 usr + 0.00 sys = 6.02 CPU) @ 40 +993.85/s (n=246783)` [download] So using sum is lots faster, apparently. Note however that I'm not using method calls; I just set up a dummy array holding hashes of hashes that I'm using.	[reply] [d/l] [select]
(tye)Re3: Am I developing poor style? by tye (Sage) on Jan 17, 2001 at 00:56 UTC
Ah, to have list-context dereference semantics... `$header{"Number of Drills"}= sum ( values @drill_arr==>tool==>% )==>count;` [download] yummy. - tye (but my friends call me "Tye")	[reply] [d/l]
Re: Am I developing poor style? by chipmunk (Parson) on Jan 17, 2001 at 00:43 UTC
Instead of creating an anonymous sub which you call and then discard, you could use `do` with a block: `$header{"Number of Drills"} = do { my $ret; foreach my $l ( @drill_arr ) { foreach ( values %{$l->tool} + ) { $ret += $_->count; } } $ret; };` [download]	[reply] [d/l] [select]
(tye)Re: Am I developing poor style? by tye (Sage) on Jan 17, 2001 at 00:41 UTC
You can replace "&{ sub {" with "do {" if you drop the "return". - tye (but my friends call me "Tye")	[reply]
Re: Am I developing poor style? by chromatic (Archbishop) on Jan 17, 2001 at 01:01 UTC
I wouldn't use an anonymous sub for that first bit. Maybe it's one case where map in a void context would work. On the other hand, if this is something you might want to do later, there's no reason not to turn it into a function itself. Regardless, here's the damage that Dominus may cheer and merlyn will jeer: `$header{'Number of Drills'} = (map { $total += $_->{count} foreach (values %{ $_->{tool} }); $total } @drill_arr)[-1];` [download] Yuck. :) Update: I spent too long getting this to work and forgot about do. Don't hate me because I wrote perverse code. Just live and learn.	[reply] [d/l]
Understanding Structures through Variable Names by tedv (Pilgrim) on Jan 17, 2001 at 04:24 UTC
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(); } }` [download] 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	[reply] [d/l] [select]
Re: Am I developing poor style? by coreolyn (Parson) on Jan 17, 2001 at 00:48 UTC
No time to test so I'll just give you somve verbal thoughts from the hip. In your first example you lose near nothing by more clearly labling your $1 var as $drill. Using 'map' could truely be your friend. Finally played with 'map' the other day... it's a wonderful efficiency tool. for loading arrays. coreolyn	[reply]
(tye)Re2: Am I developing poor style? by tye (Sage) on Jan 17, 2001 at 00:52 UTC
I'd avoid the deep indentation that is based on the contents that preceed it. If you decide to rename @layer_list, %header, or "Number of Drills" then you may have to reindent a bunch of lines in order to preserve your current style. - tye (but my friends call me "Tye")	[reply]
Re: Am I developing poor style? by Anonymous Monk on Jan 17, 2001 at 00:36 UTC
Your second example would be more efficient if you replaced "} grep {" with "and", IMHO.	[reply]
Re: Am I developing poor style? by el-moe (Scribe) on Jan 17, 2001 at 23:57 UTC
Ok... so some things I see that you all are talking about... 1. I need to have better naming of vars. 2. My class design is not very good. That is my first attempt at OOPerl. I will read the perltoot again. I made the object with Class::Struct ... is that a good idea? 3. The anonymous sub thing would be better with a do block. 4. Don't run multiple greps... use "and" instead. 5. Keep reading perlmonks.org so I can get stronger with this terrific language. Did I miss anything? Thanks for the insight all. I really appreciate your guidance. Prost, Moe	[reply]