Code Review for Symmetric Difference

Code Review for Symmetric Difference
0

#1

Warning Probably don’t want to read if you haven’t started the Algorithms yet

Here is my Code for the Symmetric Difference exercise. It passes all the tests but I feel like it’s overly complicated could use a critical code review to point out what I should have seen.

Object.defineProperty(Array.prototype, 'removeItem', {
  configurable: true,
  writable: true,
  value: function removeItem (index) {
    'use strict'

    var length = this.length;
    if  (index != length - 1) {
      this[index] = this[length-1];
    }
    this.pop();
  
    return this
  }
});

function filter_diff(outputArray, inputArray){
  return inputArray.filter((val)=> {
    //remove any duplicates in inputArray
    while(inputArray.indexOf(val) != inputArray.lastIndexOf(val)){
      inputArray.removeItem(inputArray.lastIndexOf(val)); 
    }
    let foundItem = outputArray.indexOf(val);
    if(foundItem === -1){
      return true;
    }else{
      outputArray.removeItem(foundItem);
      return false;
    }
  });
};


function sym(...args) {
  var ansArray = [];
  ansArray =   args.reduce((acc,arr)=>{
    let filteredArray = filter_diff(acc, arr);
    return [...acc, ...filteredArray];
  },[]);
  
  return ansArray;
}

console.log(sym([1, 2, 3, 3], [5, 2, 1, 4]));

#2

Your code has been blurred out to avoid spoiling a full working solution for other campers who may not yet want to see a complete solution. In the future, if you post a full passing solution to a challenge and have questions about it, please surround it with [spoiler] and [/spoiler] tags on the line above and below your solution code.

Also, try to be very specific in what constitutes “overly complicated”. Do you mean more concise? More efficient?

Thank you.


#3

Thanks, didn’t know about the Spoiler Tag , will use it from now on. First I’m concered with it being Expressive. Can you tell what it does just from reading the code. Next I’m concerned with Big O (Time then Space).


#4

This:

function sym(...args) {
  var ansArray = [];
  ansArray =   args.reduce((acc,arr)=>{
    let filteredArray = filter_diff(acc, arr);
    return [...acc, ...filteredArray];
  },[]);
  
  return ansArray;
}

can be simplified to:

const sym = (...arrays) => arrays
  .reduce((symDiffArr, arr) => symDiffArr
    .concat(filter_diff(symDiffArr, arr)), []);

Basically, if you are going to use const and let, then do not mix in var. Also, since you are were using let and the spread operator, then you should use other ES6 syntax like arrow functions. I believe the above function reads betters because acc did not have a real meaning, but symDiffArr does. Also, since the function arguments were arrays, it makes more sense to refer to them as arrays instead of args.

Although it was a good exercise in use the Object.defineProperty method, the Array.prototype.removeItem method you created does the same thing thing that the built-in Array.prototype.splice function does. The only difference is, your method will only remove a single item, where the splice method can remove multiple items and add new items at a specified index. Therefore, you could rewrite your filter_diff function as:

function filter_diff(outputArray, inputArray){
  return inputArray.filter((val)=> {
    //remove any duplicates in inputArray
    while(inputArray.indexOf(val) != inputArray.lastIndexOf(val)){
      inputArray.splice(inputArray.lastIndexOf(val),1); 
    }
    let foundItem = outputArray.indexOf(val);
    if(foundItem === -1){
      return true;
    }else{
      outputArray.splice(foundItem,1);
      return false;
    }
  });
};

and the above can be further refactored into:

const filter_diff = (outputArray, inputArray) => inputArray
  .filter(val=> {
    while(inputArray.indexOf(val) !== inputArray.lastIndexOf(val)){
      inputArray.splice(inputArray.lastIndexOf(val),1); 
    }
    const foundItem = outputArray.indexOf(val);
    return foundItem === -1 ? true : (outputArray.splice(foundItem,1), false);
  });

#5

Yep, it is overly complicated.

I don’t think your logic for symdiff is straight forward, especially where you use bunch of indexOf() to compute a single difference between arrays. This hinders readability and some performance.

Readability isn’t that good because each statement in filter_diff() does many things under the hood in a roundabout way without clarifying why.

Also, there is a remove operation going on within the same array that you are iterating. i.e) mutating array that your are currently iterating over
This is often risky and catches readers attention breaking the reading flow.

Performance can be improved by removing unnecessary O(n) operation such as indexOf() even though the overall running time is roughly O(n^2).

If you stick to the definition of symmetric difference, then your resulting code will be much clearer.
I don’t know if you will find this clearer, but I usually do it this way. Take a look after you improve your code:

const union = (a, b) =>
  a.concat(b)
    .reduce((obj, elem) => {
      obj[elem] = elem
      return obj
    }, {})

const intersection = (a, b) =>
  a.reduce((obj, elem) => {
    if (b.indexOf(elem) >= 0)
      obj[elem] = elem  
    return obj   
  }, {})

const exclude = (blackList, src) =>
  src.filter(elem => blackList[elem] === undefined)

const symdiff = (a, b) =>
  exclude(
    intersection(a, b),
    Object.values( union(a, b) ))

const symdiffAll = (...args) => args.reduce(symdiff)

symdiffAll([1, 2, 3], [2, 3, 4], [1, 5, 6]) // [4, 5, 6]