If I understand your code correctly, it looks like you want to render tables with 3 records each, with the #header template for the top of each table and the #closer template for the bottom of each table, except for the last one, which should end with #footer instead of #closer.
First, the little things:
- I think you have some unused variables in your code: It looks like
dLength and done are never used.
limit is a magic number and seems out of place in your template. If I were you I would pass that number to the template function along with your other data.- You render
<div id="resourceParameter"> several times, but element IDs must be unique in a document. We shouldn't re-use an ID.
- For the last table, the
</div> is never rendered, so the above div is never closed! Fixing this is discussed below.
- You're compiling templates inside of templates, which means that every time the outer template is rendered, you recompile the inner template, which is unnecessary and negates most of Underscore.js' rendering speed advantage. Fixing this is discussed toward the end.
Deal with groups of items, not index math
Now, on to the meat of the question. I think the biggest code smell here is the modulo arithmetic, i.e. <% if(index % limit == 0 ) { %> et al. This works, but it's hard to read. Instead, we can make the code match our intent: We want tables with 3 records each, so let our code deal with the data in groups of 3. This saves us from having to do arithmetic on their indexes—or worring about their indexes at all. Changing only the #recourseParameterTemp template, we have this:
<script type="text/template" id="recourseParameterTemp">
<% var group %>
<% while(group = recourseParameter.splice(0, limit), group.length) { %>
<%= _.template($('#header').html())({ "parameterCode": parameterCode }) %>
<% _.each(group, function (item, index) { %>
<% if(item.description) { %>
<tr>
<td colspan=5><%= item.description %></td>
</tr>
<% } %>
<% if(recourseParameter.length) { %>
<%= _.template($('#closer').html())() %>
<span>Continue ... </span>
</div>
<% } %>
<% }) %>
<% } %>
<%= _.template($('#footer').html())() %>
</script>
Here's a fiddle of it in action.
I think further improvements are possible, however. For one thing, why is <div id="recourseParameter"> inside a sub-template, but </div> is inside the main template? Not only is this an odd inconsistency, it actually introduced the missing </div> bug mentioned above. We can fix this by moving <span>Continue...</span> and </div> into the #closer template, and also adding </div> to the #footer template (see this fiddle), but the bug points to a more general code smell: Templates with no closing tags, or no opening tags. If a template closes all of the tags it opens, it's much easier to notice problems like our missing </div>.
The single responsibility principle
We usually talk about the single responsibility principle as it relates to object-oriented programming, but it's very useful here, too: Our templates are meant to isolate blocks of code that are related, and a great template has a single responsibility. Our #recourseParameterTemp template has a bunch of responsibilities—it iterates through the data, it decides whether or not to render each <tr>, and it decides what kind of footer render. That's too much! Using this principle, a number of possible improvements reveal themselves.
In my changes, I introduced an additional loop, i.e. I put a while around the _.each. This makes it a little harder to read, but also reminds us that it's often good to extract the inside of a loop into a sub-template that has a single responsibility. Extracting the inside of the while loop into a template called #groupTmpl (along with the #closer/#footer changes mentioned above) gives us this:
<script type="text/template" id="groupTmpl">
<%= _.template($('#header').html())({ parameterCode: parameterCode }) %>
<% _.each(group, function (item, index) { %>
<% if(item.description) { %>
<tr><td colspan=5><%= item.description %></td></tr>
<% } %>
<% }) %>
<% if(recourseParameter.length) { %>
<%= _.template($('#closer').html())() %>
<% } else { %>
<%= _.template($('#footer').html())() %>
<% } %>
</script>
<script type="text/template" id="recourseParameterTmpl">
<% var group %>
<% while(group = recourseParameter.splice(0, limit), group.length) { %>
<%= _.template( $('#groupTmpl').html() )( {
recourseParameter: recourseParameter,
parameterCode: parameterCode,
group: group } ) %>
<% } %>
</script>
Our intent has become clearer. We see that #recourseParameterTmpl's single responsibility is to render a sub-template (#groupTmpl) once for each group, and that's all it does. This makes it easier to see the intent of the rest of the code, now in #groupTmpl. It also makes it easier to see that #groupTmpl still has multiple responsibilities: It renders the #header sub-template, it loops through each item in group, and it decides what kind of footer to render.
(You probably also noticed that I changed #recourseParamaterTemp to #recourseParameterTmpl; I did this just because "temporary" is frequently shortened to "temp" in programming; "tmpl" is less ambiguous.)
One footer template
We could extract the inside of the _.each loop into a sub-template, but I think we can agree that that's probably overkill. The main thing we can improve here is that #groupTmpl is deciding what kind of footer to render, but that shouldn't be its responsibility. Instead, we should just render one template, and let that template decide what the footer should look like. Looking at our existing #closer and #footer templates, we can see that they're almost identical, except for the contents of <span>. It makes sense to consolidate these into one template. Removing #closer entirely, #footer and #groupTmpl now look like this (here's the fiddle):
<script type="text/template" id="footer">
</tbody>
</table>
<span>
<%= recourseParameter.length ? "Continue..." : "All are done" %>
</span>
</div>
</script>
<script type="text/template" id="groupTmpl">
<%= _.template($('#header').html())({ parameterCode: parameterCode }) %>
<% _.each(group, function (item, index) { %>
<% if(item.description) { %>
<tr><td colspan=5><%= item.description %></td></tr>
<% } %>
<% }) %>
<%= _.template($('#footer').html())({ recourseParameter: recourseParameter }) %>
</script>
Now #groupTmpl's intent is very clear, and we don't have any duplicated footer code.
Self-contained templates close their own tags
This is looking pretty good, but we still have the problem I mentioned above: #header opens tags that it doesn't close, and #footer closes tags that it doesn't open.
An alternative is to combine those templates into a single #wrapper template. Since Underscore.js templates don't have good support for "nested" templates, this gets a little hairy, unless we do one simple thing—which we ought to do anyway: Initialize the sub-template functions outside of the template itself. Instead of doing this:
<%= _.template($('#groupTmpl').html())( ... ) %>
We should do this, in our JavaScript:
var groupTmpl = _.template( $('#groupTmpl').html() );
...for each of our templates, and then pass the compiled template functions as locals when we use a template, which will allow us to do this in a template:
<%= groupTmpl( ... ) %>
In addition to looking cleaner, this avoids recompiling the sub-template every time the (outer) template is compiled, as mentioned above.
Along with this, instead of passing around a bunch of variables (recourseParameter, parameterCode, etc.) among our templates, it's probably a good idea to use the variable option to _.template. That is, when we call var tmpl = _.template(html, { variable: 'locals' }), then render it with tmpl({ foo: "bar" }), foo will be available as locals.foo inside the template, instead of just foo. While this seems less convenient, it lets us just pass locals to sub-templates instead of specifying the whole object. Instead of passing { variable: 'locals' } every time we compile a template, though, we can set it globally using _.templateSettings.variable = 'locals'.
Finally...
...we end up with templates that look like this:
<script type="text/template" id="wrapperTmpl">
<div id="recourseParameter">
<h1>Parameter : <%= locals.parameterCode %></h1>
<table>
<thead>
<tr><th colspan=5>header</th></tr>
</thead>
<tbody>
<%= locals.bodyTmpl(locals) %>
</tbody>
</table>
<span>
<%= locals.recourseParameter.length ? "Continue..." : "All are done" %>
</span>
</div>
</script>
<script type="text/template" id="groupTmpl">
<% _.each(locals.group, function (item, index) { %>
<% if(item.description) { %>
<tr><td colspan=5><%= item.description %></td></tr>
<% } %>
<% }) %>
</script>
<script type="text/template" id="recourseParameterTmpl">
<% locals = _.defaults({ bodyTmpl: locals.groupTmpl }, locals) %>
<% while(locals.group = locals.recourseParameter.splice(0, locals.limit), locals.group.length) { %>
<%= locals.wrapperTmpl( locals ) %>
<% } %>
</script>
And JavaScript that looks like this:
_.templateSettings.variable = "locals";
var tmpl = _.template( $('#recourseParameterTmpl').html() ),
locals = {
limit: 3,
parameterCode: "foo",
recourseParameter: [
{ description: "Foo 1" },
// ...
]
},
renderedTmpl
;
locals.wrapperTmpl = _.template( $('#wrapperTmpl').html() );
locals.groupTmpl = _.template( $('#groupTmpl').html() );
// You could also automate the above with jQuery, e.g.:
//
// $("[type='text/template']").each(function() {
// var el = $(this), id = el.attr('id');
// locals[id] = _.template( el.html() );
// });
renderedTmpl = tmpl(locals)
$('body').append(renderedTmpl)
Here's the final fiddle. I hope that's helpful!
