Mdthbs

I wanted to implement a selection sort and wanted to make sure that I'm doing it correctly. I wanted to do it in a way that's efficient and use recursion. Please let me know if I am doing this correctly or if there is a better way for me to do it.

     /**

         * selectionSort

         * @param toSort

         * @param sorted

         * @returns {Array}

         */

        function selectionSort(toSort, sorted=[]) {

            if (!toSort.length) {

                return sorted;

            }

            let minIndex = findMinimum(toSort);

            sorted.push(...toSort.splice(minIndex, 1))

            return selectionSort(toSort, sorted);

        }



        function findMinimum(arr) {

            let minIndex=0;

            arr.forEach(function (item, index) {

                if(item < arr[minIndex]) {

                    minIndex = index;

                }

            })

            return minIndex;

        }





        const testCase = [64, 25, 12, 22, 11]

        const answer = selectionSort(testCase);

I'd suggest the following changes:

1. Use a loop instead of recursion


2. Use for/of instead of .forEach()
   


3. push a single value instead of using an array with one element in it


4. cache the lowest value so far so you don't have to constantly refetch it on every comparison


5. Use a temporary array for the sort so the function is non-destructive to the source array (consistent with most array methods)


6. Use const where you can.

Code:

 /**

     * selectionSort

     * @param toSort (not modified)

     * @param sorted (new sorted array)

     * @returns {Array}

     */

    function selectionSort(toSort, sorted = []) {

        if (!toSort.length) {

            return sorted;

        }

        // make copy so we don't modify source

        const sortData =  toSort.slice();



        while (sortData.length) {

            const minIndex = findMinimum(sortData);

            sorted.push(sortData[minIndex]);

            // remove min item from data left to be sorted

            sortData.splice(minIndex, 1);

        }

        return sorted;

    }



    function findMinimum(arr) {

        let minIndex = 0, minValue = arr[0];

        for (const [index, item] of arr.entries()) {

            if (item < minValue) {

                minIndex = index;

                minValue = item;

            }

        }

        return minIndex;

    }





    const testCase = [64, 25, 12, 22, 11]

    const answer = selectionSort(testCase);

    console.log(answer);

Review

• 
  findMinimum means to me that you find the minimum value in an array of items. Since your function returns the index instead, call it indexOfMinimum.


• Prefer the use of const over let if you only assign a variable once: let minIndex = findMinimum(toSort); -> const minIndex = findMinimum(toSort);.


• Use arrow notation to write more compact functions: function (item, index) -> (item, index) =>.


• Your documentation seems like waisted space. If you document a public function (which is a good thing), put in some effort to write a clear description of the function, not just the name of the method copied.


• Use whitespace to write more idiomatic javascript:
  
  
  
  
  • 
    let minIndex=0; -> let minIndex = 0;
    
  
  
  • 
    if(item < arr[minIndex]) -> if (item < arr[minIndex])
    
  
  
  
  

Rewritten:

function selectionSort(toSort, sorted=[]) {

    if (!toSort.length) {

        return sorted;

    }

    const minIndex = indexOfMinimum(toSort);

    sorted.push(...toSort.splice(minIndex, 1))

    return selectionSort(toSort, sorted);

}



function indexOfMinimum(arr) {

    let minIndex = 0;

    arr.forEach((item, index) => {

        if (item < arr[minIndex]) {

            minIndex = index;

        }

    })

    return minIndex;

}