Bad Habits in Udacity's HTML5 Game Development: An Example

[elfs]

This screenshot more or less perfectly illustrates what I consider wrong with the Udacity HTML5 game development class.  We’re in the middle of a routine for drawing sprites to the screen, and so far this particular lesson, on asset atlasses and sprite packing and all the rest, has been fine, but there’s something in this particular code that makes me twitch hard.    Take a look at the “hlf” object created in the middle of this function.  What is it doing?

It’s doing nothing.  It’s not a transformation.  It’s not analysis.  It’s not functional.  It’s just a renaming of one variable (“center of x”) to another (“half of x”).  It doesn’t even help the user understand what’s going on.

But it’s also doing something else.  It’s creating a new object in the context of this function.  That object goes onto the heap until the Javascript VM performs garbage collection, at which point the entire VM pauses to make sure no references are tampered with as it cleans up.  Different VMs (V8, SpiderMonkey, Chakra, etc.) perform garbage collection on different criteria, but all monitor memory growth, watch for times when memory usage exceeds a certain point, and perform routine garbage collection on an interval.  Every HTML5 game designer in her right mind should be doing absolutely everything she can to prevent the garbage collector from triggering, and if it must, giving it as little as possible to clean up.  A long VM pause is going to make one hell of a mess of your jealously protected 30 frames per second, and depending upon the tightness of your code it may throw off timing routines as well.  Remember: setTimeout(func, 60) doesn’t promise to run func exactly 60 milliseconds later; it promises to run func “as soon as possible” after 60 milliseconds has passed.  Relying on that while throwing in possible ~100 millisecond garbage collection pauses is a sure recipe for failure.

I understand that this is a basic course, and it’s going to be missing a lot of details.  Memoization and hyper-functional up-front calculations are fairly advanced subjects.  But there’s a difference between not covering advanced topics and actively teaching students a bad habit.

Comments

[ziggadon.livejournal.com]

The thing that sticks out as utterly crazy to me is that in drawImage() they're using hlf.x AND spt.x (and y). So all that does is create confusion and obfuscates the code just a little bit more.

[prock.livejournal.com]

I have to say, I don't find this particular example very compelling. I take more issue with the cryptic variable names than the use of a language feature which might come with caveats. Remember, "garbage collection" and other managed language features exist to free programmers from the niggly details of memory management. That this abstraction is leaky is well know, but you can hardly blame people for using the abstraction as intended.

[sqlrob.livejournal.com]

Realtime(ish) programming comes with a lot of differences from other programming. You need to be very aware of those caveats and they're just churning out code monkeys that don't know how to deal with reality. It's pretty rampant in the C/C++ world as well, where people don't get that you really do need to pay attention, despite all the helpers.

[elfs.livejournal.com]

The problem is that I don' think this is just an issue that "comes with caveats." We're talking about gaming in the browser platform, a place where there are lots of caveats. The "GC Pause" is a well-known issue among HTML5 game developers, leading to conversations about The Write-Only DOM and Never Delete Anything. This example throws all of that hard-earned experience out the window, and I believe it does a disservice to the students to engrain a bad habit.

As for cryptic variable names, I have no problem with short names in a function small enough to live in an IDE window. I'm a Haskell hacker, I like names like 'x' and 'xs' to denote 'item' and 'list of items' in a function.

[shaterri.livejournal.com]

Note that hlf.x isn't the same as spt.x; it's actually spt.cx . Of course, the fact that spt has member variables 'x' and 'cx' is another issue, but I presume that's an inherited one and not something the devs whipped up for their local libraries.

[shaterri.livejournal.com]

I vigorously disagree. Hard-earned experience is hard-earned for a reason; for teaching purposes, it's not only appropriate but absolutely essential to throw away (or at least sweep under the rug) more advanced ways of doing things. The purpose of this code is not efficiency; it's clarity. Mind you, I think there's a good argument to be made (absent comments or context I don't have) that it falls down on that front too, but I fault it much more for that than I do for not dancing around the particular hoops created by the GC, especially since those hoops may or may not actually exist. (Several JS interpreters, for instance, will cheerfully Do The Right Thing and create hlf on the stack where it belongs)

[ziggadon.livejournal.com]

Sorry, you're right, I misread that as hlf.x = spt.x, rather than spt.cx. Either way, the creation of that temporary variable seems silly.

[prock.livejournal.com]

The "GC Pause" is a well-known issue among HTML5 game developers

Sure, the abstraction leaks. All abstractions leak. The point isn't that you should ignore leaky abstractions. The point is that you should pay attention to them when they start leaking. There are all kinds of abstraction leaks that exist. Plugging leaks before the abstractions start to leak is a good example of premature optimization.

I'm going to guess that they aren't using unit tests or TDD either.

What's the target audience for the course? Beginners who don't even know what a memory model is yet? Experts who already know everything except html5 game coding? Or is it a sophomore level hybrid course where they should be getting an introduction to garbage collection as well as the html5 content?

Premature Optimization != !Deoptimization

[cadetstar.livejournal.com]

I agree that premature optimization is bad, but there is absolutely no reason for the creation of the hlf variable. hlf.x is just a reference to spt.cx. Why not just replace all instances of hlf.x with spt.cx and *not* create the additional variable?

Creating that additional variable can also introduce unneeded code later on that can be reduced if you see the obvious part, e.g. -

function incoming(a) {
  var b = {
    m: a.x,
    n: a.y
  };
  /*
    Lots of other code that doesn't operate on a or b
   */

  return b.m - a.x;
}

This is obviously a contrived example, but it illustrates my point. This function will always return 0, but if you don't see the declaration and return on the same screen, it's not immediately obvious.

I (and I believe Elf as well), have no problem with a declaration like so:

var b = {
  x: (a.x + a.y) / 2,
  y: (a.x - a.y) / 2
}

Because that actually has a transformation in it, but that is not the case in the example Elf posted.

-Michael

[prock.livejournal.com]

I agree that premature optimization is bad, but there is absolutely no reason for the creation of the hlf variable. hlf.x is just a reference to spt.cx. Why not just replace all instances of hlf.x with spt.cx and *not* create the additional variable?

You're arguing for coding around abstraction leaks defensively. That's premature optimization. There is no reason a good compiler can't allocate the variable on the stack, and if it does, you may have made the code more difficult to maintain. If it allocates it on the heap, then maybe you'll have a problem, but chances are that you won't until you run into resource constraints, another case of abstraction leakage.

If code quality is an issue here, then your variable names "a" and "b" are likewise problematic. Maybe you'll excuse yourself and say "this is just a code example". It's not clear why that excuse works for a broken idiom, but doesn't work for a defensive idiom.

You have a choice to make, you can make the code as clear as possible, so that maintenance costs are minimized, or you can insert an arbitrary number of defensive practices to guard against leaky abstractions, or anything in between. Different contexts will lead to different decisions. The context of learning generally pushes the decision towards using the abstractions at face value to focus on the particular course content. If the content is the specific abstraction leak, then that's a different story. Remember, this is a course context. The focus is not "best practices" it's about learning about the technology. I assume no one is doing unit tests, no one is doing TDD, no one is doing code reviews, no one is using source code repositories, etc.

[cadetstar.livejournal.com]

My argument is completely irrelevant to garbage collection or memory leaks. What is the benefit to the following (variable names are just for brevity):

var a, b, c, d, e, g, h, i, j, k, l, m, n, o;
o = 1;
n = o;
m = n;
l = m;
k = l;
j = k;
i = j;
h = i;
g = h;
f = g;
e = f;
d = e;
c = d;
b = c;
a = b;

This is adding a layer of abstraction that is completely unnecessary to the code that was shown in the original post. You already have references to the two pertinent pieces of information. You're saying it's fully valid to add two more references to the *exact* piece of information? I will admit, I have, on occasion, created my own version of object, either so I can extend it with additional methods, or reformat the data so that it make more sense internal to the application I'm developing. That is not the case here. The code declares two new references to existing data without an apparent improvement in readability, accessibility or clarity.

I agree that an introductory course is not necessarily the place to teach complete "best practices", but neither should it teach inherently bad practices which must then be unlearned later.

-Michael

[prock.livejournal.com]

The code declares two new references to existing data without an apparent improvement in readability, accessibility or clarity.

Using aliases for the purposes of documentation is a fairly standard practice.

In the code snippet provided, presumably hlf is an alias for the center of the sprite, indicating that we are using half it's width and height to position the sprite. I could certainly take issue with the use of variable names here. It might be that the person who wrote the code struggled to figure out what cx/cy where, and when they did, they chose to document that in the code. It's not the best job I've seen at doing that, but that is certainly a standard idiom that used commonly in programming. In fact, I'll be happy to admit that I used the hlf identifier as a hint when trying and figure out what the code was doing. As an identifier, hlf carries more semantic information than cx/cy.

I will admit, I have, on occasion, created my own version of object, either so I can extend it with additional methods, or reformat the data so that it make more sense internal to the application I'm developing.

In this case, the object was not extended, but rather narrowed, something which is a not unreasonable thing to do.