0
\$\begingroup\$

I have implemented binary search algorithm in JavaScript:

var myArray = [], 
    searchNum = undefined;

// below function is used to capture the 
// commandline parameters for array and the
// number to be searched
(function(){
    process.argv.forEach(function (val, index, array) {
        var idx = 0, ar = undefined;

        try{

            // get the commandline argument for 
            // array values 
            if(index === 2){
                myArray = myArray.concat(val.split(",").map(function(num){
                    return parseInt(num);
                }));    
            }

            // third index is the number to be searched.
            if(index === 3){
                searchNum = parseInt(val)
            }

        }catch(e){
            console.log(e)
        }

    });
})();

console.log(" SEARCH NUMBER ",searchNum," in array ",myArray);

console.log(binary_search(myArray,searchNum,0,myArray.length));

function binary_search(numberArray, numberToSearch, lowIndex, maxIndex){
    var totalLength = maxIndex - lowIndex;

    var midIndex = parseInt(totalLength/2),
        str = "";

    /*
        If Lower Index is equal to Higher Index,
        that means number is not found and hence there is 
        a collision in pointers and hence return 
        as 'Can't be found'
    */  
    if(lowIndex === maxIndex){
        return "can't be found";
    }   

    /*
        setting the actual middle index 
        by adding the computed middle index with lower index.
    */
    midIndex = lowIndex + midIndex;

    // if number found
    if(numberArray[midIndex] === numberToSearch){

        str = " Number "+numberToSearch+" found at position "+midIndex;

        return str;

    // if number in middle index is less than the number to be searched
    // set the lower Index to new value i.e. a index position next higher to 
    // middle Index 
    }else if(numberArray[midIndex] < numberToSearch){

        lowIndex = midIndex + 1;


    // number to be searched is less than the number present at middle Index
    // set new maxIndex value i.e. index which is previous position to the
    // middle index     
    }else if(numberArray[midIndex] > numberToSearch){

        maxIndex = midIndex;

    }else{

        return "can't be found";

    }

    return binary_search(numberArray, numberToSearch, lowIndex, maxIndex);
} // end of method binary_search

When I run the above code the output is as follows,

E:\RahulShivsharan\MyPractise\DesignPatternsInJavaScript>node binarySearch.js 12,34,56,78,90 24
 SEARCH NUMBER  24  in array  [ 12, 34, 56, 78, 90 ]
can't be found

E:\RahulShivsharan\MyPractise\DesignPatternsInJavaScript>node binarySearch.js 12,34,56,78,90 34
 SEARCH NUMBER  34  in array  [ 12, 34, 56, 78, 90 ]
 Number 34 found at position 1

Can you please review my code and please suggest me if there is any room for further improvement.

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

Parsing command line arguments

I mentioned this in a previous review, it seems I wasn't specific enough, but this is an inefficient and unnatural way to parse command line arguments:

    process.argv.forEach(function (val, index, array) {
        var idx = 0, ar = undefined;

        try{

            // get the commandline argument for 
            // array values 
            if(index === 2){
                myArray = myArray.concat(val.split(",").map(function(num){
                    return parseInt(num);
                }));    
            }

            // third index is the number to be searched.
            if(index === 3){
                searchNum = parseInt(val)
            }

        }catch(e){
            console.log(e)
        }

    });

What's wrong with it?

  • For each index 0, 1, 2, 3, the loop compares the index against 2 and 3 repeatedly
  • If there are not enough arguments, the loop should not even begin
  • The command line parsing logic fails its purpose by not failing in case the arguments are invalid

The natural way to parse would be:

  • Check process.argv.length and fail if invalid
  • Check the type of each argument and fail if invalid

Consider this alternative, no looping, nice and simple:

function parseArgs(prog, argv) {
  if (argv.length != 2) {
    throw `usage: node ${prog} NUMS_CSV NUM`;
  }

  function validInt(s) {
    var num = parseInt(s, 10);
    if (isNaN(num)) {
      throw "Not a valid number: " + s;
    }
    return num;
  }

  return {
    nums: argv[0].split(',').map(validInt),
    target: validInt(argv[1])
  };
}

var args = parseArgs(process.argv[1], process.argv.slice(2));

Parsing integers

When using parseInt, it's recommended to specify the radix parameter, because although base 10 is a common default, it's not guaranteed across different implementations. So to parse base 10 numbers, write parseInt(x, 10) instead of parseInt(x).

API design

The binary_search function searches for an element and returns two kinds of strings as result:

  • can't be found
  • Number N found at position X

This is very limiting. It would be better to have binary_search return an index or -1 if not found, and move the logic of formatting a string result into a dedicated function.

\$\endgroup\$
  • \$\begingroup\$ Why do you recommend specifying the radix 10 for parseInt when that is the default? \$\endgroup\$ – kamoroso94 Jul 10 '17 at 12:26
  • 1
    \$\begingroup\$ @kamoroso94 thanks for pointing out, I clarified. According this page, the default is not always 10, that's why it's good to write explicitly always. \$\endgroup\$ – janos Jul 10 '17 at 12:33

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.