bad code to good golf
Uri Guttman
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 ------ u...@stemsystems.com -------- http://www.stemsystems.com
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- http://jobs.perl.org
Michael G Schwern
> 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 work
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!
Marcus Holland-Moritz
Also at 21, also strict safe, and doesn't "misuse" $a ;-)
sub new {bless{splice@_,1},pop}
Marcus
--
Quality Control, n.:
The process of testing one out of every 1,000 units coming off
a production line to make sure that at least one out of 100 works.
Robert Wilson
> sub new {bless{@_[1..$#_]},shift}
>
> Was my first shot at 23. Then I figured I could knock off that range op...
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.
Uri Guttman
>> even better than just be a poorly written constructor is that it is
>> inside a 'factory' module that is inherited by 80 other modules!!
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 Guttman
rw> On Sunday 09 December 2007, Michael G Schwern (Michael G Schwern
rw> <sch...@pobox.com>) wrote:
>> sub new {bless{@_[1..$#_]},shift}
>>
>> Was my first shot at 23. Then I figured I could knock off that range op...
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 does
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.
Marcus Holland-Moritz
> >>>>> "MGS" == Michael G Schwern <sch...@pobox.com> writes:
>
> MGS> sub new {$a=shift;bless{@_},$a}
>
> MGS> 21. And it's even strict clean. :)
>
> that is nice. the other replies beat it though!
Nope, Michael's solution is just as short. We're all stuck
at 21. :-)
--
inbox, n.:
A catch basin for everything you don't want to deal with, but
are afraid to throw away.
A. Pagaltzis
> 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/>
A. Pagaltzis
>> }
>
>
> 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/>
Michael G Schwern
>
> 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
Michael G Schwern
> is there any guarantee of evaluation order in arg lists? will the
> bless/splice always be executed before the pop?
I believe it is undefined, so it's not recommended that you do anything order
dependent in the argument list. But it is stable.
As implemented, "complex" expressions are evaluated first, then simple ones.
This leads to classic oddities like this:
perl -wle 'sub foo { print join " ", @_ } $i = 1; foo($i, $i++, $i++)'
3 1 2
This is not specific to Perl, but I believe a C thing as well.
> 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.
You fell into the trap! That's an improper constructor, can you tell us why?
--
Robrt: People can't win
Schwern: No, but they can riot after the game.
Uri Guttman
MGS> Uri Guttman wrote:
>> is there any guarantee of evaluation order in arg lists? will the
>> bless/splice always be executed before the pop?
MGS> I believe it is undefined, so it's not recommended that you do
MGS> anything order dependent in the argument list. But it is stable.
so that breaks the pop solutions IMO. it may not always work in all perl
versions.
>> 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.
MGS> You fell into the trap! That's an improper constructor, can you tell us why?
i forgot the shift of the class arg which i had on my test code. it was
a paste error. and it makes it even longer.
Shmem
Well, if we were to return an array reference instead of a hash ref,
sub new{bless\@_,shift}
works. Why does the shift get executed before an array reference is
constructed - but not if a hashref is constructed - from an array?
0--gg-
--
_($_=" "x(1<<5)."?\n".q·/)Oo. G°\ /
/\_¯/(q /
---------------------------- \__(m.====·.(_("always off the crowd"))."·
");sub _{s,/,($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e,e && print}
A. Pagaltzis
> Well, if we were to return an array reference instead of a hash
> ref,
>
> sub new{bless\@_,shift}
>
> works. Why does the shift get executed before an array
> reference is constructed - but not if a hashref is constructed
> - from an array?
It probably doesn’t. Either way, evaluation order is a red
herring: your construction will work regardless of which
subexpression is evaluated first!
You managed to confuse yourself. Congratulations. :-) (Or you
are underhandedly trying to confuse us. In that case, sorry bub,
better luck next time. :-) )
What happens is that `\` takes a *reference* to the array. And
of course when you do that, any modifications of the referent,
including *subsequent* modifications, will be seen by anyone who
holds a reference to it.
In contrast, the hash constructors make a *copy* of the array,
and that copy will not be affected but subsequent modifications
to the source array.
Anyone who has trouble following should consider the difference
between the following two:
sub new { bless \@_, shift }
# vs
sub new { bless [@_], shift }
Consider how many arrays are involved in either case, and which
one is affected by the `shift`.
Uri Guttman
AP> * shmem <georg....@millenux.com> [2007-12-10 12:55]:
>> Well, if we were to return an array reference instead of a hash
>> ref,
>>
>> sub new{bless\@_,shift}
AP> In contrast, the hash constructors make a *copy* of the array,
AP> and that copy will not be affected but subsequent modifications
AP> to the source array.
and that is beside that point that it totally is broken. the specified
API is a list of key/value pairs. how would an array of those be used as
an object? i don't feel like doing a linear search to get arg values. if
i wanted that i would use lisp assoc lists! :)
Steve Fink
> Well, if we were to return an array reference instead of a hash ref,
>
> sub new{bless\@_,shift}
>
> works. Why does the shift get executed before an array reference is
> constructed - but not if a hashref is constructed - from an array?
No it doesn't. It breaks in various and subtle ways, at least on a
couple versions of Perl that I used.
I actually did this once. (The class was representing vectors to
overload various operators, so it made sense to use a blessed array
ref.) The problem is that perl doesn't really refcount @_, so \@_
gives you a very unstable object whose contents are apt to change
underneath you for no apparent reason. I think you're probably fine as
long as you're within the call that the @_ belongs to, but if you ever
return it, weird stuff happens.
I can't come up with a simple test case for it. But it seems
reasonable that "fixing" the problem would slow everything down, and
in this case I'd rather have the speed. I'm not convinced that it's
really a bug anyway, as long as it doesn't crash. Making a copy is
what I really intended with that code; I didn't *want* a reference to
an argument list. I'm not even sure what that would mean.
Winter Christian
To me the following seventeen seems acceptable, at least as long
as there's certainty that the "abused" additional key/value pair
doesn't clash with class functionality, and it even circumvents
the evaluation order thingy:
sub new{bless{0,@_},$_[0]}
-Chris
COC AG, Marktler Strasse 50, 84489 Burghausen, Deutschland, Amtsgericht Traunstein, HRB 17218, USt-ID-Nr. DE 251 388 010
Vorstand: Ralf Schuster (Vorstandsvorsitzender), Andreas Bublak, Renate Linhart, Aufsichtsratsvorsitzender: Günter W. Engerer
COC AG, Marktler Strasse 50, 84489 Burghausen, Germany, Commercial Registry Traunstein, HRB 17218, VAT Reg.No. DE 251 388 010
Managing Board: Ralf Schuster (CEO), Andreas Bublak, Renate Linhart, Chairman of the Supervisory Board: Günter W. Engerer
Uri Guttman
WC> To me the following seventeen seems acceptable, at least as long
WC> as there's certainty that the "abused" additional key/value pair
WC> doesn't clash with class functionality, and it even circumvents
WC> the evaluation order thingy:
WC> sub new{bless{0,@_},$_[0]}
my double reverse thing has the same flaw. but i don't think the extra
key/value is legit (being the judge of this contest :).
Shmem
no good to hide away... *sigh*
From the keyboard of A. Pagaltzis [10.12.07,13:41]:
> * shmem <georg....@millenux.com> [2007-12-10 12:55]:
> > Well, if we were to return an array reference instead of a hash
> > ref,
> >
> > sub new{bless\@_,shift}
> >
> > works. Why does the shift get executed before an array
> > reference is constructed - but not if a hashref is constructed
> > - from an array?
>
> It probably doesn't. Either way, evaluation order is a red
> herring: your construction will work regardless of which
> subexpression is evaluated first!
>
> You managed to confuse yourself. Congratulations. :-) (Or you
> are underhandedly trying to confuse us. In that case, sorry bub,
> better luck next time. :-) )
Thank you ;-)
Had I tried to confuse you, I had added a smiley at the end, but
that has not been the case. See, that's my way to write bugs:
in gnawing on a solution, forgetting completely things that are
obvious to me in other circumstances.
Had somebody else posted that, I would have responded likewise,
but as every time, one is blind to his own bugs ;-)
0--gg-
> What happens is that `\` takes a *reference* to the array. And
> of course when you do that, any modifications of the referent,
> including *subsequent* modifications, will be seen by anyone who
> holds a reference to it.
>
> In contrast, the hash constructors make a *copy* of the array,
> and that copy will not be affected but subsequent modifications
> to the source array.
>
> Anyone who has trouble following should consider the difference
> between the following two:
>
> sub new { bless \@_, shift }
> # vs
> sub new { bless [@_], shift }
>
> Consider how many arrays are involved in either case, and which
> one is affected by the `shift`.
>
>
--
Michael G Schwern
>>>>>> "WC" == Winter Christian <Christia...@coc-ag.de> writes:
>
> WC> To me the following seventeen seems acceptable, at least as long
> WC> as there's certainty that the "abused" additional key/value pair
> WC> doesn't clash with class functionality, and it even circumvents
> WC> the evaluation order thingy:
>
> WC> sub new{bless{0,@_},$_[0]}
>
> my double reverse thing has the same flaw. but i don't think the extra
> key/value is legit (being the judge of this contest :).
Yeah, but an honorable mention for creativity!