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.

I would like to increase my performance of js code write a clean code... can you tell me how to do it... providing my fiddle below with js code...

http://jsfiddle.net/YYg8U/

var myNumbersToSort = [-1, 2, -3, 4, 0.3, -0.001];

function getClosestToZero(numberSet) {
    var i = 0, positiveSet = [], positiveClosest = 0;
    for (i = 0; i < numberSet.length; i += 1) {
        positiveSet.push(numberSet[i] >= 0 ? numberSet[i] : numberSet[i] * -1);
    }
    positiveClosest = Math.min.apply(Math, positiveSet);
    return numberSet[positiveSet.indexOf(positiveClosest)];
}

alert(getClosestToZero(myNumbersToSort));
share|improve this question

5 Answers 5

In code you posted, you pass through array three times instead of one:

  • 1st pass when pushing absolute values to auxillary array

  • 2nd pass when searching for minimum value (in Math.min)

  • 3rd pass when you get the index of minimum value (in indexOf)

It is more efficiently to do it in just one pass:

function getClosestToZero(set) {
  if(0 === set.length) return null;
  var closest = Math.abs(set[0]), result = 0;
  for(var i in set) {
     var next = Math.abs(set[i]);
     if(closest > next) {
       result = i;
       closest = next;
     }
  }
  return result;  
}

Also for the purpose of readability I will recommend you to use Math.abs library function instead of implementing it by yourself.

Note: The small theoretical disatavantage of my code consists in that it compares first element with itself. You can rewrite for loop to avoid it or ignore it because its impact is insigninficant.

share|improve this answer
    
What is the advantage of 0 === set.length vs the more natural set.length === 0? –  Dmitri Zaitsev Dec 14 '13 at 16:30
1  
@DmitriZaitsev this kind of inverse notation is usually used to be sure that you did not accidently write set.length = 0 as your conditional statement –  zavg Dec 14 '13 at 16:37
    
Ah yes - good point! –  Dmitri Zaitsev Dec 14 '13 at 17:48
function lowestEpsilon(of) {

    return of.reduce(minimal, [Infinity, Infinity]).shift()

    // pass around a pair of [value, Math.abs(value)] and replace
    // it if the current item has a better absolute value.
    function minimal(previously, value) {
        var absolute = Math.abs(value)
        if ( absolute < previously[1] )
            return [value, absolute]
        return previously
    }
}

lowestEpsilon([1,2,-0.5])

lowestEpsilon([50,0.3,-0.2,1,50])

Ensure you have [].reduce available in your environment.

Prefer comprehensible code over fancy algorithms. Don't optimise prematurely.

Hard to find the best balance of 'clean and performant' until you've profiled your application and use cases. Do you have large arrays or small arrays as input? Is this the slowest call in your application?

Thanks to tomdemuyt for correcting my interpretation of the problem.

Bonus: what do you expect to be the output for [-0.5, 0.5] as an input?

share|improve this answer
    
I love the simplicity of an array over the object I chose. –  konijn Dec 14 '13 at 21:03

Processing an array and coming up with 1 result is what .reduce() was meant for.

The problem is that reduce only has 1 'previous' and I did not want to recalculate the absolute value of previous every time. This can be prevented by passing on a simple object. The function could easily be extended to return the value, abs value and the array index.

var myNumbersToSort = [-1, 2, -3, 4, 0.3, -0.001];

function getClosestToZero( set )
{
  return set.reduce( function( o, i )
  {
    return ( Math.abs( i ) < o.abs )? { value : i , abs: Math.abs( i ) }:o;
  } , { value : set[0] , abs : Math.abs(set[0]) } ).value;
}

alert(getClosestToZero(myNumbersToSort));

or, we could build on the answer of MrMinshall:

var myNumbersToSort = [-1, 2, -3, 4, 0.3, -0.001];

function getClosestToZero( set )
{
  return set.sort( function(a,b){ return Math.abs(a) -  Math.abs(b) } )[0];
}       
alert(getClosestToZero(myNumbersToSort));
share|improve this answer

There's nothing wrong with a bit of competition, but here's my take on it

function getClosestToZero(numberSet) {
    if (!numberSet || !numberSet.length) return;
    var closestNumber = numberSet[0];
    for (var i = 0; i++ < numberSet.length;)
        if (fastAbs(numberSet[i]) < fastAbs(closestNumber)) 
            closestNumber = numberSet[i];
    return closestNumber;
}

function fastAbs(x) { return (x < 0 ? -x : x);}

Well, more lines doesn't mean it's slower. The main bottleneck for everyone is Math.abs. There's a much faster implementation of Math.abs, which I also used in this perf test.

Additionally, you one should not use for-in when looping through an array. It's because for-in will run over on properties other than the number-indexed members (yes, Arrays are still objects.). And besides, for-in is a very slow loop.

share|improve this answer

I get the same result with this one-liner:

alert(myNumbersToSort.sort()[0])

I don't know if the sort() array method is any faster than your code but you could test it with a long loop.

share|improve this answer
2  
This is slower though. Also, this doesn't check for positives. –  Benjamin Gruenbaum Dec 14 '13 at 16:03
3  
Good points. Worth leaving this here as a reminder to others to not jump to the same conclusion. –  Simon Minshall Dec 14 '13 at 16:09

Your Answer

 
discard

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.