

Ask HN: Budding game programmer humbly requests code critique - slow_bro

Recently rejected from a few big Flash game companies because of my code samples. Would really appreciate some pointers or tips from a spare pair of programmer eyes!<p>http://willchou.com/asteroids1.1<p>http://willchou.com/symptom/<p>I've done a little prior reading on game architecture and I'm currently looking through the Flixel and FlashPunk libraries for models to follow.
======
chipsy
Here are my notes:

Use ON_ENTER_FRAME events and getTimer(), not TIMER. The Timer class is a low-
accuracy system good for tweens, but it runs aground of Flash's "elastic
racetrack" throttling behavior: Flash will try to keep a balance between
rendering frames and code execution and that conflicts with the absolute
control a game needs. ON_ENTER_FRAME will keep your main loop running once-
per-frame, which is what you want; with getTimer() you can still apply a dT to
your objects.

You're using strings in your enumerated constants. These will compare slower
than integers. However, the bigger concern for performance is.....

You're using the Flash transform functions on a per-frame basis for rotations.
This will always be slow; software rendering is fast only at simple memory
copies and nothing else, which translates to using copyPixels as much as
possible in the inner loop. Hence the performant way to architect Flash games
is to prerender a giant spritesheet with draw(), use that as a lookup table
for each rotation, and to draw the result onto a single surface rather than to
maintain objects on the display list, because the Flash display list will do
event processing per object. If you need to finely control draw layers, defer
the actual rendering operation into a queue for each layer, and do it all in
one pass later.

Your "GameObject" won't scale well for a more complex game because it mix
three concerns(drawing, motion, collision) in a single class. This caveat is
true of the Flixel/Flashpunk architectures too; a more factored approach would
be to push these things into components and instance them on each entity type.
For an example of a more factored entity system look at Pushbutton Engine.

Anyway, it looks pretty good overall, but it's a simple game. The companies
may be looking for something a little more elaborate.

~~~
slow_bro
Yeah that first point.. I should've changed it a long time ago.

Interesting point about the enumerations. I actually used to use ints and
starting using strings when I saw some sample do it; I thought that having
strings safeguarded better against accidentally equivalent values across
enumerations. But now I think of it you can just one-time randomly generate
some large unique int per value.

I had thought that industry professionals probably move away from the Flash
display list. Your third point makes a lot of sense.

Thanks so much chipsy! :D

------
ismarc
chipsy covers a good number of concerns, and I unfortunately don't have time
to enumerate the full list of issues seen in both, so I'll quickly cover some
of the higher level issues:

1\. (asteroids) Tracking state in a series of global variables and then
relying on that state in timer callbacks to implement your gameloop leads to
unpredictable gamerate and framerates.

2\. Embedding the core game logic in mxml file is "bad form". I would suggest
creating a game class that is instantiated and called from within that initial
mxml file, but it's basically to get the launcher.

3\. (symptom) You have entirely too many, pointless loops. You iterate over
the same list of objects numerous times in multiple locations. A good example
would be in the most recent Level.as update() function. You iterate over the
GameObjects 3 different times (not counting the inner loop for collision
detection) when this can be reduced to 1 loop.

4\. Variable names...they're poor. An example is:

for each (var object:GameObject in O) {

What is O? Scroll to the top of the function. O is the result of getObjects().
Well, getObjects obviously gets some objects, but which ones? What type of
collection does it use? Go to getObjects() definition and it uses a Vector,
but you use the name O to build the vector internal to getObjects as well. The
variable name should, at a minimum, provide a hint as to what purpose it
serves. game_objects would be good, or gameObjects, or activeObjects. And, I
can only guess what RBC and WBC even mean. I'm guessing maybe it's red blood
cell and white blood cell? Even the associated class files don't provide any
information, so from the point of view of someone reading the code, RBC and
WBC are random letter strings representing a "something"

~~~
slow_bro
Thanks ismarc!

I appreciate all your pointers, but I feel that some of them are somewhat
subjective.

1\. True, chipsy pointed this out and I actually changed it last night. I'm
not sure how global variables specifically effect unpredictable gamerate (and
I don't think they're actually global anyways - they're class variables of the
main game mxml).

2\. True, I can see how the application mxml can get too big in code size
without a "Game" object. I also changed this last night. ;)

3\. You're right that functionally all of those loops can be reduced into one,
but I feel that that comes at some cost of code readability and extensibility.
For example, if my game logic was 10x longer than it is now it might be best
modularized into discrete methods that reflect the separation of loops that I
have currently.

4\. Fair enough, but I think that this point is also somewhat subjective. E.g.
the meanings of RBC/WBC are pretty clear with a single class definition lookup
(it's right there in the class comment).

I hope that my response doesn't seem ungrateful, I really appreciate your
help. Please let me know if you think I'm mistaken on any of these points (I
am learning after all).

~~~
ismarc
On 1, I didn't see a class declaration inside the mxml file for asteroids, but
I may have missed it. For the unpredictablility, if global, any portion of the
application can modify the values. This means you don't have any access
control or format enforcement. This leads to the potential of unintended
changes at any point in the application (say, the level was changed somewhere
else) and you now have to catch all possible changes in the game loop rather
than providing the proper update on assignment.

On 3, I had to read through the code several times to even make sure the last
loop would run for each object. You could have one loop (with one inner loop)
and perform the updates (one was 1 line, the other 3-4 if I remember right)
and keep the same clarity. It can also be performance impacting given too many
objects.

On 4, some of them are subjective (the rbc/wbc for sure), but letters are free
and these are being used as code samples.

Code reviews are always subjective, but if it's going to be a code sample, it
should stand on its own using best practices and comments when they're not
reasonable to follow. Given the sample, I'd ask things like "how does it
perform with 1000 game objects?" I hope I didn't give the impression I thought
it was bad, it just has some common mistakes that, given a project of larger
scope, would quickly become unmaintainable without lots of effort.

~~~
slow_bro
Okay, gotcha. Thanks again. :)

