Re^2: HollyGame gamekit (almost @ CPAN)

my $self = { $x => shift, $y => shift, $w => shift, $h => shift };
[download]
This has many problems. Apart from using four, uninitialised, package variables; $self actually ends up with this value:
{ "" => $_[4] }
[download]

Well seen, kcott++. I really read

my $self = { x => shift, y => shift, w => shift, h => shift };
[download]

instead.

Another thing with this line - original or as misread by me - is order of evaluation. I don't know by heart if Perl guarantees left-to-right order in this case. I can't find a reason why it should not, and if I could get my brain out of stand-by mode, I would probably compare with %hash=( %oldhash, 'this' => 'overwrites', 'anything' => 'found', 'in' => '%oldhash' ); and consider this harmless.

But the point is: I have to think about it. Maybe I have to RTFM. And apart from that, it is not elegant at all.

In our code review system, I would mark this line as "bad style, probably wrong", and it would have to be changed (or explained by some comments). I would not spend more time considering if it is really wrong. It is sufficient that it is not clearly right.

Our review system uses peer review, i.e. one developer reads the code of another developer, and the roles of coder and reviewer are swapped quite often. Understanding data flow and algorithms used is hard enough, and finding errors in there is even harder. Errors in our code can have the potential to hurt or - in the worst case - kill people, and so we want clean code.

That does not mean that we can't use some tricks in our code. But if we do, those tricks are commented to explain what happens. Tricks are quite rare, because our target hardware usually has some spare CPU cycles and some spare memory. So instead of using tricks, we prefer code that wastes a little bit of memory and / or CPU, but is easier to read.

Yes, I'm aware that games rarely kill real people, but code that is clean and easy to read makes it harder to accidentally hide bugs.

Alexander

--
Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)

Comment on Re^2: HollyGame gamekit (almost @ CPAN) Select or Download Code

Replies are listed 'Best First'.
Re^3: HollyGame gamekit (almost @ CPAN) by kcott (Archbishop) on Oct 17, 2017 at 06:47 UTC
G'day Alexander, "Another thing is order of evaluation. I don't know by heart if Perl guarantees left-to-right order in this case. I can't find a reason why it should not, ... I would probably compare with `%hash=( %oldhash, ...)` and consider this harmless." The thing with "`%hash = (%default, %overwrite)`" is that, given hashes have no inherent order, it could evaluated in various orders like these (additional parentheses added to highlight the contents of the two hashes). `%hash = ( (a => 1, b => 2), (a => 101, b => 102) ); %hash = ( (a => 1, b => 2), (b => 102, a => 101) ); %hash = ( (b => 2, a => 1), (a => 101, b => 102) ); %hash = ( (b => 2, a => 1), (b => 102, a => 101) );` [download] However, regardless of whatever random order `%default` and `%overwrite` present themselves in, the key-value pairs of the two hashes won't become intermixed. For example, this evaluation won't occur: `%hash = ( a => 102, b => 2, a => 1, b => 102 );` [download] Right at the end of "perldata: List value constructors" there's: "If a key appears more than once in the initializer list of a hash, the last occurrence wins: ..." So, `$_[4]` would have been the sole value in the `%$self` hash. Anyway, that's all rather academic. The sigils on the keys would have been picked up by `strict`. A naīve fix to declare those variables beforehand would have ended up with "uninitialized value" `warnings`. And anyway, as you say, it's very bad style to start with: unpacking `@_` with five separate `shift` operations mixed in with other code is pretty much asking for a hard-to-find bug in the future. I probably would not have written the code like this at all; however, working with what's there, I would have had `&Box` called more like "`Box($class, $args)`", where `$args` was a hashref with the "x y w h" keys. Alternatively, if stuck with a bad but published interface, I might have changed the implementation to, perhaps, something like: `my $class = shift; my $self = {}; @$self{qw{x y w h}} = @_; bless $self, $class;` [download] — Ken	[reply] [d/l] [select]

Replies are listed 'Best First'.

Re^3: HollyGame gamekit (almost @ CPAN)
by kcott (Archbishop) on Oct 17, 2017 at 06:47 UTC

G'day Alexander,

"Another thing is order of evaluation. I don't know by heart if Perl guarantees left-to-right order in this case. I can't find a reason why it should not, ... I would probably compare with %hash=( %oldhash, ...) and consider this harmless."

The thing with "%hash = (%default, %overwrite)" is that, given hashes have no inherent order, it could evaluated in various orders like these (additional parentheses added to highlight the contents of the two hashes).

%hash = ( (a => 1, b => 2), (a => 101, b => 102) );
%hash = ( (a => 1, b => 2), (b => 102, a => 101) );
%hash = ( (b => 2, a => 1), (a => 101, b => 102) );
%hash = ( (b => 2, a => 1), (b => 102, a => 101) );
[download]

However, regardless of whatever random order %default and %overwrite present themselves in, the key-value pairs of the two hashes won't become intermixed. For example, this evaluation won't occur:

%hash = ( a => 102, b => 2, a => 1, b => 102 );
[download]

Right at the end of "perldata: List value constructors" there's:

"If a key appears more than once in the initializer list of a hash, the last occurrence wins: ..."

So, $_[4] would have been the sole value in the %$self hash.

Anyway, that's all rather academic. The sigils on the keys would have been picked up by strict. A naīve fix to declare those variables beforehand would have ended up with "uninitialized value" warnings. And anyway, as you say, it's very bad style to start with: unpacking @_ with five separate shift operations mixed in with other code is pretty much asking for a hard-to-find bug in the future.

I probably would not have written the code like this at all; however, working with what's there, I would have had &Box called more like "Box($class, $args)", where $args was a hashref with the "x y w h" keys. Alternatively, if stuck with a bad but published interface, I might have changed the implementation to, perhaps, something like:

my $class = shift;
my $self = {};
@$self{qw{x y w h}} = @_;
bless $self, $class;
[download]

— Ken

[reply]
[d/l]
[select]