Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

Rationale:

  • Standardised 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 directions 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 );
};
share|improve this question

This question has an open bounty worth +50 reputation from jdog ending in 6 days.

This question has not received enough attention.

    
@200_success thanks for the edit. I hope your edit improves views, the answers I'm after are about the modularization of my code, not about what it does. Do you still think your edit will help? – jdog Apr 17 at 23:26
    
Every Code Review question is principally about your posted code in general, even if you have specific concerns about it. The title needs to reflect the purpose of your code, by site policy. (If you want to discuss general guiding principles instead of your code, then Programmers would be the place for that.) – 200_success Apr 17 at 23:36

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 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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.