HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

UI for adding roles and modules to a user list

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
modulesrolesuseraddingforandlist

Problem

I have a small UI that involves adding roles and modules to a user database array. The interface works as is, no problem at all, but it seems to have a bit of 'duplicate' code that I was hoping I could get some advice on ways to refactor, if at all possible.

JS and Knockout

```
function Db() {
var self = this;

self.userRoles = ['Executive', 'Admin', 'User'];
self.userModules = ['Explore', 'Dashboards', 'Alerts'];
self.userDatabases = ko.observableArray();
self.allRolesSelected = ko.observableArray();
self.allModulesSelected = ko.observableArray();

self.databases = _.range(5).map(function (i) {
return {
name: 'DB ' + (i + 1),
chosenRoles: ko.observableArray(),
chosenModules: ko.observableArray()
};
});

self.toggleAllRoles = function (r, e) {
var checked = e.target.checked;

if (checked) {
self.setAllRoles(r);
} else {
self.clearAllRoles(r);
}
};
self.toggleAllModules = function (r, e) {
var checked = e.target.checked;

if (checked) {
self.setAllModules(r);
} else {
self.clearAllModules(r);
}
};

self.setAllRoles = function (r) {
_.each(self.userDatabases(), function (d) {
if (d.chosenRoles.indexOf(r) === -1) {
d.chosenRoles.push(r);
}
});
};
self.setAllModules = function (r) {
_.each(self.userDatabases(), function (d) {
if (d.chosenModules.indexOf(r) === -1) {
d.chosenModules.push(r);
}
});
};

self.clearAllRoles = function (r) {
_.each(self.userDatabases(), function (d) {
d.chosenRoles.remove(r);
});
};
self.clearAllModules = function (r) {
_.each(self.userDatabases(), function (d) {
d.chosenModules.remove(r);
});
};

ko.computed(function () {

Solution

Here are a few items that I could think of. I know nothing about Knockout so if these changes do not work in that framework (I don't know why they wouldn't).

The first thing I would do is wrap the code with an IIFE to create a private scope. Again, this might not be necessary with Knockout.

(function(){
  //your code here
})();


Next, you can DRY out your code quite a bit. For example, this:

self.toggleAllRoles = function (r, e) {
    var checked = e.target.checked;

    if (checked) {
        self.setAllRoles(r);
    } else {
        self.clearAllRoles(r);
    }
};
self.toggleAllModules = function (r, e) {
    var checked = e.target.checked;

    if (checked) {
        self.setAllModules(r);
    } else {
        self.clearAllModules(r);
    }
};


could be re-written as this just by passing one additional variable:

self.toggleAll = function ( r, e, roleOrModule ) { 
  // roleOrModule = 'role' or 'module' ( or anything not 'role' )
  self.setOrClear( r,  roleOrModule, e.target.checked );
};


You can do something similar with the setAllRoles, setAllModules, clearAllRoles and clearAllModules methods:

self.setOrClear = function( r, roleOrModule, pushOrRemove ) {
  var method = ( roleOrModule === 'role' ) ? 'chosenRoles' : 'chosenModule';
  var pushRemove = ( pushOrRemove ) ? 'push' : 'remove';
  _.each( self.userDatabases(), function (d) {
      if ( d[ method ].indexOf( r ) === -1) {
          d[ method ][ pushRemove ](r);
      }
  });
};


Lastly, I would again do this in the last section:

ko.computed(function () {
  var eachMethod = ['userRoles','userModules'];
  var chosenMethod = ['chosenRoles','chosenModules'];
  var allMethod = ['allRolesSelected', 'allModulesSelected'];

  for (var i=0, l = eachMethod.length; i  0 && _.every(self.userDatabases(), function (d) {
            return d[ chosenMethod[i] ].indexOf(r) > -1;
        });

        if (all) {
            if (self[ allMethod[i] ].indexOf(r) === -1) {
                self[ allMethod[i] ].push(r);
            }
        } else {
            self[ allMethod[i] ].remove(r);
        }
    });
  }
});


Unfortunately, I have no way to test this code.
I hope that helps!

Code Snippets

(function(){
  //your code here
})();
self.toggleAllRoles = function (r, e) {
    var checked = e.target.checked;

    if (checked) {
        self.setAllRoles(r);
    } else {
        self.clearAllRoles(r);
    }
};
self.toggleAllModules = function (r, e) {
    var checked = e.target.checked;

    if (checked) {
        self.setAllModules(r);
    } else {
        self.clearAllModules(r);
    }
};
self.toggleAll = function ( r, e, roleOrModule ) { 
  // roleOrModule = 'role' or 'module' ( or anything not 'role' )
  self.setOrClear( r,  roleOrModule, e.target.checked );
};
self.setOrClear = function( r, roleOrModule, pushOrRemove ) {
  var method = ( roleOrModule === 'role' ) ? 'chosenRoles' : 'chosenModule';
  var pushRemove = ( pushOrRemove ) ? 'push' : 'remove';
  _.each( self.userDatabases(), function (d) {
      if ( d[ method ].indexOf( r ) === -1) {
          d[ method ][ pushRemove ](r);
      }
  });
};
ko.computed(function () {
  var eachMethod = ['userRoles','userModules'];
  var chosenMethod = ['chosenRoles','chosenModules'];
  var allMethod = ['allRolesSelected', 'allModulesSelected'];

  for (var i=0, l = eachMethod.length; i < l; i++) {
    _.each(self[ eachMethod[i] ], function (r) {
        var all = self.userDatabases().length > 0 && _.every(self.userDatabases(), function (d) {
            return d[ chosenMethod[i] ].indexOf(r) > -1;
        });

        if (all) {
            if (self[ allMethod[i] ].indexOf(r) === -1) {
                self[ allMethod[i] ].push(r);
            }
        } else {
            self[ allMethod[i] ].remove(r);
        }
    });
  }
});

Context

StackExchange Code Review Q#77708, answer score: 2

Revisions (0)

No revisions yet.