#Symptoms
Symptoms
#Cause
Cause
#Solution
Solution
#End Result
End Result
#Symptoms
#Cause
#Solution
#End Result
Symptoms
Cause
Solution
End Result
#Symptoms
Your usage of the css$.fn.css method here is to do 3 things:
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():
$(controllers[cter].id).toggle(false); //hides
$(controllers[cter].id).toggle(true); //shows
$.fn.toggle() . In general it is better to say what you want, rather than how to do something you want to do. This more truly separatesis because the logicdetails of what you are attempting from how you are doing itto 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:visible ; thus those 3 uses become:
$(controllers[cter].id).is(':visible'); //returns bool
$(element).toggle(true);$(element).toggle(false);$(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 controllerdialog open at a time.
#Cause
I think the structure of this code fails to make that logic immediately obvious.
#Solution
All that is left is actually writing the implementation of toggleDialogs. In total:
#End Result
Your usage of the css method here is to do 3 things:
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():
$(controllers[cter].id).toggle(false); //hides
$(controllers[cter].id).toggle(true); //shows
This more truly separates the logic of what you are attempting from how you are doing it. To complete this separation you now should use some function that checks the state of this item; enter :visible:
$(controllers[cter].id).is(':visible'); //returns bool
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 controller open at a time.
I think the structure of this code fails to make that logic immediately obvious.
All that is left is actually writing the implementation of toggleDialogs. In total:
#Symptoms
Your usage of the $.fn.css method here is to do 3 things:
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:
$(element).toggle(true);$(element).toggle(false);$(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
All that is left is actually writing the implementation of toggleDialogs.
#End Result
While the use of high order functions in JavaScript is fine and normal, your code has other issues.
Your usage of the css method here is to do 3 things:
- show the element
- hide the element (more precisely: exact opposite of state 1)
- 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():
$(controllers[cter].id).toggle(false); //hides
$(controllers[cter].id).toggle(true); //shows
This more truly separates the logic of what you are attempting from how you are doing it. To complete this separation you now should use some function that checks the state of this item; enter :visible:
$(controllers[cter].id).is(':visible'); //returns bool
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 controller open at a time.
I think the structure of this code fails to make that logic immediately obvious.
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:
- not caching the inner jQuery object
- 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. In total:
$(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;
});
});
});