Refactoring JavaScript to follow best practices

NAM - Radium furniture polishphoto © 2006 Marshall Astor | more info (via: Wylio)Back in August this year, Mehul Harry, our ASP.NET evangelist, published a how-to blog post on adding keyboard support to the ASPxDataView. As he does sometimes, he asked me to check it for general readability — we all ask each other to do this every now and then: it’s amazing what errors a different pair of eyes can pick up in the text. Anyway, I can’t remember whether I found anything in his writing or not because I did find something awkward in the JavaScript being demonstrated in the post (which was written by the R&D team). At the time I promised Mehul I’d write something up on what I found — thinking it would be the following week — but it’s taken me until now (a good 3 months later) to follow up on my promise to fix and polish the code.

When I wrote the JavaScript chapter in Professional DevExpress ASP.NET Controls, I espoused a particular best practice for coding in JavaScript -- minimizing your footprint in the global space. The optimal plan is to do as jQuery and YUI do: have one (or at the most two) global variables. Apart from the usual reasons for not using global variables which apply across all languages (well, apart from those that have no locals I suppose), the problem is doubled in JavaScript and other dynamic languages: redefining an existing global causes no error. Good luck on finding out that you’ve just stomped on some global variable without knowing about it; many intense sessions with Firebug lie in your future.

In the book, I showed how this could be done by declaring a single global helper object and then having all previous global members be members of that object. Let’s do that here. First of all, get a hold of the CodeCentral code from here, and then follow along. (I’ll note that I recreated the project from scratch with a new ASP.NET project, so my “infrastructural” code that I may show won’t be exactly the same as you see in CodeCentral.) Before making any changes, run the app to ensure that you have no errors.

The first thing we should do is get rid of the script block and put the JavaScript into its own JS file. Add a new JScript item to the Scripts folder (I called mine unimaginatively JSRefactor.js) and cut/paste the JavaScript into it. This is what you should have:

var dataviewPerformingCallback = false;

function AddKeyboardNavigationTo(dv) {
dv.BeginCallback.AddHandler(function (s, e) { dataviewPerformingCallback = true; });
dv.EndCallback.AddHandler(function (s, e) { dataviewPerformingCallback = false; });
ASPxClientUtils.AttachEventToElement(document, "keydown",
function (evt) {
return OnDocumentKeyDown(evt, dv);
});
}

function OnDocumentKeyDown(evt, dv) {
if (typeof (event) != "undefined" && event != null)
evt = event;
if (NeedProcessDocumentKeyDown(evt) && !dataviewPerformingCallback) {
if (evt.keyCode == ASPxKey.Left /*37-Left Arrow*/) {
if (dv.cpPageIndex > 0)
dv.PerformCallback('Prev');
return ASPxClientUtils.PreventEvent(evt);
} else if (evt.keyCode == ASPxKey.Right /*39-Right Arrow*/ && dv.cpPageIndex < dv.cpPageCount - 1) {
dv.PerformCallback('Next');
return ASPxClientUtils.PreventEvent(evt);
}
}
}

function NeedProcessDocumentKeyDown(evt) {
var evtSrc = ASPxClientUtils.GetEventSource(evt);
if (evtSrc.tagName == "INPUT")
return evtSrc.type != "text" && evtSrc.type != "password";
else
return evtSrc.tagName != "TEXTAREA";
}

You can then replace the script block with this code by dragging the JS file to your header section of your ASPX file:

<script src="Scripts/JSRefactor.js" type="text/javascript"></script>

Rerun the app to make sure it still works. Now, to me, apart from the global variable issue (there are four in this minor piece of code), the code just looks like a C# developer wrote it. OK, the anonymous function declarations are idiomatic JavaScript, but that’s about it.

Now to refactor. My first set of refactorings was fairly minor: I avoid == and != like the plague (see here for details) and so I replaced them with === and !==. In this example code, there’s no difference between the two but forcing yourself to using triple-equals may save you some debugging heartache in the long run and will make you look like a JavaScript pro. (CodeRush notes: we don’t support this code refactoring yet and we should.)

The next refactoring is going to lay the foundation: we’ll add a new global object that will start to act as a repository for all the other JavaScript code.  Following along with another JavaScript best practice, we’ll be setting it up so that it uses a closure to hide data from the outside world (in other words, it’ll have private members — if we were talking about a C# object, that is). Also, because scoping in JavaScript is function-based we’ll set it up so that the object is created as a result of a function call. And to make sure we don’t have to name that function (and therefore have two global variables) we’ll declare it anonymously and immediately execute it. Ready?

var jmbHelper = function () {
var C = "private";
return {
A: 1,
B: true
};
} ();

Reading from the top down, I’m declaring a new global variable (that is, it’s a new member of the window object) to have the return value of the immediately-executed (spot the function call parentheses on the last line) anonymous function. The function returns an object, which has two public place-holder members: A, of value 1, and B, of value true. It has one private place-holder member C that, because of the joys of JavaScript closures is only visible throughout the anonymous function but not outside.

Now to add some oomph to this object. If you look at the original code, you’ll see that the only function that should be visible publicly is the AddKeyboardNavigationTo() method (it’s used by the DataView’s ClientSideEvents.Init string/function) . Everything else is and should be private. So we’ll make that function a member of the returned object (basically replacing A and B) and everything else becomes part of the function itself (replacing C, in essence). I decided to make AddKeyboardNavigationTo()a function that called a private function, but the rest is as it was. (Note that I had to change the C# Page_Load code slightly to reference jmbHelper.AddKeyboadNavigationTo instead of just AddKeyboadNavigationTo.)

var jmbHelper = function () {

var dataviewPerformingCallback = false;

function HookKeyboard(dv) {
dv.BeginCallback.AddHandler(function (s, e) { dataviewPerformingCallback = true; });
dv.EndCallback.AddHandler(function (s, e) { dataviewPerformingCallback = false; });
ASPxClientUtils.AttachEventToElement(document, "keydown",
function (evt) {
return OnDocumentKeyDown(evt, dv);
});
}

function OnDocumentKeyDown(evt, dv) {
if (typeof (event) !== "undefined" && event !== null)
evt = event;
if (NeedProcessDocumentKeyDown(evt) && !dataviewPerformingCallback) {
if (evt.keyCode === ASPxKey.Left /*37-Left Arrow*/) {
if (dv.cpPageIndex > 0)
dv.PerformCallback('Prev');
return ASPxClientUtils.PreventEvent(evt);
} else if (evt.keyCode === ASPxKey.Right /*39-Right Arrow*/ && dv.cpPageIndex < dv.cpPageCount - 1) {
dv.PerformCallback('Next');
return ASPxClientUtils.PreventEvent(evt);
}
}
}

function NeedProcessDocumentKeyDown(evt) {
var evtSrc = ASPxClientUtils.GetEventSource(evt);
if (evtSrc.tagName === "INPUT")
return evtSrc.type !== "text" && evtSrc.type !== "password";
else
return evtSrc.tagName !== "TEXTAREA";
}

return {
AddKeyboardNavigationTo: function (dv) { return HookKeyboard(dv); }
};
} ();

You can check that this code works as before, but with only one global object: jmbHelper (and obviously you try and give this a unique name).

Now we can do some further refactorings like renaming of variables (which originally were long since we didn’t want global identifier clashes). However, there’s actually a bug that perhaps is made more obvious by having wrapped this code into a single object: although AddKeyboardNavigationTo() can be called for any number of controls, there is only one flag that shows whether a control is in callback mode or not.  That’s a simple change in one way (add another property to the dv instance) or to have an array of flags. The first is just as bad as the global variable situation (how do you know you won’t choose an identifier that’s already being used?), so we’ll use the second. Here’s my final JavaScript code (at least for this article):

var jmbHelper = function () {

var inCallback = [];

function canProcessKeyDown(evt) {
var evtSrc = ASPxClientUtils.GetEventSource(evt);
if (evtSrc.tagName === "INPUT") {
return evtSrc.type !== "text" && evtSrc.type !== "password";
}
return evtSrc.tagName !== "TEXTAREA";
}

function processKeyDown(evt, dv) {
if (inCallback[dv]) { return true; }

evt = event || evt;
if (!canProcessKeyDown(evt)) { return true; }

// look for left arrow, process it if we can
if ((evt.keyCode === ASPxKey.Left) && (dv.cpPageIndex > 0)) {
dv.PerformCallback('Prev');
return ASPxClientUtils.PreventEvent(evt);
}

// look for right arrow, process it if we can
if ((evt.keyCode === ASPxKey.Right) && (dv.cpPageIndex < dv.cpPageCount - 1)) {
dv.PerformCallback('Next');
return ASPxClientUtils.PreventEvent(evt);
}

return true;
}

function hookKeyboard(dv) {
inCallback[dv] = false;
dv.BeginCallback.AddHandler(function (s, e) { inCallback[dv] = true; });
dv.EndCallback.AddHandler(function (s, e) { inCallback[dv] = false; });
ASPxClientUtils.AttachEventToElement(document, "keydown",
function (evt) { return processKeyDown(evt, dv); });
}

return {
addKeyboardNavigationTo: function (dv) { hookKeyboard(dv); }
};
} ();

Some notes:

  1. hookKeyboard never returned anything, so I changed the definition of addKeyboardNavigationTo. Since the returned object is defined inside the scope of the outer function, its addKeyboardNavigationTo method has access to that internal (private) function because of the closure (I didn’t mention this before)
  2. Notice how I use the function scoping rules for the dv parameter in hookKeyboard. Even the anonymous handlers being set up in there have access to it.
  3. processKeyDown has had the biggest changes:
    • I’m using guard clauses to get out quickly, should the control be already in a callback or should the control process its own left/right arrow keys.
    • Notice the idiomatic JavaScript to set evt to event, or if it’s undefined or null to evt itself.
    • Event handlers return true (“we didn’t handle it so allow the event to propagate”) or false (“cancel the event, we’ve dealt with it”).
    • Although JavaScript has the “feature” that all identifiers declared in the scope of a function are visible throughout the function, I belong to the Douglas Crockford and JSLint camp where you should declare identifiers before you use them. Hence I’ve reordered the private members to be declaration-first. (Note that the final code passes JSLint’s default tests.)
  4. By convention, normal function names in JavaScript should start with a lowercase letter. An uppercase letter indicates a constructor. I changed all *my* function names; can’t do anything about existing ones.

I hope you found that little discourse on JavaScript refactoring and polishing to be of interest. It actually helped me a lot: I’ve been trying to work out how to make our JavaScript support in CodeRush better and this refactoring exercise, although fairly simple, solidified some thoughts for me. I see a webinar between me, Mark Miller, and the IDE Tools team in my future...

2 comment(s)

Thanks Julian, your article is brilliant!

By the way, I love it when you write "Idiomatic JavaScript".

8 December, 2010

Ruddy awesome. It seems that Community Server replaces the word JavaScript immediately followed by a colon to "BLOCKED SCRIPT". Even in normal text (that is, in the middle of a <p> element with no other markup) where it has no effect.

Sigh. I must've spent at least 20 minutes so far trying to get this article to appear properly in C/S. It doesn't help that every time I update it, I have to remember that C/S doesn't send the post tags to Windows Live Writer and I have to set them manually. <grumble>

Cheers, Julian

8 December, 2010

Please login or register to post comments.