4
\$\begingroup\$

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 );
};
\$\endgroup\$
  • \$\begingroup\$ @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? \$\endgroup\$ – jdog Apr 17 '16 at 23:26
  • \$\begingroup\$ 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 Software Engineering would be the place for that.) \$\endgroup\$ – 200_success Apr 17 '16 at 23:36
3
+50
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The styling is all opinion based. His style will not affect how ECMAScript is interpreted; However, you make a very strong point about using this instead of call. \$\endgroup\$ – THE AMAZING Apr 22 '16 at 17:20
  • \$\begingroup\$ True, it does not matter to the interpreter. But while it may be an opinion that inconsistently styled code is harder to read, it's a fact that some developers will complain about it. \$\endgroup\$ – Joshua Dawson Apr 22 '16 at 19:42
  • \$\begingroup\$ Thanks for pointing out the style. I'm mainly writing PHP, so there I have PSR-2 to stick to - I find it great to eliminate discussions about which exact style is best. I try to apply it to JavaScript as well, because I know no style guide for it. Any pointers welcome \$\endgroup\$ – jdog Apr 23 '16 at 7:59
  • \$\begingroup\$ @John Dawson regarding returning a separate object, in my opinion unit testing should not be black box based, but also test private methods. I prefer more test coverage of under-engineered classes than less test cases for well engineered classes. But it may just be because I'm not as experienced with JS as with backend work. \$\endgroup\$ – jdog Apr 23 '16 at 8:00
  • \$\begingroup\$ @jdog Yeah, particulars of which styling convention is not as important, but it's best to write a project using the same style. Surprisingly, I've found JavaScript libraries to be pretty consistent with style at least with variable naming and bracket placement. If you're looking for js style guides, I'd recommend looking into w3schools.com/js/js_conventions.asp or javascript.crockford.com/code.html. There's also a pretty nifty online js interpreter at jsfiddle.net that has a 'tidy' feature that will standardize code you've written. \$\endgroup\$ – Joshua Dawson Apr 24 '16 at 2:01
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ === checks type as well as the value. == checks just the value. \$\endgroup\$ – THE AMAZING Apr 22 '16 at 17:22
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

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

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