Stories
Slash Boxes
Comments
NOTE: use Perl; is on undef hiatus. You can read content, but you can't post it. More info will be forthcoming forthcomingly.

All the Perl that's Practical to Extract and Report

use Perl Log In

Log In

[ Create a new account ]

Ovid (2709)

Ovid
  (email not shown publicly)
http://publius-ovidius.livejournal.com/
AOL IM: ovidperl (Add Buddy, Send Message)

Stuff with the Perl Foundation. A couple of patches in the Perl core. A few CPAN modules. That about sums it up.

Journal of Ovid (2709)

Monday November 24, 2008
09:03 AM

Stupidly Clever Code

[ #37930 ]

When pairing with another programmer, I wrote some code which he described as "clever". Here it is.

sub xpc_namespaces {
    my $self       = shift;
    my $values     = $self->{xpc_namespaces};
    my %namespaces = map { reverse %$_ } values %$values;
    return \%namespaces;
}

If someone refers to my code as "clever", I should consider that a code smell and look for a better way of writing it. I know what that code does and why it does it, but I think that's because I wrote it.

I noticed the useless variable, so I thought about tightening it, but realized the logical conclusion was this:

sub xpc_namespaces {
    { map { reverse %$_ } values %{ shift->{xpc_namespaces} } };
}

That's just, well, wrong. Because of the structure of our XML, it's guaranteed we won't have collisions, but it's still an atrocity. Unless you have no choice, code should be written for programmers, not computers (SICP for the win!).

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
 Full
 Abbreviated
 Hidden
More | Login | Reply
Loading... please wait.
  • I actually find the 'bad' version easier to read :)
  • sub xpc_namespaces {
        my $self       = shift;
        my @mapping    = values %{ $self->{xpc_namespaces} };
        my %namespaces = map { reverse %$_ } @mapping;
        return \%namespaces;
    }

    • I'm not even sure what part was clever. I was thinking it was the reversal of the hash, turning keys to values and vice-versa.
      • It’s not easily apparent that the code is dealing with a HoH and the map expression reverses reading direction twice. Consider if it were possible to write something like this:

        my %namespaces
            = shift->{xpc_namespaces}
                ->values
                ->map( sub { %{$_}->reverse } )

        (This is not quite possible in this way even with autobox.)