1
\$\begingroup\$

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;
}
```
\$\endgroup\$
4
  • \$\begingroup\$ Both in the title of the post as well as in the code comments, you suggest that this problem were different if the arrays had the same size. However this is not true. Such statements may suggest to interviewers that you didn't completely understand the task. \$\endgroup\$ – RoToRa Mar 13 '20 at 9:07
  • \$\begingroup\$ Thanks for the comment. That is because a portion of the code that doesn't look right to me was added to handle arrays of different sizes. I was getting [1, 2, 3, undefined] and had to work around it. The size of inputs issue is specific to this implementation, not to the problem itself. \$\endgroup\$ – Matheus Deister Veiga Mar 13 '20 at 14:40
  • 1
    \$\begingroup\$ I'm sorry, but your thinking is wrong. This algorithm you wrote (nor any other reasonable other algorithm) would be different if the arrays had the same size. If you start with one array [1, 2] then it does't matter if the second array is [], [3], [3, 4], or [3, 4, 5]. Any reasonable algorithm would first place the elements 1 and 2 in the result array and then the elements of the seond array. The sizes of the arrays never play any role at all. \$\endgroup\$ – RoToRa Mar 16 '20 at 21:46
  • \$\begingroup\$ I know it is wrong. That is the point of asking for the code review. The code and the comments look too focused on something that should not have been an issue. A good rephrasing of the question would be: Merge two sorted arrays. And then, on the description: My code reads like I'm solving two separate problems, so I think I'm missing something. Is there a better way to implement it? \$\endgroup\$ – Matheus Deister Veiga Mar 17 '20 at 5:08
4
\$\begingroup\$

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 for j < arr2.length && i < arr1.length instead.

  • Checking valueToPush for undefined 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, ex j < arr2.length.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for the comments. 1 and 2 are good points. 3 is what made me post this code here. It works, but doesn't look right to me too. I could not come up with a cleaner solution. Checking for if(! ValueToPush) fails because the number 0 evaluates to false in JS, so the code breaks if any of the arrays has a 0. Eliminating the target size and using your index based condition could help me remove this ugly break statement and do the remainder operation after the loop. \$\endgroup\$ – Matheus Deister Veiga Mar 13 '20 at 15:06
  • \$\begingroup\$ Just to be clear, I don't mean just valueToPush == undefined, but whole valueToPush = array1[i]. Your condition basically checks for "is one array already done". I would just do it by checking i against array size instead of checking if you got "undefined" from array. Also your code won't work if there is actually undefined in the array, because your code will recognise it as end of array. \$\endgroup\$ – K.H. Mar 13 '20 at 15:22
3
\$\begingroup\$
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++ and j++ - that means you take element arr[i] and arr[j] and

incrementation follows after this operation

valueToPush = ( arr1[i] < arr2[j] ) ? arr1[i++] : arr2[j++]

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the comment. It's algorithm interview practice, so using standard functions kind of misses the point, even though on a real life setting, I'd probably be using the arrow function, spread operator and built in sort too. The valueToPush one-liner is pretty cool, didn't know I could increment values inside of array index like this. \$\endgroup\$ – Matheus Deister Veiga Mar 13 '20 at 14:57

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.