Dangling underscores

A pull request at work sparked a bit of conversation[1] about using underscores in JavaScript to describe a function as being private. It was this code specifically:

exports.elasticsearchProxy = {
    _renameIndex: function(query) {
        if (query['index']) {
            query.index = 'test_' + query.index;

    search: function(query, callback) {
        es_client.search(query, callback);

    mget: function(query, callback) {
        es_client.mget(query, callback);

Because our logic isn’t as separated from our database as we’d like, many of our API tests touch the ES database. In order to avoid futsing up our actually data, we just want to make sure it’s only ever talking to indexes starting with “test_”.

My suggestion was that we should refactor this so that we don’t need to be exposing what should be a private method.

The argument was that this is a private method; it’s convention in JS to just use this underscore syntax. But that argument really just comes down to “that’s the way it’s always been done.”

This was fine back in the dark days of JavaScript, but we’re much more organised these days. And as our industry grows more mature it’s our responsibility to keep up with it.

Leaving something public really limits how much you can change later – regardless of it being an “underscore private method” you’re still letting others see it, and you’ve no way to know that it’s safe to delete that method once you’ve changed your implementation.

Even if you know that’s not a concern,[2] it’s still a good habit to get out of. And a free one at that.

exports.elasticsearchProxy = function () {
    'use strict';

    var renameIndex = function (query) {
        if (query.index) {
            query.index = 'test_' + query.index;

    return {
        search: function (query, callback) {
            es_client.search(query, callback);

        mget: function (query, callback) {
            es_client.mget(query, callback);

[1] Which had nothing to do with the feature set being added, as always seems to be the case: comment on the low hanging fruit, rather than the structure or flow of the code.

[2] In this example, the proxy probably won’t be used again and has no need to be extended.

BrowserQuest’s Event Handling

This time I want to look at how BrowserQuest handles event happening and how reactions are triggered.

(Last time, I did a quick write up on animation in BrowserQuest.)

Just from playing through the game, you can see that a few things happen when you die: the you explode in a puff of smoke, and the ‘revive’ screen pops up. Lets see how they’re throwing that event.

You are dead.
You are dead.

The die method in the Character object is the one that handles our player dieing, and it’s pretty simple: stop trying to attack whatever we were attacking, and mark us as dead. The bit we’re interested in this post though is the callback.

die: function() {
    this.isDead = true;

    if(this.death_callback) {

this.death_callback is a variable which is set in the onDeath method. I’m not all that sure why this is a public variable, but public variables seem to be the common way of storing data throughout the project. BrowserQuest was never intended to be an example of clean coding standards, so I won’t hold it against them. But this is a problem since those public properties could be relied upon existing by other developers (either in or outside of the dev team). This makes refactoring later on really hard if you don’t want to risk having unintended side affects of changing these properties.

Calling the event onDeath is something I hadn’t thought of doing. The events is like any other that we come across in JavaScript: onClick, onFocus, and the like. So it makes a lot of sense for it to start with the ‘on’ prefix.

Something that limits this feature is that there can only be one callback. That’s very different to how we think of other events. Usually you can bind as many actions as you like to onClick so you’d expect the same here, but it seems that you can only have one callback per event in this system. It’s possible that this feature was never added because they didn’t think they needed it. You can see that they don’t need it by running a grep on “onDeath” and there’s only one place it’s used. (For the Character object at least.) Why waste time on implementing a fancy event system with multiple callbacks when you just need one?

Strangely though, there is another onDeath-like event which gets thrown but is handled by the game class, rather than the character. When the character dies, if you have the credits open, they should be replaced with with “revive” screen. (Code here.) I’m not sure why you wouldn’t want to just bind that code to your already existing event.

In my demo code, I made an array of elements who wanted to listen to a certain event, like ‘onKeyDown’ and then passed the event to each of those objects via their own onKeyDown method. I haven’t stress tested this at all, since there wasn’t any complicated code to be processing. I don’t think that there will be much of a difference in performance by storing an array of callbacks though.

It’s becoming clear that drawing from BrowserQuest alone for ideas of architecture won’t be enough, so I’m going to be looking at other games where the source is available in the near future to look into other ways.

Looking at BrowserQuest’s Animation Handling

I’ve written a small, quick experiment for me to see if it makes sense to write an MMO using WebSockets. It was a lot of fun but mostly what I learnt was that I was coding myself into a bit of a corner. There’s lots of techniques I’ve learnt from looking at other source code, especially BrowserQuest (code) (which is a huge inspiration).

What BrowserQuest looks like whilst playing.
What BrowserQuest looks like whilst playing.

I’ve decided to draw a line under that work, do a bit of a review, and start again on sturdier feet. The first thing I want to talk about is drawing graphics and animation.

I ignored this to start with, just drawing a round circle which the player could move around. That’s mostly because my first aim was to get a number of players moving around and seeing each other move around, using web sockets.

Upgrading from just a circle wasn’t much of a hassle. Each player object has their own draw() method which handled all the drawing it had to do. Switching from a circle to a graphic mostly just meant changing one method. Additionally I now had to give the player a state. They couldn’t just be a ball, sitting there perfectly still anymore. If they were walking right, they had to be in a ‘walking right’ state, so the correct animation could be played.

I ended up with the player object having a private definitions variable (which is actually a terrible method name, should have gone with animationFrameDefinitions) with 8 objects, each describing where the frame was.

// add sprite data
var definitions = {
    'eastStandingStill': {'frames': [[0, 2], [1, 2]]},
    'eastWalking': {'frames': [[0, 1], [1, 1], [2, 1], [3, 1]]},
    'westStandingStill': {'frames': [[3, 11], [4, 11]]},
    'westWalking': {'frames': [[1, 10], [2, 10], [3, 10], [4, 10]]},
    'northStandingStill': {'frames': [[0, 5], [1, 5]]},
    'northWalking': {'frames': [[0, 4], [1, 4], [2, 4], [3, 4]]},
    'southStandingStill': {'frames': [[0, 8], [1, 8]]},
    'southWalking': {'frames': [[0, 7], [1, 7], [2, 7], [3, 7]]},
var playerSprite = new sprite('/img/clotharmor.png', {'height': 32, 'width': 32}, definitions);

Each element of the frame are coordinates for where the next frame is. I think this might be a limitation that will quickly creep up on me. What if the frame isn’t the same size as all the others? A low hanging fruit of optimisation when loading images over HTTP is to merge many images into one, so you make only one request to the server instead of one per image. In my design, I can’t do that.

I should have paid more attention to BrowserQuest’s way of doing it. They don’t have this problem because their animation class doesn’t actually know anything about the image (it’s not given a reference to the image), and it doesn’t handle any of the drawing to the canvas. Its sole job is to decide which pixels should be drawn (and when it should switch to the next frame). Whereas I have a sprite object which assigns a height and a width for each frame within the sprite, BrowserQuest has “animations”, which start at an x,y on an image, each frame as a height and width, and there’s a certain number of frames on that line. So now, there can be many mixed frame sizes within a single image.

There is a sprite class which does know about the image. It is what points to the image we want to select our sprites from. It’s in here that we can see that all the sprite definitions are kept in individual files (I guess that makes it tidier), and are brought in using require.js.

Also, something I’d not thought, each animation can run at different times. I might want to slow down the how long a frame stays on the screen for before moving to the next. That’s easily set in this animation object.

Even in sprite.js though, very little drawing is actually done to the canvas.

That job lies with the render class.

In my version I definitely want to follow in those footsteps. Whilst I have one sprite.js doing all the drawing, BrowserQuest uses three different scripts to split responsibility, and is more flexible because of it.

The actual triggering of the drawing lies in the gameloop’s tick(), like mine does, but they’ve gone with using using requestAnimationFrame, which means the frame rate is left up to the browser (who knows much better than me how quickly a redraw is possible).

I think that is about it for animation so far. I’ve learnt a lot by reviewing BrowserQuest’s code and seeing why some of the ideas they’ve used are better than my own first try. I’m planning on continuing reviewing my code, next looking at event handling.

Testing for “Not a Number” (NaN) in Javascript

When validating some text, to check if it’s a real number or not, I tried to compare it against “NaN”. I thought that was correct since that’s what Firebug tells me is returned by parseInt ("sfdsfs", 10). However, Firebug is just covering up Number.NaN, which is what you should be comparing against.

To check if a string is numeric, use the isNaN function:

if (isNaN (parseInt (somestring, 10)) {
	// is numeric

Quirks With Javascript Scope and Hoisting

Whilst drowning myself in JavaScript today, I learnt a couple of things.

First was that there’s such a technique as “hoisting“. I’ve taken for granted that defining a function anywhere in my code (whilst in scope) will be available for use. Even if I’m calling a function before it’s actually been declared (converse to what you’d consider to be procedural).

workOutSum (2, 3); // returns 5

function workOutSum (n, m) {
	return (n + m);

That’s thanks to hoisting. Declarations of functions get bumped up to the top of the scope. The same goes to variables declared with the var construct.

Continue reading Quirks With Javascript Scope and Hoisting