1
\$\begingroup\$

I want to get the difference between arr1 and arr2 and to get the correct result. However, I think my code is a little bit redundant.

function diffArray(arr1, arr2) {
var newArr = [];
// Same, same; but different.
  for(var i = 0;i<arr2.length;i++){
      var count = 0;
      for(var j = 0;j<arr1.length;j++){
          if(typeof arr2[i] == "number" && typeof arr1[j] == "number" && arr2[i] == arr1[j]) {
              count += 1;
          }
          if(typeof arr2[i] == "string" && typeof arr1[j] == "string" && arr2[i] === arr1[j]) {
              count += 1;
          }else{
            
          }
        
      }
      if(count == 0){
          newArr.push(arr2[i]);
      }
  }
  for(var j = 0;j<arr1.length;j++){
      var count = 0;
      for(var i = 0;i<arr2.length;i++){
          if(typeof arr2[i] == "number" && typeof arr1[j] == "number" && arr2[i] == arr1[j]) {
              count += 1;
          }
          if(typeof arr2[i] == "string" && typeof arr1[j] == "string" && arr2[i] === arr1[j]) {
              count += 1;
          }
      }
      if(count == 0){
          newArr.push(arr1[j]);
      }
  }
return newArr;
}

diffArray([1, "calf", 3, "piglet"], [1, "calf", 3, 4]);

\$\endgroup\$
1
4
\$\begingroup\$

Style

  • Your function name doesn't tell me whether you compute the symmetric or non-symmetric set difference. How about symmetricDifference instead of diffArray?

  • Instead of newArr I'd prefer reading e.g. result which tells me something about the role of that variable instead of its type.

  • There is an empty else { } clause in the first inner loop but not the second inner loop. Without that purely stylistic difference, the similarity of these two code blocks becomes more apparent.

  • The statements within the succeeding if clauses are identical, so you can merge them into one by combining the conditions with a logical OR operator.

  • The type checks and equality tests performed within the if conditions seem overly complex. AFAIK == is identical to === when both operands have the same type, so there is no need to mix them. Also, a generic function named diffArray should handle all possible array values, not only numbers and strings. This would allow you to simplify the two if conditions to e.g. a simple if (arr2[i] === arr1[j]) { ... }.

  • The iterator variable i is used to iterate through arr2 while iterator variable j is used to iterate through arr1. Readability improves by matching i with arr1 and j with arr2 according to their alphabetical order.

Performance

The inner loops are counting how often elements from the first array appear in the second array and vice versa. However, you are not interested in the exact count, only if count > 0. So you could use a labeled continue as soon as the count increments for the very first time:

outer: for (var i = 0; i < arr1.length; i++) {
  for (var j = 0; j < arr2.length; j++) {
    if (arr1[i] === arr2[j]) continue outer;
  }
  result.push(arr1[i]);
}

Now, it is much easier to see that the inner loop is actually just checking the existence of an element within an array. You can use the faster built-in indexOf method instead:

for (var i = 0; i < arr1.length; i++) {
  if (arr2.indexOf(arr1[i]) < 0) result.push(arr1[i]);
}

Even better, use the newer and more explicit includes method. However, this changes the semantics of your code due to the different handling of NaN:

NaN === NaN // false
[NaN].indexOf(NaN) // -1
[NaN].includes(NaN) // true

Now, your updated code could look as follows:

function symmetricDifference(arr1, arr2) {
  const result = [];
  for (const val of arr1) {
    if (!arr2.includes(val)) result.push(val);
  }
  for (const val of arr2) {
    if (!arr1.includes(val)) result.push(val);
  }
  return result;
}

Declarative vs. Imperative

Those two remaining loops actually filter the input arrays and return the remaining unique values. A more descriptive and possibly self-documenting way of writing such an operation is given by the filter method:

function symmetricDifference(arr1, arr2) {
  const difference1 = arr1.filter(val => !arr2.includes(val));
  const difference2 = arr2.filter(val => !arr1.includes(val));
  return difference1.concat(difference2);
}

However, the performance of plain for loops is superior.

Runtime Complexity

If you have to deal with larger arrays and prefer to have an implementation with higher setup costs but linear instead of quadratic runtime complexity, convert the input arrays into sets first and use the much faster set.has(val) instead of arr.includes(val).

Generalization

A generic solution which is not restricted to arrays but handles any iterable input could look as follows:

function* symmetricDifference(iterable1, iterable2) {
  const set1 = new Set(iterable1);
  const set2 = new Set(iterable2);
  for (const val of set1) if (!set2.has(val)) yield val;
  for (const val of set2) if (!set1.has(val)) yield val;
}
\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your taking time! It's very detailed. I was thinking of using "set", too. Probably I need to try to remember more methods than just sticking to if-else statement. I am using freecodecamp. So, the style was defined in that way. But next time, as you said, I need to be more careful about set-theoretic or symmetric difference. Anyway, I really appreciate it! \$\endgroup\$ Dec 3 '17 at 4:11
  • 1
    \$\begingroup\$ Thanks! It's normal that not everything mentioned in a review applies, just pick the parts you agree with. Next time, if you like, you can mention the focus of your review (e.g. performance) and you might get more specific answers. Still, open ended reviews are the most interesting to read IMHO \$\endgroup\$
    – le_m
    Dec 3 '17 at 4:36
  • \$\begingroup\$ Thanks. Since I was not getting used to Javascript itself and I was using only if-else statement, that's why I was asking like "my code is a little bit redundant". In most case, I think it's hard to answer. Next time, I will try to ask it as your advice! \$\endgroup\$ Dec 3 '17 at 4:53
1
\$\begingroup\$

Probably a moot point but lodash (and consequently underscore) have various utility functions like this i.e.

  • _.xor - symmetrical diff, this one is exactly what you're doing
  • _.difference - general diff

Given the size of the library (lodash ~4kb) and the maturity of bundlers these days (e.g. minfication, tree shaking), you really shouldn't need to be manually writing helper functions like this nowadays.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your advice. It’s getting more concise. After I get used to JavaScript, I should go along with it for avoiding redundancy. \$\endgroup\$ Dec 3 '17 at 15:39
  • \$\begingroup\$ _.difference does not compute the symmetric difference. The lodash equivalent to OPs implementation would probably be _.xor - see lodash.com/docs/4.17.4#xor \$\endgroup\$
    – le_m
    Dec 3 '17 at 17:27
  • 1
    \$\begingroup\$ @le_m yeah fair point, didn't thoroughly check the implementation was just going off the description. However, think the point I was trying to make has come across. \$\endgroup\$
    – James
    Dec 3 '17 at 20:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.