Re: HollyGame gamekit (almost @ CPAN)

G'day holyghost,

In addition to what ++soonix has written, with which I wholeheartedly agree, I would recommend you put modules in separate files, i.e. HollyGame/Box.pm, HollyGame/Enemy.pm, and so on. Add these two lines to the top of each:

use strict;
use warnings;
[download]

Then test each module individually with:

perl -c Whatever.pm
[download]

That will pick up many of your errors. For instance, in your first subroutine (&Box), you have:

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]

Here's a quick one-liner to give you an idea of what's happening:

$ perl -e 'use Data::Dump; sub Box { my $self = { $x => shift, $y => s
+hift }; dd $self } Box(qw{a b c})'
{ "" => "b" }
[download]

In the second subroutine (&is_collision) you reference "$self->{x}", "$self->{y}", and so on: those keys don't exist! However, that issue is eclipsed by the syntax errors with the if statements (already pointed out by soonix).

There's lots of other problems. For instance, you create blessed references which then appear to be silently discarded; there appears to be no encapsulation (no accessors or mutators; $self->{KEY} is used throughout); and a number of use MODULE statements appear at the end of a package but seem to be associated with the next package. I pretty much stopped looking at this point: do not consider this to be an exhaustive list.

Perhaps you need to review "perlootut - Object-Oriented Programming in Perl Tutorial"; and consider using one of the "OO Sytems" discussed in that documentation.

— Ken

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

Replies are listed 'Best First'.
Re^2: HollyGame gamekit (almost @ CPAN) by afoken (Chancellor) on Oct 17, 2017 at 05:10 UTC
`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". ;-)	[reply] [d/l] [select]
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]