6
\$\begingroup\$

I wrote this function that, given an array, calculates the closest number to zero. The conditions are the following:

  • if the array is empty, return 0
  • if the array has two values share the "closeness" to zero, return the positive (for example if -1 and 1)

Here is my code and link to it.

// Write a function that takes an array an finds the closes number to 0. 
// Disregard all 0s
// If the array is empty return 0

const tempArray = [-1, 0, 0, 5, -5, 6, -3, 2, 10, 13, 8, 70, -36, 36];

function calculateClosestTo0 (tempArray) {
  let minusTemps = [];
  let plusTemps = [];

  if (!tempArray.length) {
    return 0;
  }

  for (let i = 0; i < tempArray.length - 1; i++) {
    if (tempArray[i] < 0) {
      minusTemps.push(tempArray[i]);
    } else if (tempArray[i] > 0) {
      plusTemps.push(tempArray[i]);
    }
  }

  minusTemps.sort((a, b) => b - a)
  plusTemps.sort((a, b) => a - b)

  if (plusTemps[0] === Math.abs(minusTemps[0])) {
    return(plusTemps[0])
  }

  if (plusTemps[0] < Math.abs(minusTemps[0])) {
    return(plusTemps[0])
  }

  if (plusTemps[0] > Math.abs(minusTemps[0])) {
    return(minusTemps[0])
  }
}

calculateClosestTo0(tempArray)

What I intended to do is separate the array into minus values and positive values. The minus values I ordered by descending (cause I want the first item to be the "biggest" in minus numbers perspective) and the positive in ascending. After that, I compare the first number.

What worries me most is the I try to remove the "0" like that:

for (let i = 0; i < tempArray.length - 1; i++) {
  if (tempArray[i] < 0) {
    minusTemps.push(tempArray[i]);
  } else if (tempArray[i] > 0) {
    plusTemps.push(tempArray[i]);
  }
}

My colleagues usually frowned upon having a condition that has "else if" and nothing else. Why, I don't know.

Another thing that worries me is that ending part, where I have 3 conditions to return the correct value. Is there a smarter way to do that?

And generally: do you think that there is a better approach to such a problem?

\$\endgroup\$
  • \$\begingroup\$ "My collgues usually froun upon having a condition that has "else if" and no else. Why: I don't know." Because you leave conditional cases uncovered from your code. \$\endgroup\$ – πάντα ῥεῖ Jul 14 '18 at 15:23
  • \$\begingroup\$ @πάνταῥεῖ -- that is definitely true! However, in this case, I want to omit the zeros and I want to reduce complexity, so I didn't want to filter the values beforehand. Can you please suggest a more clear way? Thanks! \$\endgroup\$ – Chris Panayotova Jul 14 '18 at 15:28
  • 1
    \$\begingroup\$ I would think you only need to keep the current closet to 0, so you only go through source array once, and no sorting or supplementary arrays. That is, O(n) ops with O(1) supplementary mem, which I should simplify the logic a lot. Right now you are doing O(n lg n) ops (sorting) and O(n) supplementary mem (+/- arrays). \$\endgroup\$ – Kristian H Jul 14 '18 at 15:40
  • \$\begingroup\$ "My collgues usually froun ..." I agree with your colleagues most time. But Javascript is all about early return, do only necessary calculations, write only necessary code. Assuage your colleagues with comments. \$\endgroup\$ – terary Jul 14 '18 at 19:07
  • \$\begingroup\$ This could be shortened to Math.min(...arr) \$\endgroup\$ – FreezePhoenix Jul 16 '18 at 13:02
7
\$\begingroup\$

Bugs

  1. The loop

    for (let i = 0; i < tempArray.length - 1 ; i++) {
    

    omits the last array element, it should be

    for (let i = 0; i < tempArray.length ; i++) {
    
  2. If all array elements have the same sign then either plusTemps or minusTemps is an empty array, and accessing its first element in

    if (plusTemps[0] === Math.abs(minusTemps[0])) {
    

    is undefined. Those cases must be treated separately in your approach.

  3. If the array is not empty, but all array elements are zero then both plusTemps are minusTemps are empty, again making the result undefined. What would be the expected result for this case? Returning zero looks like a sensible choice to me.

Simplifications

The first and second check in

if (plusTemps[0] === Math.abs(minusTemps[0])) {
    return(plusTemps[0])
}

if (plusTemps[0] < Math.abs(minusTemps[0])) {
    return(plusTemps[0])
}

if (plusTemps[0] > Math.abs(minusTemps[0])) {
    return(minusTemps[0])
}

can be combined to

if (plusTemps[0] <= Math.abs(minusTemps[0])) {
    return(plusTemps[0])
}

if (plusTemps[0] > Math.abs(minusTemps[0])) {
    return(minusTemps[0])
}

and the second if is better done with an else:

if (plusTemps[0] <= Math.abs(minusTemps[0])) {
    return(plusTemps[0])
} else {
    return(minusTemps[0])
}

This saves a comparison and makes it clear that a value is returned in any case. And since we know that minusTemps[0] is negative we need not call Math.abs:

if (plusTemps[0] <= -minusTemps[0]) {
    return(plusTemps[0])
} else {
    return(minusTemps[0])
}

A better approach

As already mentioned in the comments and in the other answers, the solution can be determined with a single traversal of the array, without the need to create additional arrays or any filtering or sorting.

Just keep track of the closest element found so far, and replace it if a negative element with a closer distance is found, or a positive element with the a closer or the same distance.

closest stays zero until the first non-zero array element is found. If non non-zero element exists then zero is returned. This covers the case of an empty array, making the check for the array length obsolete.

function calculateClosestTo0 (arr) {
    let closest = 0
    for (let i = 0; i < tempArray.length ; i++) {
        if (closest === 0) {
            closest = arr[i]
        } else if (arr[i] > 0 && arr[i] <= Math.abs(closest)) {
            closest = arr[i]
        } else if (arr[i] < 0 && -arr[i] < Math.abs(closest)) {
            closest = arr[i]
        }
    }
    return closest
}

Using Array.reduce this can be written more concisely as

function calculateClosestTo0 (arr) {
    return arr.reduce((acc, x) =>
        acc === 0 ? x :
        x > 0 && x <= Math.abs(acc) ? x :
        x < 0 && -x < Math.abs(acc) ? x : acc
    , 0)
}

Unit tests

The first versions of my suggested code had bugs as well: It “looked” simple and correct, but returned a wrong result in special cases. It is a good idea to collect those (and similar) cases, as well as other special cases that you think of, into a list. Then each iteration of the code can be tested against that list, to ensure that it does not introduce new bugs.

Here is the list that I came up with, it should cover many of the possible situations:

function calculateClosestTo0 (arr) {
    return arr.reduce((acc, x) =>
        acc === 0 ? x :
        x > 0 && x <= Math.abs(acc) ? x :
        x < 0 && -x < Math.abs(acc) ? x : acc
    , 0)
}

let tests = [
    // Single element:
    { array: [5], result: 5 },
    { array: [-5], result: -5},
    // Positive element is closer to zero:
    { array: [-3, 2], result: 2 },
    { array: [1, -2, 3, -4], result: 1 },
    // Negative element is closer to zero:
    { array: [-2, 3], result: -2 },
    { array: [-1, 2, -3, 4], result: -1 },
    // Two elements with same distance but opposite sign:
    { array: [1, -1], result: 1},
    { array: [-1, 1], result: 1},
    { array: [0, 1, -1], result: 1},
    { array: [1, 0, -1], result: 1},
    { array: [1, -1, 0], result: 1},
    { array: [0, -1, 1], result: 1},
    { array: [-1, 0, 1], result: 1},
    { array: [-1, 1, 0], result: 1},
    // Only elements of same sign:
    { array: [4, 2, 3], result: 2},
    { array: [-4, -2, -3], result: -2},
    // Empty array, or only zeros:
    { array: [], result: 0},
    { array: [0, 0, 0, 0], result: 0},
    // Closest element is at front, medium, or tail position:
    { array: [2, 0, -2, 0, -3], result: 2},
    { array: [-2, 0, 2, 0, -3], result: 2},
    { array: [-2, 0, -3, 0, 2], result: 2}
]

for (let i = 0; i < tests.length; i++) {
    console.log(calculateClosestTo0(tests[i].array) == tests[i].result)
}

\$\endgroup\$
  • \$\begingroup\$ Wow, that explanation was amazing! Thank you so much! 🙌Code works like a charm as well! \$\endgroup\$ – Chris Panayotova Jul 15 '18 at 8:15
4
\$\begingroup\$
  1. I suggest to use functional approach, which makes code more readable and concise.
  2. I suggest to write functions that solve some generic problem, not particular one.
  3. I think using word "temp" in variable names is redundant.

My suggested solution:

const distance = (from, to) => Math.abs(from - to);
const closestTo = (arr, num) => {
	if (arr.length === 0) { return num; }
	return arr.filter(x => x !== num)
  	 	  .reduce((acc, x) => distance(num, x) < distance(num, acc) ? x : acc, 
                          arr[0]);
}

console.log(closestTo([-1, 0, 0, 5, -5, 6, -3, 2, 10, 13, 8, 70, -36, 36], 0));

Updated. There is more readable, but less concise version of program.

const closestTo = (list, num) => {
	const distance = (from, to) => Math.abs(from - to);
	const excludeSelf = ([x, d]) => (x !== num);
	const byDistance = (([x1, d1], [x2, d2]) => (d1 !== d2) ? d1 - d2 : x2 - x1);

	if (list.length === 0) { return num; }
	
	const [x, d] = list
            .map(x => [x, distance(x, num)])
            .filter(excludeSelf)
            .sort(byDistance)
            .shift();
	return x;   
};

console.log(closestTo([4, -3, 1, -1], 0));

\$\endgroup\$
  • \$\begingroup\$ Hey @Stexxe! Thanks for the solution! I was thinking this can be done with .reduce(), but I couldn't think of a way and now I see how to do it! However in a bigger set of data ex. (closestTo([5, -1, 0, 0, 5, -5, 6, -3, 2, 1, 13, 8, 70, -36, 36], 0)) the function still returns -1, when 1 is needed, so I think .reduce() should be something like this?: \$\endgroup\$ – Chris Panayotova Jul 15 '18 at 8:03
  • \$\begingroup\$ If postive numbers have higher priority than negative in terms of distance, then you can sort array after filtering it and change comparison of distances (in reduce) from < to '<='. \$\endgroup\$ – Stexxe Jul 15 '18 at 14:07
1
\$\begingroup\$

I know it already has an accepted answer. However, just trying my luck with a different approach. I ran this function through @Martin R's test suite.

function calculateClosestTo0 (arr) {
    if(!Array.isArray(arr) || arr.length === 0) return 0;
    arr.push(0);

    let set = new Set(arr);
    let ar = Array.from(set);
    if(ar.length === 1) return 0;

    ar.sort((a,b) => a-b);

    const index = ar.indexOf(0);

    if(index === 0) return ar[1];
    if(index === ar.length - 1) return ar[ar.length - 2];
    if(Math.abs(ar[index-1]) === ar[index+1]) return ar[index+1];

    return Math.min(Math.abs(ar[index-1]), ar[index+1]) === ar[index+1] ? ar[index+1] : ar[index-1];
}

let tests = [
    // Single element:
    { array: [5], result: 5 },
    { array: [-5], result: -5},
    // Two elements with same distance but opposite sign:
    { array: [1, -1], result: 1},
    { array: [-1, 1], result: 1},
    { array: [0, 1, -1], result: 1},
    { array: [1, 0, -1], result: 1},
    { array: [1, -1, 0], result: 1},
    { array: [0, -1, 1], result: 1},
    { array: [-1, 0, 1], result: 1},
    { array: [-1, 1, 0], result: 1},
    // Only elements of same sign:
    { array: [4, 2, 3], result: 2},
    { array: [-4, -2, -3], result: -2},
    // Empty array, or only zeros:
    { array: [], result: 0},
    { array: [0, 0, 0, 0], result: 0},
    // Closest element is at front, medium, or tail position:
    { array: [2, 0, -2, 0, -3], result: 2},
    { array: [-2, 0, 2, 0, -3], result: 2},
    { array: [-2, 0, -3, 0, 2], result: 2},
    { array: [-3, 2], result: 2},
    { array: [-3, 4], result: -3},
    { array: [3, -4], result: 3}
]

for (let i = 0; i < tests.length; i++) {
    console.log(calculateClosestTo0(tests[i].array) == tests[i].result);
}
    

EDIT: Changed the algorithm. Earlier one was flowed as pointed out by @Martin R.

I have the same review comments as @Martin R's so I did not specify them separately.

The approach I have taken was to base my logic on the threshold.

  • I pushed the threshold in the array.
  • Removed the duplicates.
  • Tried to fail early and exit the routine.
  • Tried to use the language available functions as much as possible.

As I said, the accepted answer is already elegant. I thought of listing another approach. I do not claim it to be better, I found the flow personally approachable.

\$\endgroup\$
  • 1
    \$\begingroup\$ Btw, your code returns a wrong result for [-3, 2] (test suite extended). \$\endgroup\$ – Martin R Jul 16 '18 at 12:46

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.