Problem
I have an application which currently uses AJAX for CRUD operations on a simple Person object. The following script works fine, but I’m looking for some pointers on how to structure my code. Currently, I have four methods for CRUD operations, and I’ll extend my person module as needed, but I was hoping to get an idea of whether or not this is a nice way to manage code.
$(document).ready(function () {
//open the modal
//the update, delete and insert functions come from boxes present in a modal not shown
$('#openModal').click(function () {
$('#contact').modal();
});
//insert
$('#btnSavePerson').click(function () {
//is it a good idea to have the data objects inside all of these functions?
var data = {
personId: $('#tbPersonId').val(),
firstName: $('#tbFirstName').val(),
lastName: $('#tbLastName').val()
};
person.insert(data);
});
//delete
$('#btnDeletePerson').click(function () {
var personId = $('#tbDelete').val();
person.remove(personId);
});
//update
$('#btnUpdatePerson').click(function () {
var data = {
personId: $('#tbPersonId').val(),
firstName: $('#tbFirstName').val(),
lastName: $('#tbLastName').val()
};
console.log(JSON.stringify(data));
person.update(data);
});
//get
$('#getPeople').click(function () {
person.get();
});
//*********************person module
var person = (function () {
//the ajax object will be passed and shared between
//the functions here (type and dataType dont change in this example)
var ajax = {
type: "POST",
url: "",
data: {},
dataType: "json",
contentType: "application/json",
success: {},
error: {}
}
//************ insert
function insert(data) {
ajax.url = '../Service.asmx/InsertPerson';
ajax.success = function () {
console.log('success before setTimeout');
var successMessage = $('<div>').text('Successfully added to the database...')
.css('color', 'green')
.attr('id', 'success');
$('.modal-body').append(successMessage);
window.setTimeout(function () {
$('.modal-body').each(function () {
$(this).val('');
});
$('#contact').modal('hide');
}, 1000);
}
ajax.data = JSON.stringify(data);
ajax.error = function (xhr, ajaxOptions) {
console.log(xhr.status);
console.log(ajaxOptions);
};
$.ajax(ajax);
}
//************* delete
function remove(data) {
ajax.url = '../Service.asmx/DeletePerson';
console.log('working string: ');
var obj = { personId: data };
console.log(JSON.stringify(obj));
ajax.data = JSON.stringify(obj);
console.log(ajax.data);
ajax.success = function () {
console.log('person successfully deleted');
var successMessage = $('<div>').text('Person successfully deleted from the database')
.css('color', 'red');
$('.modal-body').append(successMessage);
window.setTimeout(function () {
$('.modal-body input').each(function () {
$(this).val('');
});
$('#contact').modal('hide');
}, 1000);
};
ajax.error = function (xhr, ajaxOptions) {
console.log(xhr.status);
console.log(ajaxOptions);
}
$.ajax(ajax);
}
//*************** update
function update(data) {
ajax.url = '../Service.asmx/UpdatePerson',
ajax.data = JSON.stringify(data),
ajax.success = function () {
console.log('update was successful');
var successMessage = $('<div>').text('Record successfully updated...')
.css('color', 'green');
$('modal-body').append(successMessage);
window.setTimeout(function () {
$('.modal-body input').each(function () {
$(this).val('');
});
$('#contact').modal('hide');
}, 1000);
};
ajax.error = function (xhr, ajaOptions) {
console.log(xhr.status);
};
$.ajax(ajax);
};
//************** get
function get() {
//****** appropriate to have this function here?
function style(json) {
json = $.parseJSON(json);
var $personArray = [];
for (var obj in json) {
var $person = $('<div>').addClass('person');
for (var prop in json[obj]) {
var label = $('<span>').text(prop).addClass('badge pull-left').appendTo($person);
var propertyData = $('<span>').text(json[obj][prop]).addClass('pull-right').appendTo($person);
}
$personArray.push($person);
}
return $personArray;
}
ajax.url = '../Service.asmx/GetPersons';
ajax.success = function (data) {
data = data.d;
console.log('ajax successful');
$('body').append(style(data));
};
ajax.error = function (xhr, ajaxOptions) {
console.log(xhr.status);
console.log(ajaxOptions);
}
$.ajax(ajax);
}
//*********** public methods
return {
insert: insert,
remove: remove,
update: update,
get: get
};
})();
});
HTML in case someone wants to see it:
<body>
<div class="navbar navbar-inverse navbar-static-top">
<div class="container">
<a href="#" class="navbar-brand">ui</a>
<button class="navbar-toggle" data-toggle="collapse" data-target=".navHeaderCollapse">
<span class="icon-bar"></span><span class="icon-bar"></span><span class="icon-bar">
</span>
</button>
<div class="collapse navbar-collapse navHeaderCollapse">
<ul class="nav navbar-nav navbar-right">
<li class="active"><a href="#">Home</a></li>
<li><a href="#">Blog</a></li>
<li><a id="openModal" href="#contact">Contact</a></li>
</ul>
</div>
</div>
</div>
<p id="getPeople" class="btn btn-block">get them peoples</p>
<div class="modal fade" id="contact" role="diaglog">
<div class="modal-dialog">
<div class="modal-content">
<div class="modal-header">
<h2>contact: </h2>
</div>
<div class="modal-body">
The improtant information from the database is going to be displayed here.
<span class="label label-success">Person ID: </span><input id="tbPersonId" class="warning" /><br />
<span class="label label-success">First Name:</span><input id="tbFirstName" /><br />
<span class="label label-success">Last Name: </span><input id="tbLastName" /><br />
<span class="label label-danger">Delete box: </span><input id="tbDelete" />
</div>
<div class="modal-footer">
<a class="btn btn-default" id="modalClose" >Close</a>
<a class="btn btn-primary" id="btnSavePerson">Save changes</a>
<a class="btn btn-danger" id="btnDeletePerson">Delete person</a>
<a class="btn btn-block" id="btnUpdatePerson">Update Person</a>
</div>
</div>
</div>
</div>
</body>
Solution
Right, I’m on a break, so this is just a few pointers to help you along. I’ll probably get back to this answer and add some more details in days to come…
the first thing I noticed is that you declared your person module somewhere half-way your ready callback function. Though the variable person will be hoisted to the top of the scope the IIFE (Immediatly invoked function expression) that assigns it cannot be hoisted. It’s an expression, so your code evaluates to:
$(document).ready(function()
{
var person;
//some code, using person as a module
person = (function()
{
}());//assignment happens here!
});
Though it’s very unlikely this will cause problems, it’s generally a good idea to move the assignment to the top of the scope:
$(document).ready(function()
{
var person = (function()
{
}());
//rest of your code here...
});
Another thing that sort of annoys me is your use of window:
window.setTimeout
But at the same time, you’re using:
console.log();
without the window reference. That’s inconsistent and, if anything, quite redundant. JS resolves the window name in the same way as it resolves console or setTimeout: it scans the current scope, if it can’t find the name there, it moves up to a higher scope until it reaches the global namespace/scope.
As you may know, the global NS is, effectively an object that has no name. (try console.log(this === window) in your console). This object has a property, called window, which is nothing more than a circular reference to this nameless object. Hence console.log(this.window === window); console.log(this.window === this) will both log true. As a result setTimeout will be very, very marginally faster than window.setTimeout anyway, so I’d suggest you loose the window where you can.
Next: the person module isn’t exactly stand-alone, in it, you assume $ to be the jQuery object, and you expect it to be defined in a higher scope. I wouldn’t do that. Here’s why:
var dependency = {foo: function()
{
return 'bar';
}};
var unsafeModule = (function()
{
return { getBar : dependency.foo};
}());
unsafeModule.getBar();//returns bar, fine...
dependency = 213;
unsafeModule.getBar();//ERROR
A better, safer approach would be:
var dependency = {foo: function()
{
return 'bar';
}};
var saferModule = (function(d)
{
return { getBar : d.foo};
}(dependency));
By passing a reference to the object my module, that object remains in scope, and my module will continue to work just fine ‘n dandy, no matter how many times the dependency variable gets reassigned.
But even better would be:
var dependency = {foo: function()
{
return 'bar';
}};
var saferModule = (function(d)
{
var mod = {setter: function(dep)
{
d = dep;
this.getBar = d.foo;
delete (this.setter);
return this;
}};
if (d === undefined) return mod;
return mod.setter(d);
}());//no dependency passed!
d.setter(dependency);//sets dependency
d.getBar();//works
When we apply this logic to your module, you can safely move the person module declaration to another file, or outside of the $(document)ready callback’s scope:
var person = (function()
{
var mod = { remove : function(){},
get : function(){}};//declare your module
return {init: function($)
{
var p;
if($ instanceof jQuery)
{
delete(this.init);//unset init method
for (p in mod)
{//copy module properties into person
this[p] = mod[p];
}
return this;
}
throw {message: 'Module needs jQ to work', code: 666};
}};
}
$(document).ready(function()
{
person.init($);//pass jQ ref to module, it'll init itself.
});
Lastly, a side-note. You’re trying to implement the module pattern. That’s good, but a module requires a closure. You can use closures all over the place to avoid, for example, excessive DOM queries.
For example:
$('#openModal').click(function () {
$('#contact').modal();
});
Now each time I click on $('#openModal'), the callback function gets called. this function will query the DOM, because $('#contact') is a DOM selector. If I click this element 10 times, the DOM will be queried 10 times. A closure can reduce this:
$('#openModal').click((function(contact)
{
return function()
{
contact.modal();//use ref in scope, no more DOM lookups required
};
}($('#contact')));//pass DOM ref here, query dom once
These sort of, seemingly trivial, optimizations are easy, but can make a difference the bigger your script gets, and the more interactive your site has to be.
Ok, another update. I’ve noticed you write code like this:
$('#getPeople').click(function()
{
person.get();
});
This is typical jQ code: passing an anonymous function to the event handler. When a callback function (ie the anonymous function) merely calls another function, wouldn’t it be easier, shorter and more efficient to just pass a reference to the function you need in the first place?
$('#getPeople').click(person.get);
It’ll do pretty much the same thing, is shorter to write and saves on a pointless function object.
The only downside being: you’re losing the call-context (within the person.get function this won’t point to person anymore). So I had a look at your person.get function. I’ll paste it here, with some comments. I’ll address some issues further down
function get()
{
//****** appropriate to have this function here?
// NO, this function will be redeclared each time person.get is called
//it should be defined somewhere else, in a higher scope
function style(json)
{
json = $.parseJSON(json);
var $personArray = [];
for (var obj in json)
{//still best to filter a for-in loop
var $person = $('<div>').addClass('person');//again, DOM query?
for (var prop in json[obj])
{//I can't see where you're using the label var...
var label = $('<span>').text(prop).addClass('badge pull-left').appendTo($person);
var propertyData = $('<span>').text(json[obj][prop]).addClass('pull-right').appendTo($person);
}
$personArray.push($person);
}
return $personArray;
}
ajax.url = '../Service.asmx/GetPersons';
ajax.success = function (data)
{//this callback can be reused, too
data = data.d;
console.log('ajax successful');
$('body').append(style(data));//avoid DOM access!
};
ajax.error = function (xhr, ajaxOptions)
{//another callback to be avoided!
console.log(xhr.status);
console.log(ajaxOptions);
}
$.ajax(ajax);
}
So, let’s consider what the get function actually needs to do its job:
- a
stylefunction - a couple of callbacks (
successandonerror) for theajaxobject literal, and its own URL - A couple of DOM elements, for example the
$('body'). That’s a pretty static element, I do believe.
So instead of getting all these objects, why not define/declare them when we’re creating our get function, and reuse them every time this function is called? Instead of declaring function get inside the IIFE that returns the person module, we could write something like:
var get = (function(body)
{
var successCallback, style,
errorCallback = function(xhr, ajaxOptions)
{//or declare as local var to function
console.log(xhr.status);
console.log(ajaxOptions);
};
successCallback = function (data)
{
console.log('ajax successful');
//use DOM reference, parse it here already... SRP!
body.append(style($.parseJSON(data.d)));
};
style = function(json)
{
var personArray = [], obj, prop, person;//declare at top of scope
for (obj in json)
{//I do hope this isn't an array!
if (json.hasOwnProperty(obj))
{//best to check, still
person = $('<div class="person"'>);//avoid addClass call
for (prop in json[obj])
{
if (json[obj].hasOwnProperty(prop))
{//no point in assigning return val to vars you do not use
$('<span class="badge pull-left">')
.text(prop).appendTo(person);
$('<span class="pull-right">')
.text(json[obj][prop]).appendTo(person);
}
}
personArray.push(person);
}
}
return personArray;
};
//the actual get function:
return function()
{
ajax.url = '../Service.asmx/GetPersons';
ajax.success = successCallback;
ajax.error = errorCallback;
return $.ajax(ajax);
};
}($('body')));//either pass as argument
Now what I’ve done here is sort-of created a get module within the main person module. You can (and probably eventually will) work some more on this, since all of your person module functions perform ajax requests, you might add a more generic getAjax function that you choose not to expose.
A function that gouverns the ajax object, and keeps track of the callbacks and urls each function in the module requires, and that sets the ajax object accordingly:
var doAjax = function()
{
var ajax = {},//your ajax object literal
callers = {get: {url: '../Service.asmx/GetPersons',
success: function(){},
error: function(){}},
insert: {url: '../Service.asmx/InsertPerson',
success: function(){}},
default: {url: 'some/default',
success: function(){}}
};
return function(data,caller)
{
var settings,p;
caller = caller || 'default';
settings = callers[caller] || callers.default;
for (p in settings)
{
if (settings.hasOwnProperty(p)) ajax[p] = settings[p];
}
return $.ajax(ajax);
};
}());
Now all your module functions can just call this function like so:
doAjax(myData, 'insert');
//or
doAjax(undefined, 'get');
Oh, and this code would go here:
var person = (function()
{
var doAjax = (function(){}());//in module!
}());