Status display module

Question

I've developed a template to modularize JavaScript code I write for client work. The example below is a simple class with 1 attribute and 3 functions.

Rationale:

• Standardized constructor to add or modify settings/ options
• Functions as variables used later to return only "public" functions, all else are private
• Using local "call" variable to easily replace functions in unit testing

Are there any improvements you can recommend or general best practices to look for in terms of improvement?

var dataStatus = function( constructorOptions )
{
    "use strict";

    var options = {
        displayId: 'dataStatus',
        statii: {
            'settingUp': {
                tooltip: 'Setting Up...',
                color: '808080'
            },
            'loading': {
                tooltip: 'Data Loading...',
                color: '99ee90'
            },
            'loaded': {
                tooltip: 'Data Loaded',
                color: '006400'
            },
            'changed': {
                tooltip: 'Data Changed',
                color: '8b0000'
            },
            'saving': {
                tooltip: 'Saving...',
                color: 'ffd700'
            }
        },
        defaultStatus: 'settingUp'
    };

    var local = {
        status: false
    };

    var init = function( optionsToSet )
    {
        jQuery.extend( options, optionsToSet);

        call.setStatus( options.defaultStatus );
        return this;
    };


    var inject = function (functionToReplace, injectedFunction)
    {
        call[functionToReplace] = injectedFunction;
        return injectedFunction;
    };



    var getStatus = function()
    {
        return local.status;
    };

    var setStatus = function( status, signal )
    {
        local.status = status;
        if (typeof signal == 'undefined' || signal != false){
            call.signal( call.getStatus() );
        }
    };

    var signal = function ( status ){
        if (status in options.statii){
            jQuery( '#' + options.displayId )
                .html( '<i class="fa fa-circle" title="' + options.statii[status].tooltip + '"></i>')
                .css( 'color', '#' + options.statii[status].color )
                .css( 'background-color', '#ffffff');
        }
    };


    var call = {
        init: init,
        options: options,
        inject: inject,
        getStatus: getStatus,
        setStatus: setStatus,
        signal: signal
    };

    if(typeof myPublicTestNamespace == "undefined"){//http://stackoverflow.com/a/9172377/123594
        return {
            init: init,
            getStatus: getStatus,
            setStatus: setStatus,
            signal: signal
        };
    }else{
        return call;
    };

    init( constructorOptions );
};

Answer 1

Your code has somewhat inconsistent styling which can inhibit readability and prevent the code from looking crisp and professional. For example, some of your function definitions have braces on the next line others don't. Some of your function definitions and calls put spaces between parenthesis var signal = function ( status ){, and some don't var inject = function (functionToReplace, injectedFunction).

Also, I don't believe that the last line init( constructorOptions ); is executing because you return from the constructor in both the if and else blocks right above it.

That said, using closures to keep private functions private is a great idea.

As for the modularity, while returning a separate object for for testing is a truly fascinating idea, my gut instinct goes against it. I find it somewhat troublesome because it violates cohesion with your constructor really doing two things: creating a dataStatus object and creating a dataStatus test object. This introduces room for bugs if future functions aren't properly assigned to both the real and test object. To catch these bugs, you'd want to write a test case that ensures the test object shares all the same functions as the real object. (i.e. call could have extra functions but it must also have each one that a normal dataStatus would)

I'd recommend eliminating call and if needed to use this instead. Although it might be best to just call the functions directly.

var init = function () {
    jQuery.extend(options, optionsToSet);

    setStatus(options.defaultStatus); // just call setStatus directly
    return this;
}

You can still do your testing by dissecting the methods you want from the real object.

var realObj = new dataStatus (options);
var testObj = {
    setStatus : realObj.setStatus,
    getStatus : function () {},
    signal : function () {}
};

Private functions should be tested through the testing of public functions, but in your case the only private function is for changing functions when testing.

---

UPDATE:

If you're set on testing your private functions--and I can fully understand how it could be worth it--you ought to to take care of a few things.

First, make sure the test object and the real object have the same functions. You could write a test case that checks that their functions are the same, but it may be best to just write the private functions directly as properties of the call object:

var call = {};

call.init = function () {
    // ...
}
call.inject = function () {
    // ...
}
call.getStatus = function () {
    // ...
}

// ...

if (/* determine if your unit testing each function */) {
    return call;
}
else {
    return {
        init: call.init,
        getStatus: call.getStatus,
        setStatus: call.setStatus,
        signal: call.signal
    };
}

Secondly, you will want to also test the real object. You could have millions of test cases for your test object, but since your real object isn't your test object, the real object might still break.

Answer 2

Spelling

status, like sinus, has an -es plural, not an -i - hence, the plural you want for your statii variable is statuses.

Spacing

Be consistent in the whitespace you use. In some methods, you're using the pattern on padding whitespace on both sides:

var methodName = function( argument1, argument2 )

In at least one, you've missed the padding and have this instead:

var methodName = function(argument1, argument2)

I also recommend padding space around else: }else{ should be } else { for readability.

It's a minor point, but consistency in your minor points makes consistency in the major points more natural.

Naming

Generally good variable naming, but beware that local is a reserved keyword in ES6.

Equality

JavaScript has two equality checks: == and ===. I'll spare you the extensive and weird details of each of them, but suffice to say that double-equals is rough equality (i.e. 0 == false) and triple-equals is exact equality.

You should use === when comparing with strings, booleans, undefined or null.

Attribution

It's good that you're attributing where your code came from with that comment hyperlink to SO. However, you also need to include and hyperlink the author's name.

Answer 3

strict mode is only supported in:

• Internet Explorer from version 10.
• Firefox from version 4.
• Chrome from version 13.
• Safari from version 5.1.
• Opera from version 12.

You class seems dependent on JQUERY, you should make a check to ensure JQUERY has been loaded, if not load it dynamically.

if(typeof myPublicTestNamespace == "undefined"){//http://stackoverflow.com/a/9172377/123594
        return {
            init: init,
            getStatus: getStatus,
            setStatus: setStatus,
            signal: signal
        };
    }else{
        return call;
    };

does not need an else return. just return like:

if(typeof myPublicTestNamespace == "undefined"){//http://stackoverflow.com/a/9172377/123594
        return {
            init: init,
            getStatus: getStatus,
            setStatus: setStatus,
            signal: signal
        };
    }
    return call;

i am not entirely sure init( constructorOptions ); will ever be called due to your returns above it. If so, i would consider it bad practice.

---

from @Josh Dawson

I'd recommend eliminating call and using this instead. That way you can do your testing by disecting the methods you want from the real object.

var realObj = new dataStatus (options); var testObj = {
     setStatus : realObj.setStatus,
     getStatus : function () {},
     signal : function () {}
}; 

Private functions should be tested through the testing of public functions, but your only private function is for changing functions when testing.