Problem
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?
Solution
Bugs
-
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++) { -
If all array elements have the same sign then
eitherplusTempsorminusTempsis an empty array, and accessing
its first element inif (plusTemps[0] === Math.abs(minusTemps[0])) {is undefined. Those cases must be treated separately in your approach.
-
If the array is not empty, but all array elements are zero then
bothplusTempsareminusTempsare 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)
}- I suggest to use functional approach, which makes code more readable and concise.
- I suggest to write functions that solve some generic problem, not particular one.
- 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));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.