patternjavascriptMinor
Game Server Querying
Viewed 0 times
servergamequerying
Problem
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
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
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
Solution
Good code, I only have some minor pointers.
-
When I see this:
in
-
Do not hesitate to replace some
could be
Still, all in all good code IMO.
-
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
thisincores.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
udpResponseI would callconsole.logbefore resolving. In general I shy away from doing anything after resolving.
- Not a big fan of silent failure, I would have expected an
elseforif (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.
Code Snippets
if (this.options.debug) {
console.log('IP matched: ' + this.options.host);
}if (this.options.parse) {
this.deferred.resolve(this.parse(buffer));
} else {
this.deferred.resolve(buffer);
}this.deferred.resolve( this.options.parse ? this.parse(buffer) : buffer );Context
StackExchange Code Review Q#64272, answer score: 3
Revisions (0)
No revisions yet.