A coworker and I want to get feedback on our respective implementations of a JavaScript function. Both seem fast, but we're wondering which is better stylistically.
Both functions handle what icons appear in a control panel when navigating between sections inside of an administrator dashboard.
The first function
function OLD_iconControls(selectedMenu){
// Cache the selectors
var icons = $('#sdvControlPanel .iconControls');
var menuItem = $('.menuitem');
var iconAdd = $('#iconAdd');
var iconEdit = $('#iconEdit');
var iconSave = $('#iconSave');
var iconUndo = $('#iconUndo');
var iconProcess = $('#iconProcess');
var iconFirst = $('#iconFirst');
var iconPrevious = $('#iconPrevious');
var iconNext = $('#iconNext');
var iconLast = $('#iconLast');
var controlPanel = $('#controlPanelAlign');
var sectionValue = selectedMenu.attr('val');
// Store the selectors for each menu item in variables
var help = selectedMenu.attr('0');
var student = selectedMenu.attr('1');
var residence = selectedMenu.attr('2');
var contact = selectedMenu.attr('3');
var registration = selectedMenu.attr('9');
var enroll = selectedMenu.attr('4');
var minireport = selectedMenu.attr('5');
// Hide all the icons in the left control panel
icons.css('display', 'none');
// First we display the control panel divs all at once
controlPanel.css('display', 'block');
// Show Edit, Save, and Undo icons. These are shown in every section
iconSave.css('display', 'inline-block');
iconEdit.css('display', 'inline-block');
iconUndo.css('display', 'inline-block');
// Then we display the icons within the alignment div based on the section selected.
if ( help ){
controlPanel.css('display', 'none');
}
else if ( student ){
}
else if ( residence ){
}
else if ( contact ){
iconAdd.css('display', 'inline-block');
}
else if ( registration ){
}
else if ( enroll ){
iconProcess.css('display', 'inline-block');
}
else if ( minireport ){
}
}
The second function
function iconControls(menuitemclicked){
// Used to show the proper control icons in the floating menu div(s).
// Store the value of the selected control.
var selected_val = $(menuitemclicked).attr('val');
// Using these variable names as a sort of named-constant. - Readability!
var help = '0';
var student = '1';
var residence = '2';
var contact = '3';
var registration = '9';
var enroll = '4';
var minireport = '5';
// Cache the selector as I did in he OLD function. Better coding. Handles the control div positioning as you switch between sections.
var bottomReset = $('#controlPanelAlign .controlPanels').css({bottom: '0px', position: 'absolute'});
// Hide all the icons in the left control panel
$('#sdvControlPanel .iconControls').css('display', 'none');
// First we display the control panel divs all at once
$('#controlPanelAlign').css('display', 'block');
// Show Edit, Save, and Undo icons. These are shown in every section
$('#iconSave').css('display', 'inline-block');
$('#iconEdit').css('display', 'inline-block');
$('#iconUndo').css('display', 'inline-block');
$('#controlPanelAlign').css('position', 'initial');
// Then we display the icons within the alignment div based on the section selected.
if ( selected_val == help ) {
console.log("help");
$('#controlPanelAlign').css('display', 'none');
}
else if ( selected_val == student ) {
console.log("student");
bottomReset;
}
else if ( selected_val == residence ) {
console.log("residence");
bottomReset;
}
else if ( selected_val == contact ) {
console.log("contact");
bottomReset;
$('#iconAdd').css('display', 'inline-block');
}
else if ( selected_val == registration ){
console.log("registration");
bottomReset;
}
else if ( selected_val == enroll ) {
console.log("enroll");
$('#iconProcess').css('display', 'inline-block');
$('#controlPanelAlign .controlPanels').css({bottom: 'auto', position: 'absolute'});
}
else if ( selected_val == minireport ) {
console.log("minireport");
bottomReset;
}
}
else if
to a switch statement and move the assignment of all the constants outside the function so it isn't executed everytime you run. Also, what isbottomReset;
supposed to be doing? Is that supposed to be a function call? \$\endgroup\$ – jfriend00 Jul 22 '15 at 17:01$(el).css('display','none');
is pretty much the same as as using$(el).hide();
. Better yet, create a.hidden
class like.hidden { display:none;}
and just add it to the elements you want to hide. Remove the class when you want to display it. \$\endgroup\$ – Gary Storey Jul 28 '15 at 16:58