bad code to good golf

i have had the pleasure of doing code review for a client and finding
this gem:

sub new {
my $class   = shift;
my %params  = (@_);
my $self    = {};
my $package = caller();
for my $key ( keys %params ) {
$self->{$key} = $params{$key};
}
bless( $self, $class );
return $self;
}

even better than just be a poorly written constructor is that it is
inside a 'factory' module that is inherited by 80 other modules!!

so i quickly rewrote it and the new sub doubled in speed. i can't run
the whole system so i can't yet tell how much improvement would be seen
there. but consider that this is call all over by every other module, i
suspect some noticeable speedups.

anyhow you have two challenges, if you desire. the first is very easy
but amusing when compared to the original code. rewrite that sub (a
minimal constructor that takes a key/value list and makes a hash based
object with no extra checking or work) with the shortest but cleanest
and clear code you can.

i have a clean 2 line version that would be understood by almost
anyone. it was the one i benchmarked against the original.

your second challenge is to golf the same constructor. the char count
will be between the {} of the sub body as the name and 'sub' are
constant.

i have two solutions of 33 and 24 chars so you have to beat those. the
longer one is amusing enough to show it later.

doing these reviews of large and mediocre code bases gives me access to
all sorts of bad code. would it be fun to post more snippets and taunt
them with happy fun ball?

uri

--
Uri Guttman  ------  uri@stemsystems.com  -------- [url]http://www.stemsystems.com[/url
]
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding
-
Search or Offer Perl Jobs  ----------------------------  [url]http://jobs.perl.org[/url
]

Uri Guttman wrote:
> i have had the pleasure of doing code review for a client and finding
> this gem:
>
> sub new {
>     my $class   = shift;
>     my %params  = (@_);
>     my $self    = {};
>     my $package = caller();
>     for my $key ( keys %params ) {
>         $self->{$key} = $params{$key};
>     }
>     bless( $self, $class );
>     return $self;
> }
>
> even better than just be a poorly written constructor is that it is
> inside a 'factory' module that is inherited by 80 other modules!!

If that's an example of "poorly written" code in your project, can I come wo
rk
there?  It at least gets it right, if verbosely.


> anyhow you have two challenges, if you desire. the first is very easy
> but amusing when compared to the original code. rewrite that sub (a
> minimal constructor that takes a key/value list and makes a hash based
> object with no extra checking or work) with the shortest but cleanest
> and clear code you can.
>
> i have a clean 2 line version that would be understood by almost
> anyone. it was the one i benchmarked against the original.

sub new {
my $class = shift;
return bless( {@_}, $class );
}


> your second challenge is to golf the same constructor. the char count
> will be between the {} of the sub body as the name and 'sub' are
> constant.
>
> i have two solutions of 33 and 24 chars so you have to beat those. the
> longer one is amusing enough to show it later.

sub new {bless{@_[1..$#_]},shift}

Was my first shot at 23.  Then I figured I could knock off that range op...

sub new {$a=shift;bless{@_},$a}

21.  And it's even strict clean. :)


--
THIS I COMMAND!

On Sunday 09 December 2007, Michael G Schwern (Michael G Schwern
<schwern@pobox.com> ) wrote:
> sub new {bless{@_[1..$#_]},shift}
>
> Was my first shot at 23.  Then I figured I could knock off that range op...[/color
]

my first shot was 23 also...
sub new{($a,@b)=@_;bless{@b},$a}

then i came up with this 21 a few seconds later:
sub new{bless{splice@_,1},pop}

i think that's the best i can do... i'll be very impressed if anyone does
better than 21.

>>>>> "MGS" == Michael G Schwern <schwern@pobox.com> writes:
 

MGS> If that's an example of "poorly written" code in your project,
MGS> can I come work there?  It at least gets it right, if verbosely.

well it is a live system so it must be 'right'. i was hired to review it
and the double copy of the hash data is what bothers me. as this is
called zillions of times by all those modules it is a quick and easy
speedup.

and they are hiring more outsource indians so you won't be contacted. :(

MGS> sub new {
MGS> 	my $class = shift;
MGS> 	return bless( {@_}, $class );
MGS> }

my clean version is:

sub new {
my ( $class, %self ) = @_ ;
return bless \%self, $class ;
}

i don't like using shift for args if i can help it.

MGS> sub new {bless{@_[1..$#_]},shift}

that is the same as my shorter golf answer. maybe i counted mine wrong.

MGS> sub new {$a=shift;bless{@_},$a}

MGS> 21.  And it's even strict clean. :)

that is nice. the other replies beat it though!

uri

--
Uri Guttman  ------  uri@stemsystems.com  -------- [url]http://www.stemsystems.com[/url
]
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding
-
Search or Offer Perl Jobs  ----------------------------  [url]http://jobs.perl.org[/url
]

>>>>> "rw" == robert wilson <hotaru@safe-mail.net> writes:

rw> On Sunday 09 December 2007, Michael G Schwern (Michael G Schwern
rw> <schwern@pobox.com> ) wrote: 

rw> my first shot was 23 also...
rw> sub new{($a,@b)=@_;bless{@b},$a}

rw> then i came up with this 21 a few seconds later:
rw> sub new{bless{splice@_,1},pop}

rw> i think that's the best i can do... i'll be very impressed if anyone doe
s
rw> better than 21.

you and marcus are tied for first with the same answer.

is there any guarantee of evaluation order in arg lists? will the
bless/splice always be executed before the pop?

my longer but amusing answer is this:

sub new {
bless{reverse%{{reverse@_}}}
}

it is not warning safe and it leaves '' => CLASS in the hash. but it has
no temp vars, calls to splice or shift.

uri

--
Uri Guttman  ------  uri@stemsystems.com  -------- [url]http://www.stemsystems.com[/url
]
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding
-
Search or Offer Perl Jobs  ----------------------------  [url]http://jobs.perl.org[/url
]

* Marcus Holland-Moritz <mhx-perl@gmx.net> [2007-12-09 21:20]:
> We're all stuck at 21. :-)

Well, `bless`, `shift` or `$_[0]`, `{}`, `@_`, and one comma are
all inevitable. That’s 15 characters in fixed strings alone. Then
you’ll also need to rearrange list or evaluation order somewhere,
which means at least one pair of delimiters with two indices and
a separator inside, so that’s another 5 tokens that can’t be any
shorter than 1 character.

Therefore no possible solution to this problem can be any shorter
than 20 characters under any circumstances.

Besides the solutions already posted, I found a couple of ways to
write it in 22 characters, plus two more ways to do it in 21
characters, one of should work but doesn’t because of that stupid
prototype that `bless` has.

But I didn’t manage to beat 21 characters, probably because it’s
not possible. Beating 20 would certainly be impossible.

Regards,
--
Aristotle Pagaltzis // <http://plasmasturm.org/>

* Uri Guttman <uri@stemsystems.com> [2007-12-09 19:25]:
> * Michael G Schwern <schwern@pobox.com> writes: 
>
> my clean version is:
>
> sub new {
> 	my ( $class, %self ) = @_ ;
> 	return bless \%self, $class ;
> }
>
> i don't like using shift for args if i can help it.

Personally I *always* use `shift` for the invocant, but
assignment from `@_` for all other parameters in all but a few
rare circumstances. So methods in my code always read something
like this:

sub frobnitz {
my $self = shift;
my ( $foo, $bar, $baz ) = @_;
# ...
}

It’s a nod to the fact that the invocant is not really in the
same class (no pun intended) as the other parameters. Since
`$self` is thus removed from `@_`, and is the *only* thing
removed from it, that also makes it natural to write delegative
code:

# ...
$self->get_veeblefitzer()->frobnitz( @_ );
# ...

--
*AUTOLOAD=*_;sub _{s/(.*)::(.*)/print$2,(",$\/"," ")[defined wantarray]/e;$1
}
&Just->another->Perl->hack;
#Aristotle Pagaltzis // <http://plasmasturm.org/>

A. Pagaltzis wrote:
> * Uri Guttman <uri@stemsystems.com> [2007-12-09 19:25]: 
>
> Personally I *always* use `shift` for the invocant, but
> assignment from `@_` for all other parameters in all but a few
> rare circumstances. So methods in my code always read something
> like this:
>
>     sub frobnitz {
>         my $self = shift;
>         my ( $foo, $bar, $baz ) = @_;
>         # ...
>     }
>
> It’s a nod to the fact that the invocant is not really in the
> same class (no pun intended) as the other parameters. Since
> `$self` is thus removed from `@_`, and is the *only* thing
> removed from it, that also makes it natural to write delegative
> code:
>
>         # ...
>         $self->get_veeblefitzer()->frobnitz( @_ );
>         # ...

+1


--
...they shared one last kiss that left a bitter yet sweet taste in her
mouth--kind of like throwing up after eating a junior mint.
-- Dishonorable Mention, 2005 Bulwer-Lytton Fiction Contest
by Tami Farmer