Code Review for Symmetric Difference

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]));

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).

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]
1 Like

Thank you both for taking the time to read my code and give feedback. I had thought I’d said this long ago but today I don’t see it.