current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

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
up vote 1 down vote favorite

I've been tasked with finding property differences in massive JSON structures and return them in a particular fashion ([key_1.key_2, key_1, etc]).

Here's my code, which is focused on readability so some redundant decorators are used (they will be removed). I am looking for any performance/logical problems with this approach.

It also has to be in JavaScript, Node Stack (Isomorphic).

/**
 * --Decorator for prettiness-- 
 * Checks array if key exists
 * @param needle
 * @param haystack
 * @returns {boolean}
 */
export function inArray (needle, haystack) {
  return haystack.indexOf(needle) > -1
}

/**
 * --Decorator for prettiness--  
 * Merge two arrays together
 * @param array_1 {Array}
 * @param array_2 {Array}
 * @returns {Array}
 */
export function arrayMerge(array_1, array_2) {
  return array_1.concat(array_2)
}

/**
 * Returns all differences of keys
 * Note: nested keys are returned like this [key_1.key_2, key_1.key_2.key_3]
 * @param object_compare
 * @param object_against
 * @param ignoreKeys
 * @param depth
 * @returns {Array}
 */
export function diffKeysRecursive(object_compare, object_against, ignoreKeys = [], depth = '') {
  function _isObject(mixed) {
    return typeof mixed === 'object'
  }

  function _propExists(prop, object) {
    return typeof object === 'object' && typeof object[prop] !== 'undefined'
  }

  function _depthPath(prop, depth = '') {
    return depth = depth ? `${depth}.${prop}` : prop
  }

  if (_isObject(object_against)) {
    var diffs = []
    // Iterate through the against object to see differences in the compare
    $.each(object_against, (prop, value_against) => {
      // Checks if prop should be ignored
      if(!inArray(prop, ignoreKeys)) {
        if (!_propExists(prop, object_compare)) {
          diffs.push(_depthPath(prop, depth))
        }

        // Recurse if value_against is an object
        if (_isObject(value_against)) {
          var object_compare_safe = _propExists(prop, object_compare) ? object_compare[prop] : null
          diffs = arrayMerge(diffs, diffKeysRecursive(object_compare_safe, value_against, _depthPath(prop, depth)))
        }
      }
    })
    return diffs
  } else {
    throw 'Invalid Type Supplied'
  }
}
share|improve this question
up vote 1 down vote accepted 
$.each(object_against, (prop, value_against) => {

I assume this is jQuery? To make the code more portable, I suggest dropping the dependency to jQuery and simply use a combination of Object.keys and forEach.

Object.keys(object_against).forEach(prop => {
  var value_against = object_against[prop];

  // The rest of the code here.
});

You also need to be consistent with your naming scheme. JavaScript uses camelCase syntax. You're already doing it for function names, but you need to do the same for variable names.

export function arrayMerge(array_1, array_2) {
  return array_1.concat(array_2)
}

This is redundant since it's already built-in. If you want a function-like approach, you can simply use Array.prototype.concat directly.

var mergedArrays = Array.prototype.concat.call(array1, array2);

export function diffKeysRecursive(object_compare, object_against, ignoreKeys = [], depth = '') {
  function _isObject(mixed) {
    return typeof mixed === 'object'
  }

  function _propExists(prop, object) {
    return typeof object === 'object' && typeof object[prop] !== 'undefined'
  }

  function _depthPath(prop, depth = '') {
    return depth = depth ? `${depth}.${prop}` : prop
  }

Suggesting you pull out the functions out of diffKeysRecursive and into the module as non-exported functions. The problem here is that every call to diffKeysRecursive, it creates these functions. If encapsulation is what you need, the module itself should suffice.

  function _isObject(mixed) {
    return typeof mixed === 'object'
  }

typeof null is also object. Refine this by adding a guard for truthiness.

  function _isObject(mixed) {
    return mixed && typeof mixed === 'object'
  }

function _propExists(prop, object) {
  return typeof object === 'object' && typeof object[prop] !== 'undefined'
}

This is misleading. A prop can exist with a value of undefined. Suggesting you rename the function to something like isPropUndefined.

Additionally, object[prop] may cause confusion. For example, do ({}).valueOf !== undefined, it will return true. That's because a method named valueOf exists on the prototype. To avoid this, use obj.hasOwnProperty(prop) instead, to check on the current instance and avoid resolving up the prototype.

share|improve this answer
    
Dang ty, this is exactly what I was looking for! So your point about pulling the functions outside of the function. – moaxaca May 28 at 4:58
    
Is there a performance increase with moving them outside of the scope and hoisting them in? I can kinda see the benefit, assuming it's redeclaring the function on every invocation. I am no expert in Javascripts closure scoping but I might be giving it too much merit. – moaxaca May 28 at 5:03

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.