3

Instead of using the usual do-while loops for sorting, if we use 2 for loops, would that be okay?

let bubbleSort = (arr) => {
    console.log(arr);
    for(let i=0; i<arr.length; i++) {
        for (j=0; j<arr.length; j++) {
            if (arr[j] > arr[j+1]){
                console.log("swapped (i,j)->", arr[j],arr[j+1])   
                let temp = arr[j+1];
                arr[j+1] = arr[j];
                arr[j] = temp;

            }
        }
    }

    return arr

}

This returns the correct sorted array but is it any better than the do-while loop. Both are O(n^2) time-complexity.

ggorlen
  • 44,755
  • 7
  • 76
  • 106
pi2018
  • 347
  • 2
  • 11
  • 1
    doesn't it drop an error when "arr[arr.length-1]" is compared with "arr[arr.length]"? – mangusta Aug 07 '19 at 02:15
  • 1
    this version lacks an important feature of bubble sort - stopping when no swap was done on last scan – mangusta Aug 07 '19 at 02:16
  • Are you asking us to do your homework for you? The only reason to do a bubble sort is academic understanding, using a bubble sort in a production app is counter intuitive. – Adrian Brand Aug 07 '19 at 02:39
  • 1
    @AdrianBrand Not all code questions on SO need to be about production code. We welcome purely academic and educational questions. Homework is [on-topic](https://meta.stackoverflow.com/questions/334822/how-do-i-ask-and-answer-homework-questions), but it seems presumptuous to make that assumption in this case. – ggorlen Aug 07 '19 at 02:41
  • 1
    No its not for academic purpose, I was learning bubble sort on my own and read the theory and tried to implement it and wanted feedback on what I was doing wrong. – pi2018 Aug 07 '19 at 02:45

1 Answers1

4

All loop types do, while and for are equivalent. They're just syntactic sugar for the programmer and ultimately boil down to the same thing at runtime.

What you've posted here is a mostly correct implementation of bubble sort, but there are a few points of improvement available:

  • Bug: the code is accessing an out of bounds index in the inner loop: iterate to j < arr.length - 1 instead of j < arr.length because of the arr[j+1] statement. Most languages will crash or exhibit undefined behavior for an out of bounds array access, but JS just compares the value against undefined and carries on.

  • Potential bug: the inner loop creates a global j variable in the inner loop (thanks to Kaiido for pointing this out). This variable's value will persist beyond this function and might cause unexpected behavior elsewhere in the program. Declare variables with let (or const if the variable shouldn't be reassigned) to ensure that they are scoped locally to the block.

  • After the first iteration of the outer loop, the rightmost element in the array is in its final and sorted location (because it's the largest element in the array). After the second iteration of the outer loop, the last two elements of the array are in their final sorted positions. And so on. Therefore, we can shorten the number of iterations of the inner loop as follows: j < arr.length - 1 - i.

  • If on any iteration no swaps were performed, we can break--the array is already sorted.

These optimizations don't improve the time complexity, which is O(n2) as you say, but they're worth thinking about, as similar optimization approaches can help speed up real-world sort routines.

A few style points to consider:

  • Use spacing between operators (unless they're in []s).
  • Consider using a destructuring assignment to avoid the temp variable when swapping values. As pointed out in the comments, this has a performance penalty due to the overhead of array object creation per swap, so it should be compiled out in a production build (although bubble sort wouldn't be appropriate in production anyway).
  • Be consistent with semicolons and vertical spaces.

Here's a possible re-write:

const bubbleSort = arr => {
  for (let i = 0; i < arr.length; i++) {
    let swapped = false;
    
    for (let j = 0; j < arr.length - 1 - i; j++) {
      if (arr[j] > arr[j+1]) {
        [arr[j], arr[j+1]] = [arr[j+1], arr[j]];
        swapped = true;
      }
    }
    
    if (!swapped) { break; }
  }

  return arr;
};

for (let i = 0; i < 10000; i++) {
  const arr = Array(100).fill().map(e => ~~(Math.random() * 30));
  const expected = JSON.stringify(arr.slice().sort((a, b) => a - b));
  const actual = JSON.stringify(bubbleSort(arr.slice()));
  
  if (actual !== expected) {
    throw new Error(`test failed: expected ${expected} but got ${actual}`);
  }
}

console.log("10000 tests passed");

Fun follow-up exercises:

  • Write it without loops, using recursion.
  • Allow the user to pass a custom comparator function, similar to the builtin Array#sort. This way, you can sort arrays of objects or specify the order of the sort.
  • Benchmark it against other sort routines using console.time (thanks to Medet for the idea). How much do the optimizations help? Look into using seeded random numbers for your tests.
ggorlen
  • 44,755
  • 7
  • 76
  • 106
  • Thank you for the answer! All your points are very helpful!!. I had a question regarding your point on the inner loop...how is the loop going out-of-bounds? I mean "j" has been iterated over j=0 to less than arr.length. Am I missing a point here? – pi2018 Aug 07 '19 at 02:44
  • 1
    Sorry, I found the point. The function was accessing arr[j+1] element! – pi2018 Aug 07 '19 at 02:46
  • Can we have speed comparison on sort and bubbleSort? (console.time maybe?) – Medet Tleukabiluly Aug 07 '19 at 02:51
  • I could add that, but I think OP understands that bubble sort is pretty useless, and it's probably enough to say that `Array#sort` is going to smack bubble sort something terrible. – ggorlen Aug 07 '19 at 02:53
  • Wow **don't use destructuring asssignment here**. Yes it may make for more readable code, but you are writing a sorting algorithm, creating two new Arrays per sub-iteration is a very bad idea, Garbage Collector will kick in constantly. Just for reference, using a temp variable on my FF I've got roughly 400ms for the full execution of the snippet, vs 2500ms with destructuring. – Kaiido Aug 07 '19 at 03:38
  • Hmm, interesting. If it's that bad, then why ever use destructuring? Do you have a jsperf for this? The GC should not be so greedy--I'd expect it only to run whenever a low-memory threshold is passed rather than whenever an object goes out of scope, so surely that's not the cause of the problem. Thanks for raising the issue, though, but I'm going to take more convincing than this. – ggorlen Aug 07 '19 at 03:42
  • Also, in most cases, code is going to be transpiled before deployment. [Relevant comment/thread](https://stackoverflow.com/questions/47099510/destructuring-variables-performance#comment81149052_47099510) and [benchmark](https://jsperf.com/destructuring/5). – ggorlen Aug 07 '19 at 03:49
  • Well the benchmark exactly goes my way doesn't it? In Chrome I've got 720M op/sec for simple variables vs 123M with array destructuring and 625M vs only 4M in FF! GC has different mode of kicking it. For scoped Objects browsers often make a "Minor GC", which is faster than the one that kicks in when memory blown up, but having it in a loop makes no good at all. Here is a perf record of your version: https://i.stack.imgur.com/pl47z.png and yes, it goes till the end of this 3s call. Minor GC will also kick in with the temp version, but it won't have to initiate a new Object every sub-iteration. – Kaiido Aug 07 '19 at 04:50
  • Also, the comment you pointed to was answering a question where the OP was calling it a single time. So yes, in this circumstances, that won't make any difference. But here, you are writing a very hot script. This sub-loop might be called 4950% times of the input's length. Here micro-optimisations matter, and as a rule of thumb, never be bad friend with GC when trying to reach better perfs. – Kaiido Aug 07 '19 at 04:57
  • OK, good to know and thanks for the clarifications, but you realize this post is about a bubble sort probably written by a beginner JS coder? I'm not "trying to reach better perfs" and micro-optimize, I'm giving feedback about the algorithm itself from a language-insensitive standpoint, point out some major optimizations and remark on a few code style points. I think this is a clear case where readability and syntax style is more instructive than trying to squeeze every inch of performance out of the code. I'll add a warning remark to this effect, though. – ggorlen Aug 07 '19 at 05:15
  • No, you misread me, **this is not a micro-optimization**, I said "micro-optimizations matter". A 156x faster solution can not be called micro-optimization. Also you are generating a global `j`. – Kaiido Aug 07 '19 at 05:31
  • Ah, now that's a useful catch. Thank you! Re: destructuring, okay, let's not call it a micro-optimization. It's a huge performance increase. I can remove the destructuring syntax, but it seems like keeping it with a warning makes the post most educational for a beginner: they see the syntax and can learn that sometimes it's not a good idea to use inside of a loop. How do you feel about that? – ggorlen Aug 07 '19 at 05:32