2
\$\begingroup\$

I'm fairly new to JavaScript, and wanted some critique on some code I've written based upon what I've learned recently. I have a strong background in OO programming (C/C++, C#, Java), but recently I've had to delve into JavaScript for my job. I'm working with a lot of legacy code, and want to improve it as I go.

I've written a 'toolbar' module to manage a dynamic toolbar using JQuery. I was wondering if anyone could give me some constructive criticisms on my approach. I'm finding it difficult to understand a good style, as I'm used to the very rigid constructs of other languages and the expressiveness JavaScript allows has me a little lost. I feel what I have written is clean, however I'm not sure whether I'm trying to force the OO thing too much, and if that's something I should be getting away from.

The following function creates my toolbar, and uses a closure to ensure some variables are kept private. This improves abstraction and keeps the interface clean.

function createToolbar(spec) {
    // ----- PRIVATE VARIABLES -----
    var buttons = [];
    var parentDivId = '#' + spec.divId;
    var divId = '#' + spec.divId + 'Inner';
    var stateTransition = spec.stateTransition;
    var currentState;

    var that = {};

    // ----- PRIVATE FUNCTIONS -----
    var refreshState = function () {
        $.each(buttons, function (index, button) {
            $.each(button.states, function (index, s) {
                if (currentState === s) {
                    if (button.showIf === undefined || button.showIf()) {
                        $(divId).find(button.id).show("slow");
                    } else {
                        $(divId).find(button.id).show("hide");
                    }

                    if (button.disableIf !== undefined && button.disableIf()) {
                        $(divId).find(button.id).button("disable");
                    } else {
                        $(divId).find(button.id).button("enable");
                    }

                    return false;
                }
            });
        });
    };

    var refreshHTML = function () {
        $(divId).empty();
        $.each(buttons, function (index, button) {
            $(divId).append(button.html);
            $(divId).find(button.id).button({
                label: button.label,
                icons: {
                    primary: button.primaryIcon
                }
            })
                .click(function (event) {
                event.preventDefault();
                if(button.func !== undefined) {
                    button.func();
                }
            }).hide();
        });
        $(divId).buttonset();
    };

    // ----- PUBLIC FUNCTIONS -----
    var addButton = function (buttonSpec) {

        // Check for duplicate text to modify ID accordingly
        var existing = 0;
        $.each(buttons, function(index, button) {
            if(button.label === buttonSpec.text) {
                existing += 1;
            }
        });

        buttons.push({
            states: buttonSpec.showStates,
            func: buttonSpec.onclick,
            id: '#' + buttonSpec.text + 'Btn' + existing.toString(),
            showIf: buttonSpec.showCondition,
            disableIf: buttonSpec.disableCondition,
            label: buttonSpec.text,
            html: '<button id="' + buttonSpec.text + 'Btn' + existing.toString() + '"></button>',
            primaryIcon: buttonSpec.icon
        });

        console.log(buttonSpec.icon);

        refreshHTML();
    };

    var setState = function (state) {
        currentState = state;

        if (stateTransition) {
            stateTransition();
        }

        $.each(buttons, function (index, button) {
            $(divId).find(button.id).hide("slow");
            $.each(button.states, function (index, s) {
                if (state === s) {
                    if (button.showIf === undefined || button.showIf()) {
                        $(divId).find(button.id).show("slow");
                    }
                    return false;
                }
            });
        });

        refreshState();
    };

    var clear = function () {
        buttons = [];
        refreshHTML();
    };

    var hide = function () {
        $(divId).hide();
    };

    var show = function () {
        $(divId).show();
    };

    var getCurrentState = function() {
        return currentState;
    };

    // ----- CONSTRUCTION -----

    // Create inner div for toolbar
    $('#' + spec.divId).append('<div id=' + spec.divId + 'Inner></div>');

    // Add any buttons that are part of the spec
    if (spec.buttons) {
        $.each(spec.buttons, function (index, button) {
            addButton(button);
        });
    }

    // Assign publically accessible functions
    that.hide = hide;
    that.show = show;
    that.addButton = addButton;
    that.setState = setState;
    that.clear = clear;
    that.refresh = refreshState;
    that.getCurrentState = getCurrentState;

    return that;
};

And here's a simple example of a toolbar on a page:

<html>
<head>
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.4/jquery.min.js"></script>
    <script src="https://ajax.aspnetcdn.com/ajax/jquery.ui/1.8.17/jquery-ui.min.js"></script>
    <link rel="stylesheet" type="text/css" href="https://ajax.googleapis.com/ajax/libs/jqueryui/1.8.4/themes/smoothness/jquery-ui.css" />
    <script src="toolbar.js"></script>
</head>
<body>
    <div id="toolbar"></div>

    <script>

        $(function () {

            var tools = createToolbar({
                divId: "toolbar", 
                stateTransition: function () {
                },
                buttons: [
                    {
                        showStates: ["main"],
                        text: "Draw",
                        onclick: function () {
                            console.log("Test");
                        },
                        icon: "ui-icon-pencil"
                    }, 
                    {
                       showStates: ["main"],
                       text: "Edit",
                       onclick: function() {
                           tools.setState("edit");
                        },
                        icon: "ui-icon-wrench"
                    },
                    {
                        showStates: ["main"],
                        text: "Options",
                        onclick: function() {
                            tools.setState("options");
                        },
                        icon: "ui-icon-gear"
                    },
                    {
                        showStates: ["edit"],
                        text: "Save",
                        onclick: function() {
                            console.log("Save");
                        },
                        icon: "ui-icon-document"
                    }, 
                    {
                        showStates: ["edit"],
                        text: "Update",
                        onclick: function() {
                            console.log("Update");
                        },
                        icon: "ui-icon-refresh"
                    }, 
                    {
                        showStates: ["edit"],
                        text: "Back",
                        onclick: function() {
                            tools.setState("main");
                        },
                        icon: "ui-icon-carat-1-w"
                    },
                    {
                        showStates: ["options"],
                        text: "Sound",
                        onclick: function() {
                            console.log("Sound");
                        },
                        icon: "ui-icon-volume-on"
                    }, 
                    {
                        showStates: ["options"],
                        text: "Graphic",
                        onclick: function() {
                            console.log("Graphic");
                        },
                        icon: "ui-icon-image"
                    },
                    {
                        showStates: ["options"],
                        text: "Back",
                        onclick: function() {
                            tools.setState("main");

                        },
                        icon: "ui-icon-carat-1-w"
                    }
                ]
            });

            tools.setState("main");

            console.log(document.documentElement.innerHTML);
        });
    </script>
</body>
</html>

I was less happy with the construction of the toolbar itself, as it looked a bit repetitive. I'm not really looking for criticisms on my choice of libraries, purely about the style of my JavaScript. What do you think could be improved here?

There's a live sample of this code here.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Good job on your first question. As a recommendation, use a stack snippet in your code instead of an external link. You can add a stack snippet to your code in the editor window. \$\endgroup\$ – SirPython Nov 6 '15 at 21:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.