3
\$\begingroup\$

I have written a code were I pass two sorted array of numbers from command line, and out is one array with numbers sorted.

the code is as follows,

(function(){
    'use strict';

    var getCommandLineVariables = getCommandLineVariables;  

    var mainFunction = mainFunction;

    mainFunction();

    function mainFunction(){
        var array = getCommandLineVariables();
        var firstArray = array[0], 
            secondArray = array[1],
            mergedArray = [], i = 0, j = 0;

        for(i = 0; i < firstArray.length; i++){
            for(j = j; j < secondArray.length; j++){

                if(firstArray[i] < secondArray[j]){ // if number from first array is less than number of second array
                    mergedArray.push(firstArray[i]);                                        
                    break;
                }else{ // if number of second array is less than number of first array
                    mergedArray.push(secondArray[j]);
                }

            } // end of for

            /*
                The below condition executes when 
                length of first array is greater than second array
            */
            if(j >= secondArray.length){
                mergedArray.push(firstArray[i]);
            }
        } // end of for

        /*
            If length of second array is greater than
            first array
        */
        for(j = j; j < secondArray.length;j++){
            mergedArray.push(secondArray[j])
        }

        console.log(mergedArray);    
    } // mainFunction

    /*
        Capture the numbers passed as commandline arguments
    */
    function getCommandLineVariables(){
        var arg_array = process.argv.slice(2),
            someArray = [];

        if(arg_array.length !== 2){
            throw "Pass two comma seperated numbers";
        }

        for(var i = 0; i < arg_array.length; i++){

            someArray.push((function(someString){
                var someArray = someString.split(",");

                someArray = someArray.map(function(str){
                    return parseInt(str);
                });     

                return someArray;               
            })(arg_array[i]));  

        } // end of for


        return someArray;
    } // end of getCommandLineVariables     
})();

The output of the above code is as follows,

E:\DataStructuresAndAlgorithms\array>node mergeSortedArray01.js 1,3,100 2,4,5,7,10
[ 1, 2, 3, 4, 5, 7, 10, 100 ]

E:\DataStructuresAndAlgorithms\array>

One more output

E:\DataStructuresAndAlgorithms\array>node mergeSortedArray01.js 3,27,100 2,4,5,7,10
[ 2, 3, 4, 5, 7, 10, 27, 100 ]

E:\DataStructuresAndAlgorithms\array>

Can you please review my code a check is there any room for improvement, Can the above problem be solved in other ways.

\$\endgroup\$
  • \$\begingroup\$ How do you assess the resource requirements of the procedure presented? Is that the best possible, disregarding "constant factors"? \$\endgroup\$ – greybeard Jan 23 '18 at 17:48
2
\$\begingroup\$

Walkthrough

Let me walk through the code with you and see what can be improved.

    var getCommandLineVariables = getCommandLineVariables;  

    var mainFunction = mainFunction;

These are already available in your scope, so you do not need to reassign them to a local variable with the same name.

            mergedArray = [], i = 0, j = 0;

        for(i = 0; i < firstArray.length; i++){
            for(j = j; j < secondArray.length; j++){

You initialize i and j, then you reinitialize i in the outer for loop, then do nothing with j in the inner one. You don't need to write an initialization step in the for loop if you don't want to.

            throw "Pass two comma seperated numbers";

Here you throw a string. You can throw any value, but it's more helpful for debugging purposes if you throw an Error object. Just pass this string into the Error constructor to make your own error message.

            someArray.push((function(someString){
                var someArray = someString.split(",");

                someArray = someArray.map(function(str){
                    return parseInt(str);
                });     

                return someArray;               
            })(arg_array[i]));

You pass an immediately invoked function expression (IIFE) to the push method, but this is totally unnecessary. You can execute the code above the push method and make things considerably more readable. Also, you redeclare the variable someArray inside this IIFE and use it for something else, which is pretty confusing. It'd be better to use a different variable name.

Time Complexity

Your current algorithm is O(n2) because of the nested for loops. A more efficient algorithm would be to step through both arrays at the same time, adding the next smallest element to the merged array. This approach would be O(n), so an improvement!

Changes

Altogether, this is how I would restructure your existing code:

(function(){
    'use strict';

    mainFunction();

    function mainFunction(){
        var array = getCommandLineVariables();
        var firstArray = array[0], 
            secondArray = array[1],
            mergedArray = [], i = 0, j = 0;

        for(; i < firstArray.length; i++){
            for(; j < secondArray.length; j++){

                if(firstArray[i] < secondArray[j]){ // if number from first array is less than number of second array
                    mergedArray.push(firstArray[i]);                                        
                    break;
                }else{ // if number of second array is less than number of first array
                    mergedArray.push(secondArray[j]);
                }

            } // end of for

            /*
                The below condition executes when 
                length of first array is greater than second array
            */
            if(j >= secondArray.length){
                mergedArray.push(firstArray[i]);
            }
        } // end of for

        /*
            If length of second array is greater than
            first array
        */
        for(; j < secondArray.length;j++){
            mergedArray.push(secondArray[j])
        }

        console.log(mergedArray);    
    } // mainFunction

    /*
        Capture the numbers passed as commandline arguments
    */
    function getCommandLineVariables(){
        var arg_array = process.argv.slice(2),
            someArray = [];

        if(arg_array.length !== 2){
            throw new Error("Pass two comma separated numbers");
        }

        for(var i = 0; i < arg_array.length; i++){
            var inputArray = arg_array[i].split(",");

            inputArray = inputArray.map(function(str){
                return parseInt(str);
            });

            someArray.push(inputArray);
        } // end of for


        return someArray;
    } // end of getCommandLineVariables     
})();

And this is how I would implement the more efficient algorithm I described above:

function mainFunction(){
    var array = getCommandLineVariables();
    var firstArray = array[0], 
        secondArray = array[1],
        mergedArray = [], i = 0, j = 0;

    while(i < firstArray.length && j < secondArray.length) {
        if(firstArray[i] < secondArray[j]) {
            mergedArray.push(firstArray[i]);
            i++;
        } else {
            mergedArray.push(secondArray[j]);
            j++;
        }
    }

    while(i < firstArray.length) {
        mergedArray.push(firstArray[i]);
        i++;
    }

    while(j < secondArray.length) {
        mergedArray.push(secondArray[j]);
        j++;
    }

    console.log(mergedArray);    
} // mainFunction
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Your use of break, while not wrong, can be confusing. A different, slightly less verbose way to solve this would be to take advantage of Javascript's array methods and run firstArray.concat(secondArray).sort((a, b) => a - b); or function(a, b) { return a - b} if you don't like arrow functions. It should run in approx O(n log n) time.

| improve this answer | |
\$\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.