1
\$\begingroup\$

I'm initializing a class in JavaScript with a bunch of complex property checks. Codacy is showing that this is bad design and recommends reducing cyclomatic complexity.

I'm at a loss for how to improve/streamline this, though.

var ok = require('objectkit')

var UsergridClient = function(opts) {
    var self = this

    // required properties
    if (arguments.length === 2) {
        self.orgId = arguments[0]
        self.appId = arguments[1]
    } else {
        self.orgId = ok(opts).getIfExists('orgId') || ok(config).getIfExists('usergrid.orgId')
        self.appId = ok(opts).getIfExists('appId') || ok(config).getIfExists('usergrid.appId')
    }

    // optional initializer properites
    self.baseUrl = ok(opts).getIfExists('baseUrl') || ok(config).getIfExists('usergrid.baseUrl') || 'https://api.usergrid.com' // default: https://api.usergrid.com
    self.clientId = ok(opts).getIfExists('clientId') || ok(config).getIfExists('usergrid.clientId') // default: undefined
    self.clientSecret = ok(opts).getIfExists('clientSecret') || ok(config).getIfExists('usergrid.clientSecret') // default: undefined
    self.tokenTtl = ok(opts).getIfExists('tokenTtl') || ok(config).getIfExists('usergrid.tokenTtl') || 3600 // time in seconds, default:  3600
    self.authFallback = ok(opts).getIfExists('authFallback') || ok(config).getIfExists('usergrid.authFallback') || 'none' // ('none'|'app') default: 'none'

    // optional post-init properties
    self.paginationPreloadPages = 0 // number of pages to preload
    self.paginationCacheTimeout = 300 * 1000 // default: 300 seconds
    self.paginationCursors = [] // array of pagination cursors

    if (self.orgId !== undefined && self.appId !== undefined) {
        return self
    } else {
        throw new Error('"orgId" and "appId" parameters are required when instantiating UsergridClient')
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question \$\endgroup\$ – Simon Forsberg Nov 28 '15 at 23:50
  • \$\begingroup\$ @SimonForsberg not really much to say - there are some properties that are being set here by either list of arguments, opts object, or a config file. At this point I don't even understand why it's being flagged for cyclomatically complex, unless it doesn't like the multiple || inline conditionals. \$\endgroup\$ – brandonscript Nov 28 '15 at 23:52
4
\$\begingroup\$

The variable self here is redundant.

The parameter name "opts" is significantly less commonly used than "options".

It is not clear immediately what can be supplied in opts. Inferring how the function can be called from the logic of the code is error-prone.

The code implies that the various optional properties might be in one of two locations, or missing. I would ensure that callers position these values in one location for the call, making testing simpler.

The objectkit library is so verbose as to obfuscate the code quite effectively.

Simply ensure the property exists and negate the need for the getIfExists at all.

Setting default values where none is supplied can be performed easily with Object.assign polyfills and similar libraries.

The following appear to always be the same, if so, do they need to be own-properties?

self.paginationPreloadPages = 0 // number of pages to preload
self.paginationCacheTimeout = 300 * 1000 // default: 300 seconds

The following check should be at the start of the function to short-circuit all the work:

if (self.orgId !== undefined && self.appId !== undefined) {

none and app are magic strings that should be pulled into a single enumeration object.

return self is redundant if the intention is to use this as a constructor function.

So applying these changes:

var AuthFallback = {
    APP: 'APP',
    NONE: 'NONE',
};

var defaultOptions = {
    orgId: null,
    appId: null,
    baseUrl: 'https://api.usergrid.com',
    clientId: null,
    clientSecret: null,
    tokenTtl: 3600,
    authFallback: AuthFallback.NONE,
};

function UsergridClient(options) {
    if (!options || !(options.orgId && options.appId)) {
        throw new Error('options.orgId and options.appId required.');
    }

    Object.assign(this, defaultOptions, options); // or polyfill
}

UsergridClient.prototype.paginationPreload = 0; // pages.
UsergridClient.prototype.paginationCacheLifetime = 300 * 1000; // ms.
UsergridClient.prototype.paginationCursors = []; // Not sure about this one...
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Some good tips here thanks! Object.assign will work well. I'll still have to do a check for arguments.length === 2 because it can be initialized using an object or list arguments. I agree self is redundant, but design-consistency makes me want to keep it. And this also doesn't account for loading values from config if they're undefined? \$\endgroup\$ – brandonscript Nov 29 '15 at 16:02
  • \$\begingroup\$ Too bad Object.assign is ES6. I'll have to use a library to mimic using (probably) _.extend. \$\endgroup\$ – brandonscript Nov 29 '15 at 16:44
  • \$\begingroup\$ I added my final solution to my answer below. Thanks for the suggestions Ben! \$\endgroup\$ – brandonscript Nov 29 '15 at 17:24
1
\$\begingroup\$

With help from Ben, I ended up with this, using 's defaults (which supports ES5):

var AuthFallback = {
    APP: 'APP',
    NONE: 'NONE',
}

var defaultOptions = {
    baseUrl: 'https://api.usergrid.com',
    authFallback: AuthFallback.NONE,
    paginationPreloadPages: 0, // number of pages to preload
    paginationCacheTimeout: 300 * 1000, // default: 300 seconds
    paginationCursors: [] // array of pagination cursors
}

var UsergridClient = function(options) {
    var self = this

    if (arguments.length === 2) {
        self.orgId = arguments[0]
        self.appId = arguments[1]
    } 

    _.defaults(self, options, ok(config).getIfExists('usergrid'), defaultOptions)

    if (!self.orgId || !self.appId) {
        throw new Error('"orgId" and "appId" parameters are required when instantiating UsergridClient')
    }
}

Much cleaner!


(old)

Turns out Codacy definitely didn't like the nested || statements. Adding in a helper function to handle the properties solved the problem:

var UsergridClient = function(opts) {
    var self = this

    // required properties
    if (arguments.length === 2) {
        self.orgId = arguments[0]
        self.appId = arguments[1]
    } else {
        self.orgId = getProperty(opts, 'orgId')
        self.appId = getProperty(opts, 'appId')
    }

    // optional initializer properites
    self.baseUrl = getProperty(opts, 'baseUrl', 'https://api.usergrid.com') // default: https://api.usergrid.com
    self.clientId = getProperty(opts, 'clientId')
    self.clientSecret = getProperty(opts, 'clientSecret')
    self.tokenTtl = getProperty(opts, 'tokenTtl') // token ttl in seconds, server default: 3600
    self.authFallback = getProperty(opts, 'authFallback', 'none') // ('none'|'app') default: 'none'

    // optional post-init properties
    self.paginationPreloadPages = 0 // number of pages to preload
    self.paginationCacheTimeout = 300 * 1000 // default: 300 seconds
    self.paginationCursors = [] // array of pagination cursors

    if (self.orgId !== undefined || self.appId !== undefined) {
        return self
    } else {
        throw new Error('"orgId" and "appId" parameters are required when instantiating UsergridClient')
    }
}

function getProperty(opts, name, defaultValue) {
    return ok(opts).getIfExists(name) || ok(config).getIfExists(util.format('usergrid.%s', name)) || defaultValue
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You can use getProperty in the else part of first if as well, just omit the third argument. Also why use utils.format when simple 'usergrid.'+name will suffice? And why using self if simply using this will not break anything? (Might be a NodeJS custom that I'm not familiar with, but it's less readable for me). And you're polluting the global namespace with getProperty function. You also require only one of orgId and appId. \$\endgroup\$ – Torinthiel Nov 29 '15 at 9:50
  • \$\begingroup\$ I'd already caught using getProperty in the first else like you suggested - didn't update here though. util.format is a design preference for consistency's sake; it also eliminates having to check type. Global namespace isn't polluted because Node's require doesn't export the function (it's effectively private). And self is mostly a design choice too; there are arguments made that using self can avoid prototype clobbering. \$\endgroup\$ – brandonscript Nov 29 '15 at 15:56

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.