1
\$\begingroup\$

I solved bubble sort in an exam, but the examiner wasn't satisfied with the solution. Please tell me why my solution is unsatisfactory.

function bubbleSort(myArray){
  var temp = undefined,isSwap = false;

  for(var x = 0; x < myArray.length; x++){
   if(((x+1) <= (myArray.length - 1)) && myArray[x] > myArray[x+1]){
     temp = myArray[x];
     myArray[x] = myArray[x+1];
     myArray[x+1] = temp;
     isSwap = true;           
   }else if(isSwap === true && x === (myArray.length - 1)){
     x = -1;
     isSwap = false;
   }else{
     continue;
   }
 }
 return myArray;
}

Just to see the performance I added a print statement i.e. console.log in the start of the loop like below

for(var x = 0; x < myArray.length; x++){
    console.log(myArray);

This is what I am able to see for the specific input:

console.log(bubbleSort([5,1,6,2,0]));

Output:

rahul@rahul:~/myPractise/Algo$ node sorting.js 
[ 5, 1, 6, 2, 0 ]
[ 1, 5, 6, 2, 0 ]
[ 1, 5, 6, 2, 0 ]
[ 1, 5, 2, 6, 0 ]
[ 1, 5, 2, 0, 6 ]
[ 1, 5, 2, 0, 6 ]
[ 1, 5, 2, 0, 6 ]
[ 1, 2, 5, 0, 6 ]
[ 1, 2, 0, 5, 6 ]
[ 1, 2, 0, 5, 6 ]
[ 1, 2, 0, 5, 6 ]
[ 1, 2, 0, 5, 6 ]
[ 1, 0, 2, 5, 6 ]
[ 1, 0, 2, 5, 6 ]
[ 1, 0, 2, 5, 6 ]
[ 1, 0, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
[ 0, 1, 2, 5, 6 ]
\$\endgroup\$
  • 4
    \$\begingroup\$ "Please tell me why my solution in unsatisfactory". You should ask your examiner why they weren't satisfied. \$\endgroup\$ – Zeta Jul 20 '16 at 6:52
  • \$\begingroup\$ Please state in the title of your question only what the purpose of your code is. \$\endgroup\$ – Heslacher Jul 20 '16 at 7:53
5
\$\begingroup\$

As @Zeta said, only your examiner can tell you why he wasn't satisfied. But here are a couple of suggestions anyway:

  • Variable names are important and myArray doesn't mean much. You can try inputArray for example.
  • Changing the index of a for loop is usually not a good idea, because it makes the code a bit more difficult to understand (not by a lot, but why make it harder?).

This is another go at the same algorithm, which I think is more readable:

function bubbleSort(inputArray)
{
  var index;
  var done = false;
  var temp;

  while ( !done )
  {
    done = true;
    for(var index = 0; index < inputArray.length; index++)
    {
      if ( inputArray[index] > inputArray[index+1] )
      {
        temp = inputArray[ index ];
        inputArray[ index ] = inputArray[ index+1 ];
        inputArray[ index+1 ] = temp;
        done = false;
      }
    }
  }

  return inputArray;
} 
\$\endgroup\$
4
\$\begingroup\$
function bubbleSort(myArray){
  var temp = undefined,isSwap = false;

  for(var x = 0; x < myArray.length; x++){
   if(((x+1) <= (myArray.length - 1)) && myArray[x] > myArray[x+1]){
     temp = myArray[x];
     myArray[x] = myArray[x+1];
     myArray[x+1] = temp;
     isSwap = true;           
   }else if(isSwap === true && x === (myArray.length - 1)){
     x = -1;
     isSwap = false;
   }else{
     continue;
   }
 }
 return myArray;
}  

Lets go from top to bottom.

  • declaring multiple variables on the same line reduces the readability of your code. Readability is a key aspect in programming. If a reader of your code can't grasp at first glance what your code is doing then you have done something wrong.

  • you should declare your variables as near to their usage as possible. This means e.g you should declare the temp variable inside the first if block.

  • you are using 2 times myArray.length -1 so it would be better to extract this to a variable.

  • The swapping of the array elements should be done in a separate method so its intent is more clear and your bubbleSort() method will be shorter.

  • the continue in the else part is superflous because the loop wouldn't do anything different than continue.

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

Your code following some clean up that @Heslacher explained.

function bubbleSort(inputArray) {
    var n = inputArray.length;
    var hasSwapped = false;
    for (var i = 0; i < n; i++) {
        if (i < n - 1 && inputArray[i] > inputArray[i + 1]) {
            var temp = inputArray[i];
            inputArray[i] = inputArray[i + 1];
            inputArray[i + 1] = temp;
            hasSwapped = true;
        } else if (hasSwapped && i === n - 1) {
            i = -1;
            hasSwapped = false;
        }
    }
    return inputArray;
}

Attention for

  1. Spacing
  2. Unnecessary code
  3. Variable names
    1. Prefer i for counters
    2. Avoid myXyz for anything
    3. isSwap is not exact what you want to inform
  4. Complex boolean expressions
  5. Avoid parenthesis on boolean expression ===, == have lower precedence than other operators.

Operators precedence in Javascript.

\$\endgroup\$

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.