Problem
I think the algorithm implementation itself is reasonably efficient, but I’m not sure about proper use of good practices in software development in the code (variable naming, idioms, comments, error handling in inputs etc).
I’m attempting to get a first developer job, and since interviewers also evaluate how “clean” the code is, the main point of the question is what is considered good practice, but efficiency improvements are also welcome.
mergeSortedArrays = function(arr1, arr2){
if(!(Array.isArray(arr1) && Array.isArray(arr2))){
throw new TypeError("Need two arrays to merge")
};
if(arr2.length === 0 && arr1.length === 0){
throw new RangeError("Cannot merge empty arrays")
};
let i = 0;
let j = 0;
const targetSize = arr1.length + arr2.length;
const mergedArray = [];
// main loop
while(mergedArray.length < targetSize){
/**
* Works until completion for arrays of the same size
*/
if(arr1[i] < arr2[j]){
valueToPush = arr1[i]
i++
}else{
valueToPush = arr2[j]
j++
}
/**
* For arrays with different sizes, it is safe to push the remainder to the sorted array, given that both inputs are sorted.
*/
if(valueToPush === undefined){
const remainingItems = j > arr2.length ? arr1.slice(i) : arr2.slice(j)
mergedArray.push(...remainingItems)
break
}
mergedArray.push(valueToPush)
}
return mergedArray;
}
```
Solution
I think considering approach that you chose, your code is alright. A few points:
-
What I don’t like is that you get error when passing 2 empty arrays? Why would you do that? Imagine I am generating arrays of different sizes and using your code to merge and sometimes I would pass 2 empty arrays. Should I really need to handle that as special case in my code? No, just return empty array too.
-
I’d eliminate
targetSize
variable and check forj < arr2.length && i < arr1.length
instead. -
Checking
valueToPush
forundefined
feels a bit off for me. It works, but doesn’t really feel right. In many languages you would get something like IndexOutOfRange error or something. Maybe it’s idiomatic in Javascript (probably not), but I think nicer and cleaner would be to check if you are still in range of array, exj < arr2.length
.
const mergeSortedArrays = (arr1, arr2) => [...arr1, ...arr2].sort((a,b) => a-b);
const ar1 =[-7, 2, 4, 22, 66, 99];
const ar2 = [1, 5, 9, 88];
console.log( mergeSortedArrays(ar1, ar2) );
// [ -7, 1, 2, 4, 5, 9, 22, 66, 88, 99 ]
I don’t know if it’s not too simple solution, but one-line Arrow function solves this problem…
OR
in your code instead of IF / ELSE “Works until completion for arrays of the same size” you can:
- use shorter form ternary operator
? :
- instantly
i++
andj++
– that means you take elementarr[i]
andarr[j]
and
incrementation follows after this operation
valueToPush = ( arr1[i] < arr2[j] ) ? arr1[i++] : arr2[j++]