6
\$\begingroup\$

I made a package for Node.js that allowed the querying of game servers (or any UDP server(untested)) using UDP.

What it does is sends the query to the server and adds the query into a queue. When the message is received it retrieves the response from the server, parses it and removes the query from the queue.

Am I using the promises correctly and am I going about it the correct way?

Structure

  • lib
    • core.js
    • index.js
    • protocols
      • iw4
        • master.js

core.js

var Q = require('q'),
    Class = require('extend.class'), //https://www.npmjs.org/package/extend.class
    lookup = Q.denodeify(require('dns').lookup);

module.exports = Class.extend({
    init: function() {
        this._super();
        this.options = {};
        this.maxAttempts = 1;
        this.attempt = 1;
        this.finished = false;
        this.udpTimeoutTimer = false;
    },
    start: function() {

        var self = this;
        return this.lookup()
            .then(function() {
                return self.run();
            });

    },
    lookup: function() {
        var deferred = Q.defer();

        if (!('host' in this.options))
            deferred.reject(new Error('No host specified'));

        if (this.options.host.match(/^\d+\.\d+\.\d+\.\d+$/)) { //matched IP only
            if (this.options.debug) {
                console.log('IP matched: ' + this.options.host);
            }
            this.options.address = this.options.host;
            deferred.resolve();
        } else if (this.options.host.match(/^\d+\.\d+\.\d+\.\d+\:\d+$/)) { //matched IP:PORT, parse accordingly
            var temp = this.options.host.split(':');
            this.options.address = temp[0];
            this.options.port = temp[1];
            deferred.resolve();
        } else {  //host is a domain, look it up and retrieve the IP
            var self = this;
            return lookup(this.options.host).then(function(address) {
                if (self.options.debug) {
                    console.log('Looked up: ' + self.options.host + ' resolved to: ' + address);
                }
                self.options.address = address;
                deferred.resolve();
            });
        }

        return deferred.promise;
    },
    udpSend: function(buffer) {

        var deferred = Q.defer(),
            self = this;

        if (!('port' in this.options)) {
            return deferred.reject(new Error('Attempted to send without setting a port'));
        }
        if (!('address' in this.options)) {
            return deferred.reject(new Error('Attempted to send without setting an address'));
        }
        if (typeof buffer === 'string') {
            buffer = new Buffer(buffer, 'binary');
        }

        process.nextTick(function() {
            self.udpSocket.send(buffer, 0, buffer.length, self.options.port, self.options.address, function() {
                if (self.options.debug) {
                    console.log('Sent buffer: ' + buffer);
                }
            });
            self.udpTimeoutTimer = setTimeout(function() {
                //if query isn't finished and hasn't received a response yet, time it out.
                if (!self.finished) {
                    return self.deferred.reject('Timeout');
                }
            }, self.options.timeout || 3000);
        });

        return deferred.promise;
    },
    udpResponse: function(buffer) {
        this.finished = true;
        var self = this;
        clearTimeout(this.udpTimeoutTimer);
        if (this.options.parse) {
            this.deferred.resolve(this.parse(buffer));
        } else {
            this.deferred.resolve(buffer);
        }
        if (this.options.debug) {
            console.log('Received response: ' + buffer);
        }
        return Q.fcall(function() {
            return self;
        });
    },
});

index.js

var dgram = require('dgram'),
    udpSocket = dgram.createSocket('udp4'),
    activeQueries = [],
    Q = require('q');

udpSocket.unref();
udpSocket.bind(1337);
udpSocket.on('message', function(buffer, rinfo) {
    for (var i = 0; i < activeQueries.length; i++) {
        var query = activeQueries[i];
        if (query.options.address != rinfo.address) {
            continue;
        }
        if (query.options.port != rinfo.port) {
            continue;
        }
        return query.udpResponse(buffer)
            .then(function(thisQuery) {
                var i = activeQueries.indexOf(thisQuery);
                if(i >= 0) {
                    activeQueries.splice(i, 1); //query is finished, remove it.
                }
            });
    }
});

module.exports = {
    query: function(options) {

        var type = (options.type || '').replace(/[^0-9a-zA-Z\\\/\-]/g, '');
        var protocol = require('./protocols/' + type);

        var query = new protocol();
        query.udpSocket = udpSocket;
        query.type = type;
        query.deferred = Q.defer();

        for (var i in options) {
            query.options[i] = options[i];
        }

        activeQueries.push(query);

        process.nextTick(function() {
            query.start();
        });

        return query.deferred.promise;
    }
};

protocols/iw4/master.js

module.exports = require('../../core').extend({
    run: function() {
        return this.udpSend(this.options.buffer || '\xff\xff\xff\xffgetservers IW4 61586 full empty\x00');
    },
    parse: function(buffer) {
        var servers = [];
        var bufferString = buffer.toString('binary');
        bufferString = bufferString.slice(0, -3);
        var tempSplit = bufferString.split(/\\/);
        tempSplit.forEach(function(server) {
            var len = server.length;
            if (len == 6) {
                var ip = '';
                for (var i = 0; i < len - 2; i++) {
                    ip += server[i].charCodeAt(0) + '.';
                }
                ip = ip.slice(0, -1);
                port = (server[len - 2].charCodeAt(0) << 8) + server[len - 1].charCodeAt(0);
                servers.push({
                    ip: ip,
                    port: port
                });
            }
        });
        return servers;
    }
});

A generic request would look like so:

var request = require('./lib');

request.query({
    type: 'iw4/master',
    host: '127.0.0.1',
    port: 20810,
    timeout: 3000,
    parse: true
}).then(function(servers) {
    console.log(servers); //array of servers in {ip:ip, port:port} format
}, function(error) {
    console.log(error);
});
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't have time to go into a full review - but while your use of promises is syntactically correct, perhaps you should consider resolving the promise with the resolved address, rather than having a side effect (re your first set of code, you have this.options.address = address; deferred.resolve();. Instead you could have deferred.resolve(address);. This would make the code easier to test, for one! Also, if you call return from within a then, the then call will return that value when it resolves. \$\endgroup\$ – Dan Pantry Sep 30 '14 at 14:13
  • 1
    \$\begingroup\$ Ben, if you could change the last code snippet code to a link, that would be better. You are only supposed to post your own code (unless, of course, you ARE John Resig:) \$\endgroup\$ – konijn Sep 30 '14 at 19:21
  • \$\begingroup\$ @konijn Thanks, removed code and added reference. \$\endgroup\$ – Ben Fortune Sep 30 '14 at 20:08
2
\$\begingroup\$

Good code, I only have some minor pointers.

  • When I see this:

    if (this.options.debug) {
        console.log('IP matched: ' + this.options.host);
    }
    

    in lookup and udpResponse, then I wonder why these functions have to know whether we are in debug mode. You should declare your intent to log by calling a single logging function. That function should know about debug mode and decide whether to actually log something.

  • I prefer to see comments prior to the statement, it preps the mind for what comes next, since code has a flow a->b->c->d you dont want to read a comment about a when you expect to be reading about b. Hope this makes sense, the coffee did not yet kick in.
  • 'use strict'; should be considered
  • There is so much this in cores.js.. Since you deal with a singleton, you might want to read up on the self revealing module pattern.
  • Do not hesitate to replace some if calls with ternaries:

    if (this.options.parse) {
        this.deferred.resolve(this.parse(buffer));
    } else {
        this.deferred.resolve(buffer);
    }
    

    could be

    this.deferred.resolve( this.options.parse ? this.parse(buffer) : buffer );
    
  • In udpResponse I would call console.log before resolving. In general I shy away from doing anything after resolving.
  • Not a big fan of silent failure, I would have expected an else for if (len == 6) {, I also would have liked a comment for 6 because that is one mysterious magical constant.

Still, all in all good code IMO.

\$\endgroup\$
  • \$\begingroup\$ Hi & thanks. Couple things, if I'm wanting to write a comment for an if/else statement block using my current formatting, how would you go about this? } else if(foo==bar) { Would the comment go above/below/split the else statement onto multiple lines? About the logging, do you mean put the if(debug) logic into a separate logging function and call the function in place of my current if statements. Also, what advantages would the revealing module pattern give me? Do you have any usage examples that would apply to my current code? \$\endgroup\$ – Ben Fortune Nov 14 '14 at 14:09
  • \$\begingroup\$ Commenting else blocks is always ugly, do what you feel reads well. Logging -> your understanding is correct. For revealing pattern -> You use this far less, making your code much more readable. If you care about such things you also get a built in split between private/public data/functions. \$\endgroup\$ – konijn Nov 14 '14 at 14:18
  • \$\begingroup\$ One last thing :), how would I tie my extending classes into the revealing module pattern? \$\endgroup\$ – Ben Fortune Nov 14 '14 at 14:50
  • 1
    \$\begingroup\$ There should be no difference, the revealing module returns an object in object notation, so you put the module within Class.extend( function core(){ //private stuff //public stuff ... return { ... } }() ); \$\endgroup\$ – konijn Nov 14 '14 at 15:10
  • \$\begingroup\$ Couple more things, when extending my core class from the protocol folder, I add a method called run which sends the buffer. The only problem here is this run method needs to be run from the core. I also need to be able to call the function as a constructor, since when the query is called, it's pushed into a query stack to wait for a response. Is this still possible? \$\endgroup\$ – Ben Fortune Nov 14 '14 at 16:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.