I'm looking for criticism on the following method used to determine if a collection of Integers forms an arithmetic sequence:
public boolean isArithmetic(Integer[] nums)
{
Set<Integer> result = new HashSet<Integer>();
Collections.sort(Arrays.asList(nums));
for(int i = 0; i < nums.length -1; i++)
{
result.add(nums[i+1] - nums[i]);
}
if(result.size() > 1)
return false;
return true;
}
The basic method you're using seems to me rather roundabout. If you're going to write a loop to walk through the collection anyway, why not just check for the correct criteria there? e.g., something like this:
Integer diff = nums[1] - nums[0];
for (int i=2; i<nums.length; i++)
if (nums[i] - nums[i-1] != diff)
return false;
return true;
This can save quite a bit of work, and (especially if the numbers don't form an arithmetic sequence) may save a fair amount of storage as well. More importantly, it makes the intent immediately obvious, which (it seems to me) that inserting all the differences into a hash table doesn't.
One other specific detail of your code bothers me:
if(result.size() > 1)
return false;
return true;
Anything of the form if (x) return true; else return false; can be expressed as return x;. In your case, the sense is reversed, so it can be return !x;, so you just want return result.size() == 1; (but, as noted above, I'd rather eliminate this entirely).
If you really want to use the approach you've taken, I think it becomes a lot more reasonable if you take your loop and wrap it up into a generic algorithm. I'd probably call it adjacentDifference. Then you end up with something on this general order:
Set<Integer> result = new HashSet<Integer>();
Collections.sort(Arrays.asList(nums));
Algorithms.adjacentDifference(nums, result);
return result.size() == 1;
That's simple and neat enough that as long as you're sure this is outside the critical path (i.e., the extra time and space used don't matter) it would at least be worth considering.
