Is there a better way of writing this JavaScript, or is it idiomatic?

Question

My background is with C and python, and Javascript is fairly new to me, so I'm wondering if this is reasonable code.

In particular, is it appropriate to use the higher order function makecb in this context, or is there a better/shorter/clearer way of writing it?

$(function(){
 var controllers = {
 "#showhelp": { name: "Help", id: "#help" },
 "#showlog": { name: "Log", id: "#log" }
 };
 function hideModals(){
 for(cter in controllers){
 $(controllers[cter].id).css("display", "none");
 $(cter).text("Show "+controllers[cter].name);
 }
 }
 function showModal(cter){
 hideModals();
 $(controllers[cter].id).css("display","block");
 $(cter).text("Hide "+controllers[cter].name);
 }
 function makecb(controller){
 return function(){
 if($(controllers[controller].id).css("display")=='none'){
 showModal(controller);
 }else{
 hideModals();
 }
 }
 }
 for(cter in controllers){
 $(cter).click(makecb(cter));
 }
});

The idea is that the only thing that has to be done to add a new modal dialogue is to add the description. The appropriate html is as follows:

<div id="bottombar">
 <a id="showlog" href="#">Show Log</a> 
 <a id="showhelp" href="#">Show Help</a> 
</div>
<div id="log" style="display:none"></div>
<div id="help" style="display:none"></div>

PS: If you have the privileges needed to add the tags data-driven-programming or idiom and you feel it would be appropriate, I'd appreciate it.

Answer 1

General tips:

• For calls done more than once and return the same results, cache them in a variable
• Look out for variable declarations. Always use var unless they are known to be somewhere in the outer scope. Otherwise, forgetting it would make it an implied global - which is bad.
• Localize variables whenever necessary. This is to prevent global conflict from destroying your references.
• Hard-coding is bad enough. Hard-coding in two or more places is worse. In your code, you are hard coding the ids to for modals, the links, and on the JS to link them both. Better use existing references and modify it to fit other references. In this case, use the href hashes. They were built to point to ids.
• DRY (Don't Repeat Yourself). Factor out common operations and turn them into their own functions. For example, the link text replace operation.
• In jQuery, chain when necessary, especially when the previous operation returns the elements needed for the next operation. This avoids multiple calls to jQuery just to re-reference the element for the next operation.

Here's a working fiddle

Here's a modified HTML

<div id="bottombar">
 <!--semantics and progressive enhancement tip: use hash to point to id'ed sections-->
 <a href="#log">Show Log</a>
 <a href="#help">Show Help</a> 
</div>
<!--HTML sections should be id'ed so hash links can jump to them if JS is off-->
<!--Use classes to refer to element of common functionality, both in CSS and JS-->
<div id="log" class="modal"></div>
<div id="help" class="modal"></div>

CSS:

/*use external style instead of inline*/
.modal{
 display:none;
}

here's a packed version (no comments):

$(function ($) {
 var modals = $('.modal');
 function setText(prependText, oldText) {
 return prependText + ' ' + oldText.substring(oldText.indexOf(' ') + 1)
 }
 $('#bottombar').on('click', 'a', function (event) {
 event.preventDefault();
 var $this = $(this),
 modalid = $this.attr('href'),
 modal = modals.filter(modalid);
 if (modal.is(':visible')) {
 modal.hide();
 $this.text(function (index, oldText) {
 return setText('Show', oldText)
 })
 } else {
 modals.hide();
 modal.show();
 $this.text(function (index, oldText) {
 return setText('Hide', oldText)
 }).siblings().text(function (index, oldText) {
 return setText('Show', oldText)
 })
 }
 })
});

here's a commented version:

//use the provided local jQuery and don't rely on the global.
//That way, name conflicts won't affect your inner code
$(function ($) {
 //get all modals and reference them
 var modals = $('.modal');
 //factor out the link text replace operation
 //avoid regex on minor operations as it is slow
 function setText(prependText, oldText) {
 return prependText + ' ' +oldText.substring(oldText.indexOf(' ')+ 1);
 }
 //use delegation to add your handler. This avoids putting handlers on each link
 $('#bottombar').on('click', 'a', function (event) {
 //prevent the default action. Although hashes work, this one prevents
 //the page from jumping upwards
 event.preventDefault();
 //cache items used over and over again in the following operations
 var $this = $(this),
 modalid = $this.attr('href'),
 modal = modals.filter(modalid);
 if (modal.is(':visible')) {
 //if modal is visible
 //hide the current modal
 modal.hide();
 //replace the current link text
 $this.text(function (index, oldText) {
 return setText('Show', oldText);
 });
 } else {
 //if modal is not visible
 //hide other modals
 modals.hide();
 //show the current modal
 modal.show();
 //set link texts accordingly
 $this
 .text(function (index, oldText) {
 return setText('Hide', oldText);
 })
 .siblings()
 .text(function (index, oldText) {
 return setText('Show', oldText);
 });
 }
 });
});

• brice: Thanks Joseph. It felt wrong while writing it, and removing the controllers object is much cleaner. I feel stupid for not thinking of grabbing all the modals using a class too! Much yet unlearned, it seems...

Answer 2

While the use of high order functions in JavaScript is fine and normal, your code has other issues.

Symptoms

Your usage of the $.fn.css method here is to do 3 things:

1. show the element
2. hide the element (more precisely: exact opposite of state 1)
3. enumerate the state of the visibility of the element (check if it is in state 1 or state 2)

It would be better code to have cases 1 and 2 be the same method call, differentiated by a boolean parameter. In this case I would use $.fn.toggle(). In general it is better to say what you want, rather than how to do something you want to do. This is because the details of how to do something tend to change, while the logic remains the same. To complete this separation you now should use some function that checks the state of this item; enter in this case :visible; thus those 3 uses become:

1. $(element).toggle(true);
2. $(element).toggle(false);
3. $(element).is(':visible');

However, you aren't just toggling a single element's visibility here; the link text is changing at the same time. Atop that, the overall logic is that the system can only have at most one dialog open at a time.

Cause

I think the structure of this code fails to make that logic immediately obvious.

Solution

Let's abstract the implementation away a little into a method (to be written farther down) and rewrite the click event as if it already exists:

$(function () {
 var $buttons = $('#bottombar a'),
 controllers = {
 "showhelp": { name: "Help", id: "#help" },
 "showlog": { name: "Log", id: "#log" }
 };
 $buttons.click(function (e) {
 var that = this;
 e.preventDefault();
 toggleModal(function () {
 return that === this && !$(controllers[this.id].id).is(':visible');
 });
 });
});

Here now there are a few things I am not particularly fond of:

1. not caching the inner jQuery object
2. checking the state of each dialog by checking its implementation when we already have a structure modeling it

We can easily kill both of those with a simple modification to the controllers object:

$(function () {
 var $buttons = $('#bottombar a'),
 controllers = {
 "showhelp": {
 name: "Help",
 dialog: $("#help"),
 visible: false
 },
 "showlog": {
 name: "Log",
 dialog: $("#log"),
 visible: false
 }
 };
 $buttons.click(function (e) {
 var that = this;
 e.preventDefault();
 toggleDialogs(function () {
 return that === this && !controllers[this.id].visible;
 });
 });
});

All that is left is actually writing the implementation of toggleDialogs.

End Result

$(function () {
 var $buttons = $('#bottombar a'),
 controllers = {
 "showhelp": {
 name: "Help",
 dialog: $("#help"),
 visible: false
 },
 "showlog": {
 name: "Log",
 dialog: $("#log"),
 visible: false
 }
 };
 
 function toggleDialogs(visiblefn) {
 $buttons.each(function () {
 var isVisible = visiblefn.call(this),
 model = controllers[this.id];
 model.dialog.toggle(isVisible);
 if (isVisible) {
 $(this).text('Hide ' + model.name);
 } else {
 $(this).text('Show ' + model.name);
 }
 model.visible = isVisible;
 });
 }
 $buttons.click(function (e) {
 var that = this;
 e.preventDefault();
 toggleDialogs(function () {
 return that === this && !controllers[this.id].visible;
 });
 });
});

• Bill Barry: jsFiddle here
• brice: Cheers Bill! I didn't even know about preventDefault(), and never thought about grabbing the object for the dialog using jquery to put it in the conrtroller. +1