patternjavascriptMinor
Plugin for communication protocols in browser(s)
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
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:
Also, consider the following
-
You should use your comments as a guide for naming, for this comment:
I would expect the function to be called something like
-
Ternary statements, I love them, but you have to know that in general an
-
In
I would much rather have
-
One more hardcoded constant
-
In
because
Furthermore, in the log object, you check for the type of
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,eare 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.onlinea number of times, did you intend to callthis.online()?
- In
retry()you have3600and10as 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 optionI 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
_his not used anywhere
- underscores are silly here, the variables are scoped to the function
- first setting
xhrtofalseand then tonew 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 optionxdr.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.