HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Plugin for communication protocols in browser(s)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
communicationbrowserpluginforprotocols

Problem

I would like to get some feedback from the community regarding a plugin I recently moved from requiring jQuery to a stand alone JS implementation. It is meant to handle browser communication protocols such as XHR, XDR, WS & WSS without a lot of fuss.

The project can also be found @ comm.js.

```
/**
* Description: Handles AJAX, XDR, WS & WSS protocols
*
* Fork me @ https://www.github.com/jas-/comm.js
*
* Author: Jason Gerfen
* License: GPL (see LICENSE)
*/

(function(window, undefined){

'use strict';

var comm = comm || function(o){

/**
* @object defaults
* @abstract Default set of options for plug-in
*
* @param {String} appID Unique identifier
* @param {String} url Specified URL param
* @param {Mixed} data String/Boolean/Object
* @param {Boolean} debug Enable or disable debugging options
* @param {Object} bind Element to which this plug-in is bound
* @param {Boolean} async Default to async communication
* @param {String} method Method of communication (GET, PUT, POST, DELETE)
* @param {Function} callback Callback function for success
* @param {Object} precallback Callback prior to send
* @param {Object} errcallback Callback on errors
*/
var defaults = {
appID: 'comm.js',
url: '',
data: false,
debug: false,
async: true,
method: 'get',
logID: '',
callback: function(){},
precallback: function(){},
errcallback: function(){}
};

/**
* @method _setup
* @scope private
* @abstract Initial setup routines
*/
var _setup = _setup || {

/**
* @function save
* @scope private
* @abstract Primary initialization of window.crypto API
*
* @param {Object} o Plug-in option object

Solution

Interesting question,

2 upfront general notes first:

  • Run your code through JsHint.com, there are a ton of little things you could fix



  • Run your code through a beautifier, there are a ton of inconsistencies



Also, consider the following

  • You have too many 1 letter variable s,i,o,x,y,e are pretty well understood, but for the rest you should have meaningful names



  • If you had meaningful names, and good function names, then your comments would not have to be so extensive



  • You use _ for private stuff, I agree with Crockford, either you make it private (with an IIFE or other means) or you dont and then you drop the underscore.



  • @abstract Primary initialization of window.crypto API



  • You seem to depend on this.online a number of times, did you intend to call this.online() ?



  • In retry() you have 3600 and 10 as magical, unnamed constants



-
You should use your comments as a guide for naming, for this comment:

@abstract Determine mode of communication based on browser type and option


I would expect the function to be called something like determineCommunicationMode, not decide. Especially since you call decide only twice

-
Ternary statements, I love them, but you have to know that in general an if statement is preferred if there is no else block ( Ternary three )

-
In xdr: function(o, d, c){, you use only c once here:

xdr.open('post', o.url+'?cmd='+c);


I would much rather have xdr.open( 'post', o.url ); and let the caller take care of concatenating the command, otherwise this is a bit hard coded..

-
One more hardcoded constant 100 in xdr

-
In ajax you could simply do

ajax: function(o, d, c){
        var r = false
            , xhr = new XMLHttpRequest();
            , reg = new RegExp(document.location.href);


because

  • _h is not used anywhere



  • underscores are silly here, the variables are scoped to the function



  • first setting xhr to false and then to new XMLHttpRequest(); does not make sense



  • Comma's first is a movement I respect, but expect some slamming over that choice



  • /function/.test(typeof(o.precallback))



Furthermore, in the log object, you check for the type of error, debug etc. every single instead of once up front. Also, you are not checking whether console actually exists. I would have a init function which checks for console and it's function and provides a dummy implementation in case those are missing.

Code Snippets

@abstract Determine mode of communication based on browser type and option
xdr.open('post', o.url+'?cmd='+c);
ajax: function(o, d, c){
        var r = false
            , xhr = new XMLHttpRequest();
            , reg = new RegExp(document.location.href);

Context

StackExchange Code Review Q#55881, answer score: 4

Revisions (0)

No revisions yet.