Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change
 
PerlMonks  

Re: The Boy Scout Rule

by hippo (Chancellor)
on Jan 25, 2015 at 14:04 UTC ( #1114438=note: print w/replies, xml ) Need Help??


in reply to The Boy Scout Rule

To answer the last question, I have to say that

my $value = [ $x => $y ] -> [ $y <= $x ];

would not pass my code review. It is clever, but apparently pointlessly so. The fat comma in particular appears to be present only to cause confusion - a normal comma would add (a little) clarity. This code, even if commented, would cause many programmers to pause while they worked out quite what it was doing. That may only take a couple of seconds for an expert but pity the poor Perl newcomer who stumbles upon this.

For my money the ternary conditional version is perfectly clear and without overhead and would be the way I would choose to code this operation.


Regarding code reviews in general and workspace policies - I am essentially freelance and therefore am exposed to a wide range of different restrictions and policies. Generally speaking there are coding standards and a lot of the time these are enforced for the most part automatically (ie. on commit or pre-release). I don't do any pair-programming but there are code reviews of varying nature most of which would fit into your "lightweight" category. They don't tend to dwell on the minutiae; it is more a case of establishing clarity of purpose and eliminating flaws in security and robustness and promoting efficiency.

It is my personal belief (opinion alert!) that it will benefit any programmer to be exposed to code written in a wide variety of styles. That is partly why I am here in these hallowed halls. Here I see idioms, layouts, compound operators, data structures and algorithms which I would not generally have considered myself, to say nothing of being introduced to many useful modules which would otherwise have escaped my attention. With that in mind, communication between programmers whether on online fora or within (or between) development teams or even RLMs such as Perl Mongers are to be encouraged.

Thanks for this interesting meditation.

Hippo

Replies are listed 'Best First'.
Re^2: The Boy Scout Rule
by Your Mother (Archbishop) on Jan 25, 2015 at 20:34 UTC

    my $value = [ $x => $y ] -> [ $y <= $x ]; would not pass my code review.

    I agree itís confusing the first time and the => should be a skinny comma :P instead. That said, the Schwartzian transform is even more confusing the first time you see it. No one in a post 5.6 Perl world would suggest rewriting it with a bunch of temp arrays and for blocks. So, I advocate simple little idioms like the above when they offer something more than clever/pretty code.

    So, thinking to possibly defend the clever/pretty one, I tried a Benchmark, which I'm not necessarily doing right so someone please jump in if itís badly formed, and the one that might be the most semantically clear and I assumed would be the slowest is the fastest by a good measure. I forgot that List::Util is XS.

    use strictures; use Benchmark "cmpthese"; use List::Util "min"; # This is XS. my @xy = ( [ 1, 0 ], [ 0, 1 ], [ 0, 0 ], [ 1, 1 ], [ 1_000_000, 999_999 ], [ 999_999, 1_000_000 ] ); my $m; # Avoid void in comparisons. cmpthese(10_000_000, { list_util => sub { $m = min(@$_) for @xy }, ternary => sub { $m = $_->[0] < $_->[1] ? $_->[0] : $_->[1] for +@xy }, clever => sub { $m = [ $_->[0], $_->[1] ]->[ $_->[0] <= $_->[1] + ] for @xy }, });
    Rate clever ternary list_util clever 347584/s -- -56% -68% ternary 792393/s 128% -- -28% list_util 1096491/s 215% 38% --

      Several problems:

      • With just 6 comparisons & assignments being run; the overhead of two subroutine calls -- the one you wrapped your tests in and the one Benchmarks wraps those in internally -- becomes a significant factor in the tests.

        Pass strings instead of subs to remove one layer of sub-call overhead; and allow benchmark to eval them into subroutines. (It's going to anyway!)

        Use an internal loop multiplier to re-balance the test/overhead.

      • By passing your args wrapped in anonymous arrays -- thus forcing the ternary to do 3 dereferences; and the clever to do 4 dereferences; whereas List::Util only does one -- you bias the tests strongly in List::Util's favour.

        Most min() operations will operate on simple scalars so use those instead.

      • (Minor.) There is little value in using different and big integers; they are all just IVs (or UVs) as far as the comparisons are concerned.

        Test for differences in ordering (branch/no branch) by coding separate tests.

      The upshot is that the ordering makes no consistent difference (Ie. it flip flops from run to run); and that the ternary is hands down winner for the two simple scalars, common case:

      use strictures; use Benchmark "cmpthese"; use List::Util "min"; # This is XS. cmpthese -1, { list_util_nb => q[ my( $x, $y ) = ( 0, 1 ); my $m = min( $x, $y +) for 1 .. 1000; ], ternary_nb => q[ my( $x, $y ) = ( 0, 1 ); my $m = $x < $y + ? $x : $y for 1 .. 1000; ], clever_nb => q[ my( $x, $y ) = ( 0, 1 ); my $m = [ $x, $y + ]->[ $x <= $y ] for 1 .. 1000; ], list_util_b => q[ my( $x, $y ) = ( 1, 0 ); my $m = min( $x, $y +) for 1 .. 1000; ], ternary_b => q[ my( $x, $y ) = ( 1, 0 ); my $m = $x < $y + ? $x : $y for 1 .. 1000; ], clever_b => q[ my( $x, $y ) = ( 1, 0 ); my $m = [ $x, $y +]->[ $x <= $y ] for 1 .. 1000; ], }; __END__ C:\test>junk30 Rate clever_b clever_nb list_util_nb list_util_b ternar +y_b ternary_nb clever_b 1210/s -- -11% -67% -70% - +79% -80% clever_nb 1356/s 12% -- -63% -67% - +76% -78% list_util_nb 3694/s 205% 172% -- -9% - +34% -40% list_util_b 4062/s 236% 200% 10% -- - +28% -33% ternary_b 5630/s 365% 315% 52% 39% + -- -8% ternary_nb 6107/s 405% 351% 65% 50% + 8% -- C:\test>junk30 Rate clever_nb clever_b list_util_b list_util_nb ternar +y_b ternary_nb clever_nb 1297/s -- -5% -68% -69% - +75% -77% clever_b 1372/s 6% -- -66% -67% - +74% -75% list_util_b 4078/s 214% 197% -- -3% - +22% -27% list_util_nb 4190/s 223% 205% 3% -- - +20% -25% ternary_b 5228/s 303% 281% 28% 25% + -- -6% ternary_nb 5556/s 328% 305% 36% 33% + 6% --

      List::Util::min() will obviously win in both speed and clarity for the min( @array ) case.


      With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
      Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
      "Science is about questioning the status quo. Questioning authority". I'm with torvalds on this
      In the absence of evidence, opinion is indistinguishable from prejudice. Agile (and TDD) debunked

        Thank you for the detailed fix. Unbiasing benchmarks is something I have been needing to improve for a long time.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://1114438]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others lurking in the Monastery: (4)
As of 2021-04-19 02:34 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found

    Notices?