modules
74 views
Skip to first unread message
James Kelly
Aug 14, 2012, 3:15:09 AM8/14/12
to kern...@googlegroups.com
Hi Alan
Here is my code for a module. It's ugly. I also realise that I should be extending my kernel to add the jquery stuff rather than just couple to jquery in my module.
Kernel.module.define('ItemLists', {
init: function () {
hub.listen('save-item-complete', function (message) {
//collect all the itemLists
hub.getItemLists();
});
hub.listen('itemlist-search-complete', function (data) {
//Add the data onto the ItemLists page
var container = $("#ItemListsContainer");
container.empty();
$.each(data, function (index, value) {
container.append('<div class="accordian-group" id="subcontainer-' + index + '"></div>');
subcontainer = $("#subcontainer-" + index);
subcontainer.append('<div id="subcontainer-header-' + index + '" class="accordian-heading"><a class="accordian-toggle" href="#subcontainer-body-' + index + '" data-parent="#ItemListsContainer" data-toggle="collapse"><h3>' + value.Id + ': ' + value.Name + '</h3></a></div>');
subcontainer.append('<div id="subcontainer-body-' + index + '" class="accordian-body collapse"><div class="accordian-inner well"><button class="btn" id="show-map-' + value.Id + '">Show Map</button><button class="btn" id="make-map-' + value.Id + '">Map Map</button><button class="btn" id="make-report-' + value.Id + '">Make Report</button><table id="item-list-table-' + value.Id + '" class="table"><thead><tr><td>Description</td><td>CollectedBy</td><td>DateCollected</td></tr></thead><tbody></tbody></table></div></div>');
table = $("#item-list-table-" + value.Id + " tbody");
$.each(value.Inspections, function (index, value) {
table.append("<tr><td>" + value.Description + "</td><td>" + value.CollectedBy + "</td><td>" + value.DateCollected + "</td></tr>");
});
$('#show-map-' + value.Id).click(function () {
hub.broadcast('show-map');
hub.retreiveItemListLocations(value);
});
container.append(subcontainer);
});
});
$("#item-list-refresh").click(function () {
hub.getItemLists();
});
}
});
As you can see, I'm putting a lot of dom code into some places that I really don't want to be, but I am not sure how to go about breaking out the rendering when I need to be able to dynamically build the lists based on the data that I am listening for.
If I wanted to use a templating library, say Hogan.js (or a choice of others), then what sort of extension point am I shooting for? Should that be an extension on the Kernel or the Hub? How would I then go about implementing it to start cleaning up the above? If I can generate a useful extension, happy to contribute it, but I just don't know where to start. Are there other options aside from templating?
Thanks in advance for any help
James
alindsay55661
Aug 14, 2012, 1:28:15 PM8/14/12
to kern...@googlegroups.com
James, these are all very good questions, and there are many possible answers. I will share a few pointers that I think translate well across most projects.
1. Keep your code in small bite-sized functions.
This will help you write clean and easy to read modules. In particular, take care not to expand your methods to do too many things. I would start by refactoring your init method to show only what is needed to initialize the module, break out the listener callbacks, they are separate functions.
Kernel.module.define('ItemLists', {
init: function () {
// Store a reference to the module
var self = this;
// Listen for hub events
self.hub.listen('save-item-complete', self.getItemLists);
self.hub.listen('itemlist-search-complete', self.renderItems);
// Listen for user events
$("#item-list-refresh").click(self.getItemLists);
},
getItemLists: function (message) {
// Not needed but helpful as a matter of convention
var self = this;
//collect all the itemLists
self.hub.getItemLists();
},
renderItems: function (data) {
//Add the data onto the ItemLists page
var self = this,
container = $("#ItemListsContainer");
container.empty();
$.each(data, function (index, value) {
container.append('<div class="accordian-group" id="subcontainer-' + index + '"></div>');
subcontainer = $("#subcontainer-" + index);
subcontainer.append('<div id="subcontainer-header-' + index + '" class="accordian-heading"><a class="accordian-toggle" href="#subcontainer-body-' + index + '" data-parent="#ItemListsContainer" data-toggle="collapse"><h3>' + value.Id + ': ' + value.Name + '</h3></a></div>');
subcontainer.append('<div id="subcontainer-body-' + index + '" class="accordian-body collapse"><div class="accordian-inner well"><button class="btn" id="show-map-' + value.Id + '">Show Map</button><button class="btn" id="make-map-' + value.Id + '">Map Map</button><button class="btn" id="make-report-' + value.Id + '">Make Report</button><table id="item-list-table-' + value.Id + '" class="table"><thead><tr><td>Description</td><td>CollectedBy</td><td>DateCollected</td></tr></thead><tbody></tbody></table></div></div>');
table = $("#item-list-table-" + value.Id + " tbody");
$.each(value.Inspections, function (index, value) {
table.append("<tr><td>" + value.Description + "</td><td>" + value.CollectedBy + "</td><td>" + value.DateCollected + "</td></tr>");
});
$('#show-map-' + value.Id).click(function () {
self.hub.broadcast('show-map');
self.hub.retreiveItemListLocations(value);
});
container.append(subcontainer);
});
}
});
Doing this allows you to see more clearly what is going on here. When you initialize your module you tell it to listen for a few hub events and a user click event. That sounds like correct behavior for module initialization. The holes also become apparent when you break things out this way and it will be easy to fix. This is where I may not be helpful because I don't have the rest of your application. But one thing that stuck out to me is that you are getting the itemLists in two places (on 'save-item-complete' and on '#item-list-refresh' click), but nothing is done with this list. Maybe you are doing something with it in the call to get it. If so, I recommend doing a refactor as I did above and make sure that each method really does only one thing. hub.getItemLists should only get lists, nothing more.
2. Abstracting the base library can be helpful for certain cases but is costly
It is a great thought to move jQuery out of your module entirely, but this comes at a cost which doesn't make sense in all applications. You want to do this if you need maximum flexibility and the option to switch out jQuery for another library in the future. The downside here of course is that you can't use any library specific widgets, including jQuery widgets. The widgets themselves must be written to run on your abstracted base library. So this really makes more sense for applications that can afford a large investment. If you are able to commit to a single library it will save you time and give you the option to use anything else that requires that library, right out of the box. This is by far the easiest solution and honestly probably the most common. I would not worry too much about abstracting the base library unless you really need the flexibility.
3. Use a (precompilable) template library for markup
This will be very helpful in the long run even though it requires some setup. Writing markup as javascript strings is just a bad idea. Using a template library allows you to put all your markup into separate files where it belongs. Now, there is a distinction here between templates that compile on the fly in the browser and those that can be precompiled. There are pros and cons to each but I prefer precompiled templates.
If you precompile templates then they are available to you in your application when it loads. You don't have to fetch them via ajax which means you can synchronously render your application. This turns out to be a huge help when you are rendering your application in lots of little blocks (aka modules) and/or you need to control the timing. Of course you can do this via callbacks but the code cleans up nicely when you call render as a synchronous method.
Hogan.js does support precompiling so that should work fine. I use Handlebars.js.
4. Provide a rendering extension to Kernel and/or specific rendering module methods
If you want a common rendering pattern (which you likely do) then extend Kernel to provide it. Here is a very basic extension you can use (not tested):
Kernel.extend(Kernel, {
onStart: function(module) {
// Conditionally render the module
if (module.renderTo) {
// Define a render function if one doesn't exist
var render = module.render || function() {
// Get the template, assumes they are stored in global 'Templates'
var template = Templates[module._internals.type];
// Render the module
$(module.renderTo).html(template.render());
};
// Render
module.render();
}
}
});
This will automatically render your module when it is started. It assumes that you are storing your precompiled templates in the following format: Templates[moduleType]. In your case that would be Templates.ItemLists. You'll note, though that no data is being passed this template. That might be fine for putting a module on the page but as you mentioned you need to update the contents of the module. You can accomplish this by rerendering targeted parts of the module or even rerendering the entire module, backbone.js style. Now you have a module that is starting to look like this:
Kernel.module.define('ItemLists', {
renderTo: '#ItemListsContainer',
init: function () {
// Store a reference to the module
self = this;
// Listen for hub events
self.hub.listen('save-item-complete', self.getItemLists);
self.hub.listen('itemlist-search-complete', self.render);
// Listen for user events
$("#item-list-refresh").click(self.getItemLists);
// Make sure to use delegate so clicks are handled after rerender
$(self.renderTo).delegate(".show-map").click(function() {
self.showMapClick($(this));
});
},
getItemLists: function (message) {
// Not needed but helpful as a matter of convention
var self = this;
//collect all the itemLists
self.hub.getItemLists();
},
showMapClick: function(button) {
var self = this;
self.hub.broadcast('show-map');
self.hub.retreiveItemListLocations(button.id);
},
// Add the data onto the ItemLists page
render: function (data) {
var self = this;
// Get the template, assumes they are stored in global 'Templates'
var template = Templates[self._internals.type];
// Render the module
$(self.renderTo).html(template.render(data));
}
});
Ok, these are just examples and really there are lots of ways to do things. But from this perspective you start to see more what is happening in your application. If I knew more about the overall application I could help make design decisions which is really where a lot of Kernel.js work is. Because the library is an architecture, you quickly learn that you'll be making architectural decisions as you build your app. Eventually I am going to provide more examples of some of the decisions I have made that should be helpful. Hopefully this helps in the meantime.
Alan
James Kelly
Aug 14, 2012, 8:57:24 PM8/14/12
to kern...@googlegroups.com
Alan
Thanks very much for the advice and the refactoring. The time you have taken here is most appreciated. Conceptually, it all makes very good sense and has cleared up some of my cloudiness (and bad code!). This certainly serves as an good example exercise in what not to do for me. The modules was the main area of pain in the application so far, there isn't too much code in either the application kernel, or the application hub yet, as most of the code is to display existing information. Once I have refactored it all to remove all the inline javascript rendering code, I will be a lot happier. Once that is all done for everything then I would be more comfortable putting more of my application up, perhaps even with a few modifications, it could be some sort of example.
Just to clarify on your point that hub.getListItems doesn't seem to do anything, the following code is what I have, which is stored in my application.hub.js file.
hub.getItemLists = function () {
var self = this;
Kernel.restRequest({
type: 'GET',
url: 'itemlist',
success: function (data) {
self.broadcast('itemlist-search-complete', data);
},
failure: function (er) {
self.broadcast('error-message',er);
}
});
}
Instead of returning data in a callback, I just broadcast a message with the data, which if you have a look at in the module, I listen for and do something with. While I obviously know what's happening, I can see that if you just looked at the module, then it appears that nothing is happening.
I like to communicate with messages, so would the below be more transparent?
my_module.js
getItemLists: function (message) {
// Not needed but helpful as a matter of convention
var self = this;
//collect all the itemLists
self.hub.broadcast('get-list-items');
}
my_hub.js
this.hub.listen('get-list-items', function () {
var self = this;
Kernel.restRequest({
type: 'GET',
url: 'itemlist',
success: function (data) {
self.hub.broadcast('itemlist-search-complete', data);
},
failure: function (er) {
self.hub.broadcast('error-message',er);
}
});
})
I'm not sure I am gaining much by doing it this way. What are your thoughts?
alindsay55661
Aug 15, 2012, 11:57:29 AM8/15/12
to kern...@googlegroups.com
Ok I see. The second option definitely suggests that any further action will come in via an event which is perhaps a bit more clear. I think I prefer it, but as long you document and follow the pattern everywhere I'm not sure it makes much of a difference. This seems to be merely a preferential option, do what you feel most comfortable with (just be sure to document it).
Alan
James Kelly
Aug 15, 2012, 7:16:26 PM8/15/12
to kern...@googlegroups.com
I think I prefer it too. Best keep it consistent as you have said and it will be easier to maintain in the long run. Thanks.
James
Reply all
Reply to author
Forward
0 new messages