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]);
-
\$\begingroup\$ 2ality.com/2015/01/es6-set-operations.html \$\endgroup\$Slai– Slai2017年12月03日 17:12:03 +00:00Commented Dec 3, 2017 at 17:12
2 Answers 2
Style
Your function name doesn't tell me whether you compute the symmetric or non-symmetric set difference. How about
symmetricDifference
instead ofdiffArray
?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 nameddiffArray
should handle all possible array values, not only numbers and strings. This would allow you to simplify the twoif
conditions to e.g. a simpleif (arr2[i] === arr1[j]) { ... }
.The iterator variable
i
is used to iterate througharr2
while iterator variablej
is used to iterate througharr1
. Readability improves by matchingi
witharr1
andj
witharr2
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;
}
-
\$\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\$kimi Tanaka– kimi Tanaka2017年12月03日 04:11:23 +00:00Commented Dec 3, 2017 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– le_m2017年12月03日 04:36:57 +00:00Commented Dec 3, 2017 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\$kimi Tanaka– kimi Tanaka2017年12月03日 04:53:53 +00:00Commented Dec 3, 2017 at 4:53
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.
-
\$\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\$kimi Tanaka– kimi Tanaka2017年12月03日 15:39:08 +00:00Commented Dec 3, 2017 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– le_m2017年12月03日 17:27:23 +00:00Commented Dec 3, 2017 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– James2017年12月03日 20:36:03 +00:00Commented Dec 3, 2017 at 20:36