current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Given an array of numbers I've to write a function to return the string "Arithmetic" if the sequence follows an arithmetic pattern or return "Geometric" if it follows a geometric pattern. If the sequence doesn't follow either pattern return "neither arithmetic and geometric".

I'm a beginner and I thought of following solution first. How do I improve on it and write better code next time? 

var arr =[2,4,6,8,10];// [2,6,18,54]- Geometric
var arthCount=0;
var geoCount=0;
    function ArthGeo(){
        for (var i=0;i<arr.length-2;i++){
             if(arr[i+1]-arr[i]===arr[i+2]-arr[i+1]){
                 arthCount++;
            }
            else if(arr[i+1]/arr[i] ===arr[i+2]/arr[i+1]){
                 geoCount++;
            }
            else return "neither arithmetic nor geometric";
        }
         if(arthCount===arr.length-2)
             return "Arthematic series";
         else if(geocount===arr.length-2)
             return "Geometric series";
     }    
 console.log(ArthGeo());
share|improve this question
    
This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. – SuperBiasedMan Oct 9 at 16:10
    
I edited it. Thanks – CodeM Oct 9 at 16:15

2 Answers 2

up vote 2 down vote accepted

This is great example to practice identifying the right abstraction.

In testing to see if an array is geometric or arithmetic series, what you are really doing is testing if a particular mathemtical relation holds for each element. That is, the heart of your program is in these 2 lines:

if(arr[i+1]-arr[i]===arr[i+2]-arr[i+1]){
//...
else if(arr[i+1]/arr[i] ===arr[i+2]/arr[i+1]){

Let's go ahead and extract just these relations into their own functions:

// This tells if an arithmetic relation holds at a particular index of the array
// The function signature here is chosen to work with javascript's every method
// The reason will become clear below.
function isArithmeticAtIndex(elm, i, arr) {
  if (i < 2) return true; // trivially true when it can't be false
  return arr[i] - arr[i-1] === arr[i-1] - arr[i-2];
}

function isGeometricAtIndex(elm, i, arr) {
  if (i < 2) return true;
  if (arr[i-1] == 0 || arr[i-2] == 0) return false; // can't divide by 0
  return arr[i] / arr[i-1] === arr[i-1] / arr[i-2];
}

Now we'll create a generic function that tells us if a particular mathematical relation holds at every elment of an array:

function hasSeriesRelation(arr, relationFn) {
  // geometric and arithmetic series (and most others) 
  // don't make sense with less than 3 elements
  if (arr.length < 3) return false;
  return arr.every(relationFn);
}

At this point, we are basically done. We can use what we've written as follows:

a = [1,2,3,4];
hasSeriesRelation(a, isArithmeticAtIndex);
// => true
a = [1,2,3,5];
hasSeriesRelation(a, isArithmeticAtIndex);
// => false
a = [1,2,4,8];
hasSeriesRelation(a, isGeometricAtIndex);
// => true
a = [1,2,4,9];
hasSeriesRelation(a, isGeometricAtIndex);
// => false

But writing hasSeriesRelation(a, isGeometricAtIndex) is cumbersome, so let's wrap these up in their own functions, and make them array methods:

Array.prototype.isArithmeticSeries = function() { return hasSeriesRelation(this, isArithmeticAtIndex) };
Array.prototype.isGeometricSeries = function() { return hasSeriesRelation(this, isGeometricAtIndex) };

And now we can write things like:

[1,2,4,8].isGeometricSeries();
[1,2,3,4].isArithmeticSeries();

Side Note: Some consider extending Array.prototype bad practice. I think it can be okay depending on your circumstances. See this post for more discussion. In any case, it's only marginally relevant to this discussion, because you can easily rewrite the above to functions with signatures like function isArithmeticSeries(arr)

Finally, note that we have gained the ability to quickly and easily add new types of series. For example, we might want to identify Fibonacci-like series:

function isFibonacciAtIndex(elm, i, arr) {
  if (i < 2) return true; // trivially true when it can't be false
  return arr[i] === arr[i-1] + arr[i-2];
}

a = [1,1,2,3,5,8];
hasSeriesRelation(a, isFibonacciAtIndex);
// => true
share|improve this answer
up vote 2 down vote

Bug-ish

Right now, if I use your ArthGeo function and pass in an array like this:

[2, 6]

It will always return `Arithematic series". Or, if I do this:

[2]

I get:

ReferenceError: geocount is not defined

You should add some checking at the very beginning of your function that somehow communicates to the code that is using this function that the array is too small for the function to work properly. Perhaps you could throw an exception.

Design

Your design is a little awkward. You have a function that will return one of three string values: "Arithematic series", "Geometric series", or "neither arithmetic nor geometric".

That looks a little awkward for whatever is using this:

if(ArthGeo() === "Arithematic series") {
    ...
}

It's poor design that this function is returning a string to symbolize a result. Usually strings are returned and the string itself is used for displaying to an interface. In this case, the string is treated as a value that needs to be checked against. And, to add, string comparison can be fairly slow.

I recommend that you split up this into two functions/methods that like isArith and isGeo. Perhaps you could contain these new code snippets in an object like SeriesChecker. That might look like this:

var SeriesChecker = {
    isArith: function(arr) {
        ...
    },
    isGeo: function(arr) {
        ...
    }
}

Then, each method would return true if it is its respective series type, and false if not.

This is a much better design because it makes much more sense when being used:

if(SeriesChecker.isGeo(arr)) {

}

No need to treat strings as some ternary value that is only going to be used in some sort of conditional statement.

Here is a final note I have for you on this: When writing code like this that will be used by some other code, think about how that code is going to your code. If something's not quite right, then you need to re-design your code.

share|improve this answer
    
small but imo important point on naming you may be interested in, which argues against names like SeriesChecker. Indeed, it is the series itself which is geometric, arithmetic, etc, or not. The best OO solution would probably be to turn your arrays into proper Series objects like series = new Series(arr); series.isGeometric(), rather than use a static utility class. – Jonah Oct 10 at 17:41

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.