Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Addressed both issues in this codereview post.

Fixed the indexing issue and the call mechanism of the function expression.

Looking for perfect code. Hope this is the last rev.

    /***************************************************************************************************
    **ALGORITHMS
    ***************************************************************************************************/

    // self used to hold client or server side global
    (function () {

        "use strict";

        // holds (Pub)lic properties
        var Pub = {},

            // holds (Priv)ate properties
            Priv = {},

            // holds "imported" library properties
            $A;

        (function manageGlobal() {

            // Priv.g holds the single global variable, used to hold all packages
            Priv.g = '$A';

            if (this[Priv.g] && this[Priv.g].pack && this[Priv.g].pack.utility) {
                this[Priv.g].pack.algo = true;
                $A = this[Priv.g];
            } else {
                throw new Error("algo requires utility module");
            }
        }());

        Pub.swap = function (arr, i, j) {
            var temp = arr[i];
            arr[i] = arr[j];
            arr[j] = temp;
        };

        // checks to see if sorted
        Pub.isSorted = function (arr) {
            var i,
                length = arr.length;
            for (i = 1; i < length; i++) {
                if (arr[i] < arr[i - 1]) {
                    return false;
                }
            }
            return true;
        };

        // repeatedly orders two items ( a bubble ) at a time
        Pub.bubbleSort = function (arr) {
            var index_outer,
                index_inner,
                swapped = false,
                length = arr.length;
            for (index_outer = 0; index_outer < length; index_outer++) {
                swapped = false;
                for (index_inner = 0; index_inner < length - index_outer; index_inner++) {
                    if (arr[index_inner] > arr[index_inner + 1]) {
                        Pub.swap(arr, index_inner, index_inner + 1);
                        swapped = true;
                    }
                }
                if (!swapped) {
                    break;
                }
            }
            return arr;
        };

        // repeatedly finds minimum and places it the next index
        Pub.selectionSort = function (arr) {
            var index_outer,
                index_inner,
                index_min,
                length = arr.length;
            for (index_outer = 0; index_outer < length; index_outer++) {
                index_min = index_outer;
                for (index_inner = index_outer + 1; index_inner < length; index_inner++) {
                    if (arr[index_inner] < arr[index_min]) {
                        index_min = index_inner;
                    }
                }
                if (index_outer !== index_min) {
                    Pub.swap(arr, index_outer, index_min);
                }
            }
            return arr;
        };

        // repeatedly places next item in correct spot using a "shift"
        Pub.insertionSort = function (arr) {
            var index_outer,
                index_inner,
                value,
                length = arr.length;
            for (index_outer = 1; index_outer < length; index_outer++) {
                value = arr[index_outer];

                // JavaScript optimization - index_inner >=0 is removed
                // as the array index will return undefined for a negative index
                for (index_inner = index_outer - 1; (arr[index_inner] > value);
                        index_inner--) {
                    arr[index_inner + 1] = arr[index_inner];
                }
                arr[index_inner + 1] = value;
            }
            return arr;
        };

        // module complete, release to outer scope
        this[Priv.g] = $A.extendSafe(this[Priv.g], Pub);
    }).call(this);
share|improve this question

1 Answer 1

up vote 3 down vote accepted

I would remove Pub.swap and copy its code where you need it.

That's because function calls are expensive, so better avoid calling a function at each iteration if possible.

And try to cache the expressions in loop's condition. For example,

var lengthMinusIndexOuter = length - index_outer;
for (index_inner = 0; index_inner < lengthMinusIndexOuter; index_inner++) {

instead of

for (index_inner = 0; index_inner < length - index_outer; index_inner++)
share|improve this answer
    
The caching sound like a good idea, but I don't think a function call is so costly that it is worth making the code less DRY. –  user34330 Jan 26 '14 at 2:15
    
@www.arcmarks.com You would be surprised of how costly are function calls: see slides 10~19 of Extreme JavaScript Performance slideshow –  Oriol Jan 26 '14 at 3:16
    
over 3 years old... –  user34330 Jan 26 '14 at 18:29
    
@www.arcmarks.com Well, newer javascript engines are more optimized and can do more operations per second. But inline code is still faster than function calls, because inline code runs faster too. –  Oriol Jan 26 '14 at 20:50

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.