Probe for open application on a TCP port

Question

I need to examine an application port to see if it opens and I need to put the retry count as a parameter. Since I'm new to Node I wanted to get your feedback on it for improvements.

checkAppPr: function (port) {
 var result = Promise.defer();
 var checkStatus = function (next, result, times) {
 portscanner.checkPortStatus(port, '127.0.0.1', function (error, status) {
 if (error) {
 result.reject(error);
 } else {
 if (status === 'open') {
 console.log("Application status: open");
 result.resolve();
 } else {
 times--;
 if (times > 0) {
 setTimeout(function () {
 next(next, result, times);
 }, 1000);
 } else {
 result.reject("start timeout");
 }
 }
 }
 })
 }
 checkStatus(checkStatus, result, 20);
 return result.promise;
 }

Answer 1

First, I'd suggest to improve on naming. checkAppPr doesn't really tell me anything about what this function does. Naming it to checkAppPort would make more sense.

Next, you've got some hard-coded literals in your code like 127.0.0.1, 20, 1000. Without some sensible names, they don't mean anything. Put them in constants along with the convention of using capital letters and underscores as names.

Your code is an example of one function trying to jam too much functionality. You can split this into two functions, one that checks for any port and resolves/rejects, and another function that repeatedly calls the first one. You've done it already, except the function is nested in the other.

Here's my take on your code. Longer, but splits out responsibilities.

// Returns a promise that resolves when the port is open
checkPortStatus: function(port, host){
 return new Promise((resolve, reject) => {
 portscanner.checkPortStatus(port, host, function(error, status) {
 if(error)
 reject(error);
 else if(status === 'open')
 resolve(status);
 else
 reject(new Error('Port is not open'));
 });
 });
},
// Your API function
checkAppPort: function(port, retriesLeft) {
 const TIME_BETWEEN_CHECKS = 1000;
 const HOST = '127.0.0.1';
 const RETRIES = 20;
 // Setting a default to retriesLeft
 retriesLeft = retriesLeft === void 0 ? RETRIES : retriesLeft;
 if(!port) throw new Error('Port is required');
 if(retriesLeft === 0) return Promise.reject('Timed Out');
 return new Promise((resolve, reject) => {
 // We call our checker. When it resolves, it calls this promise's resolve.
 // If it rejects, we do added work.
 this.checkPortStatus(port, host).then(resolve, error => {
 setTimeout(() => {
 // Call this function again, with one less retry. However, we hook our
 // resolve and reject to the promise of the new call effectively making
 // a chain when it keeps failing.
 this.checkAppPort(port, retriesLeft - 1).then(resolve, reject);
 }, TIME_BETWEEN_CHECKS);
 });
 });
}

• 07_05_GuyT: Thanks A lot 1+ ,One thing that make me confused is the arrow function we are using node 0.12.7 so its not supported can you please adopt it ?so I can use it...
• Joseph: @shopiaT You can regular functions with a trailing bind(this) like then(function(){...}.bind(this))
• 07_05_GuyT: Thanks, do you mean for checkPortStatus like ? this.checkPortStatus(port, host).then(function(resolve, error) { setTimeout(function() { .... if so for where should I put the bind(this) ? can you please provide example if its not right?
• 07_05_GuyT: Hi Again please see my updated answer do I do it right ? i didnt use the bind but maybe I miss something here....?
• Jaromanda X: if(retriesLeft === 0) Promise.reject('Timed Out'); goes nowhere, does nothing, should that have been a return Promise.reject(...