This seems to be working fine. I am pretty sure there is a better way to do it. How can my code be improved performance-wise? The main thing bothering me is the nested for
-loop.
function arithGeo (arr) {
var isGeometric = false;
var isArithmetic = false;
var diff = arr[2] - arr[1];
var ratio = arr[2] / arr[1];
for (var i = 0; i < arr.length - 1; i++) {
var j = i + 1;
if (diff !== arr[j] - arr[i]) {
for (var k = 0; k < arr.length - 1; k++) {
var l = k + 1;
if (ratio !== arr[l] / arr[k]) {
break;
} else if (l === arr.length-1) {
isGeometric = true;
break;
}
}
} else if (j === arr.length - 1) {
isArithmetic = true;
}
}
if (isGeometric === true) {
return "geometric";
} else if (isArithmetic === true) {
return "arithmetic";
}
else {
return -1;
}
}
console.log(arithGeo([2,4,6,8]));
console.log(arithGeo([2,6,18,54]));
console.log(arithGeo([2,6,1,54]));
1 Answer 1
I agree with @janos that sometimes returning a string and sometimes returning -1
makes this function hard to use. In addition, it's unclear what your function is supposed to return for input that is both an arithmetic and a geometric sequence (every element is the same).
I find it odd that you base the difference and ratio on elements at indexes 1 and 2 rather than 0 and 1. Such an arbitrary choice seems like an unnecessary failure mode for short input arrays.
Note that if the ratio is 0 or infinite, you can immediately declare the sequence to be not geometric.
A better approach is to assume that a sequence is arithmetic or geometric until proven otherwise.
function arithGeo(arr) {
var diff = arr[1] - arr[0];
var ratio = arr[1] / arr[0];
var isArithmetic = true, isGeometric = isFinite(ratio) && ratio != 0;
for (var i = 1, j = 2; (isArithmetic || isGeometric) && j < arr.length; i++, j++) {
if (isArithmetic && arr[j] - arr[i] != diff) {
isArithmetic = false;
}
if (isGeometric && arr[j] / arr[i] != ratio) {
isGeometric = false;
}
}
return isArithmetic && isGeometric ? 'both' :
isArithmetic ? 'arithmetic' :
isGeometric ? 'geometric' : '';
}
For clarity, you might as well write it as two independent loops, because the classification problems are independent of each other. There would be more lines of code, but the complexity of the algorithm remains the same. Except in degenerate cases, only one of the loops is going to see much action anyway.
function arithGeo(arr) {
var i, j;
var diff = arr[1] - arr[0];
var isArithmetic = true;
for (i = 1, j = 2; isArithmetic && j < arr.length; i++, j++) {
if (isArithmetic && arr[j] - arr[i] != diff) {
isArithmetic = false;
}
}
var ratio = arr[1] / arr[0];
var isGeometric = isFinite(ratio) && ratio != 0;
for (i = 1, j = 2; isGeometric && j < arr.length; i++, j++) {
if (isGeometric && arr[j] / arr[i] != ratio) {
isGeometric = false;
}
}
return isArithmetic && isGeometric ? 'both' :
isArithmetic ? 'arithmetic' :
isGeometric ? 'geometric' : '';
}