Code structure for a login modal dialog.

Question

I'm using twitter mobile and I have a login-dialog. login-Dialog is a modal dialog. To call it the user calls loginDialog.show();

I'm not all that comfortable with the coding style I've adopted (i'm very much self schooled in JavaScript) and I'd be interested to hear peoples opinions on the code structure and process handling. For example I always seem to use expressive functions instead of function declarations. (some of the nitty gritty is ommitted for focus).

var loginDialog = (function($)
{
 /**
 * Essentially the init function
 */ 
 var show = function()
 { 
 // This actually shows the dialog
 $("#login-dialog").modal({backdrop: "static", keyboard: false});
 applyBindings();
 }
 var hide = function()
 {
 $("#login-dialog").modal("hide");
 }
 var applyBindings = function()
 {
 $("#login-dialog .btn-primary").click(onLoginClick);
 };
 var onLoginClick = function()
 {
 // validate form, attempt login...
 // logginIn is a jQuery Deferred.
 var loggingIn = myApi.login(username, password);
 loggingIn.done( handleResponse );
 loggingIn.fail( handleError );
 };
 var validate = function(username, password)
 {
 // simple form validation
 };
 var handleResponse = function(json)
 {
 // check json psuedo code
 if (json != "Ok") 
 {
 handleError();
 }
 else
 {
 handleSuccess();
 }
 };
 var handleError = function(msg)
 { 
 msg = msg || "There was a problem communicating with the server";
 var tmpl = _.template($("#alert-tmpl").html(), { head: "Error", body: msg});
 $("#login-dialog form").append(tmpl);
 $("#login-dialog .btn-primary").removeAttr("disabled");
 };
 var handleSuccess = function() 
 { 
 hide();
 };
 return {
 show: show,
 hide: hide
 }
})(jQuery);

• Flambino: Can't recommend using "brace on new line" in JavaScript. JS interpreters will insert a semicolon at the end of a line, if it "thinks" it's missing. I see you don't have a newline between your final return and the following brace - if you did, the function would always return undefined. You probably know this, which is why you wrote it. Trouble is that the style then becomes inconsistent. But it's your call; just know that style can cause bugs in JS. Speaking of consistency, you might want to add some semis after the function declarations. There's one after applyBindings but it's random.
• JonWells: @Flambino. Your right my semi-colons are sloppy after functions and I do agree with you about the braces (but sadly this is just about the only convention I MUST follow at work). My real concern is the process, the way the dialog is wrapped and whether I should be using expressive functions or function declarations.
• Flambino: Of course, just thought I'd mention it. You should really have a talk with your bosses, though, if they're forcing you to use brace-on-new-line in one of the few languages where it can have serious side-effects, and isn't just a religious debate :)

Answer 1

First off, I'd retrieve all the elements right away, instead of running $(...) again and again:

var loginDialog = (function($) {
 var dialog, form, button, template;
 // Get the elements once, on page load
 $(function () {
 dialog = $("#login-dialog");
 form = dialog.find("form");
 button = dialog.find(".btn-primary");
 template = $("#alert-tmpl");
 });
 function show() {
 dialog.modal({
 backdrop: "static",
 keyboard: false
 });
 }
 // ... et cetera ...
})(jQuery);

As you can see, I've also put the variable declaration first. If you use expressive functions, you should do the same with all those variables too (which is why I usually prefer to use standard function declarations instead - fewer variables to declare).

(oh, and, yes, I've moved the braces around, but we've covered that in the comments :)

Generally, though, it seems quite fine, and well-structured. There are some functions that could be dropped, since they just call a different function - for instance handleSuccess which is basically an alias for hide. On the other hand it's nicely descriptive to have a handleSuccess function. And if you want to more on success than simply call hide, it's of course nice to have encapsulated.

Lastly, in handleError you .append the error message to the form, so if there are more errors the form will keep growing. Don't know if that's by design, but I imagine you might want to only show one error at a time (and perhaps even remove it on success).