Conditionally showing some icons

Question

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;
    }
}

Answer 1

The first one

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 ){
}

What is the purpose of keeping all the values distinct from one another if they're mutually exclusive? Having them separated rather than using a single variable (in the second one's case, selected_val) can open the door to issues involving mistaken intent when more than one condition is true. Though this likely won't be a problem with a menu where a user can only choose one, it is something that could be problematic elsewhere.

Unless there's a specific purpose to the order this appears in, the if-statements that have nothing in them (so student, residence, and minireport) can be removed. Only one of the conditions can hold true, so having them as distinct if-statements is unnecessary.

Also, why cache selectors like iconNext or iconPrevious? They aren't used anywhere else in the function, so unless their blocks of memory are used elsewhere that isn't shown here, it seems unnecessary. Whether or not caching them brings any benefit is really a matter of how this particular function is used in the code not shown here--if they get taken out of the cache after each call, trying to cache things really brings no benefit.

The second one

Unless the console.logs are necessary, they can be removed and several of the if-statements can be combined to avoid repetition.

Answer 2

This changes applies for both files but I changed just the first one.

You should pay attention to comments.

var config = {
    iconSelectors: {
        icons: $('#sdvControlPanel .iconControls'),
        menuItem: $('.menuitem'),
        addIcon: $('#iconAdd'),
        editIcon: $('#iconEdit'),
        saveIcon: $('#iconSave'),
        undoIcon: $('#iconUndo'),
        processIcon: $('#iconProcess'),
        firstIcon: $('#iconFirst'),
        previousIcon: $('#iconPrevious'),
        nextIcon: $('#iconNext'),
        lastIcon: $('#iconLast'),
        controlPanelAlign: $('#controlPanelAlign')
    },
    // Don't know why you got this attributes on the selectedMenu but ...
    setAttributes: function(selectedMenu) {
        return {
            help: selectedMenu.attr('0'),
            student: selectedMenu.attr('1'),
            residence: selectedMenu.attr('2'),
            contact: selectedMenu.attr('3'),
            registration: selectedMenu.attr('9'),
            enroll: selectedMenu.attr('4'),
            minireport: selectedMenu.attr('5')
        }
    }
};

function OLD_iconControls(selectedMenu) {
    // This is NOT used !
    var sectionValue = selectedMenu.attr('val');
    var attributes = config.setAttributes();
    // Hide all the icons in the left control panel
    config.iconSelectors.icons.css('display', 'none');

    // First we display the control panel divs all at once
    config.iconSelectors.controlPanelAlign.css('display', 'block');

    // Show Edit, Save, and Undo icons. These are shown in every section
    config.iconSelectors.saveIcon.css('display', 'inline-block');
    config.iconSelectors.editIcon.css('display', 'inline-block');
    config.iconSelectors.undoIcon.css('display', 'inline-block');

    // Then we display the icons within the alignment div based on the section selected.
    if (attributes.help) {
        config.iconSelectors.controlPanelAlign.css('display', 'none');
    } else if (attributes.student) {
        // Do something!
    } else if (attributes.residence) {
        // Do something!
    } else if (attributes.contact) {
        iconAdd.css('display', 'inline-block');
    } else if (attributes.registration) {
        // Do something!
    } else if (attributes.enroll) {
        config.iconSelectors.processIcon.css('display', 'inline-block');
    } else if (attributes.minireport) {
        // Do something!
    }
}

• The code is more readable
• You have an object (maybe on the top of the file) where other programmer will look for some jQuery object and add a new one if you have more icons in the future
• The function code is more concise and you'll understand what it does just reading it