Skip to content

Instantly share code, notes, and snippets.

@martin-wintz
Last active August 29, 2015 14:19
Show Gist options
  • Save martin-wintz/0757d2f8475b646eda53 to your computer and use it in GitHub Desktop.
Save martin-wintz/0757d2f8475b646eda53 to your computer and use it in GitHub Desktop.

This actually does very little as far as refactorings go. Lines 276, 279, 283 are essentially the only things that are deduplicated. The rest is still duplicated, just within the function instead of in two separate functions.

The result is that there isn't really much gain. Theoretically, if you wanted to implemented a third field that works similarely to settings/preferences, you couldn't reuse this method. You'd have to walk through it, try to understand what it does, and add the appropriate if statements and so forth. No abstraction was really created.

I personally couldn't understand what the differences were between the two objects based on this, so I refactored it as I went through:

// This object is a good reflection of the key differences between these two objects
// We can then effectively ask ourselves why these differences exist:
var perOrganizationUserDataMapping = {
  preferences: {
    propertyName: 'preferencesPerOrganization', // What does preferences vs settings mean?
    subPropertyName: 'taskManager', // why do we need this extra nesting? Why does it need to be named differently?
    validate: function (data) { return _.has(data, 'taskManager'); }, // Why does this have validation and not the other
    // Why do we pass in the data that actually matters as a property for preferences
    // but the object itself for settings?
    passedInAs: 'property'
  },
  settings: {
    propertyName: 'settingsPerOrganization',
    subPropertyName: 'options',
    validate: function () { return true; },
    passedInAs: 'itself'
  }
};


// The real question that still remains unanswered to me is, why do we need both of these?
// They serve the exact same purpose, to define some sort of structured data for a user
// per organization.

function _setPerOrganizationUserData(user, data, mode) { // data and mode not great names, don't know what they contain
  var organizationId = apiUtils.deflate(user.organization, true);

  // silent failure
  if (!data || !_.isObject(data)) { return; }

  // this was a silent failure, I added an exception
  if (organizationId.toString() !== data.organization.toString()) {
    throw new Error('Trying to change settings for something other than current organization');
  }

  // Later on we add organization back to data, so why did we delete it?
  // why is there and orgId and an organization?
  delete data.organization;
  delete data.orgId;

  var propertyName = perOrganizationUserDataMapping[mode].propertyName;

  // I would add a comment here explain this was implemented as a list so we could use sub-schemas
  var userDataIndexForCurrentOrganization = _.findIndex(user[propertyName], { 'organization': organizationId });

  if (userDataIndexForCurrentOrganization !== -1) {
    _updateCurrentPerOrganizationData(user, data, userDataIndexForCurrentOrganization, mode);
  } else {
    _createPerOrganizationData(user, organizationId, data, mode);
  }
}

function _updateCurrentPerOrganizationData(user, data, index, mode) {
  var propertyName = perOrganizationUserDataMapping[mode].propertyName;
  var subPropertyName = perOrganizationUserDataMapping[mode].subPropertyName;

  delete data.organization;
  delete data.orgId;

  user[propertyName][index][subPropertyName] = data;
}

function _createPerOrganizationData(user, organizationId, data, mode) {
  var propertyName = perOrganizationUserDataMapping[mode].propertyName;
  var subPropertyName = perOrganizationUserDataMapping[mode].subPropertyName;

  if (!perOrganizationUserDataMapping[mode].validate(data)) { return; } // silent failure
  
  var mappedData = { organization: organizationId };
  var passedInAs = perOrganizationUserDataMapping[mode].passedInAs;
  mappedData[subPropertyName] = (passedInAs === 'property') ? data[subPropertyName] : data;
  user[propertyName].push(mappedData);
}

I made the comments inline. The real crux of the issue here, as I mentioned in a comment, is that these should not be two separate database fields. The differences are so minor between them , and in all cases no really explainable, that there is no real clean way to abstract them out (which is why my code is equally confusing as yours, the only benefit being that now, theoretically, implementing a new property of this type would take changing 4 lines of code in a config object rather than having to step through the whole method).

Let's talk over skype if you have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment